fold 'kola testiso' into 'kola run'#4377
Merged
nikita-dubrovskii merged 34 commits intocoreos:mainfrom May 5, 2026
Merged
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
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.
…tests Add a helper function that skips tests if no artifacts are present.
The dev build naming pattern now uses '.devN' or '-devN' instead of '.dev.N' format: fedora-coreos-43.20260429.20.dev1-qemu.x86_64.qcow2 rhcos-10.2.20260428-dev0-qemu.x86_64.qcow2
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
7088abc to
24caeb4
Compare
Add SkipBaseChecksTag to all ISO live tests to temporarily skip base checks that look at the journal for failures. This is needed until the bootupd fix is merged. Related: coreos/fedora-coreos-tracker#2136
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
24caeb4 to
f545d33
Compare
dustymabe
previously approved these changes
May 4, 2026
Mark the live ISO tests with the reprovision tag because those require a lot of memory and should run serially.
dustymabe
approved these changes
May 5, 2026
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