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

go.mod update to containerd v2 #3173

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Jul 4, 2024

Similar to #2902 - with the difference that it uses personal forks for pending PRs against important dependencies (updated against containerd v2).

Currently:

  • fully updated to containerd v2 without any indirect to v1.7
  • does build and run
  • all dependencies have been updated to containerd v2
  • unit test pass
  • rootless tests pass
  • rootful tests pass - some timeout failures currently
  • windows is working

Upsteam dependencies to merge:

Upstream dependencies that do not have a proper PR (only our fork):

  • Moby sysinfo package - folder has been copied under here for now - there is a couple of PRs hanging around but they look very stale (Derek' six months+ Update containerd to 2.0 moby/moby#47255 )
  • hcsshim: has to be patched against their latest released version, and not main - main is breaking API, which would mean updating containerd against that (and it is probably unstable / untested anyhow). Our fork has a v2-backport branch that does that.
  • nydus: current fork is bare minimum and there is definitely some more work needed on their CI tooling needed - it is based off main, which might not be the right choice - finally, nydus depends on stargz and hcsshim, which means these two have to get in first

@apostasie apostasie mentioned this pull request Jul 4, 2024
@apostasie apostasie force-pushed the go-mod-containerd-v2 branch 5 times, most recently from 21c4664 to 753ce12 Compare July 4, 2024 22:55
@apostasie
Copy link
Contributor Author

@AkihiroSuda

See initial comment up there ^.

That was quite a trek, but CI is finally mostly green - and nothing depends on containerd v1.
I am a bit concerned over the failing ones though, timeouts feel like maybe something is deadlocked somewhere.

Anyhow, obviously this cannot be merged, since it depends on my forks - upstream PRs have to be merged first.
I opened a bunch of them already, and have more to come from my forks.
I can use some help there to move them along - I am not sure why they are blocked - I guess people are busy.

Specifically, moving these two that have been there for a couple of weeks now would get us started:

Then we can look into the best approach for hcsshim and the rest.

Cheers.

@apostasie apostasie changed the title go.mod update to containerd v2 [CANARY] go.mod update to containerd v2 Jul 5, 2024
@apostasie
Copy link
Contributor Author

Will rebase soon now that the stargz PR is in.

@apostasie apostasie force-pushed the go-mod-containerd-v2 branch 6 times, most recently from 76887e0 to 7b70603 Compare July 10, 2024 21:17
@apostasie
Copy link
Contributor Author

apostasie commented Jul 10, 2024

Latest state of affairs:

  • see top level comment for dependencies status updated list
  • CI is green EXCEPT test-integration-20.04-v1.6.33 which seems to hang on something and timeout - do we care? is 1.6 going to be deprecated with the release of v2?
  • next important focus will be hcsshim - see discussion at: Vendor containerd 2.0 microsoft/hcsshim#1958 (comment)

@apostasie
Copy link
Contributor Author

Status update:

Work is going on for nydus: containerd/nydus-snapshotter#604

Nydus may be the last dep we need (as we may be able to ignore hcsshim for now).

@apostasie apostasie force-pushed the go-mod-containerd-v2 branch 4 times, most recently from 3a6f5f7 to 30b1c33 Compare July 25, 2024 22:06
@apostasie
Copy link
Contributor Author

apostasie commented Jul 25, 2024

@AkihiroSuda we are getting closer.

nydus was a lot more work than I thought and I had to send multiple PRs for a variety of issues that are pending.

At this point, we need nydus to come through, and then the soci PR to get in (maybe we can convince them to have it on an ad-hoc branch meanwhile, as they want to wait post-v2-release).

As far as nerdctl CI here is concerned, we get consistently green, with two exceptions:

Question for you: when ctd v2 is out, should we still care about that (u 20.04 + containerd 1.6)? I can go and debug this, but only want to spend time on this if we actually care about it :-)

LMK your thoughts.

Thanks.

@AkihiroSuda
Copy link
Member

Thank you @apostasie for this hard work!

nydus was a lot more work than I thought and I had to send multiple PRs for a variety of issues that are pending.

