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

Updated containerd1.7; google.golang.org/protobuf #1706

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

helsaawy
Copy link
Contributor

@helsaawy helsaawy commented Mar 21, 2023

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.

@helsaawy helsaawy changed the title Updated containerd1.17; google.golang.org/protobuf Updated containerd1.7; google.golang.org/protobuf Mar 21, 2023
@helsaawy helsaawy force-pushed the protobuff-google branch 8 times, most recently from 9a15d31 to 4a5886a Compare March 23, 2023 05:20
@helsaawy helsaawy marked this pull request as ready for review March 23, 2023 17:15
@helsaawy helsaawy requested a review from a team as a code owner March 23, 2023 17:15
@kevpar
Copy link
Member

kevpar commented Mar 23, 2023

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
Copy link
Contributor Author

helsaawy commented Mar 23, 2023

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
Copy link
Member

kevpar commented Mar 24, 2023

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
Copy link
Contributor

katiewasnothere commented Mar 24, 2023

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
Copy link
Contributor Author

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
Copy link
Member

kevpar commented Mar 24, 2023

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
Copy link
Contributor Author

helsaawy commented Mar 24, 2023

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

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

@helsaawy
Copy link
Contributor Author

helsaawy commented Apr 5, 2023

rebased to fix merge conflicts

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

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

@helsaawy helsaawy force-pushed the protobuff-google branch 4 times, most recently from d115c70 to fa9f441 Compare May 5, 2023 14:32
@helsaawy helsaawy force-pushed the protobuff-google branch 3 times, most recently from b58bd6d to a4487f8 Compare May 18, 2023 16:10
@helsaawy helsaawy force-pushed the protobuff-google branch from a4487f8 to 942ab47 Compare May 26, 2023 17:34
@helsaawy helsaawy force-pushed the protobuff-google branch 4 times, most recently from 1f2ed6a to c78d252 Compare June 3, 2023 00:44
Copy link
Contributor

@ambarve ambarve left a comment

Choose a reason for hiding this comment

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

Minor question but LGTM otherwise!

scripts/Update-Proto.ps1 Outdated Show resolved Hide resolved
@helsaawy helsaawy force-pushed the protobuff-google branch 2 times, most recently from 2729488 to 79a6d0c Compare June 5, 2023 22:52
@helsaawy
Copy link
Contributor Author

helsaawy commented Jun 5, 2023

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

helsaawy added 2 commits July 6, 2023 13:37
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 <hamzaelsaawy@microsoft.com>
Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy helsaawy force-pushed the protobuff-google branch from 2be52e1 to f60f498 Compare July 6, 2023 17:38
@helsaawy helsaawy merged commit decae4b into microsoft:main Jul 10, 2023
@helsaawy helsaawy deleted the protobuff-google branch July 10, 2023 15:25
@helsaawy helsaawy mentioned this pull request Jul 19, 2023
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
* 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 <hamzaelsaawy@microsoft.com>

* Vendor protobuf import changes

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

---------

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants