-
Notifications
You must be signed in to change notification settings - Fork 998
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
ci: update interop workflow #3331
Conversation
861d637
to
c7a2f6c
Compare
libp2p/github-mgmt#105 is merged. Let me know once this is ready for review. |
c7a2f6c
to
1bead15
Compare
- name: Upload image tar | ||
uses: actions/upload-artifact@v3 | ||
with: | ||
name: ping-image | ||
path: ./test-plans/ping-image.tar |
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.
Now that we are using plain docker compose, can't we just build the image as part of the workflow here, upload it to GitHub's container registry and reference it here? That seems a lot simpler than storing it as an artifact.
cc @MarcoPolo
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.
In more detail, why don't we have the following:
- A workflow per repository that only runs on pushes to master that builds a docker container and uploads it to the container registry under the tag "master".
- A workflow per repository that only runs on tag pushes and builds a docker container and uploads it to the container registry under the Git tag, i.e.
0.45.0
- A PR workflow that builds a container of the current commit, uploads it to the container registry under the tag of the PR number.
Once (3) finishes, we can then kick off a dependent workflow that runs a matrix of containers against each other.
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.
You can upload to a container registry if you want. If the containerImageID is a tag, then docker will automatically try to fetch that tag. You don't have to upload image artifacts if you don't want to.
That said, I think the approach of building the container from the commit is better. For one it's guaranteed to be reproducible. You don't have multiple commits from the same PR mutating the tag number. Two, it's the same workflow on master, PRs, or tags. Three, when you introduce a container registry, now you need to deal with authentication, limits, and garbage collecting from that container registry.
The one benefit I see to container registries is them acting as a cache for faster tests. But we can cache layers already using GitHub Actions cache.
I'm not sure the extra work involved in setting up and maintaining a registry is worth it. But of course, if you want to feel free. I'm just sharing my thoughts from thinking about this as well.
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 for sharing your thoughts. Perhaps a mix of both would be the best solution?
- The
master
tag would only ever produce one container so no GC needed, i.e. the container for master would be updated every time the master branch gets updated. That hurts reproducibility a bit but it is not a big deal IMO. We already accept that merging one PR affects other PRs. - The containers for releases are useful for other implementations too. I don't think we can utilize the GH Actions Cache here? i.e.
go-libp2p
testing against releases ofrust-libp2p
- Not uploading the container of the current commit/PR is a good idea, those would just be garbage we need to collect.
If we can remove container builds of master and tags from this workflow, then the Dockerfile also gets simpler because we always just build for the current source, no overrides needed.
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'm not sure the extra work involved in setting up and maintaining a registry is worth it. But of course, if you want to feel free. I'm just sharing my thoughts from thinking about this as well.
GitHub makes authenticating and interacting with its own registry really easy so that wouldn't be much work in my experience.
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 think the ability to checkout an arbitrary commit of test-plans and be able to run the exact those exact tests is a very good feature (reproduicibility). As long as we hold that, I don’t mind using tagged images in a registry.
But what is the benefit of tagging images in a registry? To me, the only benefit is faster builds.
The benefit is faster builds but it also just feels wrong. The source code for a test for e.g. libp2p 0.45 should never change. The version is out there, our users are using it. If we want to take interoperability seriously, I think it is good to constrain ourselves such that changing the test for a particular released version is hard.
Building the binary every time doesn't sit well for me on that front.
Maybe standard GHA cache isn’t good enough (since it caches per branch I believe), but we could cache the layers from the latest test-plans master builds as well, and I think that would cover all the use cases of tagged images.
It caches per user-defined key which can be completely arbitrary and is not restricted to branches or anything. Bear in mind that each repo only has 10GB here and at least in rust-libp2p
we already have to jump through hoops to not blow this cache limit for our regular jobs :(
The master tag would only ever produce one container so no GC needed, i.e. the container for master would be updated every time the master branch gets updated.
I don’t see the benefit here since you are building this every time. When do you get to reuse this tag?
It gets reused across all pushes on all open PRs until the next one merges. I myself have usually anywhere from 2-10 PRs open that I work on in parallel i.e. I switch to another PR once I incorporated feedback and wait for the next round of reviews.
If we can remove container builds of master and tags from this workflow, then the Dockerfile also gets simpler because we always just build for the current source, no overrides needed.
What overrides are used? I imagine you are just building from the current source.
My bad, I wrongly assumed that we would still need the "sed"-ing inside the Dockerfile. That should be gone now in any case.
One idea for released versions is that inside the test-plans repo we specify the way to build the interop image is to checkout this repo at this commit, then cd into test-plans and run
make
.
This exact step could have happened when the tag was pushed. It is like pre-built binaries of any other application here on GitHub.
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. I think we're starting to understand each other's points and we agree on the fundamentals.
Just a couple of questions for me:
It gets reused across all pushes on all open PRs until the next one merges.
Maybe there's a misunderstanding somewhere? An open rust-libp2p PR wouldn't do an interop test against rust-libp2p master (we could do that, I'm just not sure it's useful). It runs interop tests against every other released version of libp2p.
The source code for a test for e.g. libp2p 0.45 should never change. The version is out there, our users are using it. If we want to take interoperability seriously, I think it is good to constrain ourselves such that changing the test for a particular released version is hard.
I agree with this. I don't think building from source prevents this. If we constrain ourselves to never mutate an image tag, then I think we agree except on the little details. To me a sha256 hash is by definition an immutable tag of an image.
I don't have strong opinions on how rust-libp2p wants to handle building their image. If the project wants to simply submit a container tag that will be used in the interop tests, I only ask that the tag is immutable.
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.
It gets reused across all pushes on all open PRs until the next one merges.
Maybe there's a misunderstanding somewhere? An open rust-libp2p PR wouldn't do an interop test against rust-libp2p master (we could do that, I'm just not sure it's useful). It runs interop tests against every other released version of libp2p.
Interesting, I thought we also ran interop tests against our own master branch. @mxinden do I remember this incorrectly?
If not then I agree that it is not useful to have a moving master
tag.
The source code for a test for e.g. libp2p 0.45 should never change. The version is out there, our users are using it. If we want to take interoperability seriously, I think it is good to constrain ourselves such that changing the test for a particular released version is hard.
I agree with this. I don't think building from source prevents this. If we constrain ourselves to never mutate an image tag, then I think we agree except on the little details. To me a sha256 hash is by definition an immutable tag of an image.
I don't have strong opinions on how rust-libp2p wants to handle building their image. If the project wants to simply submit a container tag that will be used in the interop tests, I only ask that the tag is immutable.
The tag should definitely be immutable. As far as I know, one can append the sha256 hash of a docker image tag to the overall ID and thus guarantee that anyone pulling this image from the registry will get exactly the image with this hash. That is how I would specify those images in test-plans.
Example:
docker.io/library/hello-world@sha256:faa03e786c97f07ef34423fccceeec2398ec8a5759259f94d99078f264e9d7a
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.
It gets reused across all pushes on all open PRs until the next one merges.
Maybe there's a misunderstanding somewhere? An open rust-libp2p PR wouldn't do an interop test against rust-libp2p master (we could do that, I'm just not sure it's useful). It runs interop tests against every other released version of libp2p.
Interesting, I thought we also ran interop tests against our own master branch. @mxinden do I remember this incorrectly?
If I recall correctly, we did. I was planning to have each pull request tested against master
with the wo-testground tests, though based on intuition, without specific reasoning. Thinking about it some more, I don't see what kind of bugs this would catch, thus I suggest not investing into PR-to-master testing for now. Thoughts?
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.
But what is the benefit of tagging images in a registry?
Another benefit would be for someone to easily test their libp2p-xxx against libp2p-yyy v1.2.3 locally without having checkout a repository, build a Docker image or install libp2p-yyy toolchain, simply pulling and running a pre-build image from a public registry. Just mentioning it here. I am fine with either and fine with Go doing ad-hoc builds and Rust pushing Docker images.
2eafb06
to
3b06f0e
Compare
- update the ping testcase to follow the ones on the test-plans repo. - update project to use parent rust-libp2p intead of fetching it from git.
3b06f0e
to
d7e84bc
Compare
test-plans/Dockerfile
Outdated
WORKDIR /usr/src/libp2p | ||
|
||
# TODO fix this, it breaks reproducibility | ||
RUN apt-get update && apt-get install -y cmake protobuf-compiler |
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.
Do we still need the protobuf compiler? I thought we fixed this by inlining generated content?
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.
That isn't merged yet :)
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.
For completeness, this is tracked in #3024.
test-plans/Makefile
Outdated
@@ -0,0 +1,10 @@ | |||
all: ping-image.tar | |||
|
|||
ping-image.tar: Dockerfile Cargo.toml src |
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.
Does this properly depend on all files in src? I thought you needed wildcard
, but not sure.
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.
Yeah good point Marco, it seems to only depend on the directory creation/modification itself. Updated to use wildcard
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.
Some more thoughts!
This is looking really good already :)
run-multidim-interop: | ||
name: Run multidimensional interoperability tests | ||
needs: build-ping-container | ||
uses: "libp2p/test-plans/.github/workflows/run-testplans.yml@master" |
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 think we could save some complexity here if this were just a composite action that we call after building the images rather than a separate job.
That would allow the action within test-plans
to access local state like built docker images, configuration files without us having to up and download artifacts.
@MarcoPolo What was the reasoning for making this a workflow rather than a composite action?
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.
For example, within the same job, we wouldn't need to tar up the container because it would still be accessible by tag.
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.
Good point! I didn't realize I could make a composite action. My knowledge here was out of date. Thanks!
I'll try this out soon. I agree it'll be nice not to have to upload the image. And the upload artifacts are always directories, so it'll be nice to avoid that.
custom_git_reference: ${{ github.event.pull_request.head.sha || github.sha }} | ||
custom_interop_target: rust | ||
dir: "multidim-interop" | ||
extra-versions: ping-versions |
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.
What does extra-versions
here mean? @MarcoPolo
We seem to be referencing the same image in there that we are also tarring up.
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.
The source of truth for the capabilities of a released libp2p version are defined in version.ts
. The extra versions is more version data that is added to that versions.ts
array. This lets you add a "rust-libp2p-head" version to the defined versions and run an interop test between all defined versions on all capabilites. This combined with the ability to filter tests lets you run the interop tests that only include "rust-libp2p-head".
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 for explaining, this makes sense now.
Is there a usecase where we extend versions.ts
with more than one image and/or don't filter by the one we just added? It looks like the configuration API of this action could be simpler, i.e. it just needs the contents of one entry in versions.ts
and we always want to filter the tests for this one version.
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.
In particular, this seems to be sufficient and perhaps a bit easier to understand:
run-multidim-interop:
name: Run multidimensional interoperability tests
needs: build-ping-container
uses: "libp2p/test-plans/.github/workflows/run-testplans.yml@master"
with:
imageId: "rust-libp2p-head"
transports: ["ws", "tcp", "quic-v1", "webrtc"]
secureChannels: ["tls", "noise"]
muxers: ["mplex", "yamux"]
- The name of the version can be implied from the image ID (by convention we should make this a tag and not just a hash).
- With the composite action suggested in https://github.com/libp2p/rust-libp2p/pull/3331/files#r1073550520, we don't need to pass the artifact name of the tar'd image.
- By directly specifying the transports, secure channels and muxers, we can avoid a configuration file in our repository.
- Not sure I understand why
dir: "multidim-interop"
needs to be specified, can't this be implied by the GitHub action?
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 sure I understand why dir: "multidim-interop" needs to be specified, can't this be implied by the GitHub action?
This should be changed. Originally the "run test-plans" was generic enough that it would work for any npm test
like thing. But there's little value for this, and it would be nicer to be clearer here.
By directly specifying the transports, secure channels and muxers, we can avoid a configuration file in our repository.
It's true. With the tradeoff of dealing not being able to run this locally as easily. I think it's nice to be able to locally run: npm test -- --extra-versions=$RUST_LIBP2P/test-plans/version.json --name-filter="rust-libp2p-head"
. Inputs are also only strings, so you'd have to stringify the given example.
I agree with all the other points :)
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.
By directly specifying the transports, secure channels and muxers, we can avoid a configuration file in our repository.
It's true. With the tradeoff of dealing not being able to run this locally as easily. I think it's nice to be able to locally run:
npm test -- --extra-versions=$RUST_LIBP2P/test-plans/version.json --name-filter="rust-libp2p-head"
. Inputs are also only strings, so you'd have to stringify the given example.
How often do we need that? At the moment, this config file just sits there, not co-located with its consumer and it requires a fair amount of knowledge to figure out what it is for.
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.
It's true. With the tradeoff of dealing not being able to run this locally as easily. I think it's nice to be able to locally run:
npm test -- --extra-versions=$RUST_LIBP2P/test-plans/version.json --name-filter="rust-libp2p-head"
. Inputs are also only strings, so you'd have to stringify the given example.
Would it make sense to unify the API of the test runner to just accept a series of JSON files with one version each? i.e. it builds the matrix of tests out of all files that are passed to it. Each JSON file would contain exactly one entry.
We could then pass the JSON as a string into the workflow and it writes it to a file. If you really need to run things locally, you just have to quickly copy paste that and make the file.
To me, that seems like an acceptable trade-off for having one less configuration file around per repository :)
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 it make sense to unify the API of the test runner to just accept a series of JSON files with one version each? i.e. it builds the matrix of tests out of all files that are passed to it. Each JSON file would contain exactly one entry.
Yeah, this seems reasonable 👍 .
We could then pass the JSON as a string into the workflow and it writes it to a file. If you really need to run things locally, you just have to quickly copy paste that and make the file.
The less logic that is in the github actions yaml the better. At least for me, I find this stuff hard to work with since it only runs in CI. The simpler the yaml file is the better.
I don’t see why having this configuration file is bad. I actually think it’s a good thing because:
- It defines what the capabilities of the current rust-libp2p are.
- If a new capability is added (e.g. webtransport), then you modify this json file, not a stringified version in a github action yaml file.
- When a release is made, you simply have to copy this JSON file to
test-plans/multidim-interop/version.ts
so other implementions test against 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.
Would it make sense to unify the API of the test runner to just accept a series of JSON files with one version each? i.e. it builds the matrix of tests out of all files that are passed to it. Each JSON file would contain exactly one entry.
Yeah, this seems reasonable 👍 .
Nice, I'll open an issue on the repo.
I don’t see why having this configuration file is bad. I actually think it’s a good thing because:
- It defines what the capabilities of the current rust-libp2p are.
- If a new capability is added (e.g. webtransport), then you modify this json file, not a stringified version in a github action yaml file.
- When a release is made, you simply have to copy this JSON file to
test-plans/multidim-interop/version.ts
so other implementions test against it.
You are mentioning some good points. Happy to keep the file, thanks for getting to the bottom of this :)
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 for the work here @jxs! Direction looks good to me. Still many outstanding comments. I suggest let's try to delay some of the work to follow-up pull requests to unblock other pull requests on libp2p/rust-libp2p.
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.
Great stuff to see this working again, thank you @MarcoPolo and @jxs !
Follow-ups I'd like to see (happy to take some of these over):
- Use a composite action instead of a workflow: ci: update interop workflow #3331 (comment). We should be able to add this backwards-compatible so repositories can transition without breaking CI.
- Pre-build images for tagged versions.
- Optimise building of the Dockerfile in general to be as quick as possible. Copying the binary would be my suggestion but I am open to other solutions.
- Ideally pre-build image for
master
branch. If we have a good caching strategy for building the Dockerfile, it may not be worth prebuilding the image for master. Edit: This may be moot, see ci: update interop workflow #3331 (comment).
and remove the workspace entry on itself.
2a574d0
to
20a8032
Compare
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.
Few more thoughts :)
440e326
to
a8dab47
Compare
- build test-plans outside of the container - then move the binary to the container
a8dab47
to
c06369f
Compare
(sorry for having force pushed @thomaseizinger , didn't know you were already reviewing it 🙏 ) |
to not interfere with published crates tests.
7956a7d
to
ac2162c
Compare
instead of changing it as then we need to change the caching dir, and add it to the dockerignore because of context.
and update Makefile
44f348c
to
8ce709b
Compare
|
||
This folder defines the implementation for the test-plans interop tests. | ||
|
||
# Running this test locally |
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.
🚀 worked like a charm.
Great work @jxs @MarcoPolo @thomaseizinger. Thank you! Please add the send-it label @jxs once you want to merge. Unless I am missing something, the below is the (potential) outstanding follow-up work.
Potential format change to |
Adding the below to the list of follow-up tasks:
|
libp2p/github-mgmt#107 for reference |
- name: Install Protoc | ||
run: sudo apt-get install protobuf-compiler | ||
- name: Build image | ||
working-directory: ./test-plans |
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.
Sometimes we call things test-plans, sometimes we call them interop-tests.
I'd be in favor of calling all of this interop-tests
. test-plans
is too generic IMO and doesn't tell me anything about what they are testing.
log = "0.4" | ||
redis = { version = "0.22.1", features = ["tokio-native-tls-comp", "tokio-comp"] } | ||
tokio = { version = "1.24.1", features = ["full"] } | ||
|
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.
Any reason for this newline?
@@ -0,0 +1,14 @@ | |||
all: ping-image.tar | |||
|
|||
ping-image.tar: Dockerfile Cargo.toml src/bin/ping.rs |
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.
Technically, this also needs to rebuild on every change in libp2p but I am not sure we can or should express this here.
I am not a massive fan of Makefiles altogether. They are sometimes nice to bundle a few commands together but you run into limitations very quickly. At that point, you then have to have extra knowledge again which kind of defeats the point of trying to encapsulate it.
Given that we will be supplying images to the other repos for our release, nobody apart from us has to build this so we could get rid of this file and just inline it into our CI job.
Thoughts @libp2p/rust-libp2p-maintainers ?
@@ -0,0 +1,4 @@ | |||
FROM ubuntu:latest |
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.
This needs to align with the ubuntu version of our CI job. Perhaps it is better to use a fixed tag in both with a comment pointing to each other.
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.
sorry forgot to comment on why the change, it's due to the libc mismatch. I could not run the CI compiled ubuntu-latest bin on the Debian bullseye as it's version is older.
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.
Yeah that might happen again if either of these rolling tags update so I'd like us to point to 22.10 or whatever latest is both here and in the github workflow (runs-on:
).
custom_git_reference: ${{ github.event.pull_request.head.sha || github.sha }} | ||
custom_interop_target: rust | ||
dir: "multidim-interop" | ||
extra-versions: ping-versions |
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.
It's true. With the tradeoff of dealing not being able to run this locally as easily. I think it's nice to be able to locally run:
npm test -- --extra-versions=$RUST_LIBP2P/test-plans/version.json --name-filter="rust-libp2p-head"
. Inputs are also only strings, so you'd have to stringify the given example.
Would it make sense to unify the API of the test runner to just accept a series of JSON files with one version each? i.e. it builds the matrix of tests out of all files that are passed to it. Each JSON file would contain exactly one entry.
We could then pass the JSON as a string into the workflow and it writes it to a file. If you really need to run things locally, you just have to quickly copy paste that and make the file.
To me, that seems like an acceptable trade-off for having one less configuration file around per repository :)
Description
Addresses libp2p/test-plans#99
Notes
Links to any relevant issues
Open Questions
Change checklist