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 #1305

Closed
wants to merge 1 commit into from
Closed

Update to containerd v2 #1305

wants to merge 1 commit into from

Conversation

apostasie
Copy link

@apostasie apostasie commented Jul 4, 2024

Issue #, if available:

Description of changes:

This PR updates to containerd v2.

Testing performed:

make build and make test pass locally.
Pending further testing into nerdctl.

References to cri api v1alpha could/should be removed obviously, but that might be independently of this I guess.

Lmk your toughts.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@sondavidb sondavidb left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution! We don't plan on merging until containerd v2.0.0, but this generally looks like the right direction.

Left a couple of quick questions, but otherwise looks good. Would you like me to run the CI?

go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@apostasie
Copy link
Author

Hey, thanks for the contribution! We don't plan on merging until containerd v2.0.0, but this generally looks like the right direction.

Left a couple of quick questions, but otherwise looks good. Would you like me to run the CI?

Thanks a lot @sondavidb

Having the CI run would be lovely so I can fix any lingering issues with the transition.

Ok of course on merging only after ctd v2 is out - or whenever you folks feel this is ready.

Cheers.

@sondavidb
Copy link
Contributor

CI looks good, we'll probably end up waiting for the 2.0 release. Thanks again!

@apostasie
Copy link
Author

Just repushed without the extraneous toolchain statement.

Thanks a lot @sondavidb !

@apostasie apostasie marked this pull request as ready for review July 9, 2024 19:03
@apostasie apostasie requested a review from a team as a code owner July 9, 2024 19:03
@apostasie
Copy link
Author

@sondavidb obviously it will drift from main.
Happy to rebase whenever you need it so just ping me here

@sondavidb
Copy link
Contributor

Yeah, we just had to fix some deprecated packages in #1308 to get past our linting CI, I don't foresee this happening again anytime soon so it might be best to rebase now if you don't mind @apostasie. Anything after this I won't bother you about until 2.0 comes out.

@github-actions github-actions bot added dependencies Pull requests that update a dependency file go Pull requests that update Go code benchmarking Performance measurement testing Unit and/or integration tests labels Jul 10, 2024
@apostasie
Copy link
Author

Yup. Done.

Can you get the CI to go brrr?
(that type of rebase with dependencies versions flying all over always make me feel a bit nervous)

Thanks a lot @sondavidb !

@apostasie
Copy link
Author

apostasie commented Jul 10, 2024

Yep, it is hosed.
Damn it.
Will fix.

Signed-off-by: apostasie <spam_blackhole@farcloser.world>
@apostasie
Copy link
Author

Repushed. Confirmed working locally. Sorry for the churn.

@sondavidb
Copy link
Contributor

All good, this work is greatly appreciated on our end! Thanks again!

@apostasie
Copy link
Author

Hey @sondavidb

Question for you: would you be amenable to merging this PR into an ad-hoc / dedicated branch (something like experimental-ctdv2)?

That way we could use this branch in nerdctl as we push through for v2 - and then we would switch the dependency back to soci main / your next release, when you folks are ready to have it on main (eg: post containerd v2 release as you said earlier).
Alternatively, we could temporarily disable soci in the first version of nerdctl v2, and re-integrate it later on, but I personally feel it would be better to get public QA / exposure for the entire feature set on nerdctl v2.

Let me know your thoughts.

Cheers!

@sondavidb
Copy link
Contributor

I'll see what the team thinks and get back to you ASAP. As it's Friday afternoon I'm not expecting a consensus until after the weekend, though.

@apostasie
Copy link
Author

I'll see what the team thinks and get back to you ASAP. As it's Friday afternoon I'm not expecting a consensus until after the weekend, though.

No rush, really.
Appreciate your help! Thanks a lot and enjoy the week-end :)

@sondavidb
Copy link
Contributor

sondavidb commented Jul 26, 2024

Thanks, you too!

One more question — to be clear, we only need a branch/commit, not an entire release, correct?

@apostasie
Copy link
Author

Thanks, you too!

One more question — to be clear, we only need a branch/commit, not an entire release, correct?

Yep, would be happy with just a branch/commit.

@sondavidb
Copy link
Contributor

sondavidb commented Jul 30, 2024

Hey @apostasie, apologies this slipped my mind.

Just created a new branch, upgrade-containerd-v2, you can edit the PR to be against this branch. Feel free to ping me whenever that's done and I can take a look.

@apostasie
Copy link
Author

Thanks a lot @sondavidb
Really appreciate this!

Closing in favor of #1329

@apostasie apostasie closed this Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarking Performance measurement dependencies Pull requests that update a dependency file go Pull requests that update Go code testing Unit and/or integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants