-
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
Add a kola
verb
#85
Add a kola
verb
#85
Conversation
Now blocked on coreos/mantle#918 |
Lifting WIP on this, but we have a dependency in coreos/mantle#920 at least...there's general nontrivial work to teach kola about the 3 distributions. |
coreos/mantle#922 has landed (I had forgotten to actually merge it) |
should we wait on the discussion in #163 since i think it has implications? poor PR :( |
So now with #524, let's (1) switch this to run the unpriv tests, (2) get it in, and (3) update coreos/fedora-coreos-pipeline#31 to leverage it? |
I'm not sure if that discussion necessarily blocks this patch though, right? We can assume we have |
I think at some point during that discussion it was up in the air whether we wanted it to be a completely separate container or not. I think at this point it is safe to assume regardless of how we build it we will include it in |
src/cmd-kola
Outdated
fatal "No latest build" | ||
fi | ||
|
||
exec sudo kola -b rhcos --tapfile test.tap --qemu-image ${latest_qcow} "$@" |
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.
is -b rhcos
important here? is there a -b fcos
?
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.
Distribution flags are important. That said, we should make it a flag on the command and just have it default to one of fcos/rhcos
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.
I'd vote for changing kola to look for rhcos-
or fcos-
as a prefix in the image name for now; that same thing would also be useful for ignition spec version handling.
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.
Prefix based detection WFM.
And yes, when --ignition-version v2
will need to be specified when running RHCOS until spec 3.0.0 lands there. For FCOS we don't need to specify that parameter as it defaults to v3 on that distribution.
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.
Would probably be good to have command-line overrides as well (at least on Ignition version) so that it would be possible to test Ignition spec 3.0.0 in RHCOS without having to modify cmd-run
.
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.
I added the auto-detect logic in the command itself for now with a XXX
to upstream it into kola.
17a0dec
to
859be8c
Compare
OK, I rebased this now!
Let's get this in? |
(Though I actually didn't test it on RHCOS yet.) |
d9a8b27
to
3577116
Compare
This looks sane to me! |
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.
Generally LGTM, couple questions.
src/cmd-kola
Outdated
set -x | ||
|
||
# shellcheck disable=SC2086 | ||
exec sudo kola $distro --output-dir tmp/kola --tapfile test.tap --qemu-image "${latest_qcow}" "$@" |
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 don't have anything keying on the tapfile in this command, maybe drop it and let things specify it directly if they care about it?
Might be worthwhile to default to some extra parallel threads (via --parallel <num>
).
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, dropped the --tapfile
bit!
Might be worthwhile to default to some extra parallel threads (via
--parallel <num>
).
I left this off for now. It's tricky to get right to match all possible environments. See e.g. get_libvirt_smp_arg
in virt-install
. Hmm, though I guess I could split that out into a separate Python script and here shell out to it?
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.
Probably fair to leave out parallel
for now, it can be specified on the command-line and has the extra trickery around it as you mentioned.
|
This verb doesn't specify the platform (which defaults to |
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.
need to make cmd-kola
executable.. otherwise you get: Unknown command: kola
Huh, I'm not sure how I didn't hit that... anyway, fixed! |
latest=$(get_latest_build) | ||
if [ -n "$latest" ]; then | ||
# shellcheck disable=SC2086 | ||
ls builds/${latest}/*-qemu.qcow2 |
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.
not anything we need to do here but FYI if you have run cosa compress
this will fail.
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
if only shellcheck were happy |
This is the first place we're wrapping mantle via our main entrypoint, and hopefully we'll increase that. Example usage: `coreos-assembler kola run rhcos.basic`. Requires: coreos/mantle#920
OK right, this code predates the addition of ShellCheck. Should hopefully be happy now! |
haha - i just thought about it |
Blocked by coreos/fedora-coreos-config#2