Skip to content
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

Update youki to include capabilities fix #130

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

jprendes
Copy link
Collaborator

@jprendes jprendes commented Jun 7, 2023

Update youki dependency to include a fix for a capabilities issue that prevented it from running inside of Docker Desktop

@jprendes jprendes marked this pull request as ready for review June 7, 2023 10:58
Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor

@ipuustin ipuustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I think we can figure out the error handling question separately if there is a need to.

crates/containerd-shim-wasm/Cargo.toml Outdated Show resolved Hide resolved
@jprendes jprendes force-pushed the update-youki branch 2 times, most recently from f063855 to 49a9739 Compare June 8, 2023 15:40
Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Added a few comments.

Cargo.toml Outdated Show resolved Hide resolved
crates/containerd-shim-wasm/Cargo.toml Outdated Show resolved Hide resolved
@jprendes jprendes force-pushed the update-youki branch 2 times, most recently from 6fbf100 to dc2aed5 Compare June 9, 2023 08:45
@jprendes
Copy link
Collaborator Author

jprendes commented Jun 9, 2023

I will rebase this PR after #132 is merged

@cpuguy83
Copy link
Member

Merge conflict in Cargo.lock

@jprendes
Copy link
Collaborator Author

Rebased to fix merge conflict.

@ipuustin
Copy link
Contributor

This is important also because the oci-spec-rs 0.6.1 appears not to be compatible with the previous libcontainer SHA.

@jprendes
Copy link
Collaborator Author

The version of youki in the last push includes the commit from 2 days ago that updates oci-spec-rs to 0.6.1, but I don't see any SHA related changes in that commit.

@ipuustin
Copy link
Contributor

The youki commit where the oci-spec version is bumped is required for oci-spec 0.6.1 support for runwasi too... otherwise runwasi compilation will fail for missing Time namespace support:

   Compiling libcontainer v0.0.5 (https://github.com/containers/youki?rev=edd63c84f903aa09510ef758ea6f617e0cb8b7e1#edd63c84)
error[E0004]: non-exhaustive patterns: `LinuxNamespaceType::Time` not covered
   --> /home/ipuustin/.cargo/git/checkouts/youki-135a19ffee0653b9/edd63c8/crates/libcontainer/src/namespaces.rs:33:11
    |
33  |     match namespace_type {
    |           ^^^^^^^^^^^^^^ pattern `LinuxNamespaceType::Time` not covered
    |
note: `LinuxNamespaceType` defined here
   --> /home/ipuustin/.cargo/registry/src/github.com-1ecc6299db9ec823/oci-spec-0.6.1/src/runtime/linux.rs:792:5
    |
769 | pub enum LinuxNamespaceType {
    | ---------------------------
...
792 |     Time = 0x00000080,
    |     ^^^^ not covered
    = note: the matched value is of type `LinuxNamespaceType`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
40  ~         LinuxNamespaceType::Mount => CloneFlags::CLONE_NEWNS,
41  ~         LinuxNamespaceType::Time => todo!(),
    |

Copy link
Contributor

@ipuustin ipuustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now!

@jprendes jprendes requested a review from Mossaka June 13, 2023 15:23
@Mossaka Mossaka merged commit af075bf into containerd:main Jun 13, 2023
@jprendes jprendes deleted the update-youki branch July 23, 2023 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants