-
Notifications
You must be signed in to change notification settings - Fork 168
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
mantle/kola/testiso: support testing coreos.liveiso.fromram installs #3555
Conversation
Marking as draft since this needs both #3555 to merge and to be merged into openshift/os as a git submodule bump first. |
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.
Partially superficial review, but CI should cover things.
mantle/cmd/kola/testiso.go
Outdated
RemainAfterExit=yes | ||
# Would like to use SuccessExitStatus but it doesn't support what | ||
# we want: https://github.com/systemd/systemd/issues/10297#issuecomment-1672002635 | ||
ExecStart=bash -c "mountpoint /run/media/iso && exit 1 || exit 0" |
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.
ExecStart=bash -c "mountpoint /run/media/iso && exit 1 || exit 0" | |
ExecStart=bash -c "if mountpoint /run/media/iso 2>/dev/null; then exit 1; fi" |
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.
done
mantle/cmd/kola/testiso.go
Outdated
@@ -738,7 +767,18 @@ func testLiveIso(ctx context.Context, inst platform.Install, outdir string, mini | |||
liveConfig.AddFile(nmstateConfigFile, nmstateConfig, 0644) | |||
} | |||
|
|||
mach, err := inst.InstallViaISOEmbed(nil, liveConfig, targetConfig, outdir, isOffline, minimal) | |||
if isISOFromRAM { | |||
isoKernelArgs = append(isoKernelArgs, "coreos.liveiso.fromram") |
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.
Perhaps worthy of elevating to a const
somewhere, and also link to the PR introducing the code and in the future the docs?
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.
elevated to const
mantle/cmd/kola/testiso.go
Outdated
// When you are debugging earlyboot/initramfs issues this can be | ||
// problematic. If you find yourself wanting more logging uncomment | ||
// out the following to get it all on the console of the machine. | ||
// isoKernelArgs = append(isoKernelArgs, "systemd.log_color=0 systemd.log_level=debug systemd.log_target=console") |
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.
We could make this a CLI or environment variable too.
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.
made it a env var
99e86cc
to
917be0c
Compare
going to wait until coreos/fedora-coreos-config#2544 merges to run CI on this. |
917be0c
to
a366de1
Compare
This can merge now that f-c-c has been updated in openshift/os. |
@cgwalters - mind another stamp since I rebased? |
We've supported this for a while in coreos-installer upstream and we even already use it in the NM keyfile code paths in this function. Let's modify our test framework to support it more generically now too.
Most of the time people probably want to test an install workflow and they can't really do that if only the ISO is attached to the VM via a CDROM drive (read only media). Let's add a disk by default for convenience.
In coreos/fedora-coreos-config#2544 we added a new `coreos.liveiso.fromram` kernel argument to instruct the ISO image to boot completely from memory in order to facilitate installs back to the same disk the ISO was booted from in iso-as-disk mode (i.e. dd the ISO file to a hard drive or partition on a hard drive). Let's test here that we can boot completely from memory and that /run/media/iso is not mounted. Currently this doesn't actually do an install to the same disk that we booted from (actually the iso-as-disk tests don't actually do installs at all, but just verify they can boot), but rather just verifies the ISO isn't mounted after the live system boots up, which should be sufficient.
coreos#3554 This comment will help anyone browsing the code or viewing the error in the log to understand more.
This will be useful in the future when trying to debug earlyboot testiso issues further.
a366de1
to
fcb9173
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.
This LGTM but I didn't really have time for a real review so not approving. Sorry.
Follow-up in #3621. |
In coreos/fedora-coreos-config#2544 we added a new
coreos.liveiso.fromram
kernel argument to instruct the ISO image to boot completely from memory in order to facilitate installs back to the same disk the ISO was booted from in iso-as-disk mode (i.e. dd the ISO file to a hard drive or partition on a hard drive).Let's test here that we can boot completely from memory and that /run/media/iso is not mounted.
Currently this doesn't actually do an install to the same disk that we booted from (actually the iso-as-disk tests don't actually do installs at all, but just verify they can boot), but rather just verifies the ISO isn't mounted after the live system boots up, which should be sufficient.