Run tests with spin-registry-push as well #90

rajatjindal · 2024-04-19T09:03:53Z

This PR adds ability to run existing tests with apps pushed using spin-registry-push command. To test that, it starts a registry, managed by k3d and build/push apps to this registry.

rajatjindal · 2024-04-25T05:03:23Z

requesting review from @kate-goldenring @Mossaka @jsturtevant @vdice

vdice

Looks great to me! Excited to have these tests in place 🚀

vdice · 2024-04-25T15:50:20Z

Makefile

+
+.PHONY: integration-docker-build-push-tests
+integration-docker-build-push-tests: workloads
+	cargo test -p containerd-shim-spin-tests -- --nocapture


If this cargo test command fails, will the cleanup commands below still run?

I think this will fail currently. do you think that is a fair outcome or you would expect it to fail these tests and run the other set of tests (spin-registry-push testcases)

given we are using same k3d cluster to run both tests right now, i think if it fails, we should fail the complete job. As suggested in one of the other comment on this PR, we should do a follow up PR to separate these two tasks as separate steps in CI workflow.

I'm in support of failing the complete job if this task fails. I was more worried about lingering resources when this cargo test ... command fails and so the subsequent kubectl delete ... cleanup commands don't run. I suppose in CI it doesn't matter... but maybe in dev scenarios?

Hi @vdice, how do you suggest we handle this? one way would be to capture the output and exit code and then throw a new error after cleanup?

It looks like this logic is now in a shell script. I'd say let's not worry about it for this PR and we can add a follow-up if we run into this scenario when developing/running tests locally.

👍 , i moved it to shell script in response to other comments around unifying some of the scripts. agree that we can work on a separate PR to improve this further.

vdice · 2024-04-25T16:06:43Z

scripts/workloads-spin-registry-push.sh

+
+set -euo pipefail
+
+# Check if kubectl is installed


nit/potential follow-up: Would be nice to consolidate all of the logic shared by these two scripts. Looks like everything is the same besides the kubectl apply -f tests/workloads-{spin-registry-push | docker-build-push} bit?

agree. I didn't want to touch a lot of existing scripts in this PR to avoid breaking workflow other folks are used to when testing changes and to keep the changes contained for easy review.

I can take up another task next to cleanup the scripts a bit and try out dagger functions to see if they help simplify our workflows.

+1 to @vdice 's comment

would we be fine to add a param to this script to tell which one we want to run now.

e.g. it would become something like:

./workloads.sh "spin-registry-push-artifacts"

and

./workloads.sh "docker-build-push-artifacts"

I've taken a stab at restructuring this a bit as suggested.

renamed workloads.sh to deploy-workloads.sh - this by default deploys docker build artifacts, but we can pass a command line argument to deploy spin registry push artifacts

renamed workloads-delete.sh to pod-terminates-test.sh to reflect the actual purpose of that script

added teardown-workloads.sh to delete all the workloads deployed by deploy-workloads.sh

Kindly let me know if this looks any better. happy to hear more feedback and adjust accordingly. kindly let me know (and thank you for spending time to review the code with me here).

This looks good to me 👍

kate-goldenring

Thank's @rajatjindal! Just had a few cleanup oriented nits

images/spin-multi-trigger-app/spin.toml

scripts/setup-linux.sh

scripts/workloads-spin-registry-push.sh

tests/workloads-spin-registry-push/workload-spin-registry-push.yaml

jsturtevant

LGTM, one future improvement might be to separate the different tests into distinct tasks in the CI job.

tests/workloads-common/runtime.yaml

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

Mossaka

Thanks for working on improving the integration tests!

I'd suggest to consolidate two different integration paths into one set of workloads. Perhaps, we can create two services and have one ingress loadbalancer to route traffic to either docker-push or spin-registry-push

.github/workflows/ci.yaml

scripts/setup-linux.sh

Mossaka · 2024-04-29T16:33:34Z

scripts/setup-linux.sh

+sudo rustup target add wasm32-wasi && sudo rustup target add wasm32-unknown-unknown
+
+## setup tinygo. required for building test spin app
+echo "setting up tinygo"


Where is TinyGo needed? I thought Spin apps are built through Dockerfile which already has TinyGo.

Hello @Mossaka, we need tinygo for building this app: https://github.com/spinkube/containerd-shim-spin/tree/main/images/spin/go-hello

We are using Docker to build images and the Dockerfile in images/spin will install TinyGo. Am I missing anything?

oh apologies for the confusion.

Now that we need to run tests with "spin registry push" version as well, we push the apps in following two ways:

docker build && k3d import image (was already part of up.sh)

spin build && spin registry push <artifact> (added to up.sh). This runs on the runner machine, and therefore needs tinygo installed.

does that answer your question?

Ah right, that makes sense. Maybe as follow-up I wonder if we could run docker build and then extract the component binary out and use it as the artifact for spin registry push. (this will reduce the amount of deps we need to install on the host machine).

Mossaka · 2024-04-29T16:37:06Z

scripts/workloads-spin-registry-push.sh

+
+set -euo pipefail
+
+# Check if kubectl is installed


