DRA: fix scheduler/resource claim controller race #124931

pohly · 2024-05-17T14:45:10Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

There was a race caused by having to update claim finalizer and status in two different operations:

Resource claim controller removes allocation, does not yet get to remove the finalizer.
Scheduler prepares an allocation, without adding the finalizer because it's there.
Controller removes finalizer.
Scheduler adds allocation.

This is an invalid state. Automatic checking found this during the execution of the "with translated parameters on single node.*supports sharing a claim sequentially" E2E test, but only when run stand-alone. When running in parallel (as in the CI), the bad outcome of the race did not occur.

Special notes for your reviewer:

The fix is to check that the finalizer is still set when adding the allocation. This can be done with a complicated JSON patch (see first commit, but only if you are really, really curious!), but a local retry loop with Update calls is simpler.

The resource claim controller doesn't need this, it can do a normal update which implicitly checks ResourceVersion.

Does this PR introduce a user-facing change?

DRA: using structured parameters with a claim that gets reused between pods may have led to a claim with an invalid state (allocated without a finalizer) which then caused scheduling of pods using the claim to stop.

pohly · 2024-05-21T09:45:32Z

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

+	// JSON patch can only append to a non-empty array. An empty reservedFor gets
+	// omitted and even if it didn't, it would be null and not an empty array.
+	// Therefore we have to test and add if it's currently empty.
+	reservedForEntry := fmt.Sprintf(`{"resource": "pods", "name": %q, "uid": %q}`, pod.Name, pod.UID)


While it was fun playing around with JSON patch, I think this is taking it too far...

A simpler, more obvious approach would be to add a retry loop which uses normal Update calls and gets the latest claim on a conflict.

Implemented, ready for review again.

pohly · 2024-05-28T11:25:44Z

/retest

bart0sh · 2024-05-28T16:32:49Z

/triage accepted
/priority important-soon

pohly · 2024-06-13T12:50:37Z

/assign @kerthcet

Can you help review?

pohly · 2024-06-17T15:15:07Z

/test pull-kubernetes-node-e2e-containerd-1-7-dra

Testing kubernetes/test-infra#32774

pohly · 2024-06-17T15:49:27Z

/test pull-kubernetes-node-e2e-containerd-1-7-dra

Testing another PR: kubernetes/test-infra#32776

pohly · 2024-06-17T16:03:43Z

/test pull-kubernetes-node-e2e-containerd-1-7-dra

kerthcet · 2024-06-26T15:34:59Z

will finish the review tomorrow.

kerthcet · 2024-06-27T10:40:12Z

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

