Update Go & JS tests to conform to the multidim interop test spec. #121

MarcoPolo · 2023-01-31T19:38:11Z

Addresses #120.

Closes #113, closes #120, closes #115, closes #112.

@jxs could you update the Rust test please?

After this PR the tests in the implementation repos will be broken until we update them as well. @jxs can you take updating the rust-libp2p repo? I'll update go-libp2p.

We could do something fancier where we maintain two versions of this test runner and update the implementation repos only once. I think it's okay to break CI for a little bit to make these changes simpler (The time can be minimized if we don't merge this until {rust,go}-libp2p are ready). If folks feel differently let me know.

jxs · 2023-01-31T21:21:53Z

Hi Marco, yeah no worries I'll start on it Tomorrow :)

thomaseizinger · 2023-01-31T21:23:54Z

If we pin the action to a hash in the repo, we should be able to update in steps, right?

thomaseizinger · 2023-01-31T21:27:41Z

If we pin the action to a hash in the repo, we should be able to update in steps, right?

We might need to also change the action to not just checkout latest test-plans master but the hash it was pinned to.

I have an idea that could work ..

thomaseizinger · 2023-02-01T00:29:35Z

If we pin the action to a hash in the repo, we should be able to update in steps, right?

We might need to also change the action to not just checkout latest test-plans master but the hash it was pinned to.

I just saw that we already have test-plans-ref:

test-plans/.github/workflows/run-testplans.yml

Line 23 in 7fd08bf

test-plans_ref:

Specifying that + pinning the action to a hash should allow us a graceful upgrade without having to do things in lock step.

thomaseizinger · 2023-02-01T20:31:44Z

Can we merge my composite action before we do this? #123

That would allow us to migrate each repository individually without breaking any CI.

jxs · 2023-02-02T12:14:25Z

there's also #114. If you prefer you can cherry-pick 07a7eeb here Marco

MarcoPolo · 2023-02-02T22:17:31Z

multidim-interop/rust/v0.50/Makefile

@@ -0,0 +1,18 @@
+image_name := rust-v0.50
+commitSha = 53e477d4423106bb9f9e744a4b275dfb6213ca0a


@thomaseizinger / @jxs

This is how I imagine defining future released versions. (As an alternative to publishing to a registry).

Also a note, from testing this manually. The Rust listener I don't think is listening for a sigint. The listener doesn't exit when compose tries to stop it after the dialer finishes.

also this isn't for the released version 0.50, it's for master I believe. That needs to be fixed, but the test-to-spec update isn't backported yet. Easy to do, I just haven't done it yet.

So if I understand correctly, we download the archive and build the container from the source of the tag?

Or do you expect the test to be defined here and we just reference the downloaded source-code?

If the test is still co-located with the source-code, I don't really see the benefit of doing this. Can you lay it out please?

If the test is not co-located but defined in here, then we can just use the package manager to point the released version.

The test is colocated with the implementation (in rust-libp2p).

The code here should be minimal, fetch the repo at the specified hash/tag and then build the container.

The dockerfile here is simply because I couldn't use the one in the rust-libp2p repo since im on a Mac and haven't setup cross builds.

This way you can run these tests locally without having access to the registry.

CI will cache this as well so it shouldn't add much time relative to a registry either.

Following from that, once you've run the test-suite once locally, it will work without a network connection. There shouldn't be a case where you need to build those containers yourself IMO.

❯ docker run --rm -it ghcr.io/libp2p/rust-libp2p/ping-interop-test:v0.50.0 Unable to find image 'ghcr.io/libp2p/rust-libp2p/ping-interop-test:v0.50.0' locally docker: Error response from daemon: Head "https://ghcr.io/v2/libp2p/rust-libp2p/ping-interop-test/manifests/v0.50.0": denied.

I assume I need to authenticate with the registry somehow?

There are two problems I have with just relying on the tag.

I can't checkout this repo and run the test. Now I need to setup a registry to get this working.

It makes changes to the test implementation harder. Do I have to have a GH action publish a new container for that change? I can't do what the action does here because I would need to cross compile to linux to create the binary.

My requirements:

It should be reproducible from a given test-plans commit. If a commit passes, it should always pass in the future.

I should be able to run the tests locally, and be able to tweak tests locally.

I'm happy to cache things all day long. And if we could use the registry as a cache, I'm fine with doing that as well. That's what I thought you meant when you said "Because you can also do that if you just build the container locally and give it the right tag". But now I understand you meant that I could manually build it and tag it myself so it doesn't try to fetch from the registry. This breaks the reproducibility.