If it takes time, we can temporarily remove the support for nydus in nerdctl v2.0 and then put it back in v2.1+.
I think this is acceptable, as we are bumping up the major version.
Same applies to SOCI too.

Question for you: when ctd v2 is out, should we still care about that (u 20.04 + containerd 1.6)?

Yes, as they haven't reached EOL yet.

@apostasie apostasie force-pushed the go-mod-containerd-v2 branch 5 times, most recently from aa91775 to b297e8c Compare August 6, 2024 18:11
@apostasie
Copy link
Contributor Author

apostasie commented Aug 6, 2024

@AkihiroSuda

We are there:

  1. nydus and soci have been resolved - all dependencies we needed have been migrated and we are now using them instead of my forks
  2. the ONE exception to above is moby/pkg/sysinfo - I did fork it out into ./pkg/sysinfo - I do not see another good solution (some discussion with Sebastiaan over here: Update to containerd v2 nydus-snapshotter#604 (comment)) - also see note in go.mod and in pkg/sysinfo/README.md - let me know if this OK
  3. CI is green

EXCEPT Ubuntu 20.04 + containerd 1.6 it just hangs-up on inside TestStats and sits there forever - This definitely has to do with nerdctl stats and cgroups version, and I am out of my depth here. I'll keep investigating, but any pointer would be welcome - cc @fahedouch maybe you have an idea? (maybe related to #223) <- nvm, I figured it out. Sorry for the noise

This is ready for a first review.

@apostasie apostasie marked this pull request as ready for review August 6, 2024 18:37
@apostasie apostasie changed the title [CANARY] go.mod update to containerd v2 go.mod update to containerd v2 Aug 6, 2024
@apostasie apostasie force-pushed the go-mod-containerd-v2 branch from b297e8c to 4944d67 Compare August 6, 2024 18:40
Dockerfile Show resolved Hide resolved
@apostasie apostasie force-pushed the go-mod-containerd-v2 branch 3 times, most recently from 949d798 to d5c5602 Compare August 7, 2024 00:30
@apostasie
Copy link
Contributor Author

Rebased.

@apostasie
Copy link
Contributor Author

@AkihiroSuda PTAL at your convenience.
No rush of course.
I'll keep rebasing this when it drifts.

@AkihiroSuda AkihiroSuda requested a review from a team August 7, 2024 22:00
pkg/cioutil/container_io.go Show resolved Hide resolved
pkg/cmd/container/run_restart.go Show resolved Hide resolved
Comment on lines +36 to +42

"github.com/containerd/containerd/v2/core/remotes/docker"
"github.com/containerd/containerd/v2/core/remotes/docker/config"
"github.com/containerd/log"
"github.com/containerd/nerdctl/v2/pkg/api/types"
"github.com/containerd/nerdctl/v2/pkg/errutil"
"github.com/containerd/nerdctl/v2/pkg/imgutil/dockerconfigresolver"
Copy link
Member

Choose a reason for hiding this comment

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

Personally I don't think we need to separate these dependencies. now dependencies from github.com are split into 2 blocks. And in some places github.com/opencontainers deps are colocated with github.com/containerd deps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert this. No strong opinion one way or the other, but I am interested in having one consistent style across (which we do not have now).

go.mod Show resolved Hide resolved
Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@apostasie
Copy link
Contributor Author

lgtm, thanks

Thanks a lot for your time and review @djdongjin
Appreciated.

pkg/sysinfo/README.md Outdated Show resolved Hide resolved
@apostasie apostasie force-pushed the go-mod-containerd-v2 branch from d5c5602 to ba2a6ea Compare August 9, 2024 01:21
Signed-off-by: apostasie <[email protected]>
@apostasie apostasie force-pushed the go-mod-containerd-v2 branch from ba2a6ea to 98cc368 Compare August 9, 2024 01:27
@apostasie
Copy link
Contributor Author

@AkihiroSuda: comments addressed.

@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Aug 9, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 7aa8ca9 into containerd:main Aug 9, 2024
19 checks passed
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.

4 participants