+				claim.Finalizers = append(claim.Finalizers, resourcev1alpha2.Finalizer)
+				updatedClaim, err := pl.clientset.ResourceV1alpha2().ResourceClaims(claim.Namespace).Update(ctx, claim, metav1.UpdateOptions{})
+				if err != nil {
+					if apierrors.IsConflict(err) {


Will retryOnConflicthelp here? Then we can remove for loop here, it seems risky to me.

Thanks for the hint. It makes the code a bit simpler and adds exponential backoff.

I'm taking extra care to not do a GET in the first iteration because most of the time, the claim will be recent enough.

Pushed.

I verified again that the retry loop works: when running _output/bin/ginkgo -v --focus="with translated parameters on single node.*supports sharing a claim sequentially" ./test/e2e, the scheduler had to retry once as indicated by the log message when it gets the newer claim.

kerthcet · 2024-06-27T11:21:11Z

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

+			}
+
+			// The finalizer needs to be added in a normal update.
+			// If we were interrupted in the past, it might already be set and we simply continue.


Should we get for the laster claim to check whether the finalizer has been removed?

If it has been removed and the claim instance is stale, then the UpdateStatus below will fail and we retry with a fresh copy of the claim.

There was a race caused by having to update claim finalizer and status in two different operations: - Resource claim controller removes allocation, does not yet get to remove the finalizer. - Scheduler prepares an allocation, without adding the finalizer because it's there. - Controller removes finalizer. - Scheduler adds allocation. This is an invalid state. Automatic checking found this during the execution of the "with translated parameters on single node.*supports sharing a claim sequentially" E2E test, but only when run stand-alone. When running in parallel (as in the CI), the bad outcome of the race did not occur. The fix is to check that the finalizer is still set when adding the allocation. The apiserver doesn't check that because it doesn't know which finalizer goes with the allocation result. It could check for "some finalizer", but that is not guaranteed to be correct (could be some unrelated one). Checking the finalizer can only be done with a JSON patch. Despite the complications, having the ability to add multiple pods concurrently to ReservedFor seems worth it (avoids expensive rescheduling or a local retry loop). The resource claim controller doesn't need this, it can do a normal update which implicitly checks ResourceVersion.

The JSON patch approach works, but it is complex. A retry loop is easier to understand (detect conflict, get new claim, try again). There is one additional API call (the get), but in practice this scenario is unlikely.

kerthcet · 2024-06-28T07:55:55Z

/lgtm
/approve

The two commits seems exclusive, will you squash them?
/hold

k8s-ci-robot · 2024-06-28T07:56:01Z

LGTM label has been added.

Git tree hash: 8a6a4b040255d3614b0addbe44eaafa77377c41f

k8s-ci-robot · 2024-06-28T07:56:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kerthcet, pohly

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:

~~pkg/scheduler/OWNERS~~ [kerthcet]

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

pohly · 2024-06-28T10:53:31Z

I'd prefer to keep the two commits: the first one can serve as reference for how this would have looked like with JSON patching.

/hold cancel

k8s-ci-robot requested review from bart0sh and klueska May 17, 2024 14:45

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 17, 2024

pohly commented May 21, 2024

View reviewed changes

pohly changed the title ~~DRA: fix scheduler/resource claim controller race~~ WIP: DRA: fix scheduler/resource claim controller race May 21, 2024

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 21, 2024

pohly changed the title ~~WIP: DRA: fix scheduler/resource claim controller race~~ DRA: fix scheduler/resource claim controller race May 27, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2024

pohly force-pushed the dra-scheduler-prebind-fix branch from 843dca1 to 434e786 Compare May 28, 2024 07:49

k8s-ci-robot assigned kerthcet Jun 13, 2024

pohly mentioned this pull request Jun 13, 2024

DRA for 1.31 #125488

Merged

9 tasks

kerthcet reviewed Jun 27, 2024

View reviewed changes

pohly force-pushed the dra-scheduler-prebind-fix branch from 434e786 to 952e6aa Compare June 27, 2024 12:56

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

pohly force-pushed the dra-scheduler-prebind-fix branch from 952e6aa to caf544a Compare June 27, 2024 13:01

pohly added 2 commits June 27, 2024 15:03

DRA: fix scheduler/resource claim controller race with retry

4bddebc

The JSON patch approach works, but it is complex. A retry loop is easier to understand (detect conflict, get new claim, try again). There is one additional API call (the get), but in practice this scenario is unlikely.

pohly force-pushed the dra-scheduler-prebind-fix branch from caf544a to 4bddebc Compare June 27, 2024 13:04

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

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 28, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2024

k8s-ci-robot merged commit eb66365 into kubernetes:master Jun 28, 2024
18 checks passed

k8s-ci-robot added this to the v1.31 milestone Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRA: fix scheduler/resource claim controller race #124931

DRA: fix scheduler/resource claim controller race #124931

pohly commented May 17, 2024 •

edited

Loading

pohly May 21, 2024

pohly May 27, 2024

pohly commented May 28, 2024

bart0sh commented May 28, 2024

pohly commented Jun 13, 2024

pohly commented Jun 17, 2024

pohly commented Jun 17, 2024

pohly commented Jun 17, 2024

kerthcet commented Jun 26, 2024

kerthcet Jun 27, 2024

pohly Jun 27, 2024

pohly Jun 27, 2024

kerthcet Jun 27, 2024

pohly Jun 27, 2024

kerthcet commented Jun 28, 2024

k8s-ci-robot commented Jun 28, 2024

k8s-ci-robot commented Jun 28, 2024

pohly commented Jun 28, 2024

DRA: fix scheduler/resource claim controller race #124931

DRA: fix scheduler/resource claim controller race #124931

Conversation

pohly commented May 17, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

pohly May 21, 2024

Choose a reason for hiding this comment

pohly May 27, 2024

Choose a reason for hiding this comment

pohly commented May 28, 2024

bart0sh commented May 28, 2024

pohly commented Jun 13, 2024

pohly commented Jun 17, 2024

pohly commented Jun 17, 2024

pohly commented Jun 17, 2024

kerthcet commented Jun 26, 2024

kerthcet Jun 27, 2024

Choose a reason for hiding this comment

pohly Jun 27, 2024

Choose a reason for hiding this comment

pohly Jun 27, 2024

Choose a reason for hiding this comment

kerthcet Jun 27, 2024

Choose a reason for hiding this comment

pohly Jun 27, 2024

Choose a reason for hiding this comment

kerthcet commented Jun 28, 2024

k8s-ci-robot commented Jun 28, 2024

k8s-ci-robot commented Jun 28, 2024

pohly commented Jun 28, 2024

pohly commented May 17, 2024 •

edited

Loading