❯ docker run --rm -it ghcr.io/libp2p/rust-libp2p/ping-interop-test:v0.50.0 Unable to find image 'ghcr.io/libp2p/rust-libp2p/ping-interop-test:v0.50.0' locally docker: Error response from daemon: Head "https://ghcr.io/v2/libp2p/rust-libp2p/ping-interop-test/manifests/v0.50.0": denied.

I assume I need to authenticate with the registry somehow?

Nothing is published yet as I wanted to await the final spec of the tests! :)
The branches are ready in rust-libp2p. Just need to click the publish button in the workflow.

There are two problems I have with just relying on the tag.

I can't checkout this repo and run the test. Now I need to setup a registry to get this working.

Nothing needs to be set up once the images are published. The images will be publicly accessible.

It makes changes to the test implementation harder. Do I have to have a GH action publish a new container for that change? I can't do what the action does here because I would need to cross compile to linux to create the binary.

The fact that we build the binary outside the container is a simplification (and performance optimisation) that us rust-libp2p maintainers accepted because we all run linux. If necessary, we can switch to building inside a container. It has at the end of the day nothing to do with pulling images from a registry or building them locally.

My requirements:

It should be reproducible from a given test-plans commit. If a commit passes, it should always pass in the future.

We can (and should) pin the container hash in the versions.ts file. That will fetch the right container even if the tag gets overwritten. IMO this is a super strong commitment to reproducibility because we are running bit-for-bit the same binary on every test.

I should be able to run the tests locally, and be able to tweak tests locally.

Changing the test is tricky because it lives in another source-code repository anyway. For local dev, I'd suggest to build the container locally and add it via --extra-version. I don't think we should "cheat" the local setup by overwriting an existing test or container.

Let's chat sync. I think that'll be faster than all this back and forth :)

@MarcoPolo and I agreed in a sync-chat that using these caches is basically equivalent to a registry, just slightly leaner.

Building a docker container will reuse local / cached layers if they are present. Running an image will pull it unless it is present. If we have a central cache in S3, building and pulling from a registry are essentially equivalent, we are just downloading layers.

The benefit of building and caching over pulling is that we only need to define how to build a container in the implementation repository. Then, any commit (or tag for that matter) can be referenced within the test-plans repo here and used to run the tests.

thomaseizinger

Thanks for pushing this forward, left a few comments. It would be nice if we could split this into smaller PRs. There are a lot of different changes going on here.

.github/actions/run-interop-ping-test/action.yml

.github/workflows/multidim-interop.yml

multidim-interop/Makefile

multidim-interop/dockerBuildWrapper.sh

multidim-interop/src/generator.ts

As per outcome of discussion here: libp2p/test-plans#121 (comment)

MarcoPolo · 2023-02-09T01:09:47Z

This is ready for a review. It can be reviewed commit by commit. That's easier for me than the overhead of dealing with 6 PRs.

thomaseizinger

LGTM!

Some non-blocking questions.

I guess we should prepare the commits in rust-libp2p properly (i.e. finish the backport) before we merge this?

.github/actions/run-interop-ping-test/action.yml

multidim-interop/Makefile

MarcoPolo · 2023-02-09T17:59:01Z

I guess we should prepare the commits in rust-libp2p properly (i.e. finish the backport) before we merge this?

I have the preference of merging this, and then a PR later that updates the commit with the backport. I don't expect that backport to break anything and it would be good to unblock other work here (like rolling out the s3 caching).

thomaseizinger · 2023-02-09T23:06:33Z

I guess we should prepare the commits in rust-libp2p properly (i.e. finish the backport) before we merge this?

I have the preference of merging this, and then a PR later that updates the commit with the backport. I don't expect that backport to break anything and it would be good to unblock other work here (like rolling out the s3 caching).

Sounds good to me!

mxinden · 2023-02-10T11:19:19Z

.github/actions/run-interop-ping-test/action.yml

+    - name: Configure AWS credentials for S3 build cache
+      if: ${{ inputs.s3-access-key-id }} != "" && ${{ inputs.s3-secret-access-key }} != ""
+      uses: aws-actions/configure-aws-credentials@v1
+      with:
+        aws-access-key-id: ${{ inputs.s3-access-key-id }}
+        aws-secret-access-key: ${{ inputs.s3-secret-access-key}}
+        aws-region: ${{ env.AWS_REGION }}


