Skip to content
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

upgrade-test: add 2 upgrades test #4806

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Conversation

HuijingHei
Copy link
Member

@HuijingHei HuijingHei commented Jan 29, 2024

Enhance 2 upgrade tests:

  • kolainst: Do client-side layering tests on old build, then upgrade
    rpm-ostree to new version, then upgrade to another new update.
  • prow/e2e-upgrades: Start old build with upgraded rpm-ostree,
    do layering tests, then upgrade to another new update.

Fixes: #4776

@HuijingHei HuijingHei changed the title upgrade-test: do basic tests on old builds before upgrade rpm-ostree to new version, then upgrade to another new update upgrade-test: do basic tests on old builds before upgrade Jan 29, 2024
@HuijingHei HuijingHei force-pushed the upgrades branch 2 times, most recently from dc6b0a1 to 97657b0 Compare January 30, 2024 01:24
@HuijingHei
Copy link
Member Author

HuijingHei commented Jan 30, 2024

The basic tests include override kernel, kernel args, initramfs-etc, remove package, overlay, and the whole test took almost 30 minutes, seems too long, maybe should skip some reboots?

[2024-01-30T02:45:29.909Z] === RUN   ext.rpm-ostree.upgrades.misc-tests
[2024-01-30T03:15:37.825Z] --- PASS: ext.rpm-ostree.upgrades.misc-tests (1797.84s)

@HuijingHei HuijingHei force-pushed the upgrades branch 2 times, most recently from d9ed6f7 to 07f72de Compare January 30, 2024 10:27
tests/common/libtest.sh Fixed Show fixed Hide fixed
tests/common/libtest.sh Fixed Show fixed Hide fixed
tests/common/libtest.sh Fixed Show fixed Hide fixed
tests/kolainst/upgrades/client-layering Fixed Show fixed Hide fixed
tests/kolainst/upgrades/client-layering Fixed Show fixed Hide fixed
tests/kolainst/upgrades/upgrade-rpmostree Fixed Show fixed Hide fixed
tests/kolainst/upgrades/upgrade-rpmostree Fixed Show fixed Hide fixed
@HuijingHei HuijingHei changed the title upgrade-test: do basic tests on old builds before upgrade upgrade-test: add 2 upgrades test Jan 30, 2024
@HuijingHei HuijingHei force-pushed the upgrades branch 2 times, most recently from 885cab9 to 0586cc0 Compare January 30, 2024 10:47
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great! This kind of testing was sorely needed.

