feat(scheduler): pool pattern aware scheduler helpers - #739
Open
krishnaGajabi wants to merge 3 commits into
Open
feat(scheduler): pool pattern aware scheduler helpers#739krishnaGajabi wants to merge 3 commits into
krishnaGajabi wants to merge 3 commits into
Conversation
Rework the scheduler helpers so that a pool is selected by a regular
expression, the first step towards the poolpattern StorageClass parameter
(OEP-4268). No behaviour is wired to the new parameter yet.
The poolname and poolpattern parameters fold into a single *regexp.Regexp so
the helpers have one matching path: poolpattern compiles as given (unanchored
RE2, like lvm-localpv's vgpattern), while poolname compiles as an anchored,
quoted exact match. Quoting is required for correctness, since legal ZFS pool
names carry regex metacharacters and an unescaped "tank.prod" would also match
"tankXprod". Pool names are matched against the pool root, as the poolname
parameter may be a pool/dataset path while the ZFSNode CR only advertises
pool level names.
CapacityWeighted now reads the ZFSNode CRs and weights a node by the real on
disk usage of its matching pools, instead of summing the capacity of the
volumes this driver provisioned, so the ordering also accounts for data that
did not come through the driver. Every node with a matching pool gets an entry
even when the pools are empty, since lib-csi's scheduler moves the nodes
missing from the map to the front of the list rather than dropping them.
VolumeWeighted keeps counting ZFSVolumes, as the ZFSNode CR has no volume
count, and matches on the pool root like everything else.
Three helpers are added for the controller to use next:
- reservesSpace tells whether a volume gets a ZFS reservation and so has to
fit in a pool's free space: a zvol does unless it is sparse, a dataset only
when thinprovision is "no", since its quota is a limit and not a
reservation.
- getSuitableNodes returns the nodes having a matching pool with more than
the requested bytes free, a single pool having to hold the whole
reservation, along with whether any pool matched the pattern at all, so the
controller can tell an exhausted pool from a StorageClass naming a pool
that exists nowhere.
- resolvePool returns the matching pool with the most free space on a node,
turning a pattern into the concrete pool stored in ZFSVolume.Spec.PoolName.
The list reading helpers are shells over pure functions taking the CR slice,
which is what the new unit tests exercise.
CreateZFSVolume is updated only as far as compiling the exact name pattern for
the changed getNodeMap signature; reading the poolpattern parameter, filtering
the scheduled nodes and resolving the pool per node follow in the next change.
Signed-off-by: krishnaGajabi <gajbikrishna23@gmail.com>
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
The weight map handed to lib-csi's scheduler was keyed by the node id, taken from ZFSVolume.Spec.OwnerNodeID, while the scheduler looks the nodes up by their kubernetes node name. The two are the same only as long as no custom node id is configured; with one, no key ever matches, and since lib-csi treats a node missing from the map as least loaded and moves it to the front of the list instead of dropping it, the weighting silently stops having any effect and the nodes end up in listing order. Key the maps by the node name instead. A ZFSNode CR is named after the node id, but the node agent also gives it an owner reference to the node object and keeps that up to date, so the name is available without asking the API server for it; k8sNodeName falls back to the CR name, which is the node name as long as no custom node id is set. The volume weighted map needs the same translation the other way round, as a volume records only the node id of its node, so it now reads the ZFSNode CRs as well. This matters more than the lost ordering for what comes next: the suitable node set is going to be intersected with the scheduler's output, and a set keyed by node id would intersect to nothing, failing every space reserving volume on a cluster that has plenty of room. resolvePool keeps taking the node id, since it looks the CR up by name and the provisioning loop resolves the id anyway for ZFSVolume.Spec.OwnerNodeID. The regular CI profile cannot catch any of this, as the node name and the node id are equal there; only the custom-node-id profile relabels the nodes. Signed-off-by: krishnaGajabi <gajbikrishna23@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #739 +/- ##
========================================
Coverage 96.64% 96.64%
========================================
Files 1 1
Lines 656 656
========================================
Hits 634 634
Misses 17 17
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
|
@copilot Review this PR. Things to look for.
|
There was a problem hiding this comment.
Pull request overview
Adds pool-pattern-aware scheduler helpers and corrects custom node-ID weighting.
Changes:
- Compiles exact pool names and regex patterns consistently.
- Adds pool capacity, suitability, reservation, and resolution helpers.
- Adds comprehensive unit tests and updates controller integration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pkg/driver/schd_helper.go |
Implements pattern-aware scheduling helpers. |
pkg/driver/schd_helper_test.go |
Tests matching, weighting, suitability, and node IDs. |
pkg/driver/controller.go |
Compiles the fixed pool name before scheduling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tiagolobocastro
requested changes
Jul 29, 2026
tiagolobocastro
left a comment
tiagolobocastro
left a comment
Member
There was a problem hiding this comment.
The OEP is not approved yet @krishnaGajabi
You need to propose changing it to implementable and request maintainer review
Member
|
Framing: only getNodeMap + compilePoolPattern are actually wired in (controller.go:279,284); reservesSpace/getSuitableNodes/resolvePool/poolForNode are dormant helpers for the next PR, so the live behavior surface is small.
Minor, non-blocking Only concrete ask: also surface changes (b) pool-root VolumeWeighted cross-dataset counting and (d) empty-poolname early-error in the release notes. |
Contributor
Author
I have raised the PR to mark the OEP as implementable. PTAL |
Contributor
Author
The PR is merged |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pkg/driver/schd_helper.go:219
runSchedulerprepends nodes missing fromnmap, but this map adds explicit zero entries only for nodes with a matching empty pool. Because the controller in this PR does not yet filter or skip non-matching nodes, every topology-eligible node without the requested pool is now ordered ahead of a valid empty-pool node and provisioning is attempted there first. Please either defer this active weighting change or wire the pool-membership filtering/skip into the same PR.
for _, node := range nodelist {
for _, pool := range node.Pools {
if pattern.MatchString(pool.Name) {
nmap[k8sNodeName(node)] += pool.Used.Value()
VolumeWeighted and CapacityWeighted both order the nodes by what has already been put into a pool, by volume count and by used capacity. That says nothing about what is left: a node with a small untouched pool outranks a node with a large, moderately used one, even though the latter has far more room for the volume. Add SpaceWeighted, which orders the nodes by the free capacity of their roomiest pool matching the pattern, the same pool the volume would actually land in. lib-csi's scheduler prefers the least weighted node, so the free capacity is inverted into a weight (math.MaxInt64 - free): the more space a node has left, the less loaded it looks. It is opt-in via the scheduler storageclass parameter, CapacityWeighted stays the default. Unlike lvm-localpv's SpaceWeighted, a node whose matching pool is full keeps its entry in the map instead of being dropped. An omitted node is not excluded by lib-csi, it is treated as least loaded and moved to the front of the list, which would prefer exactly the nodes with no room. Fitting the volume stays the job of the suitable node intersection. The "roomiest matching pool of a node" lookup is now shared by the three places that need it, as maxFreePool: the pool the volume is created in, the capacity it has to fit into, and the space the node is weighted by. Signed-off-by: krishnaGajabi <gajbikrishna23@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Reworks the scheduler helpers in
pkg/driver/schd_helper.goso that a pool isselected by a regular expression rather than an exact name, and adds a third
scheduling algorithm,
SpaceWeighted. This is the first step towards thepoolpatternStorageClass parameter (OEP-4268); the parameter itself is not readyet, see Left unwired.
What this PR does
One matching path for
poolnameandpoolpattern. Both fold into a single*regexp.RegexpviacompilePoolPattern, so every helper matches the same way.poolpatterncompiles as given (unanchored RE2, the same semantics aslvm-localpv's
vgpattern);poolnamecompiles as an anchored,QuoteMeta-quotedexact match. The quoting is required for correctness: legal ZFS pool names contain
regex metacharacters, so an unescaped
tank.prodwould also matchtankXprodandselect the wrong pool. Names are matched against the pool root, since
poolnamemay be apool/datasetpath while theZFSNodeCR advertisespool-level names only.
CapacityWeightedweighs by real pool usage. It now reads theZFSNodeCRsand sums the on-disk
Usedof a node's matching pools, instead of summing thecapacity of the volumes this driver provisioned — so the ordering also accounts
for data that did not come through the driver. Every node with a matching pool
gets an entry even when its pools are empty, because lib-csi moves a node missing
from the weight map to the front of the list rather than dropping it.
VolumeWeightedstill countsZFSVolumes (theZFSNodeCR carries no volumecount) and matches on the pool root like everything else.
New
SpaceWeightedalgorithm, opt-in.VolumeWeightedandCapacityWeightedboth order by what has already been put into a pool, whichsays nothing about what is left: a node with a small untouched pool outranks a
node with a large, moderately used one that has far more room.
SpaceWeightedorders by the free capacity of a node's roomiest matching pool — the same pool the
volume would land in. lib-csi prefers the least-weighted node, so free capacity is
inverted into a weight (
math.MaxInt64 - free). Select it withscheduler: "SpaceWeighted";CapacityWeightedremains the default.Unlike lvm-localpv's
SpaceWeighted, a node whose matching pool is full keeps itsmap entry (weight
math.MaxInt64, sorting last) instead of being dropped —omission promotes a node to the front under lib-csi, which would prefer exactly
the nodes with no room. Deciding whether a volume fits stays the job of the
suitable-node intersection.
Weight maps are keyed by node name, not node id. The map handed to lib-csi was
keyed from
ZFSVolume.Spec.OwnerNodeID, while the scheduler looks nodes up bytheir Kubernetes node name. Those coincide only when no custom node id is
configured; with one, no key ever matched, so the weighting silently stopped
having any effect and nodes came back in listing order.
k8sNodeNamereads theowner reference the node agent maintains on the
ZFSNodeCR, falling back to theCR name. This matters more for what comes next: the suitable-node set is going to
be intersected with the scheduler's output, and a set keyed by node id would
intersect to nothing and fail every space-reserving volume on a cluster with
plenty of room.
Three helpers for the controller to consume next.
reservesSpace— whether a volume gets a ZFS reservation and so has to fit in apool's free space: a zvol does unless it is sparse; a dataset only when
thinprovision: "no", since its quota is a limit rather than a reservation.getSuitableNodes— the nodes having a matching pool with more than therequested bytes free (a single pool must hold the whole reservation), plus
whether any pool matched the pattern at all, so the controller can tell an
exhausted pool from a StorageClass naming a pool that exists nowhere.
resolvePool— the matching pool with the most free space on a given node,turning a pattern into the concrete pool stored in
ZFSVolume.Spec.PoolName.All three, and
SpaceWeighted, share onemaxFreePoolhelper so the three uses of"the node's roomiest matching pool" cannot drift apart.
Left unwired
Deliberately out of scope here, so this PR reviews as a self-contained helper
change:
poolpatternStorageClass parameter is not read;CreateZFSVolumeistouched only as far as compiling the exact-name pattern for the changed
getNodeMapsignaturegetSuitableNodes,resolvePoolandreservesSpacehave no callers yetGetCapacity, the clone/snapshot-restore pool guard, andvalidateVolumeCreateReqare untouchedNext in the series
The controller task wires the fresh-create path: read
poolpattern, computereservesSpaceonce, intersect the scheduler's ordered node list withgetSuitableNodesfor reserving volumes, and replace the genericcodes.Internal, "scheduler failed, node list is empty"with a capacity-awareverdict —
ResourceExhaustedwhen a matching pool exists but nothing fits(transient; external-provisioner retries with backoff and, with capacity
tracking, reschedules),
FailedPreconditionwhen the pattern matches no poolanywhere (a misconfigured StorageClass that retrying will never fix). It then
calls
resolvePoolper candidate node in the provisioning loop soSpec.PoolNamealways names a pool present on the assigned node.Then, as separate tasks:
GetCapacityunderpoolpattern; the clone/snapshotguard, which today rejects every clone under a pattern StorageClass because
poolnameis empty; StorageClass validation; and docs plus samples.Behaviour change — needs a release note
Applying the
Used-basedCapacityWeightedmetric uniformly means existingpoolnameStorageClasses change behaviour on upgrade with no opt-in: nodeordering under the default scheduler shifts from driver-provisioned-sum to the
pool's real
Used. OEP-4268 accepts this as an improvement — the scheduler nowreflects real capacity — but records that it must be called out in the release
notes.
SpaceWeightedis additive and opt-in, so it changes nothing for existingStorageClasses.
Testing
pkg/driver/schd_helper_test.gois new and covers: pool-root matching;QuoteMetaanchoring forpoolname(a.in a pool name must not over-match);both-set and neither-set rejection;
Used-based weighting under both existingalgorithms;
reservesSpaceacross the zvol/dataset ×yes/no/unset matrix;maxFreePoolselection including a full matching pool; suitable-node computationincluding the exhausted-pool and no-match-anywhere cases; and for
SpaceWeighted,that the inverted weight sorts most-free-first under an ascending sort, that a
node's largest matching pool decides rather than the sum of its pools, and that a
node with a full matching pool keeps its entry instead of being front-loaded.
Node-name keying is asserted for every map via custom-node-id cases.
make testandgo vet ./pkg/...are clean.Worth running the BDD suite with
make ci profile=custom-node-id(orprofile=all): the regular profile cannot exercise the node-name fix, since nodename and node id are equal there — only the custom-node-id profile relabels the
nodes.