Skip to content

Add DRA support to the topology-aware policy - #2

Open
bart0sh wants to merge 11 commits into
mainfrom
PR001-DRA-draft
Open

bart0sh wants to merge 11 commits into
mainfrom
PR001-DRA-draft

Conversation

@bart0sh

@bart0sh bart0sh commented Aug 29, 2026

Copy link
Copy Markdown
Owner

This PR adds optional DRA integration to the topology-aware policy, letting workloads request high-priority PCT (Performance/Turbo Class) CPUs via a Kubernetes ResourceClaim instead of (or alongside) the existing NRI-based allocation path.

What's included

  • A reusable Kubernetes client wrapper (pkg/kubernetes/client) and a generic DRA kubelet-plugin skeleton (pkg/resmgr/dra), independent of any specific policy.
  • cpuClass.dra.publish config field, plus ValidateCPUClassesForDRA guarding against overcommitting a turbo-frequency tier when multiple classes are published.
    pct.PickHpCpus/ReleaseHpCpus/Punits, giving the DRA driver a way to allocate and release high-priority CPUs from the existing PCT/SST-TF punit accounting.
  • A DRA device builder (cpuclass.dra.go) that publishes one device per high-priority cpuClass × SST-TF punit, exposing nri/pctPriority, nri/packageID, nri/punitID as selectable device attributes.
  • Full kubelet-plugin lifecycle (registration, PrepareResourceClaims/UnprepareResourceClaims, CDI spec writing, claim-state persistence and restart recovery).
  • Wiring into the topology-aware policy: Setup/Start/Stop/Reconfigure integration, pool-supply accounting for claimed CPUs (multi-container claims via a refcount), and safe refusal of dra.enabled flips or DRA-attribute changes on classes with live claims.
  • Helm chart additions: config.dra.{enabled,sharedCounters} values, RBAC, host mounts, and the base DeviceClass.
  • An e2e test (test20-dra) covering claim -> pod -> CLOS-association → scheduler-accounting → cleanup, with a feature-gate probe that skips cleanly on clusters without the KEP-5075/KEP-5517 gates.

Requirements

  • Kubernetes 1.34+ for KEP-5075 (DRAConsumableCapacity) — required.
  • Kubernetes 1.37+ for KEP-5517 (DRANodeAllocatableResources) — optional; without it DRA allocation still works but scheduler-side capacity accounting is skipped.

