refactor: share node-eligibility, pod-status and drive-ordering helpers (OP-345) - #2745
Open
kristina-solovyova wants to merge 1 commit into
Open
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Collaborator
Author
This was referenced Aug 17, 2026
kristina-solovyova
force-pushed
the
08-17-refactor_share_node_eligibility_and_pod_status_helpers_op-345_
branch
3 times, most recently
from
August 19, 2026 13:50
336b1f9 to
fa21afa
Compare
kristina-solovyova
marked this pull request as ready for review
August 19, 2026 15:29
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (08/19/26)2 reviewers were added to this PR based on Anton Bykov's automation. |
There was a problem hiding this comment.
Pull request overview
This PR lays groundwork for upcoming “auto full drives” work by consolidating several scheduling/eligibility and drive-ordering helpers into shared, tested utilities used across controllers and standalone tooling (e.g., weka-capacity CLI).
Changes:
- Centralizes node eligibility logic via
resources.NodeIneligibleReason(cordoned / not-ready / untolerated taint) and reuses it in target-node selection. - Adds shared helpers with tests:
utils.PodUnschedulable,domain.SortDriveEntriesDesc, and cluster-based Weka pod tolerations generation. - Fixes capacity-env loading for standalone callers by ensuring
LoadCapacityEnvre-derives form-cluster minimums and readsFULL_PCPUS_ONLY.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/gen-api-docs.go | Formatting-only adjustments in schema/doc generation utility. |
| internal/services/discovery/discovery.go | Comment-only refactors for clarity; no behavior changes. |
| internal/pkg/domain/resources.go | Extends DriveInfo with optional Type metadata for wire use. |
| internal/pkg/domain/drives.go | Adds SortDriveEntriesDesc helper for deterministic capacity-desc drive ordering. |
| internal/pkg/domain/drives_test.go | Adds unit tests for SortDriveEntriesDesc including non-mutation guarantee. |
| internal/controllers/utils/pod_status.go | Introduces PodUnschedulableCondition/PodUnschedulable helpers for scheduler-confirmed failures. |
| internal/controllers/utils/pod_status_test.go | Adds tests validating PodUnschedulable semantics. |
| internal/controllers/resources/pod.go | Factors shared base tolerations and adds GetWekaPodTolerationsForCluster. |
| internal/controllers/resources/pod_cpu_alignment_test.go | Minor comment/format cleanup in existing CPU-alignment tests. |
| internal/controllers/resources/node.go | Adds NodeIneligibleReason and clarifies NodeIsReady semantics (missing Ready condition treated as not-ready). |
| internal/controllers/resources/node_test.go | Adds unit tests for NodeIsReady and NodeIneligibleReason classifications. |
| internal/controllers/operations/prepull_utils.go | Switches node filtering to the shared resources.NodeIneligibleReason predicate. |
| internal/controllers/operations/prepull_utils_test.go | Adds test coverage for GetTargetNodes filtering behavior via shared eligibility logic. |
| internal/controllers/operations/enable_local_drivers_distribution.go | Replaces local readiness check with shared resources.NodeIsReady; removes duplicated helper. |
| internal/consts/consts.go | Comment clarifications around drive annotations/resources and TLC/QLC accounting. |
| internal/config/env.go | Moves/reuses form-cluster minimum derivation and ensures LoadCapacityEnv reads FULL_PCPUS_ONLY. |
| internal/config/env_test.go | Adds regression test covering CLI-style env overlay + LoadCapacityEnv behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kristina-solovyova
force-pushed
the
08-17-refactor_share_node_eligibility_and_pod_status_helpers_op-345_
branch
from
August 28, 2026 05:32
fa21afa to
a52d8cf
Compare
Comment on lines
+29
to
+31
| // SortDriveEntriesDesc returns a capacity-descending copy of entries (never mutates the input), serial | ||
| // ascending as a deterministic tiebreak so equal-capacity drives always land in the same order. This is | ||
| // what makes a numDrives pin take a node's LARGEST drives, matching capacityplanner.SortDriveCapacitiesDesc. |
Comment on lines
+26
to
+29
| // NodeIneligibleReason reports why a node cannot host a new weka pod right now — cordoned, not ready, or | ||
| // carrying a taint outside tolerations — or "" when the node is a valid placement candidate. This is the | ||
| // single predicate for "can this node receive a new pod": every caller across the operator and CLI that | ||
| // needs this check goes through it, so the classifications can never quietly diverge between call sites. |
Comment on lines
+54
to
+57
| // TestNodeIneligibleReason covers every classification NodeIneligibleReason makes, table-driven since each | ||
| // row is the same shape: build a node, call the function, compare the reason string. This is the single | ||
| // predicate shared by capacityplanner/inventory (NodeInventory/FullDrivesInventory/ExploreNodes) and | ||
| // controllers/operations (GetTargetNodes). |
This was referenced Sep 1, 2026
Open
…rs (OP-345) Groundwork split out of the auto-full-drives change so the feature commit carries only feature code. - resources.NodeIneligibleReason is now the single "can this node host a new weka pod" predicate (cordoned / not ready / untolerated taint). It replaces operations.NodeNotReady and the open-coded checks in GetTargetNodes, which had already drifted apart. - utils.PodUnschedulable reports a confirmed scheduling failure from a pod's conditions. - resources.GetWekaPodTolerationsForCluster builds weka pod tolerations for a caller that has a cluster but no live container object. - domain.SortDriveEntriesDesc orders drive entries capacity-descending with a serial tiebreak, so a per-node drive pin deterministically takes the largest drives. - LoadCapacityEnv re-derives the ALLOW_SINGLE_PARITY-lowered form-cluster minimums and reads FULL_PCPUS_ONLY. Standalone callers such as the weka-capacity CLI overlay the operator env via os.Setenv and call only LoadCapacityEnv, long after this package's init() already ran, so both settings were silently ignored there. Also bumps pkg/weka-k8s-api, which the rest of the stack needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kristina-solovyova
force-pushed
the
08-17-refactor_share_node_eligibility_and_pod_status_helpers_op-345_
branch
from
September 1, 2026 18:43
a52d8cf to
fc6be78
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
internal/pkg/domain/drives.go:31
- The SortDriveEntriesDesc doc comment references capacityplanner.SortDriveCapacitiesDesc, but that symbol does not exist anywhere in this repository. This makes the comment misleading and harder to maintain.
// SortDriveEntriesDesc returns a capacity-descending copy of entries (never mutates the input), serial
// ascending as a deterministic tiebreak so equal-capacity drives always land in the same order. This is
// what makes a numDrives pin take a node's LARGEST drives, matching capacityplanner.SortDriveCapacitiesDesc.
Comment on lines
+1053
to
+1063
| // GetWekaPodTolerationsForCluster returns the tolerations a drive or compute container's pod would get for | ||
| // cluster, without a live container object. The capacity planner needs this at inventory-collection time, | ||
| // before any container exists, to decide which nodes it can actually schedule onto. It is faithful for | ||
| // drive and compute containers specifically: GetWekaPodTolerations' only container-dependent behavior | ||
| // besides a container's own custom tolerations is the client/aux-mode NoSchedule expansion, and drive/ | ||
| // compute containers never have that mode, so their tolerations reduce to exactly this — the base set plus | ||
| // the cluster's tolerations/rawTolerations, the same inputs NewWekaContainerForWekaCluster | ||
| // (internal/controllers/factory/container_factory.go) copies onto a new container's Spec.Tolerations. | ||
| func GetWekaPodTolerationsForCluster(cluster *weka.WekaCluster) []corev1.Toleration { | ||
| return k8sapiutil.ExpandTolerations(wekaBaseTolerations(), cluster.Spec.Tolerations, cluster.Spec.RawTolerations) | ||
| } |
Comment on lines
+12
to
+13
| func PodUnschedulableCondition(pod *v1.Pod) *v1.PodCondition { | ||
| for i := range pod.Status.Conditions { |
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.
Groundwork split out of the auto-full-drives change so the feature commit carries
only feature code.
pod" predicate (cordoned / not ready / untolerated taint). It replaces
operations.NodeNotReady and the open-coded checks in GetTargetNodes, which had
already drifted apart.
conditions.
caller that has a cluster but no live container object.
serial tiebreak, so a per-node drive pin deterministically takes the largest
drives.
minimums and reads FULL_PCPUS_ONLY. Standalone callers such as the
weka-capacity CLI overlay the operator env via os.Setenv and call only
LoadCapacityEnv, long after this package's init() already ran, so both
settings were silently ignored there.
Also bumps pkg/weka-k8s-api, which the rest of the stack needs.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com