DRA: generated resource claim names #117351

pohly · 2023-04-14T11:30:58Z

What type of PR is this?

/kind cleanup
/kind api-change

What this PR does / why we need it:

One concern raised during the initial DRA reviews and again in #116254 (comment) was the risk of name conflicts between automatically and manually created ResourceClaims.

This PR addresses that by switching to generated names for automatically created ResourceClaims, with a pod.status extension to record that generated name for code where listing all ResourceClaims is impossible (kubelet) or more complex (node authorizer).

Which issue(s) this PR fixes:

Fixes #113722

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The names of ResourceClaims generated from ResourceClaimTemplate are now generated. The base name is still `<pod>-<claim name>`, but a random suffix will avoid name collisions.

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

- KEP: https://github.com/kubernetes/enhancements/issues/3063

fedebongio · 2023-04-20T16:35:00Z

/remove-sig api-machinery

thockin

Also need to fix comments in type ClaimSource - "The name of the ResourceClaim will be..."

thockin · 2023-04-24T17:12:15Z

pkg/apis/core/types.go

+// the corresponding ResourceClaim.
+type PodResourceClaimStatus struct {
+	// Name uniquely identifies this resource claim inside the pod.
+	// This must be a DNS_LABEL.


And must match a defined resource claim?

Yes. I wonder whether that should replace "must be a DNS_LABEL" because that follows from "must be the name of an entry in pod.spec.resourceClaims". I'll use both for now.

thockin · 2023-04-24T17:12:46Z

pkg/apis/core/types.go

+	Name string
+
+	// ResourceClaimName is the name of the ResourceClaim that was
+	// generated for the Pod.


...in the same namespace as this pod.

?

Right, let's call that out explicitly.

soltysh

With comments from Jordan address through use of MutationCache, the controller changes lgtm

klueska · 2023-07-11T08:36:33Z

staging/src/k8s.io/dynamic-resource-allocation/resourceclaim/resourceclaim.go

-func Name(pod *v1.Pod, podClaim *v1.PodResourceClaim) string {
-	if podClaim.Source.ResourceClaimName != nil {
-		return *podClaim.Source.ResourceClaimName
+func Name(pod *v1.Pod, podClaim *v1.PodResourceClaim) (name *string, mustCheckOwner bool, err error) {


How much overhead is it to just always check for the owner? It just feels awkward to have this extra boolean returned from here. Or better yet, why can't we just check for the owner in here and return an error if the pod passed in is not the owner whenever it "must" be checked.

klueska · 2023-07-11T08:39:04Z

pkg/kubelet/cm/dra/manager.go

+		klog.V(3).InfoS("Processing resource", "podClaim", podClaim.Name, "pod", pod.Name)
+		claimName, mustCheckOwner, err := resourceclaim.Name(pod, podClaim)
+		if err != nil {
+			return err


We should probably wrap this error to be consistent.

klueska · 2023-07-11T08:40:22Z

pkg/kubelet/cm/dra/manager.go

+		if claimName == nil {
+			// Nothing to do.
+			continue
+		}


When would there ever be no error, but claimName == nil? I would think that that would be an error condition from within resourceclaim.Name().

That's the new feature: if creating a claim was intentionally skipped (nil in the claim name field), then all code dealing with the pod can skip the claim. resourceclaim.Name indicates that with a nil claimName. Returning a special error code also would work.

This was a use case that came up with Multi-Network. We don't use this with "normal" claims yet, but if we ever need it, all releases >1.28 will support it.

If it's not an error to have a nil claim name, then it shouldn't be hidden behind an error. Maybe just adding a comment above the check as to why it might ever be nil is sufficient (at which point we can remove the comment of "Nothing to do" as that should be obvious from the new comment).

Comment changed.

klueska · 2023-07-11T08:41:50Z

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go

-			// The error usually has already enough context ("resourcevolumeclaim "myclaim" not found"),
-			// but we can do better for generic ephemeral inline volumes where that situation
-			// is normal directly after creating a pod.
-			if isEphemeral && apierrors.IsNotFound(err) {
-				err = fmt.Errorf("waiting for dynamic resource controller to create the resourceclaim %q", claimName)
-			}


I don't have a whole lot of context on why this was needed to begin with, but why is this not needed anymore? I guess it's all captured within the resourceclaim.Name() call now?

Correct. It'll check for this based on the pod status field.

klueska · 2023-07-11T11:41:19Z

pkg/kubelet/cm/dra/manager.go


+		if claimName == nil {
+			// Nothing to do.
+			continue
+		}


Can we add a comment such as the following here? Looks like it will need to be propagated to 3 places in total.

// The claim name might be nil if no underlying resource claim // was generated for the referenced claim. There are valid use // cases when this might happen, so we simply skip it.

Changed - please take another look.

klueska · 2023-07-11T11:44:17Z

staging/src/k8s.io/dynamic-resource-allocation/resourceclaim/resourceclaim.go

-func Name(pod *v1.Pod, podClaim *v1.PodResourceClaim) string {
-	if podClaim.Source.ResourceClaimName != nil {
-		return *podClaim.Source.ResourceClaimName
+func Name(pod *v1.Pod, podClaim *v1.PodResourceClaim) (name *string, mustCheckOwner bool, err error) {


OK. I don't feel too strongly, so we can leave it as is.

Generating the name avoids all potential name collisions. It's not clear how much of a problem that was because users can avoid them and the deterministic names for generic ephemeral volumes have not led to reports from users. But using generated names is not too hard either. What makes it relatively easy is that the new pod.status.resourceClaimStatus map stores the generated name for kubelet and node authorizer, i.e. the information in the pod is sufficient to determine the name of the ResourceClaim. The resource claim controller becomes a bit more complex and now needs permission to modify the pod status. The new failure scenario of "ResourceClaim created, updating pod status fails" is handled with the help of a new special "resource.kubernetes.io/pod-claim-name" annotation that together with the owner reference identifies exactly for what a ResourceClaim was generated, so updating the pod status can be retried for existing ResourceClaims. The transition from deterministic names is handled with a special case for that recovery code path: a ResourceClaim with no annotation and a name that follows the Kubernetes <= 1.27 naming pattern is assumed to be generated for that pod claim and gets added to the pod status. There's no immediate need for it, but just in case that it may become relevant, the name of the generated ResourceClaim may also be left unset to record that no claim was needed. Components processing such a pod can skip whatever they normally would do for the claim. To ensure that they do and also cover other cases properly ("no known field is set", "must check ownership"), resourceclaim.Name gets extended.

This is not something that normally happens, but the API supports it because it might be needed at some point, so we have to test it.

This addresses the following bad sequence of events: - controller creates ResourceClaim - updating pod status fails - pod gets retried before the informer receives the created ResourceClaim - another ResourceClaim gets created Storing the generated ResourceClaim in a MutationCache ensures that the controller knows about it during the retry. A positive side effect is that ResourceClaims now get index by pod owner and thus iterating over existing ones becomes a bit more efficient.

klueska · 2023-07-11T12:28:36Z

/lgtm
/approve

For kubelet changes

k8s-ci-robot · 2023-07-11T12:29:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: klueska, pohly, soltysh, thockin

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:

~~api/OWNERS~~ [thockin]
~~pkg/api/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~pkg/controller/resourceclaim/OWNERS~~ [klueska,pohly,soltysh,thockin]
~~pkg/generated/openapi/OWNERS~~ [thockin]
~~pkg/kubelet/OWNERS~~ [klueska,thockin]
~~pkg/scheduler/OWNERS~~ [thockin]
~~plugin/pkg/auth/authorizer/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/client-go/applyconfigurations/OWNERS~~ [thockin]
~~staging/src/k8s.io/dynamic-resource-allocation/OWNERS~~ [klueska,pohly,thockin]
~~test/e2e/dra/OWNERS~~ [klueska,pohly,soltysh,thockin]

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

k8s-ci-robot · 2023-07-11T12:30:06Z

LGTM label has been added.

Git tree hash: 71a041eaa079748cc524c5a3a0c5bf53fd73857e

k8s-triage-robot · 2023-07-11T16:22:48Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

This is a follow-up to kubernetes#117351 which just got merged.

k8s-ci-robot requested review from bart0sh and caesarxuchao April 14, 2023 11:32

bart0sh added this to WIP in SIG Node PR Triage Apr 14, 2023

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 20, 2023

thockin reviewed Apr 24, 2023

View reviewed changes

pohly force-pushed the dra-generated-resource-claim-names branch from d0c7f40 to 820fdc9 Compare May 3, 2023 10:58

pohly force-pushed the dra-generated-resource-claim-names branch from 820fdc9 to 67ac68e Compare May 3, 2023 12:04

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

pohly force-pushed the dra-generated-resource-claim-names branch from a8e4a55 to f408285 Compare July 7, 2023 19:47

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

pohly force-pushed the dra-generated-resource-claim-names branch from f408285 to ac5ddb8 Compare July 7, 2023 20:51

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

pohly force-pushed the dra-generated-resource-claim-names branch from ac5ddb8 to a7597e7 Compare July 8, 2023 10:38

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

soltysh approved these changes Jul 10, 2023

View reviewed changes

SIG Node CI/Test Board automation moved this from Archive-it to PRs - Needs Approver Jul 10, 2023

klueska reviewed Jul 11, 2023

View reviewed changes

pohly force-pushed the dra-generated-resource-claim-names branch from a7597e7 to aff8342 Compare July 11, 2023 11:29

klueska reviewed Jul 11, 2023

View reviewed changes

pohly added 4 commits July 11, 2023 14:23

dra: generated files

0fc62d5

dra e2e: check that not generating a ResourceClaim works

ba81087

This is not something that normally happens, but the API supports it because it might be needed at some point, so we have to test it.

pohly force-pushed the dra-generated-resource-claim-names branch from aff8342 to fec2578 Compare July 11, 2023 12:24

k8s-ci-robot assigned klueska Jul 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2023

k8s-ci-robot merged commit e0dafe5 into kubernetes:master Jul 11, 2023
15 checks passed

SIG Node CI/Test Board automation moved this from PRs - Needs Approver to Done Jul 11, 2023

SIG Node PR Triage automation moved this from Needs Reviewer to Done Jul 11, 2023

k8s-ci-robot added this to the v1.28 milestone Jul 11, 2023

pohly added a commit to pohly/kubernetes that referenced this pull request Jul 13, 2023

dra API: ensure that pod status contains no duplicate resource claims

ddc0d94

This is a follow-up to kubernetes#117351 which just got merged.

pohly mentioned this pull request Jul 13, 2023

dra API: ensure that pod status contains no duplicate resource claims #119296

Merged

klueska mentioned this pull request Jul 19, 2023

dynamic resource allocation kubernetes/enhancements#3063

Open

34 tasks

parthyadav3105 mentioned this pull request Jan 5, 2024

DRA driver is not able to allocate gpu NVIDIA/k8s-dra-driver#46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRA: generated resource claim names #117351

DRA: generated resource claim names #117351

pohly commented Apr 14, 2023 •

edited

fedebongio commented Apr 20, 2023

thockin left a comment

thockin Apr 24, 2023

pohly Apr 26, 2023 •

edited

thockin Apr 24, 2023

pohly Apr 26, 2023

soltysh left a comment

klueska Jul 11, 2023

klueska Jul 11, 2023

pohly Jul 11, 2023

klueska Jul 11, 2023

pohly Jul 11, 2023

klueska Jul 11, 2023

pohly Jul 11, 2023

klueska Jul 11, 2023

pohly Jul 11, 2023

klueska Jul 11, 2023

klueska Jul 11, 2023

pohly Jul 11, 2023

klueska Jul 11, 2023

klueska commented Jul 11, 2023 •

edited

k8s-ci-robot commented Jul 11, 2023

k8s-ci-robot commented Jul 11, 2023

k8s-triage-robot commented Jul 11, 2023

DRA: generated resource claim names #117351

DRA: generated resource claim names #117351

Conversation

pohly commented Apr 14, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

fedebongio commented Apr 20, 2023

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klueska commented Jul 11, 2023 • edited

k8s-ci-robot commented Jul 11, 2023

k8s-ci-robot commented Jul 11, 2023

k8s-triage-robot commented Jul 11, 2023

pohly commented Apr 14, 2023 •

edited

pohly Apr 26, 2023 •

edited

klueska commented Jul 11, 2023 •

edited