rust-libp2p executes this step on each pull request. Some pull requests originate from forks. Those pull requests don't have access to the above secrets. Thus this step fails.

See https://github.com/libp2p/rust-libp2p/actions/runs/4142906998/jobs/7164133938 on libp2p/rust-libp2p#3344 as an example.

I don't yet have the full picture on the caching approach via an S3 bucket. Would mounting the bucket as read-only from forks be an option? Worst case, we don't mount the bucket on forks at all, thus building all images from scratch.

Thus this step fails.

The original plan was that the if should have skipped this step. I'll check why that isn't happening.

Would mounting the bucket as read-only from forks be an option?

Unfortunately no. The s3 cache needs rw access. Maybe there's a hidden option I'm not seeing, but when I tried a read only key it failed.

My original plan was to have forks build from scratch.

I had the if statement wrong. #131 fixes it.

Did you try omitting the --cache-to option when you tried with a read-only key?

According to https://docs.docker.com/build/cache/backends/, those two options operate separately and I am surprised that --cache-from by itself would need write permissions.

If we conditionally define --cache-to, we should be set our bucket to be publicly readable and have forks use our cache.

Alternatively, I see that this also has a registry cache option which might be interesting to explore.

I really want our CI to always be fast, not just for our own PRs. (I also don't like wasting compute power.)

Happy to help with this but it would earliest be in ~5 days as I am off the grid for the next few.

Did you try omitting the --cache-to option when you tried with a read-only key?

I hadn't, that's probably it!

Probably worth figuring out how to make this bucket public and then defaulting to that public bucket if no keys are passed. I'll open an issue.

* Test if it works without s3 cache keys * Fix if statement * Fix if statement * Always use buildkit * Undo debug change * Add no cache workflow * Skip test in no-cache workflow * Update .github/workflows/no-cache-multidim-interop.yml * Same workflow; use CACHING_OPTIONS

* Add webrtc to JS test * Add onlyDial to all queries * Update versions.ts

@master

With libp2p/test-plans#121 merged, we should be able to use @master directly. This includes libp2p/test-plans@6d1aed2, thus allowing interop tests to run from fork pull requests.

@master

With libp2p/test-plans#121 merged, we should be able to use @master directly. Still pointing to a concrete git hash. This includes libp2p/test-plans@6d1aed2, thus allowing interop tests to run from fork pull requests.

This was referenced Jan 31, 2023

improve error logging for JS/TS ping test #119

Closed

Drop unrequired params for QUIC and WebRTC interop tests #118

Closed

thomaseizinger mentioned this pull request Feb 1, 2023

NG run-testplans action #111

Closed

jxs mentioned this pull request Feb 1, 2023

feat: Backport interop test to rust-libp2p v050 libp2p/rust-libp2p#3415

Merged

4 tasks

jxs mentioned this pull request Feb 2, 2023

refactor(interop): update ping to conform to the new interop spec libp2p/rust-libp2p#3423

Merged

4 tasks

p-shahi mentioned this pull request Feb 2, 2023

Update interop test implementations to conform to... #120

Closed

3 tasks

MarcoPolo force-pushed the marco/update-tests-to-spec branch from 2516f30 to 98161af Compare February 2, 2023 21:17

MarcoPolo commented Feb 2, 2023

View reviewed changes

MarcoPolo mentioned this pull request Feb 3, 2023

interop: Update interop test to match spec libp2p/go-libp2p#2049

Merged

thomaseizinger mentioned this pull request Feb 8, 2023

feat: don't publish docker containers for interop-tests libp2p/rust-libp2p#3437

Merged

4 tasks

MarcoPolo mentioned this pull request Feb 8, 2023

Cache builds with s3 buckets #125

Merged

MarcoPolo force-pushed the marco/update-tests-to-spec branch from e011ccd to 1b5649b Compare February 8, 2023 18:01

thomaseizinger reviewed Feb 8, 2023

View reviewed changes

mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Feb 8, 2023

feat: don't publish docker containers for interop-tests (#3437)

351a166

As per outcome of discussion here: libp2p/test-plans#121 (comment)

MarcoPolo marked this pull request as draft February 8, 2023 19:35

MarcoPolo added 6 commits February 8, 2023 16:36

Update Go test implementations to match new spec

d23f238

Update JS test implementation

f180bfb

Update Rust test Implementation

90c6f72

Update root Makefile

0dc6bea

Update runner to new spec

76717cf

Use composite action and S3 caching

59b5d90

MarcoPolo force-pushed the marco/update-tests-to-spec branch from c4d0a96 to 59b5d90 Compare February 9, 2023 00:47

MarcoPolo marked this pull request as ready for review February 9, 2023 01:08

Not using GHA cache anymore

482e8ac

MarcoPolo mentioned this pull request Feb 9, 2023

interop-tests: use new action with s3 caching libp2p/go-libp2p#2076

Merged

thomaseizinger approved these changes Feb 9, 2023

View reviewed changes

.github/actions/run-interop-ping-test/action.yml Outdated Show resolved Hide resolved

multidim-interop/Makefile Show resolved Hide resolved

MarcoPolo force-pushed the marco/update-tests-to-spec branch from e56e1ef to 7fd9539 Compare February 9, 2023 16:47

Try removing access key from env

406df30

MarcoPolo force-pushed the marco/update-tests-to-spec branch from 7fd9539 to 406df30 Compare February 9, 2023 16:59

mxinden reviewed Feb 10, 2023

View reviewed changes

mxinden mentioned this pull request Feb 10, 2023

fix(identify): don't close stream in protocol::recv libp2p/rust-libp2p#3344

Merged

4 tasks

MarcoPolo mentioned this pull request Feb 10, 2023

Make build cache bucket public #132

Closed

MarcoPolo and others added 3 commits February 10, 2023 15:21

Add Browser WebRTC test (#130)

4c2d3dc

* Add webrtc to JS test * Add onlyDial to all queries * Update versions.ts

Remove unneeded timeout overrides

e9cd3e6

MarcoPolo merged commit 3b6d87b into master Feb 11, 2023

mxinden mentioned this pull request Feb 13, 2023

ci(interop): Use run-interop-ping-test master latest hash libp2p/rust-libp2p#3462

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Go & JS tests to conform to the multidim interop test spec. #121

Update Go & JS tests to conform to the multidim interop test spec. #121

MarcoPolo commented Jan 31, 2023 •

edited

Loading

jxs commented Jan 31, 2023

thomaseizinger commented Jan 31, 2023

thomaseizinger commented Jan 31, 2023

thomaseizinger commented Feb 1, 2023

thomaseizinger commented Feb 1, 2023 •

edited

Loading

jxs commented Feb 2, 2023

MarcoPolo Feb 2, 2023 •

edited

Loading

MarcoPolo Feb 2, 2023

thomaseizinger Feb 2, 2023

thomaseizinger Feb 2, 2023

MarcoPolo Feb 2, 2023

thomaseizinger Feb 2, 2023 •

edited

Loading

MarcoPolo Feb 3, 2023

thomaseizinger Feb 3, 2023

MarcoPolo Feb 3, 2023

thomaseizinger Feb 7, 2023

thomaseizinger left a comment

MarcoPolo commented Feb 9, 2023

thomaseizinger left a comment

MarcoPolo commented Feb 9, 2023

thomaseizinger commented Feb 9, 2023

mxinden Feb 10, 2023

MarcoPolo Feb 10, 2023 •

edited

Loading

MarcoPolo Feb 10, 2023

thomaseizinger Feb 10, 2023

thomaseizinger Feb 10, 2023

MarcoPolo Feb 10, 2023

		@@ -0,0 +1,18 @@
		image_name := rust-v0.50
		commitSha = 53e477d4423106bb9f9e744a4b275dfb6213ca0a

Update Go & JS tests to conform to the multidim interop test spec. #121

Update Go & JS tests to conform to the multidim interop test spec. #121

Conversation

MarcoPolo commented Jan 31, 2023 • edited Loading

jxs commented Jan 31, 2023

thomaseizinger commented Jan 31, 2023

thomaseizinger commented Jan 31, 2023

thomaseizinger commented Feb 1, 2023

thomaseizinger commented Feb 1, 2023 • edited Loading

jxs commented Feb 2, 2023

MarcoPolo Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

MarcoPolo commented Feb 9, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

MarcoPolo commented Feb 9, 2023

thomaseizinger commented Feb 9, 2023

Choose a reason for hiding this comment

MarcoPolo Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoPolo commented Jan 31, 2023 •

edited

Loading

thomaseizinger commented Feb 1, 2023 •

edited

Loading

MarcoPolo Feb 2, 2023 •

edited

Loading

thomaseizinger Feb 2, 2023 •

edited

Loading

MarcoPolo Feb 10, 2023 •

edited

Loading