kola: Add a platform-independent tag
#3059
Conversation
|
I'm not opposed to having a tag that signifies "platform independent", but I think it needs some more thought and I'm interested in input from @jlebon too. I think I'd prefer to not do the second part where we tie it back to qemu at the same time but rather modify our pipeline code to call kola with some sort of option that tells it to not run platform-independent tests on platforms where we don't want those to run. We could probably re-use the existing "tag" mechanism for this rather than creating a new thing in the json directly. |
|
Thinking on this a bit more, maybe we do keep the current code here that ties it to qemu still and just add a new toplevel flag for force running "platform independent" tests on non-qemu platforms. Either way still want to get input from Jonathan when he gets back. |
I'm leaning toward this one. This keeps current pipeline code as-is and we can add the flag later. |
cgwalters
force-pushed
the
test-platindep
branch
from
e5352ea
to
dadd465
Compare
So in fedora-coreos-config we grew this "standard" for documenting the tags for tests: https://github.com/coreos/fedora-coreos-config/blob/50ee01a89075d8ef204f1f40c5d89ab060748aac/README.md#tests-layout And we've semi-standardized on the idiom `platforms: qemu` with a manual documentation flag. But just look at the level of duplication: ``` $ git grep 'This test should pass everywhere' tests/kola/binfmt/qemu:# - This test should pass everywhere if it passes anywhere. tests/kola/clhm/ignition-warnings/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/content-origins/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/extensions/module:# - This test should pass everywhere if it passes anywhere. tests/kola/extensions/package:# - This test should pass everywhere if it passes anywhere. tests/kola/files/validate-symlinks:# - This test should pass everywhere if it passes anywhere. tests/kola/networking/force-persist-ip/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/networking/hostname/fallback-hostname/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/networking/kargs-rd-net:# - This test should pass everywhere if it passes anywhere. tests/kola/networking/mtu-on-bond-ignition/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/networking/mtu-on-bond-kargs:# - This test should pass everywhere if it passes anywhere. tests/kola/networking/no-default-initramfs-net-propagation/bootif:# - This test should pass everywhere if it passes anywhere. tests/kola/networking/prefer-ignition-networking/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/podman/dns/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/root-reprovision/filesystem-only/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/root-reprovision/linear/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/root-reprovision/luks/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/root-reprovision/raid1/test.sh:# - This test should pass everywhere if it passes anywhere. tests/kola/root-reprovision/swap-before-root/test.sh:# - This test should pass everywhere if it passes anywhere. $ ``` Instead, we can entirely avoid the need for copy-pasting lots of comments with a *self-describing* tag that's documented here - *once*. Further, as the code here mentions, I think what we really want to do is e.g. *sometimes* (at least) run tests that claim they're platform independent *not* on qemu to verify that's really true. With a semantic tag, we can clearly distinguish these tests from tests that *actually* only run on qemu.
cgwalters
force-pushed
the
test-platindep
branch
from
dadd465
to
302130e
Compare
|
OK I added a config argument. That said I think what we really want is more like #3008 |
|
Hmm yeah we definitely seem to have some new firstboot provisioning timeout in RHCOS 😢 |
You think this PR would need to block on that? |
Here's a proposal: Merge these changes (after review) relatively soon - we don't block on that one person. But we also don't do a mass-conversion of the test suite. I'll only change one or two as a demo, and any new tests can use these new changes. We can then give it a bit of time (possibly his full PTO if you feel strongly) before we try to make any big scripted changes. Ultimately remember, anything that's done here can be changed relatively easily; we don't have to consider it set in stone. |
| if !ForceRunPlatformIndependent { | ||
| for _, tag := range t.Tags { | ||
| if tag == PlatformIndependentTag { | ||
| t.Platforms = []string{defaultPlatformIndependentPlatform} | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This feels a bit awkward to me.
Feels like the logic should be more:
if this is a platform independent test
and
(we are targeting the default platform for independent tests
OR ForceRunPlatformIndependent)
then we run the test.
What this is currently doing is saying if this is an independent platform test then we set the platforms this test will run on to qemu. I get that it's functionally equivalent at the end of the day. It just feels awkward to me.
I don't know if it can be improved. Just rambling mostly.
There was a problem hiding this comment.
The reason I did this is that we can reuse the logic for checking test platforms just below. I started typing it out the way you suggest and I think it would be more readable except we have this technical debt left over around
checkPlatforms := []string{pltfrm}
// qemu-unpriv has the same restrictions as QEMU but might also want additional restrictions due to the lack of a Local cluster
if pltfrm == "qemu-unpriv" {
checkPlatforms = append(checkPlatforms, "qemu")
}
where we want to alias "qemu" and "qemu-unpriv" and...dunno. I could try to untangle that but this is definitely less code.
There was a problem hiding this comment.
I definitely want to get rid of any use of qemu-unpriv in the future and just have everything be qemu.
That's fine. |
So in fedora-coreos-config we grew this "standard" for documenting
the tags for tests:
https://github.com/coreos/fedora-coreos-config/blob/50ee01a89075d8ef204f1f40c5d89ab060748aac/README.md#tests-layout
And we've semi-standardized on the idiom
platforms: qemuwitha manual documentation flag. But just look at the level
of duplication:
Instead, we can entirely avoid the need for copy-pasting lots of
comments with a self-describing tag that's documented here - once.
Further, as the code here mentions, I think what we really want to
do is e.g. sometimes (at least) run tests that claim they're
platform independent not on qemu to verify that's really true.
With a semantic tag, we can clearly distinguish these tests
from tests that actually only run on qemu.