fold 'kola testiso' into 'kola run'#4377
Open
nikita-dubrovskii wants to merge 31 commits intocoreos:mainfrom
Open
fold 'kola testiso' into 'kola run'#4377nikita-dubrovskii wants to merge 31 commits intocoreos:mainfrom
nikita-dubrovskii wants to merge 31 commits intocoreos:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a large but valuable refactoring that folds the kola testiso command into kola run. This improves the consistency and maintainability of the test suite. The implementation moves the ISO test logic into a new mantle/kola/tests/iso package and refactors the QEMU cluster platform API to be more extensible and modular, which is a great improvement.
I've found a few issues that could be improved:
- There are a couple of places where
panic()is used inside a goroutine for error handling, which can crash the entire test runner. It would be better to handle these errors more gracefully, for example by logging them. - There is a potential goroutine leak in the
awaitCompletionhelper function due to atime.Sleepthat doesn't respect context cancellation. - A file path is constructed using
strings.Replace, which is fragile. Using thepath/filepathpackage would be more robust.
Overall, this is a solid refactoring. My comments are focused on improving robustness and resource management.
Contributor
Author
|
It's not yet 100% ready - i'm reworking metal PXE&ISO installation, so those tests still rely on code from |
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
3 times, most recently
from
9499239 to
9816db6
Compare
nikita-dubrovskii
changed the title
Contributor
Author
|
Finished metal PXE&ISO installation. Almost ready for review. |
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
34d0988 to
ac38a76
Compare
Contributor
Author
|
s390x run: |
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
ac38a76 to
aa167c6
Compare
Member
|
@nikita-dubrovskii this is an incredible amount of work. Thank you for taking this on. This does make it hard to grok all in one go, though. I'm still struggling a bit with the first commit even. As I go I'm questioning old code that we have and I want to challenge if some of it is necessary. I broke out some initial commits that challenge the original code (and also a few commits from this PR) into #4486 I also rebased the commits from this PR on top of that in https://github.com/dustymabe/coreos-assembler/commits/dusty-kola-testiso-fold/ if you're interested in a rebased version of this PR. Let me know what you think. |
Contributor
Author
|
@dustymabe Thx, checked your PR. Looks sane to me, and many thanks for better commit messages, describing purpose of changes. The only thing holding me back is dealing with rebase&merge conflicts, but not a big deal. |
Member
Thanks!
Those should be handled already over in https://github.com/dustymabe/coreos-assembler/commits/dusty-kola-testiso-fold/ Once #4486 merges (if it gets approved) I can push that rebase over here if you like. |
Contributor
Author
Yes, please. |
Member
|
I did some more looking at this today. The overall strategy, not just the first commit. I'm still forming opinions and not really ready to share anything yet. |
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
bd58831 to
5369e81
Compare
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
5369e81 to
7582d66
Compare
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
7582d66 to
d75f86b
Compare
Split the monolithic live-iso.go file into several focused files where each file contains a specific set of tests: - live-pxe.go: PXE network boot tests - live-iscsi.go: iSCSI boot and installation tests - live-as-disk.go: ISO-as-disk installation tests - common.go: Shared test utilities and helper functions This improves code organization and makes it easier to locate and maintain specific test suites.
Add journal dumping to signalFailureUnit to capture diagnostic information when the system enters emergency.target during ISO tests. After switch root occurs during live ISO testing, coreos-installer runs from the real root filesystem rather than the initramfs, which means ignition-virtio-dump-journal.service is no longer enabled. This change ensures that when emergency.target is reached, the journal is still dumped to the virtio port, allowing platform.StartMachine to capture errors. The unit now checks if ignition-virtio-dump-journal.service is enabled, and if not, manually executes the journal dump script from the dracut emergency shell setup module.
Move the 'inst-insecure' and 'pxe-kargs' parameters from local test options to global QEMUOptions structure, making them available as command-line flags for all ISO tests: - --inst-insecure: Skip signature verification on metal image - --pxe-kargs: Additional kernel arguments for PXE boot tests
Migrate the last remaining test from the legacy 'kola testiso' command into the standard 'kola run' This completes the migration of all ISO tests and removes the now-obsolete testiso.go file.
Extract duplicate completion channel reading logic into a reusable CheckTestOutput helper function. This reduces code duplication across iso tests.
The kola test harness already performs console and journal checks
automatically after each test completes, making the duplicate checks
in awaitCompletion unnecessary.
Call flow in mantle/kola/harness.go:
```
runProvidedTests
-> runTest
-> handleConsoleChecks("console", ...)
-> CheckConsole
-> handleConsoleChecks("journal", ...)
-> CheckConsole
```
Update the test to use the existing MachineBuilder API after the testiso migration.
Also remove the 4k sector size test variant as it's technically impossible to configure. The AddIso() function and iso-as-disk implementation do not support sector size configuration - the ISO is attached as a raw, readonly device without any sector size options. The Disk.SectorSize field only applies to regular disk images added via AddDisk() or AddDisksFromSpecs(), not to ISOs attached via AddIso(). Since iso-as-disk tests boot directly from the ISO file (not a disk image), sector size configuration is not applicable.
Only wait for SSH address when usermode networking is enabled. This allows ISO tests without networking to proceed without unnecessary SSH address waiting.
Add Instance() method to expose the underlying QemuInstance from a platform.Machine. This allows tests to access QEMU-specific functionality not available through the generic interface.
Extract the Ignition config path logic into a reusable IgnitionPath() method that returns the path where the Ignition config will be written. This allows callers to get the Ignition config path before rendering, which is useful for tests that need to create symlinks or perform other operations on the config file path before the machine is started.
Allow callers to pass nil userdata/config when Ignition configuration is not needed, such as in PXE boot scenarios where configuration is provided through alternative means.
Remove deprecated machine struct fields and methods that are no longer needed after the migration all ISO tests under kola.
Replace enableUefi and enableUefiSecure boolean flags with a single firmware string field in IsoTestOpts for simpler and more consistent firmware configuration across all ISO tests.
Add SetNetbootIndex() method to QemuBuilder to allow configuring the bootindex property for network devices. This enables proper boot order control when using PXE/network boot, as an alternative to the legacy -boot order=n option. The bootindex property is mutually exclusive with -boot order, as most firmware implementations support one or the other but not both. When bootindex is set, the -boot order option is automatically omitted.
Simplify PXE test network setup by using QemuBuilder's built-in methods (SetNetbootP, SetNetbootIndex, EnableUsermodeNetworking) instead of manually constructing QEMU network device arguments.
…ests Add helper function in common.go to validate presence of live artifacts.
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
f0629a4 to
962e44e
Compare
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the legacy kola testiso command into a suite of standard kola tests within the iso package. It introduces a MachineBuilder pattern in the QEMU platform to allow for more flexible machine configuration, particularly for PXE and ISO boot scenarios. Additionally, a translation layer is added to the kola wrapper to maintain backward compatibility. Feedback was provided regarding the removal of redundant comments in the flag definitions to reduce noise.
Comment on lines
+166
to
+168
| // kola run iso.* options | ||
| bv(&kola.QEMUOptions.InstInsecure, "inst-insecure", false, "Do not verify signature on metal image") | ||
| ssv(&kola.QEMUOptions.PxeKernelArgs, "pxe-kargs", nil, "Additional kernel arguments for PXE") |
Contributor
There was a problem hiding this comment.
The comment 'kola run iso.* options' is redundant and adds noise to the init function. It is better to rely on the grouping of related flags.
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
2 times, most recently
from
5e925b9 to
e4990a8
Compare
Add translate_testiso_args() function to convert legacy 'kola testiso' command to new 'kola run iso.*' format.
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
e4990a8 to
e5b1db5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This huge PR folds all
testisotests:#3989