.cci.jenkinsfile Outdated
make -C tests/kolainst upgrades-install
mv rpm-ostree-[0-9]*.rpm rpm-ostree-libs-[0-9]*.rpm /usr/lib/coreos-assembler/tests/kola/rpm-ostree/upgrades/data/
""")
kola(cosaDir: "${env.WORKSPACE}", extraArgs: "--qemu-image ./fedora-coreos*.qcow2 ext.rpm-ostree.upgrades.*", parallel: 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more of having the OS rollback to an earlier version on its first boot, but yeah this is more proper even if more wordy. :) (Kola also has support for downloading the previous image, but as part of the run-upgrade suite; I think we should really fold that back into kola run proper and support external tests.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Kola also has support for downloading the previous image, but as part of the run-upgrade suite; I think we should really fold that back into kola run proper and support external tests.)

Good suggestion, then this will make upgrade tests more easier, will change that later.

@@ -22,3 +22,9 @@ install: all
localinstall: all
rm -rf ../kola
make install KOLA_TESTDIR=../kola

upgrades-install: all
Copy link
Member

@jlebon jlebon Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, we could make it part of the destructive suite, but add a e.g. requiredTag: rpm-ostree-upgrade to it so that it doesn't run in the default testsuite run, and will only run if you pass --tag rpm-ostree-upgrade.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, thanks!

.cci.jenkinsfile Outdated Show resolved Hide resolved
## minMemory: 2048
## tags: needs-internet
## description: Start old build, upgrade rpm-ostree to new version,
## do client-side layering tests, then upgrade to another new update.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, can you expand on #4784 (comment) ? Isn't fedora-silverblue/issue-tracker#523 covered by your first test where we do an override remove on the old version and then upgrade to the new version and then upgrade again from the new version to another version (with the same cached RPM content)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant to add there like that, but then I thought if we add such tests in kolainst that can cover more tests instead of just remove, no need to change in e2e-upgrades.

But agree with you that it would be better to add the tests to e2e-upgrades instead of kolainst.

tests/kolainst/upgrades/client-layering Outdated Show resolved Hide resolved
@HuijingHei
Copy link
Member Author

/test kola-upgrade

@HuijingHei
Copy link
Member Author

HuijingHei commented Jan 31, 2024

Seems + cosa kola run 'ext.rpm-ostree.*' triggers test ext.rpm-ostree.destructive.client-layering-upgrade that has requiredTag: rpm-ostree-upgrade, will do more research.

@jlebon
Copy link
Member

jlebon commented Jan 31, 2024

OK yeah, I think I understand better now. Basically we want to test new rpm-ostree on a system composed by old rpm-ostree. There's a subtle difference from the scenario in fedora-silverblue/issue-tracker#523 though: the override remove was done with the old rpm-ostree first and then upgraded to a compose with the new rpm-ostree (but that's still composed by old rpm-ostree).

I don't think it matters for catching #4746, but does for #4727 since the pkgcache was generated by old rpm-ostree in one case but new rpm-ostree in the other. Do you agree?

I think it does make sense to test both cases, which is what you're doing with the Prow version and the kolainst version of these tests. But man... this stuff is complex. Maybe let's add a link to this PR (or maybe even this comment directly) in the tests?

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, but LGTM overall! Thank you so much for working on this.

tests/kolainst/destructive/client-layering-upgrade Outdated Show resolved Hide resolved
tests/kolainst/destructive/client-layering-upgrade Outdated Show resolved Hide resolved
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Jan 31, 2024
Before, even though a test had a required tag, explicitly passing a
pattern which matched that test name would still include it. I thought
that would be convenient, but it makes the logic less predictable. Since
we don't use many required tags today, let's instead opt for being more
explicit and always actually require the tag.

Motivated by coreos/rpm-ostree#4806 (comment).
@jlebon
Copy link
Member

jlebon commented Jan 31, 2024

Seems + cosa kola run 'ext.rpm-ostree.*' triggers test ext.rpm-ostree.destructive.client-layering-upgrade that has requiredTag: rpm-ostree-upgrade, will do more research.

Should be fixed by coreos/coreos-assembler#3710!

jlebon added a commit to coreos/coreos-assembler that referenced this pull request Jan 31, 2024
Before, even though a test had a required tag, explicitly passing a
pattern which matched that test name would still include it. I thought
that would be convenient, but it makes the logic less predictable. Since
we don't use many required tags today, let's instead opt for being more
explicit and always actually require the tag.

Motivated by coreos/rpm-ostree#4806 (comment).
@HuijingHei
Copy link
Member Author

HuijingHei commented Feb 1, 2024

I don't think it matters for catching #4746, but does for #4727 since the pkgcache was generated by old rpm-ostree in one case but new rpm-ostree in the other. Do you agree?

Yes, yes, you are totally correct.
For some reason, we build fcos-rawhide with coreos-assembler (using f39 for example) which has old rpm-ostree, but the built image include the new release rpm-ostree, likes upgrade rpm-ostree on old (previous) build. Is this the same as prow test that sync new rpm-ostree to old quay.io/fedora/fedora-coreos:testing-devel (refer to https://github.com/coreos/rpm-ostree/blob/main/ci/prow/Dockerfile.fcos#L10) ?

So we should have tests:

  • kolainst: Do client-side layering tests on old build, then upgrade
    rpm-ostree to new version, then upgrade to another new update.
  • prow/e2e-upgrades: Start old build with upgraded rpm-ostree,
    do layering tests, then upgrade to another new update.

Is my understanding is correct?

- `kolainst`: Do client-side layering tests on old build, then
upgrade rpm-ostree to new version, then upgrade to another update.
- `prow/e2e-upgrades`: Start old build with upgraded rpm-ostree,
add layering tests, then upgrade to another new update.
@jmarrero jmarrero merged commit aa4caed into coreos:main Feb 1, 2024
17 checks passed
coreos-installer download -p qemu -f qcow2.xz -s testing --decompress
mv rpm-ostree-[0-9]*.rpm rpm-ostree-libs-[0-9]*.rpm /usr/lib/coreos-assembler/tests/kola/rpm-ostree/destructive/data/
""")
kola(cosaDir: "${env.WORKSPACE}", extraArgs: "--qemu-image ./fedora-coreos*.qcow2 --tag rpm-ostree-upgrade ext.rpm-ostree.destructive.client-layering-upgrade")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, this could now just be

    kola(cosaDir: "${env.WORKSPACE}", extraArgs: "--qemu-image ./fedora-coreos*.qcow2 --tag rpm-ostree-upgrade")

since it's the only test with that tag.

Also, we probably want skipUpgrade: true here.

And finally, it doesn't matter much, though re. --qemu-image ....qcow2, that would belong better as a platformArgs argument. (It would matter if we e.g. didn't disable the default upgrade tests.)


/tmp/autopkgtest-reboot 2
;;
"2")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should verify here that the booted ref is kola to make sure e.g. stage finalization didn't fail.

@HuijingHei HuijingHei deleted the upgrades branch February 2, 2024 02:27
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Feb 2, 2024
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci: verify that no regression on older builds after updating rpm-ostree
3 participants