DRA: CDI: maximum annotation length

[pohly]

What would you like to be added?

This code checks that "plugin name" (= DRA driver name) + "CDI device ID" with "_" as separator is at most maxNameLen = 63:

kubernetes/pkg/kubelet/cm/util/cdi/cdi.go

Lines 104 to 108 in 3ec6a38

	name := pluginName + "_" + strings.ReplaceAll(deviceID, "/", "_")
	if len(name) > maxNameLen {
	return "", fmt.Errorf("invalid plugin+deviceID %q, too long", name)
	}

Can that happen for valid driver and CDI device IDs? Do CDI driver authors have to avoid this? If yes, we need to document it.

/cc @klueska @bart0sh
/sig node

Why is this needed?

Avoid surprises...

[ffromani]

/triage accepted
/priority important-longterm

not sure between important-soon or important-longterm, taking a conservative approach for starters

[varunrsekar]

@pohly Could maxNameLen atleast be 127/128 chars? If deviceID is a uuid, then it already takes up 36 chars.

Example that will break: foobar.resource.example.com_dd84eb13-3eca-4caf-9042-4f556c8f6d48 (65 chars)

We use UUIDs when working with vGPUs and mediated devices. So having atleast 127 chars will help satisfy that.

[pohly]

I'm not sure where the maxNameLen = 63 limit comes from. @bart0sh, can you clarify?

/assign @bart0sh

[varunrsekar]

Just found that k8s has some limits on the annotation key size: https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set

Annotations are key/value pairs. Valid annotation keys have two segments: an optional prefix and name, separated by a slash (/). The name segment is required and must be 63 characters or less, beginning and ending with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_), dots (.), and alphanumerics between. The prefix is optional. If specified, the prefix must be a DNS subdomain: a series of DNS labels separated by dots (.), not longer than 253 characters in total, followed by a slash (/).

But in the annotation key (<prefix>/<name>), only the name segment needs to be 63 characters or less where as the prefix can be upto 253 chars.

In our case, we have <prefix>=cdi.k8s.io, <name>=pluginID+deviceID which maybe it can just be <prefix>=<pluginID>, <name>=deviceID

[bart0sh]

I'm not sure either, but it's probably related to these 2 places:

• https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set
• https://github.com/cncf-tags/container-device-interface/blob/main/pkg/cdi/annotations.go#L91

@elezar @kad Any other ideas?

[pohly]

/priority important-soon

We are finalizing the DRA API and this has implications for it.

[elezar]

As @bart0sh points out, the 63 character limit is due to the use of the name section of the annotation key. Note that the cdi.k8s.io prefix is assumed by downstream consumers (containerd and cri-o) to determine whether CDI devices are requested if the dedicated CRI fields are not available.

Note that the name serves no practical purpose other than preventing colissions in keys. Furthermore, it is unrelated to the fully-qualified CDI device names which are passed as a comma-separated list in the corresponding annotation value.

The original thinking behind allowing a user to specify the plugin name is to allow some human-readability as to the source of the annotations, but maybe this flexibility is not required. We could also replace getAnnotations(string, string) (string, error) with an implementation that unconditionally returns a uuid instead.

@varunrsekar when you mention vGPUs and mediated devices, where are the UUIDs used? In the CDI device name, or in the annotation keys that are generated?

[bart0sh]

We are finalizing the DRA API and this has implications for it.

Are we still going to use annotations in the DRA API ? I thought that usage of annotations should be deprecated in favour of using CRI field.

[elezar]

We are finalizing the DRA API and this has implications for it.

@pohly which implications does this have? If we remove the logic for cleaning and prepending the driver name, we can construct the name section ourselves and leave it opague to the user. This may make some things more difficult to debug, but the annotation creation is opague to a DRA driver implementer in any case.

We already do this for the device plugins and call:

annotations, err := cdi.GenerateAnnotations(types.UID(resourceID), "devicemanager", sortedCDIDevices)

where devicemanager could be replaced by dra globally if we assume a unique claim ID.

[pohly]

This may limit the length of strings exchanged between Kubernetes and DRA drivers (device ID) and/or of the driver name.

We currently have a limit on the driver name as part of the Kubernetes API (DNS subdomain, max 63 characters), but not for the device ID:

kubernetes/staging/src/k8s.io/kubelet/pkg/apis/dra/v1alpha3/api.proto

Lines 70 to 74 in 5298964

	message NodePrepareResourceResponse {
	// These are the additional devices that kubelet must
	// make available via the container runtime. A resource
	// may have zero or more devices.
	repeated string cdi_devices = 1 [(gogoproto.customname) = "CDIDevices"];

[elezar]

In the context of DRA the claim ID is used at present. My question was how we wish to expose this to driver authors? We could make this an implementation detail on the kubelet side and always generate a uuid.

Also to clarify, the name that is being discussed here is NOT the CDI device name which I assume that your are referring to as "device ID". For the relevant annotations, these are generated with the form:

map[string]string{
"cdi.k8s.io/{{ .DriverName }}_{{ .ClaimID }}": "vendor1.com/example1=device1,vendor2.com/example2=device2"
}

(note that I have used devices from two different vendors here, but in practice these will probably be the same vendor.)

My suggesion is that we change this so that we have:

map[string]string{
"cdi.k8s.io/dra_{{ .UUID }}": "vendor1.com/example1=device1,vendor2.com/example2=device2"
}

where we would generate .UUID for each call to GetAnnotations.

As a follow-up question: What are the uniqueness guarantees for the Claim ID? If this is guaranteed to be unique for a given container, then we could use the Claim UID as the unique identifier. Looking at the implementation it seems that this is always set from the UID for the claim object.

[pohly]

In the context of DRA the claim ID is used at present.

Indeed:

kubernetes/pkg/kubelet/cm/util/cdi/cdi.go

Line 51 in ea0ab06

annotations, err := updateAnnotations(map[string]string{}, driverName, string(claimUID), cdiDevices)

I'm trying to find out how long a UID can be at most. Surprisingly, I found no validation of it.

Regardless of that, embedding the driver name is a problem because it may be 63 characters long, which leaves nothing for the "device ID" = "claim ID".

@varunrsekar: I suppose the claim UID is what you saw in your failure case, not the UID you use in your driver. Is that correct?

[bart0sh]

Passing CDI devices as annotations should be deprecated soon and removed from the Kubelet. To my understanding it should happen after EOL of the Kubernetes 1.28.

Kubernetes 1.29 supports passing CDIDevices with CRI field and has Device plugin CDI support enabled by default.

[pohly]

Do we have a tracking issue for this? If not, can you create one?

[varunrsekar]

@varunrsekar: I suppose the claim UID is what you saw in your failure case, not the UID you use in your driver. Is that correct?

@pohly Sorry for the confusion! What I saw in the CDI annotations was indeed the claim UID.

[bart0sh]

Created an issue as requested: #125210