-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add iscsi+multipath test; add some improvements to test strategy #3789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice series! Some comments but LGTM overall.
mantle/cmd/kola/testiso.go
Outdated
@@ -90,6 +90,7 @@ var ( | |||
"iso-offline-install.mpath.bios", | |||
"iso-offline-install-fromram.4k.uefi", | |||
"iso-offline-install-iscsi.ibft.uefi", | |||
"iso-offline-install-iscsi.ibft-with-multipath.bios", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor/optional: we use mpath
in the one other test that exercises it. Maybe iso-offline-install-iscsi.ibft-mpath.bios
for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ironically enough we actually don't want consistency here. We don't want to match
coreos-assembler/mantle/cmd/kola/testiso.go
Lines 580 to 583 in 4d4e284
if kola.HasString("mpath", components) { | |
enableMultipath = true | |
inst.MultiPathDisk = true | |
} |
It's probably worth a comment to explain the nuance here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. I looked a little bit closer.. kola.HasString()
will only match if the full string of the component matches. so ibft-with-mpath
wouldn't match and we'd be fine.
Are you good with ibft-with-mpath
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to ibft-with-mpath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
butane_config = strings.ReplaceAll(iscsi_butane_config, "COREOS_INSTALLER_KARGS", "--append-karg netroot=iscsi:10.0.2.15::::iqn.2024-05.com.coreos") | ||
butane_config = strings.ReplaceAll(iscsi_butane_config, "COREOS_INSTALLER_KARGS", "--append-karg netroot=iscsi:10.0.2.15::::iqn.2024-05.com.coreos:0") | ||
case "ibft-with-multipath": | ||
butane_config = strings.ReplaceAll(iscsi_butane_config, "COREOS_INSTALLER_KARGS", "--append-karg rd.iscsi.firmware=1 --append-karg rd.multipath=default --append-karg root=/dev/disk/by-label/dm-mpath-root") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. This normally should also require --append-karg rw
as per https://github.com/openshift/os/blob/master/docs/faq.md#q-does-rhcos-support-multipath-on-the-primary-disk. Wonder why it's not required here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my original testing I always added rw
but then when I got here I was challenging every kernel argument and testing without them and it still passed the test. I went back and removed it in my original setup and it wasn't needed there either from what I could tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
followup in #3797
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Dusty for going in depth on the iscsi test and making all those tweaks !
The unit kept getting killed when entering emergency.target, which isn't very useful since some of the messages that hadn't been flushed yet were the ones that revealed the root cause of the failure. Let's set up the ordering/deps such that it doesn't get taken down when entering emergency.target. Also add a sleep 1 in the Destroy to allow for some remaining messages to get flushed.
In my experience the value of this has been little to none. It mostly just forces us to filter this output through jq -r .MESSAGE Let's make it just output text like the non-testiso journal.txt files are today.
There's no bios on aarch64, ppc64le or s390x.
The ip=ibft seems to only dictate what interface to use when bringing up networking, but isn't important if you are using DHCP IIUC. The rd.iscsi.initiator also isn't important here. It appears to work without it.
This will help get more test coverage.
Makes it a little bit easier for the reader to understand what the options are doing without having to study the iscsiadm man page.
We can use a random value here because we don't have ACL's enabled for this test. Let's just make it a random UUID to make it more clear the value isn't significant.
This has the same effect, just uses one fewer command!
When we go to emergency.target it's standard to use OnFailureJobMode=isolate.
The format doesn't need the extra pieces, which might be confused as important. Let's use the simplest possible name here.
Here we modify the iscsi setup to define two ports `:0` and `:1` that both expose the same underlying block device, which means that multipath can be set up on top of those two paths. We then add a new test that provides `root=/dev/disk/by-label/dm-mpath-root` and `rd.multipath=default` to the kernel command line to perform the test. Note that this modifies the targetd setup and the iPXE config for all of the tests, but they still work fine, just accessing `:0` and leaving `:1` alone.
462d48f
to
d7d352d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
/cherry-pick rhcos-4.16 |
@dustymabe: #3789 failed to apply on top of branch "rhcos-4.16":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This is required by ostree. This sometimes worked I think because `ignition-remount-sysroot.service` has no ordering against `ostree-prepare-root.service` and so they'd race. But really, we shouldn't rely on that Ignition unit. See also: coreos#3789 (comment)
This is required by ostree. This sometimes worked I think because `ignition-remount-sysroot.service` has no ordering against `ostree-prepare-root.service` and so they'd race. But really, we shouldn't rely on that Ignition unit. See also: #3789 (comment)
Follow-up in #3799. |
See individual commit messages.