Use google.golang.org/protobuf instead of github.com/gogo/protobuf by kzys · Pull Request #220 · containerd/cgroups

kzys · 2022-02-17T05:20:57Z

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

See containerd/containerd#6564 for the detail.

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

kzys · 2022-02-17T17:50:37Z

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.

kzys · 2022-02-18T20:31:48Z

Makefile


 proto:
 	protobuild --quiet ${PACKAGES}
+	go-fix-acronym -w -a Cpu -a Rss -a Tcp \


go-fix-acronym is containerd/protobuild#50

And it is merged!

estesp · 2022-02-21T22:13:10Z

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?

kzys · 2022-02-21T22:46:46Z

@estesp Yes. containerd/cgroups and containerd/ttrpc would have a new branch (ttrpc is being discussed in containerd/ttrpc#97).

AkihiroSuda · 2022-10-18T03:20:20Z

What’s current status of this?

kzys · 2022-10-19T15:34:51Z

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

kzys · 2022-10-28T23:42:03Z

Tested this gogo-less cgroups with hcsshim and containerd.

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

AkihiroSuda · 2022-11-03T01:22:26Z

.github/workflows/ci.yml

+      - name: Install Go
+        uses: actions/setup-go@v2
+        with:
+          go-version: '1.17.x'


Any reason not to use 1.19 ?

Because this PR is old. Updated.

AkihiroSuda · 2022-11-03T01:22:42Z

.github/workflows/ci.yml

+          echo "${{ github.workspace }}/bin" >> $GITHUB_PATH
+
+      - name: Checkout cgroups
+        uses: actions/checkout@v2


Can we use v3?

We'd like to migrate off from gogo/protobuf. The package is looking for new ownership since 2020. See containerd/containerd#6564 for the detail. Signed-off-by: Kazuyoshi Kato <[email protected]>

estesp

LGTM

thaJeztah · 2022-11-03T11:45:10Z

🎉 awesome work, @kzys !

AkihiroSuda · 2022-11-03T23:26:41Z

Shall we release v1.1.0?

kzys · 2022-11-04T18:01:48Z

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

thaJeztah · 2022-11-04T18:30:02Z

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/

kzys · 2022-11-04T19:18:32Z

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

thaJeztah · 2022-11-04T19:33:07Z

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)

AkihiroSuda · 2023-01-30T16:30:07Z

stats/v1/metrics.pb.go

-	proto.RegisterType((*RdmaStat)(nil), "io.containerd.cgroups.v1.RdmaStat")
-	proto.RegisterType((*RdmaEntry)(nil), "io.containerd.cgroups.v1.RdmaEntry")
-	proto.RegisterType((*NetworkStat)(nil), "io.containerd.cgroups.v1.NetworkStat")
-	proto.RegisterType((*CgroupStats)(nil), "io.containerd.cgroups.v1.CgroupStats")


Where has this gone?

kzys force-pushed the bye-gogo branch 3 times, most recently from c987035 to 0686207 Compare February 17, 2022 17:38

kzys marked this pull request as ready for review February 17, 2022 17:39

kzys force-pushed the bye-gogo branch 2 times, most recently from 518baa2 to 07fa92f Compare February 17, 2022 17:43

kzys mentioned this pull request Feb 17, 2022

protoc-gen-gogoctrd wouldn't work without gogo/protobuf containerd/containerd#6219

Closed

kzys force-pushed the bye-gogo branch 2 times, most recently from 9c33513 to 9752089 Compare February 18, 2022 20:30

kzys commented Feb 18, 2022

View reviewed changes

kzys requested a review from estesp February 21, 2022 19:58

kzys force-pushed the bye-gogo branch 15 times, most recently from 7fca754 to d862c38 Compare March 1, 2022 19:51

dmcgowan approved these changes Mar 15, 2022

View reviewed changes

kzys force-pushed the bye-gogo branch 8 times, most recently from 2fc1226 to 75101da Compare August 26, 2022 23:27

crazy-max mentioned this pull request Oct 5, 2022

Migrate away from gogo moby/buildkit#3119

Closed

kzys force-pushed the bye-gogo branch from 75101da to cd2ccc8 Compare October 19, 2022 15:32

dmcgowan approved these changes Nov 1, 2022

View reviewed changes

AkihiroSuda reviewed Nov 3, 2022

View reviewed changes

AkihiroSuda approved these changes Nov 3, 2022

View reviewed changes

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

7746639

We'd like to migrate off from gogo/protobuf. The package is looking for new ownership since 2020. See containerd/containerd#6564 for the detail. Signed-off-by: Kazuyoshi Kato <[email protected]>

kzys force-pushed the bye-gogo branch from cd2ccc8 to 7746639 Compare November 3, 2022 03:24

estesp approved these changes Nov 3, 2022

View reviewed changes

estesp merged commit f0f7617 into containerd:main Nov 3, 2022

This was referenced Jan 30, 2023

vendor: github.com/containerd/cgroups/v3 v3.0.1 moby/moby#44886

Merged

go.mod: github.com/containerd/cgroups/v3 v3.0.1, github.com/docker/docker v24.0.0 containerd/nerdctl#1952

Merged

AkihiroSuda reviewed Jan 30, 2023

View reviewed changes

Conversation

kzys commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzys commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzys Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

kzys Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

estesp commented Feb 21, 2022

Uh oh!

kzys commented Feb 21, 2022

Uh oh!

AkihiroSuda commented Oct 18, 2022

Uh oh!

kzys commented Oct 19, 2022

Uh oh!

kzys commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

kzys Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

kzys Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 3, 2022

Uh oh!

AkihiroSuda commented Nov 3, 2022

Uh oh!

kzys commented Nov 4, 2022

Uh oh!

thaJeztah commented Nov 4, 2022

Uh oh!

kzys commented Nov 4, 2022

Uh oh!

thaJeztah commented Nov 4, 2022

Uh oh!

AkihiroSuda Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kzys commented Feb 17, 2022 •

edited

Loading

kzys commented Feb 17, 2022 •

edited

Loading

kzys commented Oct 28, 2022 •

edited

Loading