-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix: use correct wasm shims names #9519
Conversation
Fix the wasm shim detection and the containerd configuration generation. Prior to this commit, the binary and the `RuntimeType` values were not correct. Signed-off-by: Flavio Castelli <fcastelli@suse.com>
You might take a look at https://github.com/k3s-io/k3s/tree/master/tests/integration and https://github.com/k3s-io/k3s/tree/master/tests/e2e and see where you think this might fit best. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9519 +/- ##
==========================================
- Coverage 49.12% 42.92% -6.20%
==========================================
Files 151 154 +3
Lines 13475 13523 +48
==========================================
- Hits 6619 5805 -814
- Misses 5515 6550 +1035
+ Partials 1341 1168 -173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Since this can be validated on a single server node, this should be written as an integration test. |
The test needs to download some binaries from the internet and put them on specific locations on the filesystem. From what I've seen, the integration tests are not doing that (see for example the longhorn one, which bails out if The e2e tests, on the other hand, seem to allow this kind of operations. Because of that, I was more inclined towards writing a e2e tests. Thoughts? |
Hmm, thats fair, we don't want to add binaries to a host without them knowing, so E2E it is. |
@brandond , @dereknola: I've added a new e2e test. I've taken inspiration from other tests that are already inside of the code base. Let me know if there's anything I have to change to make it more idiomatic |
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 accidentally butchered the LICENSE and README files. Can you revert those changes?
Add a e2e test that runs some demo WebAssembly applications using the dedicated containerd shims. Note: this is not an integration test because we need to install some binaries (the special containerd shims) on the host. Signed-off-by: Flavio Castelli <fcastelli@suse.com>
9bab53c
to
33e541c
Compare
@dereknola doh! sorry about that... I've fixed the issue with a force push |
It seems the amount of e2e tests ran by drone is limited (see here). Is that done by purpose? Should I add my new e2e test over there? |
I am assuming that this change was not in |
@Mossaka no, they are not part of a tagged release of k3s. They will be part of the next release |
Proposed Changes
Change how containerd configuration is generated with regards to WebAssembly shims.
Types of Changes
Use correct binary names and the associated
RuntimeType
. Prior to this, there was quite a mixup betweenv1
andv2
shims.Verification
spin
and theslight
shims from here/usr/bin/containerd-shim-spin-v2
and/usr/bin/containerd-shim-slight-v1
Check the contents of
/var/lib/rancher/k3s/agent/etc/containerd/config.toml
, it should have the following lines:Then, to test the slight-shim is actually working, create the following Deployment:
Then, to test the spin-shim is actually working, create the following Deployment:
You should now have 2 Pod running.
Both Pods are small web application. To access them we have to create the following resources:
Then run the following commands:
Testing
This was already covered by the unit tests, however that didn't help with the detection of the issue.
Does k3s have the concept of integration tests? The idea would be to automate the verification steps. I would be happy to write this integration test.
Linked Issues
User-Facing Change
Further Comments
CC @brandond and @vitorsavian who initially reviewed #8751