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 |
d783729 to
4c8cac7
Compare
|
/retest |
|
👋 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
}| if diff := cmp.Diff(tc.want, got.SecurityContext.NamespaceOptions); diff != "" { | ||
| if diff := cmp.Diff( | ||
| tc.want, | ||
| got.SecurityContext.NamespaceOptions, |
There was a problem hiding this comment.
is the need for this change a sign that the generated types are changing in ways that will break consumers?
| @@ -156,7 +156,9 @@ func makeFakePodSandbox(t *testing.T, m *kubeGenericRuntimeManager, template san | |||
| Ip: ip, | |||
| }) | |||
| } | |||
| podSandBoxStatus.Network.AdditionalIps = additionalPodIPs | |||
| if len(additionalPodIPs) > 0 { | |||
There was a problem hiding this comment.
curious why this change was required
| @@ -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
}
changing a dedupe map from a comparable struct to a pointer smells like a bug
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?
| @@ -3070,8 +3072,8 @@ func TestToKubeContainerImageVolumes(t *testing.T) { | |||
| "empty volumes": {}, | |||
| "multiple volumes": { | |||
| pullResults: imageVolumePulls{ | |||
| volume1: imageVolumePullResult{spec: imageSpec1}, | |||
| volume2: imageVolumePullResult{spec: imageSpec2}, | |||
| volume1: imageVolumePullResult{spec: &imageSpec1}, | |||
There was a problem hiding this comment.
similar question about why a change from struct to pointer was required, and if that represents changes in the types that will break consumers as well
|
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 |
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.: