Updated containerd1.7; google.golang.org/protobuf by helsaawy · Pull Request #1706 · microsoft/hcsshim

helsaawy · 2023-03-21T16:35:44Z

Update to containerd 1.7 and move to google.golang.org/protobuf from github.com/gogo/protobuf/gogoproto.

Changes are broken out in two commits: second commit has vendor updates, so they don't clog reviewing the original change

These two changes are intertwined, since containerd 1.7 moves and changes its ttrpc task server definitions (containerd/containerd#6827) and protobuff generation (containerd/containerd#6841), as well as some other API changes.
Containerd updated to "github.com/containerd/typeurl/v2", so now the types it registers by default (in "github.com/containerd/containerd/runtime"'s init) are not available to the shim code unless the hcsshim repo either (1) switches to typeurl/v2 or (2) duplicates the containerd typerurl registration code.
Since 1.7 does not use gogoproto, all references to it (eg "github.com/gogo/protobuf/types".Empty) have to be updated.
Upgrading to google.golang.org/protobuf also requires updating the containerd/cgroups dependency to v3
(github.com/containerd/cgroups/v3/cgroup1/stats/), since the older v1 still uses gogoproto.
Additionally, the containerd protoc-gen-gogoctrd is now deprecated and not included in containerd 1.7, so that generator is removed.

The changes could be isolated, so only ./cmd/containerd-shim-runhcs-v1 (and internal code it relies on) that implements the containerd task server and interfaces with containerd is updated.
However, this would result in bifurcating the project in two, with parts of the code base using the deprecated protobuf packages and marshalling, and requiring two different protobuf generation procedures (including cloning containerd 1.6 to use the now-deprecated protoc-gen-gogoctrd generator).
This would likely add more work and complicate project management.
It is also not guaranteed that there would be a clean split between all the code, as there is likely shared code that would need to be updated to either use the new protobuf type ("google.golang.org/protobuf/types/known/emptypb", "google.golang.org/protobuf/types/known/timestamppb", and co) or keep the old types from "github.com/gogo/protobuf/types"

The protoc-gen-go-grpc generator do not allow directives such as gogoproto.customname, so the go-fix-acronym command is used to update acronym customization (similar to what containerd does).

Added protobuild and protobuf grpc and ttrpc generators to tools.go so they are tracked and vendored, and can be trivially installed via go install.

Added an Update-Proto.ps1 script to re-gerenate protobuf files locally and in GitHub CI.

kevpar · 2023-03-23T17:22:39Z

Can we split this into two PRs:

Moving off of gogo
Updating to containerd 1.7, and any other changes specific to that

Edit: To clarify, I see from your description that we need to move to gogo to move to 1.7, but I'm wondering if we also have to move to 1.7 to move to gogo?

helsaawy · 2023-03-23T17:28:19Z

Edit: To clarify, I see from your description that we need to move to gogo to move to 1.7, but I'm wondering if we also have to move to 1.7 to move to gogo?

Yes, since if we move off of gogo, then our marshalling will differ from containerd1.6, and we wont be able to communicate with them (and likely won't implement task server correctly, since the return types will be different).

edit: the task server interface specifies gogo return types: https://github.com/containerd/containerd/blob/de33abf0547cb56bed2b7ea74cbbf0fe903c00b8/runtime/v2/task/shim.pb.go#L3429
plus, the requests themselves have gogo types within them:
https://github.com/containerd/containerd/blob/de33abf0547cb56bed2b7ea74cbbf0fe903c00b8/runtime/v2/task/shim.pb.go#L36

kevpar · 2023-03-24T17:56:26Z

Yes, since if we move off of gogo, then our marshalling will differ from containerd1.6, and we wont be able to communicate with them (and likely won't implement task server correctly, since the return types will be different).

I'm confused by this. As long as both sides are speaking protobuf, it shouldn't matter what internal implementation types they use, right?

katiewasnothere · 2023-03-24T18:38:04Z

I'm confused by this. As long as both sides are speaking protobuf, it shouldn't matter what internal implementation types they use, right?

@kevpar I think there's some bug in the protobuf code. With the cri-containerd tests, I've been seeing errors like this even on main because the protobuf versions are different:

runpodsandbox_test.go:1927: failed to exec `ps` in the uvm with ttrpc: cannot marshal unknown type: *shimdiag.ExecProcessRequest

helsaawy · 2023-03-24T18:57:23Z

I'm confused by this. As long as both sides are speaking protobuf, it shouldn't matter what internal implementation types they use, right?

technically since grpc isnt fully self-describing, it needs a type to unpack it into when unmarshalling the byte payload.
if the (ttrpc) server is expecting a "time".Time instead of "google.golang.org/protobuf/types/known/timestamppb".Timestamp (or similar for Any), it will error out since the underlying data won't fit. (I think the Any and Empty types are the same, but I doubt theres a guarantee for that compatibility)

kevpar · 2023-03-24T19:50:17Z

technically since grpc isnt fully self-describing, it needs a type to unpack it into when unmarshalling the byte payload. if the (ttrpc) server is expecting a "time".Time instead of "google.golang.org/protobuf/types/known/timestamppb".Timestamp (or similar for Any), it will error out since the underlying data won't fit. (I think the Any and Empty types are the same, but I doubt theres a guarantee for that compatibility)

I must be missing something here. time.Time would never be sent via protobuf, instead we would send google.protobuf.Timestamp, and only on the client or server end does it convert it into a Go type to work with. As long as the same protobuf definitions are used on both ends, and the protobuf wire format is used to transport between, I don't see how the client's or server's choice of language package would make a difference.

Is there a specific example of the issue you can share?

helsaawy · 2023-03-24T20:08:14Z

I must be missing something here. time.Time would never be sent via protobuf, instead we would send google.protobuf.Timestamp, and only on the client or server end does it convert it into a Go type to work with. As long as the same protobuf definitions are used on both ends, and the protobuf wire format is used to transport between, I don't see how the client's or server's choice of language package would make a difference.

oh, youre right, I was mistaken.
but the issue still remains that the generated protobuf structs live in containerd for the taskserver
so if we wanted to switch to google.golang.org/protobuf while on containerd1.6, we'd either need to

fork their runtime/v2/task/shim.proto file and generate it with the new protobuf stack; or
have half our code deal with the old gogo types and the other half use the new one

helsaawy · 2023-04-05T17:37:44Z

rebased to fix merge conflicts

katiewasnothere

lgtm. Should we make like a last release of the shim pre-containerd 1.7 change?

ambarve

Minor question but LGTM otherwise!

helsaawy · 2023-06-05T22:53:06Z

Rebased to get mockgen changes, and update dependencies to the latest version

Update to containerd 1.7 and move to `google.golang.org/protobuf` from `github.com/gogo/protobuf/gogoproto`. These two changes are intertwined, since containerd 1.7 changes its ttrpc task server definitions and protobuff generation (as well as some other API changes). Additionally, the task server gRPC code is imported from containerd directly, rather than being generated here, and that code now explicitly imports `google.golang.org/protobuf` instead of `github.com/gogo/protobuf/gogoproto`. Upgrading to `google.golang.org/protobuf` also requires updating the `containerd/cgroups` dependency to v3 (`github.com/containerd/cgroups/v3/cgroup1/stats/`). The new `protoc-gen-go-grpc` generators do not allow directives such as `gogoproto.customname`, so the `go-fix-acronym` command is used to update acronym customization (which is what containerd does). Updated `Protobuild.toml` to specify new generators. Added an `Update-Proto.ps1` script to re-generate protobuf files locally and in GitHub CI. Add `protobuild` and protobuff `grpc` and `ttrpc` generators to `tools.go` so they are tracked and vendored, and can be trivially installed via `go install`. Signed-off-by: Hamza El-Saawy <[email protected]>

Signed-off-by: Hamza El-Saawy <[email protected]>

* Update containerd1.7; google.golang.org/protobuf Update to containerd 1.7 and move to `google.golang.org/protobuf` from `github.com/gogo/protobuf/gogoproto`. These two changes are intertwined, since containerd 1.7 changes its ttrpc task server definitions and protobuff generation (as well as some other API changes). Additionally, the task server gRPC code is imported from containerd directly, rather than being generated here, and that code now explicitly imports `google.golang.org/protobuf` instead of `github.com/gogo/protobuf/gogoproto`. Upgrading to `google.golang.org/protobuf` also requires updating the `containerd/cgroups` dependency to v3 (`github.com/containerd/cgroups/v3/cgroup1/stats/`). The new `protoc-gen-go-grpc` generators do not allow directives such as `gogoproto.customname`, so the `go-fix-acronym` command is used to update acronym customization (which is what containerd does). Updated `Protobuild.toml` to specify new generators. Added an `Update-Proto.ps1` script to re-generate protobuf files locally and in GitHub CI. Add `protobuild` and protobuff `grpc` and `ttrpc` generators to `tools.go` so they are tracked and vendored, and can be trivially installed via `go install`. Signed-off-by: Hamza El-Saawy <[email protected]> * Vendor protobuf import changes Signed-off-by: Hamza El-Saawy <[email protected]> --------- Signed-off-by: Hamza El-Saawy <[email protected]>

helsaawy changed the title ~~Updated containerd1.17; google.golang.org/protobuf~~ Updated containerd1.7; google.golang.org/protobuf Mar 21, 2023

helsaawy force-pushed the protobuff-google branch 8 times, most recently from 9a15d31 to 4a5886a Compare March 23, 2023 05:20

helsaawy marked this pull request as ready for review March 23, 2023 17:15

helsaawy requested a review from a team as a code owner March 23, 2023 17:15

helsaawy force-pushed the protobuff-google branch from 4a5886a to cb76362 Compare March 23, 2023 17:26

katiewasnothere reviewed Mar 24, 2023

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

katiewasnothere reviewed Mar 24, 2023

View reviewed changes

Comment thread scripts/Update-Proto.ps1 Outdated

katiewasnothere reviewed Mar 24, 2023

View reviewed changes

Comment thread scripts/Update-Proto.ps1 Outdated

msscotb assigned kevpar and katiewasnothere Mar 29, 2023

helsaawy force-pushed the protobuff-google branch from cb76362 to cbb3260 Compare April 5, 2023 17:37

katiewasnothere approved these changes Apr 6, 2023

View reviewed changes

helsaawy force-pushed the protobuff-google branch from cbb3260 to 0cb4dd1 Compare April 14, 2023 19:52

anmaxvl mentioned this pull request Apr 17, 2023

Fixing the errors for missing enforcement points #1735

Merged

helsaawy force-pushed the protobuff-google branch from 0cb4dd1 to 274836c Compare May 4, 2023 15:05

helsaawy force-pushed the protobuff-google branch 4 times, most recently from d115c70 to fa9f441 Compare May 5, 2023 14:32

helsaawy force-pushed the protobuff-google branch 3 times, most recently from b58bd6d to a4487f8 Compare May 18, 2023 16:10

helsaawy force-pushed the protobuff-google branch from a4487f8 to 942ab47 Compare May 26, 2023 17:34

helsaawy force-pushed the protobuff-google branch 4 times, most recently from 1f2ed6a to c78d252 Compare June 3, 2023 00:44

ambarve approved these changes Jun 5, 2023

View reviewed changes

Comment thread scripts/Update-Proto.ps1 Outdated

helsaawy force-pushed the protobuff-google branch 2 times, most recently from 2729488 to 79a6d0c Compare June 5, 2023 22:52

helsaawy force-pushed the protobuff-google branch from 79a6d0c to 2be52e1 Compare June 30, 2023 16:26

helsaawy added 2 commits July 6, 2023 13:37

Vendor protobuf import changes

f60f498

Signed-off-by: Hamza El-Saawy <[email protected]>

helsaawy force-pushed the protobuff-google branch from 2be52e1 to f60f498 Compare July 6, 2023 17:38

msscotb approved these changes Jul 8, 2023

View reviewed changes

helsaawy merged commit decae4b into microsoft:main Jul 10, 2023

helsaawy deleted the protobuff-google branch July 10, 2023 15:25

helsaawy mentioned this pull request Jul 19, 2023

Vendor protobuf tools #1592

Closed

pacoxu mentioned this pull request Jul 16, 2024

bump Microsoft/hcsshim to v0.12.5: use containerd/cgroup/v3 kubernetes/kubernetes#126111

Closed

Conversation

helsaawy commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevpar commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

helsaawy commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevpar commented Mar 24, 2023

Uh oh!

katiewasnothere commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

helsaawy commented Mar 24, 2023

Uh oh!

Uh oh!

kevpar commented Mar 24, 2023

Uh oh!

helsaawy commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

helsaawy commented Apr 5, 2023

Uh oh!

katiewasnothere left a comment

Choose a reason for hiding this comment

Uh oh!

ambarve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

helsaawy commented Jun 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

helsaawy commented Mar 21, 2023 •

edited

Loading

kevpar commented Mar 23, 2023 •

edited

Loading

helsaawy commented Mar 23, 2023 •

edited

Loading

katiewasnothere commented Mar 24, 2023 •

edited

Loading

helsaawy commented Mar 24, 2023 •

edited

Loading