Known v1 limitations (by design, not oversights)

  • Only high-priority PCT classes are published as DRA devices; non-HP classes and CEL selectors targeting them are rejected at Prepare time.
    config.dra.sharedCounters is accepted by the schema but rejected at validation time — it's reserved for (KEP-5941) shared-counter publication, which isn't implemented yet.
  • A DRA-claimed CPU can still collide with a CPU already exclusively granted to a reserved-type container (updateSharedAllocations doesn't re-pin/evict reserved grants); tracked as a follow-up.
    dra.enabled: true with local-file (non-Kubernetes) configuration degrades to DRA-disabled for the process lifetime rather than retrying, since the agent skips kube-client setup in that mode.
  • Live Reconfigure() support for DRA config changes (validated cpuClass edits, live-claim-aware refusal, PostReconfigure-deferred republish) was implemented and then deferred to a future PR — not because it doesn't work, but to keep this PR's diff reviewable. Right now, any config change while DRA is (or was) enabled is refused outright with a restart-required error.

Testing

  • Unit tests across all new/touched packages (go test ./... — 27 suites, all passing).
  • test20-dra e2e test, run against a live Kubernetes 1.37 cluster with both feature gates enabled.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Claimed CPUs are not made accessible to consuming containers, and several failure paths can leak or lose claim accounting.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds Dynamic Resource Allocation support to the topology-aware policy, integrating high-priority PCT CPUs with Kubernetes ResourceClaims, CDI, scheduler accounting, and policy lifecycle management.

Changes:

  • Adds the reusable Kubernetes client, DRA plugin, CDI, persistence, and restart plumbing.
  • Publishes PCT CPU classes as DRA devices and integrates claims with topology-aware accounting.
  • Adds Helm resources, Kubernetes feature-gate provisioning, documentation, and extensive tests.
File summaries
File Description
.gitignore Ignores generated test artifacts
go.mod Adds DRA and CDI dependencies
pkg/kubernetes/client/client.go Adds shared Kubernetes client wrapper
pkg/kubernetes/client/client_test.go Tests client construction and options
pkg/kubernetes/client/testdata/kubeconfig-example.yaml Provides client test fixture
pkg/kubernetes/watch/watch.go Uses dedicated watch logger
pkg/kubernetes/watch/object.go Adds Kubernetes object watcher
pkg/kubernetes/watch/file.go Adds file-backed watcher
pkg/kubernetes/watch/watch_test.go Tests watch behavior
pkg/agent/agent.go Integrates shared Kubernetes client
pkg/agent/agent_test.go Tests agent client getters
pkg/resmgr/dra/doc.go Documents the DRA package
pkg/resmgr/dra/deps.go Defines plugin dependency interfaces
pkg/resmgr/dra/logging.go Bridges project and logr logging
pkg/resmgr/dra/state.go Persists ResourceClaim allocations
pkg/resmgr/dra/state_test.go Tests claim persistence
pkg/resmgr/dra/cdi.go Writes claim CDI specifications
pkg/resmgr/dra/cdi_test.go Tests CDI lifecycle
pkg/resmgr/dra/plugin.go Implements DRA plugin lifecycle
pkg/resmgr/dra/plugin_test.go Tests preparation and recovery
pkg/resmgr/cpuclass/cpuclass.go Exposes DRA allocation operations
pkg/resmgr/cpuclass/dra.go Builds and validates DRA devices
pkg/resmgr/cpuclass/dra_test.go Tests DRA device publication
pkg/resmgr/cpuclass/cpuclass_dra_test.go Tests allocator delegation
pkg/resmgr/cpuclass/internal/pct/pct.go Tracks DRA-held HP CPUs
pkg/resmgr/cache/cache.go Extends the container interface
pkg/resmgr/cache/container.go Exposes CDI device names
pkg/resmgr/cache/container_test.go Tests CDI name retrieval
pkg/resmgr/policy/policy.go Adds DRA lifecycle dependencies
pkg/resmgr/policy/policy_test.go Tests lifecycle forwarding
pkg/resmgr/resource-manager.go Wires clients, locks, and shutdown
pkg/resmgr/resource_manager_test.go Tests resource-manager lifecycle
pkg/resmgr/main/main.go Stops the manager on exit
pkg/resmgr/main/main_test.go Tests shutdown sequencing
pkg/apis/config/v1alpha1/resmgr/policy/cpuclass.go Adds per-class publication settings
pkg/apis/config/v1alpha1/resmgr/policy/cpuclass_test.go Tests publication defaults
pkg/apis/config/v1alpha1/resmgr/policy/zz_generated.deepcopy.go Deep-copies DRA class settings
pkg/apis/config/v1alpha1/resmgr/policy/topologyaware/config.go Adds topology-aware DRA configuration
pkg/apis/config/v1alpha1/resmgr/policy/topologyaware/config_test.go Tests DRA configuration helpers
pkg/apis/config/v1alpha1/resmgr/policy/topologyaware/zz_generated.deepcopy.go Deep-copies topology DRA settings
cmd/plugins/topology-aware/policy/topology-aware-policy.go Integrates DRA lifecycle and recovery
cmd/plugins/topology-aware/policy/resources.go Accounts claimed CPUs in supplies
cmd/plugins/topology-aware/policy/dra.go Constructs and republishes the plugin
cmd/plugins/topology-aware/policy/dra_adapter.go Adapts CPU-class operations
cmd/plugins/topology-aware/policy/dra_adapter_test.go Tests adapter forwarding
cmd/plugins/topology-aware/policy/dra_test.go Tests DRA policy integration
cmd/plugins/topology-aware/policy/pools.go Maps claims into pool accounting
cmd/plugins/topology-aware/policy/pools_test.go Tests claim pool accounting
cmd/plugins/topology-aware/policy/resources_test.go Tests supply claim behavior
cmd/plugins/topology-aware/policy/topology-aware-policy_test.go Tests claim restoration
cmd/plugins/topology-aware/policy/mocks_test.go Extends policy test mocks
cmd/plugins/topology-aware/policy/cache_test.go Updates allocation fixtures
cmd/plugins/template/policy/template-policy.go Implements new lifecycle hooks
cmd/plugins/balloons/policy/balloons-policy.go Implements new lifecycle hooks
deployment/helm/topology-aware/values.yaml Adds DRA Helm values
deployment/helm/topology-aware/templates/deviceclass.yaml Installs the CPU DeviceClass
deployment/helm/topology-aware/templates/daemonset.yaml Mounts kubelet and CDI paths
deployment/helm/topology-aware/templates/clusterrole.yaml Grants DRA API permissions
deployment/helm/topology-aware/crds/config.nri_topologyawarepolicies.yaml Updates packaged topology CRD
deployment/helm/topology-aware/crds/config.nri_balloonspolicies.yaml Updates packaged balloons CRD
config/crd/bases/config.nri_topologyawarepolicies.yaml Updates topology CRD schema
config/crd/bases/config.nri_balloonspolicies.yaml Updates balloons CRD schema
docs/resource-policy/policy/topology-aware.md Documents DRA usage
test/e2e/run.sh Adds Kubernetes gate controls
test/e2e/run_tests.sh Preserves custom VM names
test/e2e/playbook/provision.yaml Configures gated Kubernetes components
test/e2e/files/Vagrantfile.in Passes provisioning options
test/e2e/policies.test-suite/topology-aware/helm-config.yaml.in Enables DRA in test configuration
test/e2e/policies.test-suite/topology-aware/n4c16/test20-dra/code.var.sh Adds end-to-end DRA coverage
Review details
  • Files reviewed: 67/71 changed files
  • Comments generated: 8
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cmd/plugins/topology-aware/policy/topology-aware-policy.go Outdated
Comment thread cmd/plugins/topology-aware/policy/pools.go Outdated
Comment thread cmd/plugins/topology-aware/policy/topology-aware-policy.go Outdated
Comment thread pkg/resmgr/dra/plugin.go
Comment thread pkg/resmgr/dra/plugin.go Outdated
Comment thread cmd/plugins/topology-aware/policy/dra.go
Comment thread docs/resource-policy/policy/topology-aware.md Outdated
Comment thread cmd/plugins/topology-aware/policy/pools.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Claim rollback and recovery paths can leak capacity, evict unrelated workloads, or accept CPUs without applying their requested class.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

cmd/plugins/topology-aware/policy/pools.go:1671

  • If reallocation fails, this returns after ClaimCPUs and the physical class update but before incrementing claimContainerRefs. The caller therefore cannot roll back this current claim (it only receives previously successful claims), leaving CPUs permanently removed from pool supply with a zero refcount. Undo the current mark and class before returning the error.
		if err := p.reallocateEvicted(evicted, evictedCpusets, cpus, uid); err != nil {
			return policyError("dra: claim %s: evicted %d container(s) to free CPUs %s but failed "+
				"to fully reallocate them: %v", uid, len(evicted), cpus, err)
		}
  • Files reviewed: 67/71 changed files
  • Comments generated: 6
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/pools.go
Comment thread cmd/plugins/topology-aware/policy/topology-aware-policy.go
Comment thread pkg/kubernetes/client/client.go Outdated
Comment thread cmd/plugins/topology-aware/policy/resources.go
Comment thread cmd/plugins/topology-aware/policy/pools.go
Comment thread pkg/resmgr/dra/doc.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Claim accounting, rollback, persistence, and ResourceSlice consistency have unresolved correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

cmd/plugins/topology-aware/policy/resources.go:345

  • Clone retains cs.node, so calling ClaimCPUs or UnclaimCPUs on the clone still recurses into parent.FreeSupply() and mutates the original tree's ancestor supplies. This violates clone isolation (the test only checks the original leaf) and can corrupt capacity during dry-run/offer operations. Make cloned supplies non-propagating, or clone the associated ancestor chain as well.
	clone := newSupply(cs.node, cs.isolated, cs.reserved, cs.sharable, cs.grantedReserved, cs.grantedShared).(*supply)
	if len(cs.claimRefs) > 0 {
		clone.claimRefs = make(map[types.UID]cpuset.CPUSet, len(cs.claimRefs))
		for uid, cpus := range cs.claimRefs {
			clone.claimRefs[uid] = cpus.Clone()
  • Files reviewed: 67/71 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/pools.go Outdated
Comment thread cmd/plugins/topology-aware/policy/pools.go
Comment thread cmd/plugins/topology-aware/policy/pools.go
Comment thread pkg/resmgr/dra/plugin.go Outdated
Comment thread pkg/resmgr/resource-manager.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Claim accounting, persistence rollback, and ResourceSlice publication still contain correctness and recovery failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

pkg/resmgr/dra/plugin.go:404

  • The durable-state write happens only after CPUs, CDI, and in-memory state have already been removed, and its failure is discarded. Kubelet is told unprepare succeeded and will not retry, but a restart can reload the old durable claim and account those CPUs again. Persist each removal before releasing its resources; on failure retain the claim and return a per-UID error.
    pkg/resmgr/dra/plugin.go:287
  • Deleting the claim from p.claims does not undo the preceding cacheClaimStore.Save: that call runs SetPolicyEntry before the failing disk save, leaving the rolled-back claim in the cache's in-memory policyData. A later unrelated cache save can therefore persist a claim whose CDI and CPU picks were removed. Save the corrected map after deletion so the backing store's in-memory state is also rolled back.
    cmd/plugins/topology-aware/policy/pools.go:1679
  • When victim reallocation fails, this returns after the claim was marked and its CPU class applied, but before the refcount is incremented. The caller cannot release that partial mark, so the CPUs remain removed from pool capacity and any unreallocated victim may remain unmanaged. Roll back the supply/class changes and restore the evicted grants before returning the error.
		if err := p.reallocateEvicted(evicted, evictedCpusets, cpus, uid); err != nil {
			return policyError("dra: claim %s: evicted %d container(s) to free CPUs %s but failed "+
				"to fully reallocate them: %v", uid, len(evicted), cpus, err)

pkg/resmgr/resource-manager.go:187

  • PostReconfigure publication failures are still discarded, while the new CPU-class configuration has already been committed. This can leave stale ResourceSlices that advertise devices Prepare no longer accepts, with no retry until another configuration event. Propagate the failure with rollback or schedule reliable retries until the new device set is published.
    cmd/plugins/topology-aware/policy/pools.go:1532
  • Finding one CDI device from a claim still assigns every allocation in that claim to this container. Different containers may consume different request/device results from one ResourceClaim, so this can pin each container to CPUs it did not request and incorrectly increment accounting for the entire claim. Match each actual CDI device name to its corresponding ResultAlloc index and aggregate only those matches.
		claimCPUs, claimClassCPUs := classifyClaimCPUs(uid, live[uid])
  • Files reviewed: 66/70 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread pkg/resmgr/cpuclass/internal/pct/pct.go
Comment thread pkg/resmgr/dra/plugin.go Outdated
Comment thread cmd/plugins/topology-aware/policy/topology-aware-policy.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Claimed CPUs can be removed from effective pinning, stale claims can recover with the wrong class, and gated provisioning breaks on older kubeadm versions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/e2e/playbook/provision.yaml:514

  • The generated kubeadm config still hardcodes v1beta4. This path is also used when only k8s_log_verbosity (or an unrelated feature gate) is set, while the e2e runner permits older Kubernetes releases whose kubeadm does not support v1beta4; those runs fail before cluster initialization. Select v1beta3 for pre-1.31 releases (using its map-form extraArgs) and v1beta4 for 1.31+, or reject unsupported combinations explicitly.
  • Files reviewed: 67/71 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/pools.go Outdated
Comment thread pkg/resmgr/dra/plugin.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Reconfiguration can falsely reject unchanged live claims, and kubeadm provisioning remains incompatible with supported pre-1.31 releases.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

cmd/plugins/topology-aware/policy/topology-aware-policy.go:772

  • This comparison includes the dynamically advertised nri/cpus capacity, but it runs immediately after initialize() has replaced the CPU-class handler and before restoreAllocations() rebuilds non-DRA hpUsed. If an ordinary HP-class workload is running, the old snapshot has reduced capacity while the new snapshot has full capacity, so any unrelated reconfiguration is rejected whenever this class also has a live DRA claim. Compare only claim-invalidating identity/attributes, or rebuild the existing allocations before taking the new capacity snapshot.
		for _, class := range changedDRAClasses(oldDRADevices, newDRADevices) {

test/e2e/playbook/provision.yaml:514

  • The generated config always uses kubeadm.k8s.io/v1beta4 whenever either new option is set. The runner still permits older Kubernetes releases, and v1beta4 is unavailable before Kubernetes 1.31, so even setting only k8s_log_verbosity breaks provisioning on those releases. Select the kubeadm API from k8s_version and render the matching extraArgs shape (v1beta3 uses a map, while v1beta4 uses the name/value list), or reject unsupported versions before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread pkg/apis/config/v1alpha1/resmgr/policy/topologyaware/config.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Reconfiguration can falsely reject unchanged live-claim configurations, and the e2e kubeadm configuration breaks supported pre-1.31 releases.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

cmd/plugins/topology-aware/policy/topology-aware-policy.go:772

  • This compares the new handler before restoreAllocations has rebuilt its non-DRA hpUsed state. The old snapshot deducts live NRI-path HP allocations from HPCapacity, while the new snapshot reports full capacity, so an otherwise unchanged reconfiguration is rejected whenever that class also has a live DRA claim. Restore the existing allocations before taking the new snapshot, or compare only configuration/topology-derived fields and handle dynamic capacity separately.
	if p.draPlugin != nil {
		var newDRADevices []resapi.Device
		if p.cpuClasses != nil {
			newDRADevices, _ = p.cpuClasses.DRADevices(DRADriverName)
		}
		liveClasses := p.draPlugin.LiveClaimClasses()
		for _, class := range changedDRAClasses(oldDRADevices, newDRADevices) {

test/e2e/playbook/provision.yaml:514

  • The gated path unconditionally emits kubeadm v1beta4, but this test harness still accepts Kubernetes releases older than 1.31; setting the generic log-verbosity option on such a release makes kubeadm init reject the config. Select v1beta3 for pre-1.31 releases (including its map-shaped extraArgs) or explicitly reject those combinations before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread config/crd/bases/config.nri_topologyawarepolicies.yaml

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Reconfiguration can reject unchanged live DRA classes, older e2e Kubernetes releases can receive an unsupported kubeadm schema, and file-watch errors are not consumed.

Review details

Suppressed comments (2)

cmd/plugins/topology-aware/policy/topology-aware-policy.go:770

  • This snapshot is taken immediately after initialize(), before restoreAllocations() rebuilds non-DRA HP usage. Because DRADevices() subtracts hpUsed from each device's capacity, any unchanged reconfiguration made while a live DRA claim and an NRI HP workload coexist compares the old reduced capacity with the new full capacity and is rejected as a live-class change. Build the new snapshot after restoring ordinary allocations, or exclude workload-driven capacity from this compatibility check.
	if p.draPlugin != nil {
		var newDRADevices []resapi.Device
		if p.cpuClasses != nil {
			newDRADevices, _ = p.cpuClasses.DRADevices(DRADriverName)
		}

test/e2e/playbook/provision.yaml:514

  • This always emits kubeadm v1beta4 whenever either option is set, but the e2e runner still supports Kubernetes releases before 1.31, where kubeadm only accepts v1beta3. In particular, setting the generic log-verbosity option on an older selected release makes kubeadm init fail. Select both the API version and the corresponding extraArgs schema from k8s_version, or reject unsupported combinations explicitly.
  • Files reviewed: 67/71 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

DRA validation, claim cleanup, preserved-cpuset handling, device naming, and older kubeadm compatibility still have correctness gaps.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

pkg/resmgr/cpuclass/dra.go:106

  • Assoc-only classes are not necessarily non-HP: classifyAssocOnlyHP marks classes on the highest-frequency CLOS as HP (pct.go:385-391), and buildDRADevices(..., hpOnly=true) then publishes them. Skipping every class with an empty PctPriority lets two published names for the same HP CLOS each advertise the full punit capacity, defeating this overcommit guard. Validate against the runtime HP classification or otherwise include assoc-only HP tiers.
    test/e2e/playbook/provision.yaml:514
  • This hardcodes kubeadm v1beta4 whenever either new option is set, but the e2e runner still permits Kubernetes releases before 1.31, where kubeadm does not recognize that API version. In particular, enabling only the generic log-verbosity option now breaks otherwise supported older test runs. Select the kubeadm API and corresponding extraArgs shape from k8s_version, or reject unsupported versions before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/topology-aware-policy.go
Comment thread cmd/plugins/topology-aware/policy/pools.go Outdated
Comment thread pkg/resmgr/cpuclass/dra.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

DRA capacity can become stale after resource updates, LP-only configurations can be rejected incorrectly, and older e2e Kubernetes releases use an unsupported kubeadm API.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

pkg/resmgr/cpuclass/dra.go:118

  • DRADevices always calls buildDRADevices(..., hpOnly=true), so pctPriority: low classes are never published, but this validation still groups them by the low tier. Two ordinary LP classes therefore make DRA startup fail for an overcommit that cannot occur. Restrict this guard to pctPriority == "high", matching the actual published set.
    test/e2e/playbook/provision.yaml:514
  • This optional path still hard-codes kubeadm v1beta4, which is unsupported before Kubernetes 1.31. Because the e2e runner accepts explicitly pinned older releases and log verbosity is not DRA-version-gated, enabling either new option on such a release makes kubeadm init fail. Select v1beta3 (and its map-form extraArgs) for pre-1.31 releases, or reject those combinations before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/dra.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unprepare can leak claimed pool capacity, and gated provisioning fails on supported older Kubernetes releases.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/e2e/playbook/provision.yaml:514

  • The configurable e2e runner still permits Kubernetes releases older than 1.31, and setting only the generic log-verbosity option also selects this path. Those kubeadm versions do not recognize kubeadm.k8s.io/v1beta4, so kubeadm init fails. Select v1beta3 (including its map-form extraArgs) for pre-1.31 releases, or reject those releases before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/pools.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Assoc-only classes can over-advertise shared HP capacity, and gated provisioning remains incompatible with supported pre-1.31 Kubernetes releases.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/e2e/playbook/provision.yaml:514

  • This generated config is also selected when only the generic log-verbosity option is set, but kubeadm.k8s.io/v1beta4 is unavailable before Kubernetes 1.31. Since the e2e runner still accepts older explicit k8s_release values, those runs fail at kubeadm init whenever either new option is used. Select both the kubeadm API version and its version-specific extraArgs shape from k8s_version, or reject unsupported releases clearly.
  • Files reviewed: 67/71 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread pkg/resmgr/cpuclass/dra.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unprepare can release CPU ownership prematurely, and the new provisioning and file-watch paths contain reliability regressions.

Review details

Suppressed comments (2)

cmd/plugins/topology-aware/policy/pools.go:1815

  • If Unprepare races with the final container release, this early return preserves the pool mark but not the PCT allocation hold: UnprepareResourceClaims has already called ReleaseHpCpus and removed the claim from p.claims. A new claim can therefore pick the same CPUs while the old container is still running. Retain/re-account the HP hold for this tombstoned claim and release it only when the final container reference is dropped.
	if p.claimContainerRefs != nil && p.claimContainerRefs[uid] > 0 {
		return

test/e2e/playbook/provision.yaml:514

  • This gated path is also used for the generic log-verbosity option, but kubeadm.k8s.io/v1beta4 is unsupported before Kubernetes 1.31 while the e2e runner still accepts older k8s_release values. Such runs fail at kubeadm init; select both the kubeadm API version and the corresponding extraArgs shape from k8s_version, or reject unsupported combinations explicitly.
  • Files reviewed: 67/71 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Kubernetes provisioning compatibility and filesystem watch error handling contain unresolved failures.

Review details

Suppressed comments (1)

test/e2e/playbook/provision.yaml:514

  • This config path is also selected when only k8s_log_verbosity is set, but kubeadm.k8s.io/v1beta4 is unsupported before Kubernetes 1.31. Since the e2e runner still accepts older k8s_release values and explicitly documents verbosity as independent of the DRA gates, those runs fail at kubeadm init. Select v1beta3/v1beta4 from k8s_version and render the matching extraArgs shape, or reject unsupported releases before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved publication-error handling, claim accounting, reconfiguration, validation, and kubeadm compatibility defects can leave DRA unavailable or incorrectly accounted.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

test/e2e/playbook/provision.yaml:514

  • This path is also used when only k8s_log_verbosity is set, but run.sh still accepts arbitrary Kubernetes releases. kubeadm.k8s.io/v1beta4 is unavailable before Kubernetes 1.31, so enabling the new verbosity option on an older supported test VM makes kubeadm init reject the config; those releases require v1beta3, whose extraArgs shape is different. Generate the API version/argument shape from k8s_version, or reject unsupported versions before provisioning.
    pkg/resmgr/cpuclass/dra.go:113
  • This groups pctPriority: low classes even though production calls buildDRADevices with hpOnly=true, so those classes can never publish a device or overcommit DRA capacity. Enabling DRA therefore rejects an otherwise harmless configuration containing two published low-priority classes. Skip statically non-HP managed classes here; assoc-only classes still need grouping because their HP status is hardware-dependent.
  • Files reviewed: 67/71 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/dra.go Outdated
Comment thread pkg/resmgr/dra/plugin.go Outdated
Comment thread pkg/resmgr/dra/plugin.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Restart accounting, CPU-class validation, and older Kubernetes provisioning contain correctness regressions.

Review details

Suppressed comments (3)

pkg/resmgr/cpuclass/dra.go:118

  • This validation includes managed pctPriority: low classes even though DRADevices(..., hpOnly=true) never publishes them. As a result, enabling DRA with two ordinary low-priority classes fails startup with a tier-overcommit error despite those classes producing no DRA devices and consuming no DRA capacity. Restrict conflict checking to classes the handler actually classifies as HP (and add a two-low-class regression case).
    test/e2e/playbook/provision.yaml:514
  • The configurable path always emits kubeadm.k8s.io/v1beta4, but run.sh still accepts arbitrary older k8s_release values and the new generic k8s_log_verbosity option selects this path too. Kubernetes before 1.31 does not support v1beta4 (and v1beta3 uses the older map-shaped extraArgs), so those existing e2e configurations now fail at kubeadm init. Generate the API version and extraArgs shape from k8s_version, or reject unsupported combinations before provisioning.
    pkg/resmgr/dra/plugin.go:575
  • Continuing after AccountHpCpus fails leaves this persisted claim absent from hpDRAUsed. If punit IDs/topology changed across restart, the allocator can subsequently pick the same physical CPUs through a current punit while the recovered container still owns them. Overcommit is already handled inside AccountHpCpus without returning an error, so an actual error means accounting could not be restored and startup should fail rather than register the driver in an unsafe state.
  • Files reviewed: 67/71 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Restart reconciliation, fsnotify error handling, and older kubeadm compatibility contain unresolved reliability issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/e2e/playbook/provision.yaml:514

  • This always emits kubeadm v1beta4 whenever either new option is set. The e2e runner still accepts arbitrary older Kubernetes releases, but kubeadm only supports v1beta4 from Kubernetes 1.31 onward, so enabling the generic log-verbosity option on an older release makes kubeadm init fail. Select v1beta3 for pre-1.31 releases (or reject those combinations explicitly).
  • Files reviewed: 67/71 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread pkg/resmgr/cpuclass/internal/pct/pct.go
@pravk03

pravk03 commented Sep 8, 2026

Copy link
Copy Markdown

/cc

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The optional provisioning path breaks Kubernetes releases before 1.31 by always using kubeadm v1beta4.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/e2e/playbook/provision.yaml:514

  • This config path is also selected when only k8s_log_verbosity is set, but kubeadm.k8s.io/v1beta4 is unavailable before Kubernetes 1.31 while the e2e runner still permits older k8s_release values. Those runs now fail at kubeadm init; select the kubeadm API and matching extraArgs shape from k8s_version, or reject these options for unsupported releases before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/pools.go Outdated
Comment thread docs/resource-policy/policy/topology-aware.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Claim safety and older Kubernetes provisioning have unresolved correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

test/e2e/playbook/provision.yaml:514

  • This still hard-codes v1beta4 whenever either new option is used. The e2e runner supports Kubernetes releases older than 1.31, whose kubeadm cannot decode that API, so setting even only k8s_log_verbosity breaks provisioning. Select v1beta3 for versions before 1.31 and v1beta4 otherwise.
  • Files reviewed: 67/71 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/topology-aware-policy.go Outdated
Comment thread pkg/resmgr/dra/plugin.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

DRA class-application failures are still hidden, live reconfiguration is overly rejected, and provisioning/watch reliability issues remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

cmd/plugins/topology-aware/policy/pools.go:1653

  • This error check cannot detect hardware programming failures: cpuclass.Handler.UseClass logs both cpufreq/PCT errors and always returns nil (pkg/resmgr/cpuclass/cpuclass.go:342-349). Consequently a failed SST CLOS association still commits the claim's supply mark and reports allocation success. Add a DRA class-application API that propagates the underlying error (or change UseClass to do so) so this rollback path is reachable.
		if err := p.applyClassCPUs("apply", uid, classCPUs); err != nil {

test/e2e/playbook/provision.yaml:514

  • This branch is selected even when only the generic log-verbosity option is set, but kubeadm releases before Kubernetes 1.31 do not accept kubeadm.k8s.io/v1beta4; the e2e runner still permits pinning those releases. Such runs now fail at kubeadm init. Select v1beta3 for older k8s_version values (using its map-shaped extraArgs) or reject that combination before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread cmd/plugins/topology-aware/policy/topology-aware-policy.go
bart0sh and others added 11 commits September 9, 2026 16:36
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The change spans concurrency-sensitive resource accounting, persistent recovery, kubelet integration, and deployment behavior requiring final human validation.

Review details
  • Files reviewed: 67/71 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Durable unprepare recovery, assoc-only class validation, and older kubeadm compatibility still have correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

pkg/resmgr/cpuclass/dra.go:107

  • This groups every assoc-only class by CLOS before the runtime HP classification is known, so it rejects duplicate classes on a low-priority CLOS even though buildDRADevices(..., hpOnly=true) publishes neither of them. For example, two aliases for CLOS 1 plus a higher-frequency CLOS 2 are valid in the PCT allocator, which classifies only CLOS 2 as HP, but this validation still fails on CLOS 1. Validate only the classes that IsHPClass says will actually be published, or perform the duplicate-capacity check against the built device set.
    test/e2e/playbook/provision.yaml:514
  • The configurable path is also selected when only k8s_log_verbosity is set, but kubeadm.k8s.io/v1beta4 is unavailable before Kubernetes 1.31. Since the runner still accepts explicitly pinned older Kubernetes releases and does not restrict the generic verbosity option, those runs fail at kubeadm init. Render the v1beta3 schema for pre-1.31 releases (its extraArgs shape also differs), or reject this option for unsupported releases before provisioning.
  • Files reviewed: 67/71 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread pkg/resmgr/dra/plugin.go
Comment on lines +426 to +432
delete(p.claims, uid)
if saveErr := p.deps.ClaimStore.Save(p.claims); saveErr != nil {
p.claims[uid] = cs
perUID[uid] = fmt.Errorf("dra plugin: UnprepareResourceClaims: ClaimStore.Save: %w", saveErr)
continue
}
p.deps.ClaimUnprepare(uid, cs.Allocs)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants