Skip to content

fix: filter excess shard pods in scheduling issue check - #273

Merged
jdheyburn merged 1 commit into
valkey-io:mainfrom
daanvinken:fix/inner-scheduling-check-ignores-excess-shards
Jun 23, 2026
Merged

fix: filter excess shard pods in scheduling issue check#273
jdheyburn merged 1 commit into
valkey-io:mainfrom
daanvinken:fix/inner-scheduling-check-ignores-excess-shards

Conversation

@daanvinken

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #258. The inner handlePodSchedulingIssues call in the reconcileValkeyNodes requeue branch (line 167) still lists all cluster pods, including those from excess shards pending removal. During a concurrent rolling update and scale-in, this can block progress.

Implementation

Filter findPodSchedulingIssue to skip pods whose shard-index label is >= the desired shard count. Only pods belonging to desired shards are checked for scheduling issues. Excess shard pods are ignored since they will be cleaned up by handleScaleIn.

Limitations

None. The filter is safe because excess shard pods are always cleaned up by handleScaleIn regardless of their scheduling state.

Testing

Compiles clean. Existing test failures on main are pre-existing (unrelated reconcileValkeyNode signature change).

Follow-up to valkey-io#258. The inner handlePodSchedulingIssues call in the
reconcileValkeyNodes requeue branch still lists all cluster pods,
including those from excess shards pending removal. During a concurrent
rolling update and scale-in, this can block progress.

Filter findPodSchedulingIssue to skip pods with a shard-index label
>= the desired shard count, so only pods belonging to desired shards
are checked for scheduling issues.

Signed-off-by: Daan Vinken <daanvinken@tythus.com>
@daanvinken
daanvinken force-pushed the fix/inner-scheduling-check-ignores-excess-shards branch from 7a2e950 to 2cf60fd Compare June 23, 2026 16:18
@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a shard-index guard inside findPodSchedulingIssue so that pods belonging to excess shards (those with a shard-index label value ≥ cluster.Spec.Shards) are skipped when checking for unschedulable pods during a concurrent rolling-update + scale-in.

  • The filter is applied consistently: pods with no shard-index label or a non-integer label value still fall through and are checked as before, keeping the pre-existing behaviour safe.
  • handlePodSchedulingIssues is called in two places (line 167 and line 278); the filter is now active at both call sites, which is correct — by line 278, handleScaleIn has already claimed ownership of excess-shard pods, so skipping their scheduling state is appropriate at that point too.

Confidence Score: 5/5

Safe to merge — the change is a narrow, well-scoped filter with no effect on desired-shard pods and correct fallthrough for pods with missing or unparseable shard-index labels.

The diff touches a single helper function. The filter correctly uses >= rather than >, handles the no-label and bad-label cases by falling through to the existing check, and is consistent with how the second handlePodSchedulingIssues call site (line 278) already operates after handleScaleIn has claimed excess pods. No unintended side-effects were identified.

No files require special attention; the only changed function is findPodSchedulingIssue.

Important Files Changed

Filename Overview
internal/controller/valkeycluster_controller.go Adds a shard-index filter in findPodSchedulingIssue to skip excess-shard pods; logic is correct, edge cases (missing label, non-integer label) are handled by falling through to the existing check.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[reconcileValkeyNodes requeue=true] --> B[handlePodSchedulingIssues]
    B --> C[findPodSchedulingIssue]
    C --> D[List all cluster pods]
    D --> E{For each pod}
    E --> F{Has shard-index label?}
    F -- No --> H[Check podSchedulingIssueForPod]
    F -- Yes --> G{Parse label as int}
    G -- Parse error --> H
    G -- success --> I{si >= desiredShards?}
    I -- Yes: excess shard pod --> J[Skip — handleScaleIn owns this pod]
    I -- No: desired shard pod --> H
    H --> K{Issue found?}
    K -- Yes --> L[Return scheduling issue\nSet Degraded + Unschedulable conditions\nRequeueAfter 10s]
    K -- No --> E
    E -- All pods checked, no issue --> M[Return nil — no issue]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[reconcileValkeyNodes requeue=true] --> B[handlePodSchedulingIssues]
    B --> C[findPodSchedulingIssue]
    C --> D[List all cluster pods]
    D --> E{For each pod}
    E --> F{Has shard-index label?}
    F -- No --> H[Check podSchedulingIssueForPod]
    F -- Yes --> G{Parse label as int}
    G -- Parse error --> H
    G -- success --> I{si >= desiredShards?}
    I -- Yes: excess shard pod --> J[Skip — handleScaleIn owns this pod]
    I -- No: desired shard pod --> H
    H --> K{Issue found?}
    K -- Yes --> L[Return scheduling issue\nSet Degraded + Unschedulable conditions\nRequeueAfter 10s]
    K -- No --> E
    E -- All pods checked, no issue --> M[Return nil — no issue]
Loading

Reviews (1): Last reviewed commit: "fix: filter excess shard pods in schedul..." | Re-trigger Greptile

@jdheyburn jdheyburn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, thank you!

@jdheyburn
jdheyburn merged commit e490468 into valkey-io:main Jun 23, 2026
8 checks passed
sandeepkunusoth pushed a commit to sandeepkunusoth/valkey-k8s-operator that referenced this pull request Jul 5, 2026
## Summary

Follow-up to valkey-io#258. The inner `handlePodSchedulingIssues` call in the
`reconcileValkeyNodes` requeue branch (line 167) still lists all cluster
pods, including those from excess shards pending removal. During a
concurrent rolling update and scale-in, this can block progress.

## Implementation

Filter `findPodSchedulingIssue` to skip pods whose `shard-index` label
is >= the desired shard count. Only pods belonging to desired shards are
checked for scheduling issues. Excess shard pods are ignored since they
will be cleaned up by `handleScaleIn`.

## Limitations

None. The filter is safe because excess shard pods are always cleaned up
by `handleScaleIn` regardless of their scheduling state.

## Testing

Compiles clean. Existing test failures on main are pre-existing
(unrelated `reconcileValkeyNode` signature change).

Signed-off-by: Daan Vinken <daanvinken@tythus.com>
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.

2 participants