Convert `k8s.io/kubelet/pkg/apis/dra` from gogo to protoc #133026

saschagrunert · 2025-07-17T11:13:00Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Use standard protoc for the dra instead of gogo.

Which issue(s) this PR is related to:

Part of #96564

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

Removed deprecated gogo protocol definitions from `k8s.io/kubelet/pkg/apis/dra` in favor of `google.golang.org/protobuf`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

saschagrunert · 2025-07-18T07:36:06Z

/retest

saschagrunert · 2025-07-21T08:06:23Z

/retest

k8s-ci-robot · 2025-07-25T07:41:01Z

New changes are detected. LGTM label has been removed.

saschagrunert · 2025-07-25T08:04:25Z

staging/src/k8s.io/kubelet/pkg/apis/dra/v1beta1/zz_generated.conversion.go

+	out.state = in.state
 	out.Namespace = in.Namespace
-	out.UID = in.UID
+	out.Uid = in.Uid
 	out.Name = in.Name
-	out.XXX_NoUnkeyedLiteral = in.XXX_NoUnkeyedLiteral
-	out.XXX_sizecache = in.XXX_sizecache
+	out.unknownFields = *(*[]byte)(unsafe.Pointer(&in.unknownFields))
+	out.sizeCache = in.sizeCache


@liggitt is there a way to avoid converting the internal types? This will lead into build failures, we may also wanna consider a different type of conversion here.

I missed this question... did you get this resolved?

Not by using conversion-gen, which may require a feature for that. I switched to manually maintain the conversion for now in staging/src/k8s.io/kubelet/pkg/apis/dra/v1beta1/conversion_internal.go.

@pohly I don't really understand what is going on with this file ... this is trying to do version-to-version conversion between grpc proto types, not REST API types? Is there fuzzed round-trip testing done to make sure these convert with fidelity and aren't missing any fields?

If we do end up sticking with a hand-written conversion like this, as a general comment, use of unsafe.Pointer in a non-generated conversion file is super fragile, the generator has checks to make sure the in/out is memory-identical before using this.

Separately, I realized there's nothing requiring changes under kubelet/pkg/apis to go through API review, which is disconcerting, any general kubelet approver can accidentally approve API changes (fixed in #133275)

/hold

one possibility is to improve conversion-gen to stop trying to set these unexported fields, so we can mostly lean on the generation bits for memory equality testing and mechanical field copying

POC in https://github.com/liggitt/kubernetes/commits/dra-proto/

that led down a rabbit trail:

teaching conversion-gen it can't be sure about converting a field means it will generate the autoConvert_... function with warnings, but not the final Convert_... function

because in this case we're fine with ignoring the unexported bookkeeping proto fields, I just copied the Convert_ implementations that delegate to autoConvert_... as-is

once I did that, conversion-gen couldn't find/use custom Convert_ functions for maps and slices to pointers, so I fixed that as well

Awesome! I was also thinking about the overall approach of skipping unexported fields in conversion-gen and would prefer that route.

updated https://github.com/liggitt/kubernetes/commits/dra-proto/ with conversion-gen tests, opened #133325 containing just the conversion-gen bits as a prereq PR

Do you want this to be merged in 1.34?

No, it can wait for 1.35

saschagrunert · 2025-07-25T08:04:49Z

hack/update-codegen.sh

-      hack/_update-generated-proto-bindings-dockerized.sh protoc "${apis_using_protoc[@]}"
+      hack/_update-generated-proto-bindings-dockerized.sh protoc "${apis[@]}"


I'll follow-up on the script part in a separate PR.

follow-up is good, if we don't have any gogo proto bindings now, let's delete all the gogo logic from that path completely (drop the generator arg check, delete generate_proto_gogo, etc)

Ref: #133271

