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 to containerd v2 #604

Merged
merged 1 commit into from
Aug 6, 2024
Merged

Update to containerd v2 #604

merged 1 commit into from
Aug 6, 2024

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Jul 22, 2024

This PR is about updating to containerd v2 rc3 - as part of the effort to bring nerdctl to v2 as well - which is tracked here: containerd/nerdctl#3173

The above PR already uses an updated fork of nydus and nerdctl tests are passing with it.

Other dependencies have also been bumped to their latest version.

Besides basic import path changes, key changes are:

  • around plugins registration
  • removal of crialpha in stargz-snapshotter
  • removal of tracing.WithHTTPRequest which has been deprecated for a while and does not do what it did in the past anyhow
  • some deprecated methods fixes for s3 and grpc

Further cleanup became necessary to fix racyness / other CI issues

  • github actions:
    • update actions version
    • curl test to avoid racy docker login vs. registry start
    • go version bump to 1.22
  • golangci
    • update syntax
  • Dockerfile
    • dependencies version and base image update
    • support arm64
    • quiet wget
  • tests
    • /entrypoint reboot_containerd expanded sleep to avoid racy shutdown

Pending CI to pass.

cc @AkihiroSuda

@apostasie apostasie changed the title Update to containerd v2 [WIP] Update to containerd v2 Jul 22, 2024
@apostasie apostasie force-pushed the main branch 7 times, most recently from 869b5cd to 9b388ab Compare July 22, 2024 23:18
Copy link

codecov bot commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.

Project coverage is 21.82%. Comparing base (f46b631) to head (e22e77d).
Report is 1 commits behind head on main.

Files Patch % Lines
export/snapshotter/snapshotter.go 0.00% 5 Missing ⚠️
pkg/backend/s3.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #604       +/-   ##
===========================================
- Coverage   34.31%   21.82%   -12.49%     
===========================================
  Files          65      122       +57     
  Lines        6598    10730     +4132     
===========================================
+ Hits         2264     2342       +78     
- Misses       4020     8070     +4050     
- Partials      314      318        +4     
Files Coverage Δ
cmd/containerd-nydus-grpc/snapshotter.go 0.00% <ø> (ø)
pkg/auth/image_proxy.go 75.51% <100.00%> (ø)
pkg/backend/backend.go 0.00% <ø> (ø)
pkg/backend/localfs.go 0.00% <ø> (ø)
pkg/backend/oss.go 0.00% <ø> (ø)
pkg/cache/manager.go 0.00% <ø> (ø)
pkg/cgroup/cgroup.go 0.00% <ø> (ø)
pkg/converter/convert_unix.go 0.00% <ø> (ø)
pkg/converter/cs_proxy_unix.go 0.00% <ø> (ø)
pkg/converter/types.go 0.00% <ø> (ø)
... and 27 more

... and 39 files with indirect coverage changes

@apostasie apostasie marked this pull request as ready for review July 31, 2024 19:29
@apostasie apostasie changed the title [WIP] Update to containerd v2 Update to containerd v2 Jul 31, 2024
@apostasie
Copy link
Contributor Author

Rebased.
CI is green except for codecov - although it is very confusing why would it be unhappy - I could not find out why in the detailed report.

@thaJeztah
Copy link
Member

x-ref; #611 (comment)

@imeoer rebased, and I included some of the fixes from @apostasie (from #604) in this patch, adding them as co-author.

My goal with these sets of PRs was to see if we could have a release with the new modules in place (removing the ones deprecated in containerd) before this module moves to v2, so that I can update the version used in BuildKit in preparation of dependencies starting to move to containerd v2 (these module moves in containerd are intended to help with that transition to make sure a common module is used between v1 and v2 codebases).

⚠️ Given that this PR removes some functionality, perhaps it warrants a v0.14.0 ❓ maybe tagging current main / #610 as v0.13.15 ?

Signed-off-by: apostasie <[email protected]>
@apostasie
Copy link
Contributor Author

Rebased.

CI is green.

Copy link
Collaborator

@imeoer imeoer left a comment

Choose a reason for hiding this comment

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

LGTM!

@imeoer imeoer merged commit 8fa319b into containerd:main Aug 6, 2024
15 of 16 checks passed
@apostasie
Copy link
Contributor Author

apostasie commented Aug 6, 2024

Thanks @imeoer !

And thanks @thaJeztah for all the prep work!

@thaJeztah do you know if/when moby is going to make the switch to containerdv2? (specifically interested in pkg/sysinfo, which AFAIK does pull in just containerd/pkg/userns and containerd/pkg/seccomp)

@thaJeztah
Copy link
Member

You're welcome! There's so many projects that have a dependency on each other - it's sometimes difficult to wrap your head around; in all honesty some of these packages may not have had an immediate impact, but some were definitely good to get out of the way to (hopefully) smoothen the transition.

I actually just Yesterday noticed the github.com/containerd/cgroups -> github.com/containerd/cgroups/v3 change in this PR (which probably could've been included already to get rid of the old version), but probably not critical.

do you know if/when moby is going to make the switch to containerdv2?

We don't have a timeline yet; I know there's some work ongoing w.r.t. containerd/errdefs to unify various errdefs implementations (containerd, moby, buildkit, others?), and there was a draft PR to test v2, but no timeline yet.

specifically interested in pkg/sysinfo, which AFAIK does pull in just containerd/pkg/userns and containerd/pkg/seccomp)

For pkg/userns, I should add a warning; I opened PRs to get rid of that dependency (moving it to a separate module), but the current approach must not end up in a release, as (after discussion) it was not in the right module, and will be move to a separate module (github.com/moby/sys/userns instead of being part of github.com/moby/sys/user); see the PR below;

For containerd/pkg/seccomp, I think we should consider making that a separate module; that may involve unifying the containerd and moby implementation (the containerd profile was originally forked from the moby repository, but took a slightly different approach).

I did some refactoring some time ago to isolate the seccomp and apparmor profiles in the moby repository with the intent to eventually move them separate, but that's been some time ago, and we likely would have to spend some time looking at the current differences between containerd and moby, and to see if we can unify the implementations so that those profiles can live in separate modules (to be shared between projects).

@apostasie
Copy link
Contributor Author

Thanks a lot for all the info @thaJeztah

I think we might just copy / fork moby/pkg/sysinfo short term in nerdctl (as done in containerd/nerdctl#3173) so that we can fix the containerd dependency - and then we will move back to moby upstream when it is migrated over.

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.

3 participants