[th/marvell-boot-live-iso] marvell: boot live ISO for accessing worker node #383
Conversation
|
|
||
|
|
||
| def marvell_bmc_rsh(bmc: BmcConfig) -> host.Host: |
There was a problem hiding this comment.
Derive from BMC just like BMCBF. Most of what you need is already in bmcbf.py. Can we uncomment that, make it more generic, and start using it here? The rest looks good
There was a problem hiding this comment.
What is the essence of the BMC API?
Currently, BMC defines methods like boot_iso_redfish(), restart_redfish(), stop(), start(), cold_boot(). But subclasses (IPUBMC, BMCBF) neither extend nor use this API. Instead, they introduce their own unrelated methods (IPUBMC: ensure_firmware(), version(); BMCBF: firmware_upgrade(), firmware_version()), so they are not really "is-a BMC".
The result is three separate classes (BMC, IPUBMC, BMCBF) that are only loosely related but tied together through inheritance. If the goal is to compose different BMC concepts, inheritance is the wrong abstraction.
The patch already provides an abstraction, marvell_bmc_rsh(), which does what callers need. Forcing it into a class hierarchy without a clear, unified API doesn’t add value.
There was a problem hiding this comment.
The main point is this line https://github.com/bn222/cluster-deployment-automation/blob/main/bmcbf.py#L44
That's essentially the same thing you're doing here:
Isn't that duplication of logic here?
There was a problem hiding this comment.
Also please elaborate on this statement "If the goal is to compose different BMC concepts, inheritance is the wrong abstraction."
The intent of BMC is to encapsulate a way to boot a system. You're booting a Marvell card, which is conceptually the same as booting an IPU or a DELL server.
There was a problem hiding this comment.
Here is the list of actions we want to perform on a Marvell card, IPU or dell server:
- boot it from an iso (boot_iso_redfish should be renamed to boot_iso), where an arg is an url pointing to the ios.
- power it off
- power it on
- power cycle it
Can you provide details why the functions in the BMC class are not fit for that?
There was a problem hiding this comment.
A MarvellBMC is quite different, and it is unclear how it can implement the same API as the baseclass BMC. And there doesn't seem to be any use for power on/off/cycle. At least for now.
For example, the "boot_iso" variant that is added in this PR is about booting the "BMC" host itself. That is fundamentally a different operation from the (Redfish based) BMC class where "boot_iso_redfish" boots an ISO on the host that the BMC manages. If anything, Marvell's "boot_iso" is more like BMC.restart_redfish(), which is about restarting the BMC itself.
And the "pxeboot" command that CDA invokes for Marvell (which actually installs a ISO on the DPU) requires additional parameters beside the plain boot_iso_redfish(). That is, because it is a special function to install a RHEL system. It is not the same operation as a plain BMC.boot_iso_redfish() method.
These are different things.
Can you provide details why the functions in the BMC class are not fit for that?
My point is that the existing BMC, IPUBMC and BMCBF classes already fail to expose a common/unified API (in the sense of "is-a" and Liskov Substitution Principle). So it isn't clear how MarvellBMC could implement such a (nonexisting) unified API. The first step to maybe address this would be to unify the two classes BMC and IPUBMC (we can ignore BMCBF, which is unused and should be just deleted). But even then, you will struggle to make sense how Marvell fits in that API.
But there is no problem here and no need to unify anything. There can be unrelated classes BMC, IPUBMC and MarvellBMC. The only ugliness is that IPUBMC subclasses BMC and pretends it would be a BMC implementation. Which it is not. The simple fix is that IPUBMC should not subclass BMC and get over the fact that these are different (albeit related) helper classes.
This notwithstanding, I added a MarvellBMC class which contains some methods and does the same thing as the code that was previously in functions at module scope. How about that?
There was a problem hiding this comment.
Much better. A few more comments below.
thom311
force-pushed
the
th/marvell-boot-live-iso
branch
2 times, most recently
from
8f26da8 to
d32b630
Compare
| return False | ||
| return "177d:b900" in rsh.run("lspci -nn -d :b900").out | ||
|
|
||
| def pxeboot( |
There was a problem hiding this comment.
- Rename to boot_iso
- Only pass in 1 string, the url to an iso.
- rename
ssh_to_bmcto_ssh_to_bmc - Implement
start(uses the hosts bmc to power on the host which will power on the card). You'll need this as the latest changes that went in turn off the host and you will not be able to access the card.
isoDeployer.py
Outdated
| n = self._master | ||
| assert n.ip is not None | ||
| assert n.bmc is not None | ||
| bmc = marvell.MarvellBMC(n.bmc) |
There was a problem hiding this comment.
Introduce MarvellClusterNode just like we have for IPU.
thom311
force-pushed
the
th/marvell-boot-live-iso
branch
2 times, most recently
from
4156a0b to
acb6286
Compare
IPUBMC.is_ipu() calls self._version_via_ssh(), which tries to rh.ssh_connect(..., timeout="5s"). But ssh_connect() would first wait for one hour trying to ping the host. After timeout of one hour, wait_ping() will abort CDA. This is not right. We try to use SSH to detect the kind of hosts. In particular, the host may not be up at all. Especially since commit fcbb05d ('clusterNode: Stop baremetal nodes during teardown.'), where hosts are now turned off. This ping check seems wrong altogether. But hanging for one hour before dying is especially wrong. Fixes: fcbb05d ('clusterNode: Stop baremetal nodes during teardown.')
Otherwise, there is nothing at info level telling that we are currently calling a 20 minute long command. When watching the log, you might think the program hangs. Log a message.
fedora-coreos-config has a git submodule "fedora-bootc". This needs to be initialized, otherwise the build is going to fail: # podman run --rm -ti --userns=host -u root --privileged -v /tmp/build/fcos:/srv/ --device /dev/kvm --device /dev/fuse --tmpfs /tmp -v /var/tmp:/var/tmp --name cosa -v /tmp/build/fedora-coreos-config:/git:ro quay.io/coreos-assembler/coreos-assembler:latest fetch Config commit: 9aebe7895a1f2055569ebc670a2212319c106d1a Using manifest: /srv/src/config/manifest.yaml error: Can't open file "/srv/src/config/manifests/../fedora-bootc/minimal-plus/manifest.yaml" for reading: No such file or directory (os error 2) failed to execute cmd-fetch: exit status 1 It is not clear to me how this ever worked. Fix this by initializing the submodules.
IPUClusterNode was a subclass of ClusterNode. But that adds mental overhead without needing inheritance. - IPUClusterNode never accesses any attributes or methods of the parent class. As the ClusterNode parent implements things too, it leaves the reviewer wonder which parts IPUClusterNode actually uses. The answer is none, which is good, because knowing that simplifies reasoning about the class. You don't need to look at ClusterNode to understand IPUClusterNode. But as IPUClusterNode subclasses ClusterNode, that was hard to see because due to the subclassing, all the parent's API and behaviors are dragged in (although being effectively unused). - the IPUClusterNode hardly implements any common API with ClusterNode. That is, it does not meaningfully extend ClusterNode with a is-a relationship (Liskov Substitution Principle). It does not implement post_config() (beside doing nothing). The only API that it implements is "start()", which is trivial. And IPUClusterNode's start() cannot even fail, so there is no need to adhere to the API of ClusterNode which returns False on failure. - IPUClusterNode has only one user: IsoDeployer's _deploy_master(). But that user is well aware that it is handling a IPUClusterNode (and not a generic ClusterNode). It consequently knows there is no need to call post_config() (or it knows that it doesn't need to handle any error from post_config()). This makes IPUClusterNode independent from ClusterNode. If you ever find a useful case where you want to create IPUClusterNode instances and treat them via "is a ClusterNode", then you can bring easily bring that back. But at that point, do it in a way that is beneficial, to justify the extra coupling. Btw, due to the dynamic nature of Python (and typing annotations), you can also call a start() method on two unrelated classes. You don't need a common class hierarchy to do that in a useful way. The real reason for this change is the upcoming MarvellClusterNode, which also *is-not-a* ClusterNode in any useful sense.
We use pxeboot to install RHEL on the DPU. For that, we need to SSH into the host (that has the DPU inside) and run a podman command. That requires that the host is up and accessible via SSH. Also, CDA is not told by configuration which kind of DPU the host has inside. Instead, to detect that it should install a Marvell DPU, it needs to SSH into the host and check lspci. If the host is not accessible, then previously installation would fail. Well, the intended approach would have been to install the OCP cluster (including the worker) first. Then the worker node would be accessible via SSH, and all would be good. However, that is not done. Hence, if the machine is not accessible, fallback to boot a CoreOS Live iso. While at it, add MarvellBMC and MarvellClusterNode classes. https://issues.redhat.com/browse/MDC-108 https://issues.redhat.com/browse/IIC-806
thom311
force-pushed
the
th/marvell-boot-live-iso
branch
from
acb6286 to
3936a49
Compare
We use pxeboot to install RHEL on the DPU. For that, we need to SSH into
the host (that has the DPU inside) and run a podman command. That
requires that the host is up and accessible via SSH.
Also, CDA is not told by configuration which kind of DPU the host has
inside. Instead, to detect that it should install a Marvell DPU, it
needs to SSH into the host and check lspci.
If the host is not accessible, then previously installation would fail.
Well, the intended approach would have been to install the OCP cluster
(including the worker) first. Then the worker node would be accessible
via SSH, and all would be good. However, that is not done.
Hence, if the machine is not accessible, fallback to boot a CoreOS Live
iso.
https://issues.redhat.com/browse/MDC-108
https://issues.redhat.com/browse/IIC-806