+1 to @vdice 's comment

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal · 2024-05-09T05:45:24Z

Hi @Mossaka, when you have some bandwidth, could you please (re)-review this PR and provide your feedback. thank you

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

Mossaka

LGTM!

Mossaka · 2024-05-13T21:41:40Z

scripts/setup-linux.sh

+sudo rustup target add wasm32-wasi && sudo rustup target add wasm32-unknown-unknown
+
+## setup tinygo. required for building test spin app
+echo "setting up tinygo"


Ah right, that makes sense. Maybe as follow-up I wonder if we could run docker build and then extract the component binary out and use it as the artifact for spin registry push. (this will reduce the amount of deps we need to install on the host machine).

rajatjindal · 2024-05-15T00:39:22Z

Hi @Mossaka, could we please merge this PR (if there are no blocking comments).

rajatjindal force-pushed the use-spin-build branch 10 times, most recently from f8e0b07 to 45ffa8a Compare April 23, 2024 11:56

rajatjindal changed the title ~~[wip] Use spin build~~ Run tests with spin-registry-push as well Apr 23, 2024

rajatjindal marked this pull request as ready for review April 23, 2024 12:35

rajatjindal requested review from Mossaka, kate-goldenring, jsturtevant, radu-matei and devigned as code owners April 23, 2024 12:35

rajatjindal mentioned this pull request Apr 23, 2024

Test applications built using spin registry push #37

Closed

vdice reviewed Apr 25, 2024

View reviewed changes

kate-goldenring reviewed Apr 25, 2024

View reviewed changes

jsturtevant approved these changes Apr 25, 2024

View reviewed changes

vdice reviewed Apr 26, 2024

View reviewed changes

tests/workloads-common/runtime.yaml Outdated Show resolved Hide resolved

rajatjindal added 4 commits April 29, 2024 12:53

update wasm path in test apps and push to local registry

2414b97

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

do spin build and push for test apps

fd08081

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

setup rust/tinygo for building spin apps

27b4d6f

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

setup spin for running spin build/registry push

2aa6846

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal force-pushed the use-spin-build branch from 45ffa8a to c60922a Compare April 29, 2024 07:23

Mossaka reviewed Apr 29, 2024

View reviewed changes

restructure to run both spin-registry-push and docker-build-push tests

be84b0a

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal added 4 commits May 6, 2024 07:43

use random forward port for redis

ddffad4

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

address review feedback

181f6f8

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

use default rust version from toolchain config

7e234fd

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rename test-workloads-delete to pod-terminates-test

a3ceefe

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal force-pushed the use-spin-build branch 2 times, most recently from 0ffb3b3 to 972e6e3 Compare May 6, 2024 02:38

restructure to use common-scripts and reduce Makefile complexity

14338b4

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal force-pushed the use-spin-build branch from 972e6e3 to 14338b4 Compare May 9, 2024 05:48

add missing teardown-workloads.sh makefile target

4e52d0e

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

Mossaka approved these changes May 13, 2024

View reviewed changes

Mossaka merged commit 50f1c33 into spinkube:main May 15, 2024
9 checks passed

vdice mentioned this pull request May 15, 2024

Spin apps fail to run - compiled with incompatible wasmtime version #79

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests with spin-registry-push as well #90

Run tests with spin-registry-push as well #90

rajatjindal commented Apr 19, 2024 •

edited

Loading

rajatjindal commented Apr 25, 2024 •

edited

Loading

vdice left a comment

vdice Apr 25, 2024

rajatjindal Apr 29, 2024

vdice Apr 29, 2024

rajatjindal May 2, 2024

vdice May 6, 2024

rajatjindal May 9, 2024

vdice Apr 25, 2024

rajatjindal Apr 29, 2024

Mossaka Apr 29, 2024

rajatjindal May 1, 2024 •

edited

Loading

rajatjindal May 6, 2024

vdice May 6, 2024

kate-goldenring left a comment

jsturtevant left a comment

Mossaka left a comment

Mossaka Apr 29, 2024

rajatjindal May 1, 2024

Mossaka May 2, 2024

rajatjindal May 2, 2024 •

edited

Loading

Mossaka May 13, 2024

Mossaka Apr 29, 2024

rajatjindal commented May 9, 2024

Mossaka left a comment

Mossaka May 13, 2024

rajatjindal commented May 15, 2024

Run tests with spin-registry-push as well #90

Run tests with spin-registry-push as well #90

Conversation

rajatjindal commented Apr 19, 2024 • edited Loading

rajatjindal commented Apr 25, 2024 • edited Loading

vdice left a 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

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

Choose a reason for hiding this comment

rajatjindal May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kate-goldenring left a comment

Choose a reason for hiding this comment

jsturtevant left a comment

Choose a reason for hiding this comment

Mossaka left a 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

rajatjindal May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatjindal commented May 9, 2024

Mossaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatjindal commented May 15, 2024

rajatjindal commented Apr 19, 2024 •

edited

Loading

rajatjindal commented Apr 25, 2024 •

edited

Loading

rajatjindal May 1, 2024 •

edited

Loading

rajatjindal May 2, 2024 •

edited

Loading