k8s-ci-robot · 2025-07-28T07:59:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, saschagrunert, SergeyKanzhelev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [dims]
~~pkg/kubelet/cm/OWNERS~~ [SergeyKanzhelev,dims]
~~staging/src/k8s.io/dynamic-resource-allocation/OWNERS~~ [dims]
~~staging/src/k8s.io/kubelet/OWNERS~~ [SergeyKanzhelev,dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Use standard protoc for the dra instead of gogo. Part of kubernetes#96564 Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

saschagrunert · 2025-07-28T09:07:06Z

cc @pohly

k8s-ci-robot · 2025-07-28T12:17:13Z

@saschagrunert: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-unit-windows-master	`58c7e1a`	link	false	`/test pull-kubernetes-unit-windows-master`

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.

saschagrunert · 2025-07-28T12:25:04Z

/skip

SergeyKanzhelev · 2025-07-28T18:49:29Z

hack/update-codegen.sh

@@ -1020,10 +1020,8 @@ function codegen::protobindings() {

    # Each element of this array is a directory containing subdirectories which
    # eventually contain a file named "api.proto".
-    local apis_using_gogo=(
+    local apis=(


can we discover those instead of listing explicitly?

(separate PR, not blocking this one)

are you asking if we can automatically distinguish which api.proto files are using gogo and which are using standard protoc, or if we can discover all api.proto files anywhere in the tree?

second. We switched to protoc everywhere. Now instead of listing APIs maybe we can discover them in the tree

yeah, it's possible, can look into doing that as a follow-up

saschagrunert · 2025-07-29T06:03:37Z

I assume having this part of the v1.34 release would be beneficial considering that the API is now v1.

SergeyKanzhelev · 2025-07-29T06:45:14Z

I assume having this part of the v1.34 release would be beneficial considering that the API is now v1.

I asked today: https://kubernetes.slack.com/archives/C0409NGC1TK/p1753728775739929

BenTheElder · 2025-07-29T20:30:07Z

/triage accepted
needs-rebase

k8s-ci-robot · 2025-07-29T20:30:16Z

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.

k8s-ci-robot requested review from andrewsykim and bart0sh July 17, 2025 11:13

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. wg/device-management Categorizes an issue or PR as relevant to WG Device Management. labels Jul 17, 2025

github-project-automation bot added this to SIG Node: code and documentation PRs Jul 17, 2025

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jul 17, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Jul 17, 2025

github-project-automation bot added this to Dynamic Resource Allocation Jul 17, 2025

github-project-automation bot moved this to 🆕 New in Dynamic Resource Allocation Jul 17, 2025

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 17, 2025

saschagrunert force-pushed the dra-proto branch from 4f97be6 to 7576b1a Compare July 17, 2025 11:16

k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Jul 17, 2025

saschagrunert force-pushed the dra-proto branch from 7576b1a to 3238db9 Compare July 17, 2025 12:42

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2025

saschagrunert force-pushed the dra-proto branch from 3238db9 to c120521 Compare July 21, 2025 07:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2025

saschagrunert force-pushed the dra-proto branch from c120521 to d0dbd10 Compare July 23, 2025 14:52

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2025

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2025

k8s-ci-robot requested a review from SergeyKanzhelev July 25, 2025 07:41

k8s-ci-robot added area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 25, 2025

saschagrunert force-pushed the dra-proto branch 2 times, most recently from 0797f74 to 062d44a Compare July 25, 2025 08:03

saschagrunert commented Jul 25, 2025

View reviewed changes

saschagrunert force-pushed the dra-proto branch 2 times, most recently from 8121c70 to a3368ee Compare July 28, 2025 07:59

Convert k8s.io/kubelet/pkg/apis/dra from gogo to protoc

58c7e1a

Use standard protoc for the dra instead of gogo. Part of kubernetes#96564 Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

saschagrunert force-pushed the dra-proto branch from a3368ee to 58c7e1a Compare July 28, 2025 08:00

SergeyKanzhelev reviewed Jul 28, 2025

View reviewed changes

saschagrunert mentioned this pull request Jul 29, 2025

WIP: Update protobindings scripts to drop gogo references #133271

Open

liggitt added this to the v1.35 milestone Jul 29, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2025

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 29, 2025

liggitt mentioned this pull request Jul 30, 2025

Fix conversion-gen handling of unexported fields and custom conversions of pointers #133325

Open

		hack/_update-generated-proto-bindings-dockerized.sh protoc "${apis_using_protoc[@]}"
		hack/_update-generated-proto-bindings-dockerized.sh protoc "${apis[@]}"

Convert k8s.io/kubelet/pkg/apis/dra from gogo to protoc #133026

Are you sure you want to change the base?

Convert k8s.io/kubelet/pkg/apis/dra from gogo to protoc #133026

Uh oh!

Conversation

saschagrunert commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

saschagrunert commented Jul 18, 2025

Uh oh!

saschagrunert commented Jul 21, 2025

Uh oh!

k8s-ci-robot commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 28, 2025

Uh oh!

saschagrunert commented Jul 28, 2025

Uh oh!

k8s-ci-robot commented Jul 28, 2025

Uh oh!

saschagrunert commented Jul 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschagrunert commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergeyKanzhelev commented Jul 29, 2025

Uh oh!

BenTheElder commented Jul 29, 2025

Uh oh!

k8s-ci-robot commented Jul 29, 2025

Uh oh!

Uh oh!

Convert `k8s.io/kubelet/pkg/apis/dra` from gogo to protoc #133026

Convert `k8s.io/kubelet/pkg/apis/dra` from gogo to protoc #133026

saschagrunert commented Jul 17, 2025 •

edited

Loading

liggitt Jul 29, 2025 •

edited

Loading

liggitt Jul 29, 2025 •

edited

Loading

liggitt Jul 28, 2025 •

edited

Loading

saschagrunert commented Jul 29, 2025 •

edited

Loading