Use google.golang.org/protobuf instead of github.com/gogo/protobuf #220
Conversation
kzys
force-pushed
the
bye-gogo
branch
3 times, most recently
from
c987035 to
0686207
Compare
kzys
force-pushed
the
bye-gogo
branch
2 times, most recently
from
518baa2 to
07fa92f
Compare
|
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
force-pushed
the
bye-gogo
branch
2 times, most recently
from
9c33513 to
9752089
Compare
Makefile
Outdated
| @@ -22,3 +22,5 @@ cgutil: | |||
|
|
|||
| proto: | |||
| protobuild --quiet ${PACKAGES} | |||
| go-fix-acronym -w -a Cpu -a Rss -a Tcp \ | |||
There was a problem hiding this comment.
go-fix-acronym is containerd/protobuild#50
|
So, we can do the same thing here we do in containerd (have a |
|
@estesp Yes. |
kzys
force-pushed
the
bye-gogo
branch
15 times, most recently
from
7fca754 to
d862c38
Compare
kzys
force-pushed
the
bye-gogo
branch
6 times, most recently
from
2fc1226 to
75101da
Compare
|
What’s current status of this? |
|
@AkihiroSuda Just rebased against main. I will test the branch against containerd and possibly hcsshim. |
|
Tested this gogo-less cgroups with hcsshim and containerd.
Actually containerd cannot have this cgroups upgrade unless hcsshim is upgraded. |
.github/workflows/ci.yml
Outdated
| - name: Install Go | ||
| uses: actions/setup-go@v2 | ||
| with: | ||
| go-version: '1.17.x' |
There was a problem hiding this comment.
Any reason not to use 1.19 ?
There was a problem hiding this comment.
Because this PR is old. Updated.
.github/workflows/ci.yml
Outdated
| echo "${{ github.workspace }}/bin" >> $GITHUB_PATH | ||
|
|
||
| - name: Checkout cgroups | ||
| uses: actions/checkout@v2 |
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]>
|
🎉 awesome work, @kzys ! |
|
Shall we release v1.1.0? |
|
@AkihiroSuda After reading https://semver.org/ again and working on hcsshim, I'm more inclined to have v2.0.0. Thoughts? |
|
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/ |
|
Ugh. We will have github.com/containerd/cgroups/v2/v2/stats by doing so, but we would reach the state eventually anyway... |
|
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) |
| 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") |
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]