Add Webadmission Controller by catblade · Pull Request #61 · kubernetes-sigs/dra-driver-cpu

catblade · 2026-02-10T18:45:47Z

Add in a webadmission controller to make sure that CPUs counts are in line together.
Change makefile and expand to make easy e2e testing.
Add in easy install targets for individual/grouped modes in Makefile.
Some changes to clean up NRI from bugs discovered with testing.

running, including some code fixes. - NRI parses CDI claim UIDs when DRA env is absent. - update test-e2e-all to force rebuild and use explicit reserved CPU wiring - resolve claim UIDs from CDI devices in NRI sync/create paths with stronger ownership checks - harden e2e failure detection and accept admission rejection as a valid negative claim-sharing outcome

pravk03 · 2026-02-10T19:52:36Z

/assign @ffromani @pravk03

pravk03

Leaving some initial comments; I will continue reviewing

pkg/admission/validation.go

pravk03 · 2026-02-12T00:23:08Z

cmd/dracpu-admission/dra-cpu-admission.go

+	}
+
+	var errs []string
+	for _, container := range pod.Spec.Containers {


I am thinking if we should just enforce the restriction a pod level instead of container level?

pod level requests >= sum of container requests (non-init containers) + claims. ,

Pro's:

Smoother migration to KEP (KEP-5517: Introduce DRA for Native Resources KEP kubernetes/enhancements#5755 (comment)).

It allows for claims to be shared between containers in the future, should we loosen the restrictions currently enforced byhttps://github.com/Shared ResourceClaim Causes Incorrect CPU Accounting #6.

Cons:

This would require that CPU requests are explicitly defined at the pod level

WDYT ? @catblade @ffromani . Would this work for slurm use-cases ?

I'm generally in favor considering this is the latest trend in the community, but this would make us dependant on another FG, because IIRC pod-level-resources (PLR) is not yet beta.

I don't think this is necessarily a problem, just stating the fact.

Maybe we put an issue to update when the other FG goes in, and for now we do container-level?

(We can figure out the Slurm use cases, regardless of the structure-we'll just map)

PLR is already in beta (since 1.34)

Changing to pod-level validation.

Looks like the recent changes still sum the container requests and not Pod-Level Resources (pod.Spec.Resources) ?

cmd/dracpu-admission/dra-cpu-admission.go

pravk03 · 2026-02-12T00:49:01Z

cmd/dracpu-admission/dra-cpu-admission.go

+
+	// Prefer allocated device info when available.
+	if total, err := h.claimCPUCountFromSlices(claim); err != nil || total > 0 {
+		return total, err


I think we can just reject when a pod references a claim that is already allocated.

+1, we don't support this scenario. But I'd still keep the validation also in the driver because it's the last line of defense.

Changing to validate in the admission controller that the claim is not already allocated.

cmd/dracpu-admission/dra-cpu-admission.go

ffromani · 2026-02-12T11:00:21Z

I'll review ASAP, thanks for posting this.

pravk03 · 2026-02-12T01:26:01Z

cmd/dracpu-admission/dra-cpu-admission.go

+	if !ok {
+		return 0, false
+	}
+	value, ok := cpuQuantity.AsInt64()


Fractional requests are rounded off here. Should we use MilliValue() ?

elsewhere we have logic to ensure the request is integral (correct thing to do!)
I think we should at least use the same code for rounding logic everywhere or do similarly the scheduler does and carry along auxiliary data with rounded, validated data and make logic on that.

Either works for me at this stage

I intentionally forced integral. Thoughts?

pkg/admission/validation.go

ffromani

Thanks for posting this! it is surely a good contribution. Validation helps and this can be further extended later on with another PR to make it mutating.

The biggest open question here is if we can assume that resource claims are available when a pod is being validated. I'm not sure this is the case. And if so, the webhooks would need to deal with resourceclaims possibly being created later on.

test/e2e/sharing_test.go

cmd/dracpu-admission/dra-cpu-admission.go

ffromani · 2026-02-13T14:59:55Z

cmd/dracpu-admission/dra-cpu-admission.go

@@ -0,0 +1,461 @@
+/*
+Copyright 2025 The Kubernetes Authors.


designwise: should this be a separate executable or should this be merged with the main plugin? If separate executable, I agree it should ship in the main (and only) container image.

I would really prefer to keep things as modular as possible. It is potential for a user to want to do something specific with their admission controller, beyond (or less than) the capabilities this one provides.

Answers on this?

The reasoning is that if we decompose at package level and if we push all the logic in the packages, both of which are desirable anyway, then the main.go code, or equivalent, just becomes trivial orchestration and wiring, which perhaps some easy command line flag processing.

If that holds, then there is an argument to merge into the main executable (packages are shared anyway) for easier shipping and to actually reduce the trivial wiring.

That said, I'm fine keeping separate entry points and executable, we trend towards pushing logic in the packages anyway. If needed, we can always revisit the decision in the future.

pkg/admission/validation.go

ffromani · 2026-02-17T14:37:07Z

Makefile

-test-e2e: ## run e2e test against an existing configured cluster
-	env DRACPU_E2E_TEST_IMAGE=$(IMAGE_TEST) DRACPU_E2E_RESERVED_CPUS=$(DRACPU_E2E_RESERVED_CPUS) go test -v ./test/e2e/ --ginkgo.v
-
-test-e2e-kind: ci-kind-setup test-e2e ## run e2e test against a purpose-built kind cluster
-


the idea here was not to assume that e2e runs against the CI/development kind cluster; our e2e tests should run just fine against a real cluster. This is to reduce the chance of hidden/implicit assumptions sneak into our own e2e tests and also to enable to use the e2e tests as validation tool for real deployments, even though not production deployments likely (but we're quite far from production ready anyway... :\ )
If possible, we should keep this distinction.

This is the last item-I'm going to think on the best way to do this. The rest should be addressed. I think Praveen had an outstanding thing he told me in slack, which hopefully I can handle early next week.

@ffromani I replaced all the e2e tests so that they will run on a cluster already up in the event that one is already up, BUT in the event that there is no cluster, starts one, runs the tests, and then shuts down the cluster. Is there a different flow you would like? I had to verify that I had this fully running with a different cluster.

pkg/driver/nri_hooks.go

pkg/driver/nri_hooks_test.go

test/e2e/e2e_suite_test.go

k8s-ci-robot · 2026-02-17T21:34:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: catblade
Once this PR has been reviewed and has the lgtm label, please ask for approval from ffromani. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

- Use same identifiers across the project - Update message to use DRA Prioritized lists is not supported - Move health check until after webhook is up - Skip entries with ResourceClaimName == nil up front

- Add comments on error conditions - Move pod validation into admission pkg - Fix up various small items found by running tests - PinToNode handles casess where pinned pods may want to be scheduled on tainted nodes - Added control plane toleration for install-admission - In cpu_assigmnet_test.go, pin exclusive-claim pods to the target node so they all run on the same node

catblade · 2026-02-19T22:52:58Z

Still owe some logic changes. Hopefully will finish those tomorrow. Feel free to review structure changes. Also have the outstanding question whether to make one big app or have admission be separate.

@ffromani @pravk03

- Move to pod rather than container totals - Simplify some logic to better map to other parts of the project - requests have to match cpu requests - Round up for fractional requests (this maybe we should change)

ffromani · 2026-02-23T15:44:57Z

Still owe some logic changes. Hopefully will finish those tomorrow. Feel free to review structure changes. Also have the outstanding question whether to make one big app or have admission be separate.

@ffromani @pravk03

We can keep the admission a separate executable (therefore with a separate entry point).

However, we should strive to push all the logic in packages, so the entrypoint (the code holding the main function) becomes very simple code orchestrating all the packages. I think this take is mostly uncontroversial, but I'm stating only for completeness.

I'll have a review pass as soon as you are happy with the upload @catblade

catblade · 2026-02-25T15:25:54Z

@ffromani I moved functionality into the packages. Please let me know if you want any more changes.

catblade · 2026-02-27T15:52:14Z

@ffromani @pravk03 Just in case I wasn't clear, this is ready for re-review.

ffromani

Thanks for the updates. The biggest factor here is the claim wait logic. It's remarkably better than before, but not sure yet. Need to check with DRA folks to doublecheck.

Please extract from this PR the many QoL changes you made and extract them in one or better more targeted PRs. I think we can merge them quicker.

Misc comments about code org, the biggest/only blocker is how we handle claims, once we figure out I think we can quickly converge.

ffromani · 2026-02-27T15:26:58Z

cmd/dracpu-admission/dra-cpu-admission.go

+	healthzPath        string
+	claimGetRetryWait  time.Duration
+	claimGetRetryTotal time.Duration
+	healthzStatus      atomicBool


I still think we don't need this, or do we? we now set asynchronously, but just once and just to cover a small (hopefully) window at startup. I'd rearrange the code not to need it and just hardcode true in the healthz page, we don't seem to need anything more complex than this anytime soon.

Or, from the other angle: what's the reason to keep it? If we HAD to keep it, can we at least use a stdlib type atomic.Bool?

The healthzStatus flag is still needed as /healthz returns 503 until the server is listening. Store is called after the server is up, so readiness probes should fail until the webhook is actually ready. Will change code to atomic.Bool.

ffromani · 2026-02-27T15:27:54Z

cmd/dracpu-admission/dra-cpu-admission.go

+type admissionHandler struct {
+	driverName         string
+	clientset          kubernetes.Interface
+	claimGetRetryWait  time.Duration
+	claimGetRetryTotal time.Duration
+}


I think we should move this type and all its methods and helpers in pkg/admission (or another package) and do only setup and wiring here in main.

ffromani · 2026-02-27T15:28:57Z

cmd/dracpu-admission/dra-cpu-admission.go

+	// Fetch the ResourceClaim and sum CPU requests for this driver.
+	// Claims can be created asynchronously (for example from Pod claim templates),
+	// so retry briefly on NotFound before treating the claim as not yet available.


this is better, but not sure it's robust enough, literally not sure. We should check in with other DRA developers. I'll ask in the slack channel.

Added some more robustness... not sure what else we can add.

ffromani · 2026-02-27T15:29:34Z

cmd/dracpu-admission/dra-cpu-admission.go

+	claimGetRetryWait = durationFromEnv("DRACPU_ADMISSION_CLAIM_GET_RETRY_WAIT", claimGetRetryWait)
+	claimGetRetryTotal = durationFromEnv("DRACPU_ADMISSION_CLAIM_GET_RETRY_TOTAL", claimGetRetryTotal)


why do these and only these have an environment variable to override?

Removing for simplicity. I was doing some testing and missed cleaning them up.

ffromani · 2026-02-27T15:40:57Z

cmd/dracpu-admission/dra-cpu-admission_test.go

+
+// TestAdmissionHandler_ValidatePodClaimsWiring ensures the handler implements
+// ClaimCPUCountGetter and that admission.ValidatePodClaims works when called with it.
+func TestAdmissionHandler_ValidatePodClaimsWiring(t *testing.T) {


likewise, all tests should be pushed alongside the code in pkg/admission

ffromani · 2026-02-27T15:41:51Z

test/e2e/testdata/pod_with_claim_no_cpu.yaml

@@ -0,0 +1,26 @@
+apiVersion: resource.k8s.io/v1


please embed using the embed package or move in testdata

ffromani · 2026-02-27T15:41:57Z

test/e2e/testdata/pod_without_resource_claim.yaml

@@ -0,0 +1,13 @@
+apiVersion: v1


- Move code out of the cmd package into the pkg directory - Use atomic.Bool instead of writing our own - Move files into the testdata directory - Harden claimCPUCount to handle other potential issues - Remove unneeded environmental overrides

k8s-ci-robot · 2026-03-09T12:41:52Z

PR needs rebase.

Details

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.

ffromani · 2026-03-09T12:59:30Z

I'll have another look shortly. I'm inclined to move forward and to iterate in future PRs.

pravk03 · 2026-03-09T19:41:53Z

pkg/admission/pod_validation_test.go

+func TestValidatePodClaims_CPURequestMatchesClaimCount(t *testing.T) {
+	getter := fakeClaimCPUCountGetter{"default/claim-4": 4}
+	pod := podWithClaims("default", "pod-ok", "claim-ref", "claim-4")
+	pod.Spec.Containers[0].Resources.Requests = corev1.ResourceList{
+		corev1.ResourceCPU: resource.MustParse("4"),
+	}
+
+	errs := ValidatePodClaims(context.Background(), pod, DefaultDriverName, getter)
+	if len(errs) != 0 {
+		t.Fatalf("expected no errors, got %v", errs)
+	}
+}
+
+// With pod-level validation, a pod with dra.cpu claims must have total CPU request equal to claim total.
+func TestValidatePodClaims_NoCPURequestWithClaimRejected(t *testing.T) {
+	getter := fakeClaimCPUCountGetter{"default/claim-2": 2}
+	pod := podWithClaims("default", "pod-claim-only", "claim-ref", "claim-2")
+
+	errs := ValidatePodClaims(context.Background(), pod, DefaultDriverName, getter)
+	if len(errs) == 0 {
+		t.Fatal("expected error (pod CPU requests 0 < claim total 2), got none")
+	}
+}


Could we convert this and perhaps the other new test files to table driven tests ?

pravk03 · 2026-03-09T19:51:25Z

pkg/admission/pod_validation.go

+func CPURequestCount(cpuQuantity resource.Quantity) int64 {
+	millis := cpuQuantity.MilliValue()
+	if millis <= 0 {
+		return 0
+	}
+	value := cpuQuantity.Value()
+	if value*1000 == millis {
+		return value
+	}
+	// Round up to whole cores: 400m -> 1, 500m -> 1, 1500m -> 2
+	return (millis + 999) / 1000
+}


We should not be rounding off standard requests. If a pod spec requests 800m and the claim is requesting 1 CPU, we currently consider this as valid, which is incorrect.

pravk03 · 2026-03-09T20:35:01Z

pkg/admission/pod_validation.go

+// ValidatePodClaims enforces at pod level: when a pod has dra.cpu claims, the sum of
+// non-init container CPU requests must equal the sum of CPUs from those claims.
+// It returns a list of errors
+func ValidatePodClaims(ctx context.Context, pod *corev1.Pod, driverName string, getter ClaimCPUCountGetter) []string {


I propose an alternate logic for this function:

Case 1: pod.Spec.Resources not populated

For individual containers with claims, standard request == claim request

If multiple containers reference the same claim, sum of standard requests from containers referencing same claim == claim request

Case 2: pod.Spec.Resources is specified

Containers with claims should not specify standard CPU requests.

pod.Spec.Resources >= sum of claim requests from pod.Spec.ResourceClaims + sum of standard requests from non-init containers

Once we introduce a mutating webhook, we could always set pod.Spec.Resources which would make migration to KEP-5517 seamless.

cc: @ffromani

pravk03 · 2026-03-09T21:06:26Z

pkg/admission/claim_count.go

+	if count < 1 {
+		count = 1
+	}
+	if req.Capacity != nil && len(req.Capacity.Requests) > 0 {


I realized there is an edge case withcpu-device-mode=grouped. In grouped mode, a count of 1 without req.Capacity doesn't mean 1 CPU - —it means 1 device (like an entire NUMA node or socket depending on --cpu-device-group-by) is allocated to the claim.

To address this, we could pass the --cpu-device-mode flag value to the webhook

If mode == individual, we can keep the logic as-is and just read the count.

If mode == grouped, we require req.Capacity to also be set. We could temporarily throw a validation error if req.Capacity is nil with a TODO to handle this later.

catblade added 5 commits February 10, 2026 12:01

chore: update version of kubernetes in kind.yaml file

f2a3fb5

feat: add in current admission controller code and updated testing

2803f74

Chore: add in additional testing and do additional Makefile cleanup

0275f09

chore: fix pre-commit issues

6563643

k8s-ci-robot requested review from pohly and pravk03 February 10, 2026 18:45

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 10, 2026

k8s-ci-robot assigned ffromani and pravk03 Feb 10, 2026

pravk03 reviewed Feb 12, 2026

View reviewed changes

ffromani reviewed Feb 17, 2026

View reviewed changes

fix: handle async events with small sleep and limited retry, configured

3c3528d

chore: update to use api pkg sets rather than making one ourselves

0b97d3a

catblade force-pushed the marlow/webadmission-validation-rebuild branch from 2ca2c64 to 0b97d3a Compare February 17, 2026 21:45

catblade added 2 commits February 18, 2026 16:33

chore: update code according to reviews

99cffbf

- Use same identifiers across the project - Update message to use DRA Prioritized lists is not supported - Move health check until after webhook is up - Skip entries with ResourceClaimName == nil up front

Chore: Moving to pod rather than container totals plus cleanup

5bd4958

- Move to pod rather than container totals - Simplify some logic to better map to other parts of the project - requests have to match cpu requests - Round up for fractional requests (this maybe we should change)

catblade added 2 commits February 23, 2026 10:17

Chore: update makefile to work with running clusters for admission

5dc2f09

chore: move code out of main files into packages

8e7f7b8

ffromani reviewed Feb 27, 2026

View reviewed changes

Chore: address review comments

e48c666

- Move code out of the cmd package into the pkg directory - Use atomic.Bool instead of writing our own - Move files into the testdata directory - Harden claimCPUCount to handle other potential issues - Remove unneeded environmental overrides

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

pravk03 reviewed Mar 9, 2026

View reviewed changes

AutuSnow mentioned this pull request Apr 3, 2026

Add CPU request validation in driver NodeStageVolume as final line of defense #108

Open

		claimGetRetryWait = durationFromEnv("DRACPU_ADMISSION_CLAIM_GET_RETRY_WAIT", claimGetRetryWait)
		claimGetRetryTotal = durationFromEnv("DRACPU_ADMISSION_CLAIM_GET_RETRY_TOTAL", claimGetRetryTotal)

Conversation

catblade commented Feb 10, 2026

Uh oh!

pravk03 commented Feb 10, 2026

Uh oh!

pravk03 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

ffromani commented Feb 12, 2026

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!

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 17, 2026

Uh oh!

catblade commented Feb 19, 2026

Uh oh!

ffromani commented Feb 23, 2026

Uh oh!

catblade commented Feb 25, 2026

Uh oh!

catblade commented Feb 27, 2026

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment