Remove gogo from proto bindings (part 1: CRI) #128653
Conversation
2410a38 to
ecfff8c
Compare
cbc9518 to
057da0a
Compare
e4cf381 to
cb48f0b
Compare
|
@saschagrunert the alternative has to be backwards compatible and AFAIK that is the main blocker of getting rid of gogo-protobug /cc: @liggitt |
|
👋 Hello! @saschagrunert @liggitt If so, a gentle reminder that the code freeze has started 02:00 UTC Friday 21st March 2025 . Please make sure any PRs have both |
|
/retest |
Signed-off-by: Sascha Grunert <[email protected]>
|
/retest |
1 similar comment
|
/retest |
|
@saschagrunert: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -76,17 +91,36 @@ function kube::protoc::protoc() { | |||
| ) | |||
| } | |||
|
|
|||
| # Generates $1/api.pb.go from the protobuf file $1/api.proto without using gogo | |||
| # $1: Full path to the directory where the api.proto file is | |||
| function kube::protoc::protoc() { | |||
There was a problem hiding this comment.
I think that could be a follow-up that transitions both the kube::protoc::protoc_gogo / kube::protoc::protoc function names to add ::internal:: segments ... keeping this as focused on the specific delta between gogo protoc is helpful for reviewing this PR
There was a problem hiding this comment.
any chance we can avoid an entire new script that replicates everything but the kube::protoc::generate_proto_gogo call?
we wouldn't need hack/_update-generated-proto-bindings-gogo-dockerized.sh if we require telling _update-generated-proto-bindings-dockerized.sh which generator to use, like this:
diff --git a/hack/_update-generated-proto-bindings-dockerized.sh b/hack/_update-generated-proto-bindings-dockerized.sh
index 163143b1af4..127f9a240a5 100755
--- a/hack/_update-generated-proto-bindings-dockerized.sh
+++ b/hack/_update-generated-proto-bindings-dockerized.sh
@@ -26,11 +26,13 @@ source "${KUBE_ROOT}/hack/lib/init.sh"
source "${KUBE_ROOT}/hack/lib/protoc.sh"
source "${KUBE_ROOT}/hack/lib/util.sh"
-if [ "$#" == 0 ]; then
- echo "usage: $0 <api_dir>..."
+if (( $# < 2 )); then
+ echo "usage: $0 [gogo|protoc] <api_dir>..."
exit 1
fi
+generator=$1; shift
+
kube::protoc::check_protoc
for api; do
@@ -47,6 +49,16 @@ for api; do
for file in "${protos[@]}"; do
dir="$(dirname "${file}")"
- kube::protoc::generate_proto "${dir}"
+ case "${generator}" in
+ gogo)
+ kube::protoc::generate_proto_gogo "${dir}"
+ ;;
+ protoc)
+ kube::protoc::generate_proto "${dir}"
+ ;;
+ *)
+ echo "Unknown generator ${generator}" >&2
+ exit 1
+ esac
done
done
diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh
index a653eed737b..bb8a43149d5 100755
--- a/hack/update-codegen.sh
+++ b/hack/update-codegen.sh
@@ -945,15 +945,15 @@ function codegen::protobindings() {
done
if kube::protoc::check_protoc >/dev/null; then
- hack/_update-generated-proto-bindings-gogo-dockerized.sh "${apis_using_gogo[@]}"
- hack/_update-generated-proto-bindings-dockerized.sh "${apis_using_protoc[@]}"
+ hack/_update-generated-proto-bindings-dockerized.sh gogo "${apis_using_gogo[@]}"
+ hack/_update-generated-proto-bindings-dockerized.sh protoc "${apis_using_protoc[@]}"
else
kube::log::status "protoc ${PROTOC_VERSION} not found (can install with hack/install-protoc.sh); generating containerized..."
# NOTE: All output from this script needs to be copied back to the calling
# source tree. This is managed in kube::build::copy_output in build/common.sh.
# If the output set is changed update that function.
- build/run.sh hack/_update-generated-proto-bindings-gogo-dockerized.sh "${apis_using_gogo[@]}"
- build/run.sh hack/_update-generated-proto-bindings-dockerized.sh "${apis_using_protoc[@]}"
+ build/run.sh hack/_update-generated-proto-bindings-dockerized.sh gogo "${apis_using_gogo[@]}"
+ build/run.sh hack/_update-generated-proto-bindings-dockerized.sh protoc "${apis_using_protoc[@]}"
fi
}| @@ -160,7 +160,7 @@ func (p *criStatsProvider) listPodStatsPartiallyFromCRI(ctx context.Context, upd | |||
| // fsIDtoInfo is a map from filesystem id to its stats. This will be used | |||
| // as a cache to avoid querying cAdvisor for the filesystem stats with the | |||
| // same filesystem id many times. | |||
| fsIDtoInfo := make(map[runtimeapi.FilesystemIdentifier]*cadvisorapiv2.FsInfo) | |||
| fsIDtoInfo := make(map[*runtimeapi.FilesystemIdentifier]*cadvisorapiv2.FsInfo) | |||
There was a problem hiding this comment.
old:
// FilesystemIdentifier uniquely identify the filesystem.
type FilesystemIdentifier struct {
// Mountpoint of a filesystem.
Mountpoint string `protobuf:"bytes,1,opt,name=mountpoint,proto3" json:"mountpoint,omitempty"`
XXX_NoUnkeyedLiteral struct{} `json:"-"`
XXX_sizecache int32 `json:"-"`
}
new:
// FilesystemIdentifier uniquely identify the filesystem.
type FilesystemIdentifier struct {
state protoimpl.MessageState `protogen:"open.v1"`
// Mountpoint of a filesystem.
Mountpoint string `protobuf:"bytes,1,opt,name=mountpoint,proto3" json:"mountpoint,omitempty"`
unknownFields protoimpl.UnknownFields
sizeCache protoimpl.SizeCache
}
the proto messages aren't comparable any more, so we can't use them as a map key, but changing the key to a pointer smells like a bug if this map was expecting equivalent instances to key the same way
There was a problem hiding this comment.
should this switch the key to just using the Mountpoint field? will that introduce bugs in the future if this type grows new fields?
There was a problem hiding this comment.
same comment applies anywhere this PR changed a map key from a struct to a pointer
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @saschagrunert @liggitt |
|
the questions about the changes to non-generated files, and the compatibility implications are still outstanding apidiff output at https://gist.github.com/liggitt/9d860c234c8e1709238234c769c9d3c1 changes for all message types:
changes for all services:
client construction changed to take an interface:
service registration changed to take an interface:
getters changed to use generics:
signature changes:
specific changes: now accessible via Size/GetSize() directly, easy change
type removals: (artifacts of gogo decoding specifics, unused externally as far as I can tell)
|
| @@ -35,6 +35,8 @@ type RemoteRuntime struct { | |||
| RuntimeService *apitest.FakeRuntimeService | |||
| // Fake image service. | |||
| ImageService *apitest.FakeImageService | |||
| kubeapi.UnimplementedImageServiceServer | |||
| kubeapi.UnimplementedRuntimeServiceServer | |||
There was a problem hiding this comment.
use UnsafeImageServiceServer and UnsafeRuntimeServiceServer instead so we get a compile error if our fake is missing methods?
| @@ -165,7 +165,7 @@ func (r *remoteImageService) imageStatusV1(ctx context.Context, image *runtimeap | |||
| } | |||
|
|
|||
| if resp.Image != nil { | |||
| if resp.Image.Id == "" || resp.Image.Size_ == 0 { | |||
| if resp.Image.Id == "" || resp.Image.GetSize() == 0 { | |||
There was a problem hiding this comment.
use the field directly?
| if resp.Image.Id == "" || resp.Image.GetSize() == 0 { | |
| if resp.Image.Id == "" || resp.Image.Size == 0 { |
| @@ -92,7 +92,7 @@ func (m *kubeGenericRuntimeManager) GetImageSize(ctx context.Context, image kube | |||
| if resp.Image == nil { | |||
| return 0, nil | |||
| } | |||
| return resp.Image.Size_, nil | |||
| return resp.Image.GetSize(), nil | |||
There was a problem hiding this comment.
| return resp.Image.GetSize(), nil | |
| return resp.Image.Size, nil |
| @@ -119,7 +119,7 @@ func (m *kubeGenericRuntimeManager) ListImages(ctx context.Context) ([]kubeconta | |||
|
|
|||
| images = append(images, kubecontainer.Image{ | |||
| ID: img.Id, | |||
| Size: int64(img.Size_), | |||
| Size: int64(img.GetSize()), | |||
There was a problem hiding this comment.
| Size: int64(img.GetSize()), | |
| Size: int64(img.Size), |
| @@ -153,7 +153,7 @@ func (m *kubeGenericRuntimeManager) ImageStats(ctx context.Context) (*kubecontai | |||
| } | |||
| stats := &kubecontainer.ImageStats{} | |||
| for _, img := range allImages { | |||
| stats.TotalStorageBytes += img.Size_ | |||
| stats.TotalStorageBytes += img.GetSize() | |||
There was a problem hiding this comment.
| stats.TotalStorageBytes += img.GetSize() | |
| stats.TotalStorageBytes += img.Size |
| @@ -738,7 +739,11 @@ func TestGenerateLinuxContainerConfigNamespaces(t *testing.T) { | |||
| t.Run(tc.name, func(t *testing.T) { | |||
| got, err := m.generateLinuxContainerConfig(&tc.pod.Spec.Containers[0], tc.pod, nil, "", tc.target, false) | |||
| assert.NoError(t, err) | |||
| if diff := cmp.Diff(tc.want, got.SecurityContext.NamespaceOptions); diff != "" { | |||
| if diff := cmp.Diff( | |||
There was a problem hiding this comment.
would it make sense to use first-class proto methods to do the comparison instead?
if !proto.Equal(tc.want, got.SecurityContext.NamespaceOptions)
| // Because of | ||
| // https://github.com/kubernetes/kubernetes/issues/106652 | ||
| // we get the string instead of a JSON struct. | ||
| json: `"msg":"[RemoteRuntimeService] Version Response","v":0,"apiVersion":"&VersionResponse{Version:0.1.0,RuntimeName:containerd,RuntimeVersion:v1.6.18,RuntimeApiVersion:v1,}"`, | ||
| json: `"msg":"[RemoteRuntimeService] Version Response","v":0,"apiVersion":"version:\"0.1.0\"`, |
There was a problem hiding this comment.
this test change makes it look like we're losing information, when really, it was just narrowing the test to only check the string prefix
old impl:
func (this *VersionResponse) String() string {
if this == nil {
return "nil"
}
s := strings.Join([]string{`&VersionResponse{`,
`Version:` + fmt.Sprintf("%v", this.Version) + `,`,
`RuntimeName:` + fmt.Sprintf("%v", this.RuntimeName) + `,`,
`RuntimeVersion:` + fmt.Sprintf("%v", this.RuntimeVersion) + `,`,
`RuntimeApiVersion:` + fmt.Sprintf("%v", this.RuntimeApiVersion) + `,`,
`}`,
}, "")
return s
}
new impl:
func (x *VersionResponse) String() string {
return protoimpl.X.MessageStringOf(x)
}
is there a reason not to check the full content?
printf: `[RemoteRuntimeService] Version Response: [apiVersion version:"0.1.0" runtime_name:"containerd" runtime_version:"v1.6.18" runtime_api_version:"v1"]`,
structured: `"[RemoteRuntimeService] Version Response" apiVersion="version:\"0.1.0\" runtime_name:\"containerd\" runtime_version:\"v1.6.18\" runtime_api_version:\"v1\""`,
// Because of
// https://github.com/kubernetes/kubernetes/issues/106652
// we get the string instead of a JSON struct.
json: `"msg":"[RemoteRuntimeService] Version Response","v":0,"apiVersion":"version:\"0.1.0\" runtime_name:\"containerd\" runtime_version:\"v1.6.18\" runtime_api_version:\"v1\""`,| @@ -181,7 +181,7 @@ func (r *remoteRuntimeService) versionV1(ctx context.Context, apiVersion string) | |||
| r.log(10, "[RemoteRuntimeService] Version Response", "apiVersion", typedVersion) | |||
|
|
|||
| if typedVersion.Version == "" || typedVersion.RuntimeName == "" || typedVersion.RuntimeApiVersion == "" || typedVersion.RuntimeVersion == "" { | |||
| return nil, fmt.Errorf("not all fields are set in VersionResponse (%q)", *typedVersion) | |||
| return nil, fmt.Errorf("not all fields are set in VersionResponse (%q)", typedVersion) | |||
There was a problem hiding this comment.
previously, this output a nice message like this:
not all fields are set in VersionResponse ("&VersionResponse{Version:,RuntimeName:,RuntimeVersion:,RuntimeApiVersion:,}")
now, it outputs a message like this:
not all fields are set in VersionResponse ("")
that's... not very useful
can we do something like this instead:
var missingFields []string
if typedVersion.Version == "" {
missingFields = append(missingFields, "Version")
}
if typedVersion.RuntimeName == "" {
missingFields = append(missingFields, "RuntimeName")
}
if typedVersion.RuntimeApiVersion == "" {
missingFields = append(missingFields, "RuntimeApiVersion")
}
if typedVersion.RuntimeVersion == "" {
missingFields = append(missingFields, "RuntimeVersion")
}
if len(missingFields) > 0 {
return nil, fmt.Errorf("not all fields are set in VersionResponse (missing %s)", strings.Join(missingFields, ", "))
}|
rebased and added a commit addressing my comments at https://github.com/liggitt/kubernetes/commits/cri-gogo/ if you want to use it |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
gogo protobuf is deprecated since years and we should find alternatives or work without it.
Which issue(s) this PR fixes:
Part of #96564
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: