fold 'kola testiso' into 'kola run'#4377
fold 'kola testiso' into 'kola run'#4377nikita-dubrovskii wants to merge 27 commits intocoreos:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
|
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
|
Finished metal PXE&ISO installation. Almost ready for review. |
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
34d0988 to
ac38a76
Compare
|
s390x run: |
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
ac38a76 to
aa167c6
Compare
|
@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. |
|
@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. |
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. |
Yes, please. |
|
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
'metal.go' is only used by testiso and contains code&data structures similar to those provided by 'machines/qemu/*.go'.
These checks are already performed by harness.go:CheckConsole.
… live-login) tests The iso.* tests use systemd units that report success or failure via virtio channels and always power off the machine. Because of this, we do not need to SSH into the instance.
nikita-dubrovskii
force-pushed
the
fold_testiso
branch
from
7582d66 to
d75f86b
Compare
2 participants
This huge PR folds all
testisotests:#3989