cgroups Use google.golang.org/protobuf instead of github.com/gogo/protobuf

We'd like to migrate off from gogo/protobuf. The package is looking for new ownership since 2020.

See https://github.com/containerd/containerd/issues/6564 for the detail.

Signed-off-by: Kazuyoshi Kato [email protected]

Feb 17 '22 05:02 kzys

This PR is ready, but it would break consumers that use cgroups from protos, such as https://github.com/Microsoft/hcsshim. The shim is importing the package like https://github.com/microsoft/hcsshim/blob/12b02a180881297d330d6261e166a97a79023fe2/cmd/containerd-shim-runhcs-v1/stats/stats.pb.go#L8 and stats.pb.go assumes that the file is generated by gogo/protobuf.

It would be better to have a branch for backporting bug fixes while other customers (hcsshim and containerd) are using gogo/protobuf.

Feb 17 '22 17:02 kzys

So, we can do the same thing here we do in containerd (have a /release/1.0.x branch anchored at the recent v1.0.3 tag that cuts all future 1.0.x releases) but I think this would be the first subproject to get that treatment. Seems like it is somewhat inevitable as we will have overlap of containerd releases that use both old and new protobuf. Does that seem right?

Feb 21 '22 22:02 estesp

@estesp Yes. containerd/cgroups and containerd/ttrpc would have a new branch (ttrpc is being discussed in https://github.com/containerd/ttrpc/issues/97).

Feb 21 '22 22:02 kzys

I see there's a couple of fixes in main that have been merged (https://github.com/containerd/cgroups/compare/v1.0.3... f8328fdc061d6fb757b370c19ad8f0d05a281a41); but not yet released. Given that this change could be (potentially?) risky, I'm wondering;

should we do a new tag with the other changes first (before merging this?)
what version should we use for the changes of this PR? (should we make it v1.1.x is it "risky" enough to warrant v2.x.x)? Mostly considering there to keep the option of doing patch/fix release for the existing v1.(0).x releases if there's a need

Mar 21 '22 18:03 thaJeztah

should we do a new tag with the other changes first (before merging this?)

We should.

what version should we use for the changes of this PR? (should we make it v1.1.x is it "risky" enough to warrant v2.x.x)?

I'm inclined to have 1.1.x. It should not impact most of clients.

Mar 21 '22 20:03 kzys

I'm inclined to have 1.1.x. It should not impact most of clients.

Given that v1.1.0 would be considered a minor (feature only, backward compatible) update, there is a chance that repositories automatically update to that version; should we do a test in some repositories (e.g. a draft PR that updates the containerd 1.5 branch to use this version) to check if it doesn't break?

Mar 21 '22 20:03 thaJeztah

Yes. I was doing https://github.com/containerd/containerd/pull/6563 but can't recall the reason to upgrade hcsshim. Let me check.

Mar 21 '22 22:03 kzys

Yes. I was doing containerd/containerd#6563 but can't recall the reason to upgrade hcsshim. Let me check.

Ah! Think I missed that one. I hope that one works (also let me know if you want me to try this branch on other repositories).

If it doesn't work, I think the only "good" option would be to make it a V2, taking into account that we have circular dependencies in various repositories, which makes updating to a non-compatible version more challenging (having a v2 means they can potentially co-exist during the transition).

Mar 22 '22 14:03 thaJeztah

Regarding hcsshim, the shim is embedding this cgroup package's message is its own message. If one of them is generated by gogo (or vice versa, probably), a protobuf serializer couldn't handle another.

https://github.com/microsoft/hcsshim/blob/7fa8bda4e6ba503caf0d53d0a4ee99b9a64ceed8/cmd/containerd-shim-runhcs-v1/stats/stats.proto#L11-L17

Apr 06 '22 17:04 kzys

Let me convert that to draft for a moment. I'd like to sync-up with hcsshim folks first to see how to handle this upgrade in there.

Apr 06 '22 17:04 kzys

Working on https://github.com/microsoft/hcsshim/pull/1356, which results https://github.com/containerd/protobuild/pull/52.

Apr 18 '22 20:04 kzys

What’s current status of this?

Oct 18 '22 03:10 AkihiroSuda

@AkihiroSuda Just rebased against main. I will test the branch against containerd and possibly hcsshim.

Oct 19 '22 15:10 kzys

Tested this gogo-less cgroups with hcsshim and containerd.

https://github.com/microsoft/hcsshim/pull/1356
https://github.com/containerd/containerd/pull/7601

Actually containerd cannot have this cgroups upgrade unless hcsshim is upgraded.

Oct 28 '22 23:10 kzys

🎉 awesome work, @kzys !

Nov 03 '22 11:11 thaJeztah

Shall we release v1.1.0?

Nov 03 '22 23:11 AkihiroSuda

@AkihiroSuda After reading https://semver.org/ again and working on hcsshim, I'm more inclined to have v2.0.0. Thoughts?

Nov 04 '22 18:11 kzys

v2 sounds like a good idea, that would keep the path open to have projects opt-in to the new thing. But it will require the module to be renamed to /v2/

Nov 04 '22 18:11 thaJeztah

Ugh. We will have github.com/containerd/cgroups/v2/v2/stats by doing so, but we would reach the state eventually anyway...

Nov 04 '22 19:11 kzys

Given that it's v2, it would allow us to rename that package to something else 🤔

(naming is hard though!)

Perhaps we could do the reverse, and rename the v1 cgroups? (as v2 basically is "default" now, so "v1" will more and more become the outlier)

Nov 04 '22 19:11 thaJeztah