-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Containerd v2 module #9306
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
Containerd v2 module #9306
Conversation
Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Derek McGowan <[email protected]>
Protobuf will automatically put the files generated for a v2 module into a v2 directory. Move them to their correct location after running the protobuild. Signed-off-by: Derek McGowan <[email protected]>
|
Skipping CI for Draft Pull Request. |
2c15c58 to
58cedff
Compare
Signed-off-by: Derek McGowan <[email protected]>
58cedff to
efc0253
Compare
Signed-off-by: Derek McGowan <[email protected]>
e8fd1b2 to
5fdf55e
Compare
| github.com/AdamKorcz/go-118-fuzz-build v0.0.0-20230306123547-8075edf89bb0 | ||
| github.com/Microsoft/go-winio v0.6.1 | ||
| github.com/Microsoft/hcsshim v0.12.0-rc.0 | ||
| github.com/Microsoft/hcsshim/test v0.0.0-20210227013316-43a75bb4edd3 |
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.
Why do we have import of this module. Why didnt we have this earlier? Because earlier also, it was being used in windows tests.
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.
Commit 1: Temporarily remove the integration/client submodule. I could not figure out a clean way to move this module to use the new v2 module name since go mod will always fail lookup since there is no tag or commit with the new module name. I'm not sure how this was ever supposed to work, but replace doesn't seem to do the trick for me.
I believe this was the only reason this submodule was created. We will add it back if still necessary, something was complaining about the checked in rsrc_amd64.syso, but no issues with CI now.
| github.com/cilium/ebpf v0.9.1 // indirect | ||
| github.com/containerd/typeurl v1.0.2 // indirect | ||
| github.com/containers/ocicrypt v1.1.6 // indirect | ||
| github.com/containerd/containerd v1.7.8 // indirect |
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.
Is this a concern (even it's an indirect dependency)? It seems like this was because of Microsoft/hcsshim...
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 a concern in the short term, we should aim to get rid of it before the final release though. I think we would only need it if we wanted to do auto migration or interface translation for plugins. Once we get this in and tagged, then we can get plugins updated.
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.
Can this be revisited to remove indirect dependency on an older containerd version since, the PR is merged now.
|
Completely missed that it was merged 🎉 Happy to see this done; thanks @dmcgowan ! |
This comment was marked as spam.
This comment was marked as spam.
Update fork-external main with upstream main @ 452ec25 Related work items: containerd#5890, containerd#7647, containerd#9218, containerd#9233, containerd#9258, containerd#9270, containerd#9274, containerd#9279, containerd#9283, containerd#9286, containerd#9289, containerd#9290, containerd#9294, containerd#9295, containerd#9297, containerd#9305, containerd#9306, containerd#9308, containerd#9316, containerd#9317, containerd#9319, containerd#9320, containerd#9321
The first 3 commits are preparing for the module and the last commit is generated with a script to make rebasing easier.
Commit 1: Temporarily remove the integration/client submodule. I could not figure out a clean way to move this module to use the new v2 module name since go mod will always fail lookup since there is no tag or commit with the new module name. I'm not sure how this was ever supposed to work, but replace doesn't seem to do the trick for me.
Commit 2: Temporarily removing imgcrypt since it uses v1 datatype via import where v2 are not expected. We can possibly try to clean this up so that we don't have a circular import. I was happy though that this was the only real circular dependency we had.
Commit 3: Does a move of the protoc generated files from a "v2" directory to the expected location. I'm not sure if there is a fix somewhere in the proto go world to handle this, the mv seems to work fine for now though. This seems similar to the other issues we have had with how inflexible protoc output is. This is done now just before our other fix script of the output.
Commit 4: Fixes a bunch of linter issues with integration/client which weren't checked before.
Commit 5: This commit is generated by the following script. It might be easier to look at the script before looking at the commit content.