-
Notifications
You must be signed in to change notification settings - Fork 44
Add DRA support to the topology-aware policy #774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
469d9e0
refactor: add pkg/kubernetes/client, rewire pkg/agent to use it
bart0sh 0c170a6
dra: introduce pkg/resmgr/dra plugin skeleton
bart0sh 432afef
cpuclass: add dra.publish config field and validation
bart0sh d0937df
pct: add PickHpCpus, ReleaseHpCpus, and Punits
bart0sh fcbb480
cpuclass: implement DRA device builder
bart0sh 45a8663
dra: implement kubelet plugin lifecycle
bart0sh 8f70761
dra: implement Prepare/UnprepareResourceClaims and CDI writer
bart0sh ec901ca
topology-aware: wire DRA plugin into the policy
bart0sh b7a3d42
helm/topology-aware: add DRA chart additions
bart0sh 53cc605
e2e: add DRA e2e test
bart0sh bbdab37
e2e: add k8s_log_verbosity var for control-plane/kubelet debug logging
bart0sh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| // Copyright 2019 Intel Corporation. All Rights Reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package topologyaware | ||
|
|
||
| import ( | ||
| policyapi "github.com/containers/nri-plugins/pkg/resmgr/policy" | ||
|
|
||
| "github.com/containers/nri-plugins/pkg/resmgr/cpuclass" | ||
| "github.com/containers/nri-plugins/pkg/resmgr/dra" | ||
| "github.com/containers/nri-plugins/pkg/utils/cpuset" | ||
| ) | ||
|
|
||
| // buildDRAPlugin constructs p.draPlugin from the current policy | ||
| // configuration (p.cfg, p.cpuClasses, p.cache) and the given backend | ||
| // options. Called once from Setup() when cfg.DRAEnabled() is true, and | ||
| // never from Reconfigure() (which refuses any change to DRAEnabled outright | ||
| // rather than tearing down/rebuilding p.draPlugin). | ||
| // | ||
| // A missing kube client or node name is treated as "DRA not ready yet" | ||
| // rather than a hard Setup() failure: this logs a warning and leaves | ||
| // p.draPlugin nil. Every other Backend lifecycle method already nil-checks | ||
| // p.draPlugin, so this degrades to "DRA disabled" rather than crashing. | ||
| // | ||
| // An empty (nil) p.cpuClasses is different: the plugin is still built, just | ||
| // with an empty device set (every *cpuclass.Handler method the adapter | ||
| // calls is nil-receiver-safe, see policyDRAAdapter's doc comment). Since | ||
| // buildDRAPlugin only ever runs once, leaving p.draPlugin nil here would | ||
| // prevent a later Reconfigure() that adds cpuClasses from ever enabling DRA. | ||
| // | ||
| // Genuine construction failures (CDI writer setup, dra.New's own | ||
| // dependency validation) are returned as errors, since those indicate a | ||
| // real misconfiguration rather than a timing issue. | ||
| func (p *policy) buildDRAPlugin(opts *policyapi.BackendOptions) error { | ||
| if opts.KubeClientFn == nil { | ||
| log.Warnf("dra: no KubeClientFn provided, DRA plugin not started") | ||
| return nil | ||
| } | ||
| kubeClient := opts.KubeClientFn() | ||
| if kubeClient == nil { | ||
| log.Warnf("dra: no kube client available yet, DRA plugin not started") | ||
| return nil | ||
| } | ||
| if opts.NodeName == "" { | ||
| log.Warnf("dra: node name not known yet, DRA plugin not started") | ||
| return nil | ||
| } | ||
|
|
||
| adapter := &policyDRAAdapter{p: p} | ||
|
|
||
| cdiWriter, err := dra.NewCDIWriter(DRADriverName, p.cdiDir) | ||
| if err != nil { | ||
| return policyError("failed to create DRA CDI writer: %w", err) | ||
| } | ||
|
|
||
| deps := dra.Deps{ | ||
| KubeClient: kubeClient, | ||
| NodeName: opts.NodeName, | ||
| // ValidateClasses captures p, not a config snapshot, so it observes | ||
| // whatever p.cfg is live when called, including after Reconfigure() | ||
| // swaps it. DRASharedCounters() is the nil-safe getter — without it, | ||
| // a Reconfigure that removes the dra: section would panic on | ||
| // p.cfg.DRA.SharedCounters here. | ||
| ValidateClasses: func() error { | ||
| return cpuclass.ValidateCPUClassesForDRA(p.cfg.CPUClasses, p.cfg.DRASharedCounters()) | ||
| }, | ||
| // ValidateCPUsInPool mirrors the check allocateClaim performs via | ||
| // poolForCPUs at container-creation time, so a claim whose picked | ||
| // CPUs straddle more than one leaf pool (e.g. a punit spanning an | ||
| // entire package while leaf pools are NUMA or L3 nodes) is rejected | ||
| // at Prepare time instead of being persisted and always failing | ||
| // later when its container is created. | ||
| ValidateCPUsInPool: func(cpus cpuset.CPUSet) error { | ||
| _, err := p.poolForCPUs(cpus) | ||
| return err | ||
| }, | ||
| DeviceLister: adapter, | ||
| ClaimAllocator: adapter, | ||
| CDIWriter: cdiWriter, | ||
| ClaimStore: dra.NewCacheClaimStore(p.cache), | ||
| ClaimUnprepare: p.unprepareDRAClaim, | ||
| WithLock: opts.WithLock, | ||
| Logger: log, | ||
| } | ||
|
|
||
| plugin, err := dra.New(DRADriverName, deps) | ||
| if err != nil { | ||
| return policyError("failed to create DRA plugin: %w", err) | ||
| } | ||
|
|
||
| p.draPlugin = plugin | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // triggerDRARepublish enqueues a DRA ResourceSlice republication when | ||
| // PCT-based HP capacity is active. Called (deferred) from AllocateResources, | ||
| // ReleaseResources, and UpdateResources so that any change to non-DRA HP CPU | ||
| // usage (hpUsed) caused by those NRI-path allocations is reflected in the | ||
| // next ResourceSlice. | ||
| // The enqueue is non-blocking and safe to call while holding the resmgr lock; | ||
| // the actual publish runs in the DRA plugin's republisherLoop goroutine, which | ||
| // acquires the lock itself after the NRI handler releases it. | ||
| func (p *policy) triggerDRARepublish() { | ||
| if p.draPlugin == nil { | ||
| return | ||
| } | ||
| if p.cpuClasses == nil || !p.cpuClasses.PctActive() { | ||
| return | ||
| } | ||
| p.draPlugin.TriggerRepublish() | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| // Copyright 2019 Intel Corporation. All Rights Reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package topologyaware | ||
|
|
||
| import ( | ||
| resourceapi "k8s.io/api/resource/v1" | ||
|
|
||
| "github.com/containers/nri-plugins/pkg/resmgr/dra" | ||
| "github.com/containers/nri-plugins/pkg/utils/cpuset" | ||
| ) | ||
|
|
||
| // DRADriverName is the DRA driver name used to publish and identify CPU | ||
| // devices for the topology-aware policy. Defined once here; referenced | ||
| // everywhere else DRA devices for this policy are published or identified. | ||
| const DRADriverName = "nri.topology-aware.cpu" | ||
|
|
||
| // policyDRAAdapter implements dra.ClaimAllocator and dra.DeviceLister by | ||
| // forwarding every call to the *current* p.cpuClasses at call time. | ||
| // | ||
| // A field holding *cpuclass.Handler directly (captured once, at | ||
| // construction) would go stale: initialize() sets p.cpuClasses = nil and | ||
| // installs a brand new *cpuclass.Handler on every Reconfigure. Because this | ||
| // adapter only ever holds the *policy back-pointer and dereferences | ||
| // p.cpuClasses fresh on each call, it always observes the handler that is | ||
| // live at call time. | ||
| // | ||
| // All *cpuclass.Handler methods used here are nil-receiver-safe (they check | ||
| // h == nil internally and return zero-values/errors), so no additional nil | ||
| // guard is required in the adapter for a nil p.cpuClasses. | ||
| type policyDRAAdapter struct { | ||
| p *policy | ||
| } | ||
|
|
||
| // Make sure policyDRAAdapter implements the interfaces the DRA plugin needs. | ||
| var ( | ||
| _ dra.ClaimAllocator = &policyDRAAdapter{} | ||
| _ dra.DeviceLister = &policyDRAAdapter{} | ||
| ) | ||
|
|
||
| // PickHpCpus routes to the current p.cpuClasses handler's PickHpCpus. | ||
| func (a *policyDRAAdapter) PickHpCpus(pkgID, punitID, n int, held cpuset.CPUSet) (cpuset.CPUSet, error) { | ||
| return a.p.cpuClasses.PickHpCpus(pkgID, punitID, n, held) | ||
| } | ||
|
|
||
| // ReleaseHpCpus routes to the current p.cpuClasses handler's ReleaseHpCpus. | ||
| func (a *policyDRAAdapter) ReleaseHpCpus(pkgID, punitID int, cpus cpuset.CPUSet) { | ||
| a.p.cpuClasses.ReleaseHpCpus(pkgID, punitID, cpus) | ||
| } | ||
|
|
||
| // AccountHpCpus routes to the current p.cpuClasses handler's AccountHpCpus. | ||
| func (a *policyDRAAdapter) AccountHpCpus(pkgID, punitID int, cpus cpuset.CPUSet) error { | ||
| return a.p.cpuClasses.AccountHpCpus(pkgID, punitID, cpus) | ||
| } | ||
|
|
||
| // IsHPClass routes to the current p.cpuClasses handler's IsHPClass. | ||
| func (a *policyDRAAdapter) IsHPClass(className string) bool { | ||
| return a.p.cpuClasses.IsHPClass(className) | ||
| } | ||
|
|
||
| // DRADevices routes to the current p.cpuClasses handler's DRADevices. | ||
| func (a *policyDRAAdapter) DRADevices(driverName string) ([]resourceapi.Device, error) { | ||
| return a.p.cpuClasses.DRADevices(driverName) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit like we'd have put essentially the whole full blown DRA driver, running almost fully contained within the policy implementation, as an completely independent second driver, the other allocation path (via the stock resmgr NRI layer) being otherwise oblivious to it than via the shared (resmgr-level) lock. That's pretty implicit awareness observable only via more variance in the NRI-triggered code paths timings (usually not visible probably), and potentially via deadlocking which is now more easy to achieve with locking responsibility to protect against the NRI code paths having been fully delegated to the policy-contained DRA driver and it's related logic/additions.
Is there a viable alternative to this, where we'd have a delegation of responsibilities with closer resemblance to how we do it on the NRI-triggered allocation/release paths ? There we basically have the (resmgr) infrastructure/machinery take care of most boilerplate, including most (or maybe all, I don't recall now by heart) of the locking/serialization. The generic resmgr/policy interface has then functions for allocation/reallocation/release and those are used to hook in the policy-specific allocation/release/CPU,memory,etc selection logic into the generic/boilerplate (NRI) driver at the resmgr level.
So going more in that direction would mean the resmgr/policy interface gaining extra functions specific to/for the DRA-based allocation/release code paths, which then would be used to hook the policy-specific DRA decision/allocation logic into the generic/boilerplate DRA machinery, which for instance could live at the resmgr level.
I admit that such a split is easier for the traditional allocation, resulting in a rather small and simple API footprint, which is one of the key reasons why such an abstraction suggested itself as an obvious choice. This might not be the case here, or at least not such a clear cut, due to (a) DRA (device) being more versatile in this regard via device names+attributes, etc. But I think we should give it a serious and honest consideration, including thinking about the next steps of 1) exposing something more via DRA from the T-A policy, maybe for instance isolated CPU cores, P/E-cores, etc., and 2) adding DRA support to the balloons policy. Then think about what are the consequences and see what kind of compromises we'd need to make with one and the other approach, so we could take an educated guess which one looks to be less painful in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps one way to drive claude towards more policy-independent DRA logic in resmgr could be asking a structure where, in addition to the topology-aware policy, the template policy would have expose all allowed CPUs in a DRA resource slice, too.
The purpose of the template policy is to provide minimal basis for anyone to implement their own policy. This would extended it to enable anyone building their own policy with DRA support, and as a side effect, would hopefully force claude to design how DRA becomes visible to multiple policies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea! I'll try to add it to the implementation. So far the plan is to cancel this PR and continue sumbitting series of smaller and hopefully easier to review PRs. The series has been already started by #782. Will submit a second one soon.