-
Notifications
You must be signed in to change notification settings - Fork 170
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
exttests: New container image with upstream tests #1745
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
FROM registry.svc.ci.openshift.org/coreos/cosa-buildroot:latest as builder | ||
COPY build.sh . | ||
RUN ./build.sh | ||
|
||
FROM quay.io/coreos-assembler/coreos-assembler:latest | ||
COPY --from=builder /usr/lib/coreos-assembler/tests/kola/ /usr/lib/coreos-assembler/tests/kola/ |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# POC for gathering upstream tests into a container | ||
|
||
We landed support for "exttests" in coreos-assembler: | ||
https://github.com/coreos/coreos-assembler/blob/98d40e6bb13adc02bcd5f02f1d5bff7bafa0780d/mantle/kola/README-kola-ext.md | ||
|
||
Since then we are successfully running upstream test suites using | ||
kola on upstream pull requests. | ||
|
||
But we also want the inverse: run upstream tests on builds outside | ||
of PRs to those repos. | ||
|
||
For example, I really want to run ostree's "transactionality" | ||
test on FCOS builds so that if a kernel change breaks it, we | ||
know that. | ||
|
||
## Proposal | ||
|
||
- Build this container in api.ci like we're building the cosa buildroot; | ||
currently done at `registry.svc.ci.openshift.org/cgwalters/cosa-exttests:latest` | ||
- Change the FCOS/RHCOS pipelines to pull this container and do: `kola run ... ext.*` | ||
(This raises some issues like the fact that we'd need to store/share the images | ||
in a way that would allow a secondary container to access them) | ||
|
||
### Alternative: coreos-assembler only | ||
|
||
Fold this into coreos-assembler. | ||
|
||
### Alternative: Make RPMs of tests and install those in coreos-assembler | ||
|
||
We have an `ostree-tests` RPM...but I'm not sure I want to deal with | ||
packaging for it. | ||
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
#!/bin/bash | ||
set -xeuo pipefail | ||
declare -A repos | ||
# Pin versions for now for reproducibility | ||
repos[ostreedev/ostree]=v2020.6 | ||
repos[coreos/rpm-ostree]=v2020.5 | ||
for repo in "${!repos[@]}"; do | ||
bn=$(basename ${repo}) | ||
git clone --depth=1 -b ${repos[${repo}]} https://github.com/${repo} ${bn} | ||
# Our "standard" for ext-installed-tests | ||
cd ${bn}/tests/kolainst | ||
make -j 4 | ||
make install | ||
cd - | ||
rm ${bn} -rf | ||
done |
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.
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.
Will throw out my proposal too here.
I think we should try to address the binding problem with this the same way we did with
tests/kola
in f-c-c. IOW, the test version should match the app version. And since lockfiles live in f-c-c, this is what dictates what tests to use.WDYT about something like this:
cosa buildextend-exttests
which read this YAML file, and for each repo/app, have it check the NEVRA in the build, clone the repo at that tag, build the exttests, and bundle them in a tarball artifact that becomes part ofmeta.json
cosa buildextend-exttests
; this artifact can then be used by any system which wants to run the tests against the FCOS build (obviously the QEMUkola run
invocation, but also e.g. AWS/GCP/$cloud
tests or whatever else wants to validate the build)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.
To clarify, this is fine to start as is too; had one comment about the code. From your commit message, I think we're aligned high-level on how we should change the approach here in the future.
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.
A whole lot going on there. Trying to wrap my head around it. ⏳ time passes ⏳
OK I have done so and I mostly like it.
I think a big negative of this is that building the tests becomes a critical path thing done synchronously, whereas I would really like to avoid hard blocking on test build failures. For example, today ostree's exttests just use crates.io to build, but as is this proposal would mean we couldn't ship FCOS if crates.io is down.
Ultimately I think as a general rule we should at least support running old tests against new code. So the FCOS pipeline could use the previous tests to start (perhaps a subset of them) and we can optionally choose whether or not to block on the new tests being built. Also an important optimization here I think is avoiding rebuilding tests if they didn't change - we'll need to debate how that works.
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.
Fixed the tag fetching. I think I'd like to propose trying this (this="exttests container") out and see how things go, and we can always revisit for a second phase with something like "exttests artifact" (your proposal).
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.
Though a definite downside of this is it requires scheduling a new pod, which will force us to either use the exttests container for the build too, or go to multi-phase (which we know we need anyways though).