Skip to content

fix(pdb): correct test assertions, pdbName call, and update docs - #304

Open
erkules wants to merge 1 commit into
valkey-io:mainfrom
erkules:feature-pdb-pershard
Open

fix(pdb): correct test assertions, pdbName call, and update docs#304
erkules wants to merge 1 commit into
valkey-io:mainfrom
erkules:feature-pdb-pershard

Conversation

@erkules

@erkules erkules commented Jul 12, 2026

Copy link
Copy Markdown

This PR closes <Issue #303 >

Summary

  • Fix pdbName call in test to include shardIndex argument
  • Change test expectation from MaxUnavailable to MinAvailable
  • Add assertion for LabelShardIndex selector in PDB test
  • Update docs/valkeycluster.md to describe per-shard PDB with minAvailable: 1 instead of maxUnavailable: 1

Features / Behaviour Changes

Instead of creating a single cluster-wide PodDisruptionBudget, create one PDB per shard.
use minAvailable: 1 isnstead of maxUnvailable

Implementation

Limitations

It is an improvement. Next step is to make pdbs configurable

Testing

All tests done

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message explains what changed and why
  • Tests are added or updated.
  • Documentation files are updated.
  • I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

@greptile-apps

greptile-apps Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR changes PodDisruptionBudget handling from one cluster-wide budget to one budget per shard. The main changes are:

  • Create per-shard PDBs named with the shard index.
  • Use minAvailable: 1 and shard-specific selectors.
  • Delete the legacy cluster-wide PDB during reconciliation.
  • Clean up managed stale shard PDBs after scale-in or when disabled.
  • Update tests and docs for the new PDB behavior.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the changed code.

T-Rex T-Rex Logs

What T-Rex did

  • Ran a focused envtest proof to validate the pdb-focused test, observing a BeforeSuite kube-apiserver timeout and zero specs executed (EXIT_CODE: 1).
  • Executed the fallback runtime proof by running the generated direct fake-client test, which completed successfully with EXIT_CODE: 0.
  • Reviewed the static/docs proof to confirm the relevant code and documentation changes and verified EXIT_CODE: 0.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
internal/controller/valkeycluster_pdb.go Moves PDB reconciliation to per-shard resources, adds legacy cleanup, and labels managed PDBs with the shard index.
internal/controller/valkeycluster_pdb_test.go Updates PDB expectations for per-shard names, selectors, and legacy migration.
docs/valkeycluster.md Documents the new per-shard PDB behavior and disabled cleanup semantics.

Reviews (3): Last reviewed commit: "fix(pdb): correct test assertions, pdbNa..." | Re-trigger Greptile

Comment thread internal/controller/valkeycluster_pdb.go
Comment thread internal/controller/valkeycluster_pdb.go Outdated
@erkules
erkules force-pushed the feature-pdb-pershard branch from f28cc22 to 621c163 Compare July 12, 2026 02:34
Comment thread internal/controller/valkeycluster_pdb.go
- Fix pdbName call in test to include shardIndex argument
- Change test expectation from MaxUnavailable to MinAvailable
- Add assertion for LabelShardIndex selector in PDB test
- Update docs/valkeycluster.md to describe per-shard PDB with
  minAvailable: 1 instead of maxUnavailable: 1

Signed-off-by: Erkan Yanar <erkan.yanar@linsenraum.de>
@erkules
erkules force-pushed the feature-pdb-pershard branch from 621c163 to 6125a8b Compare July 12, 2026 04:07
@jdheyburn

Copy link
Copy Markdown
Collaborator

Rather than changing this for everyone, I would rather provide an API on the podDisruptionBudget to allow the user to decide for themselves. We should continue the discussion on the issue you raised:

@jdheyburn

Copy link
Copy Markdown
Collaborator

@erkules Can we close this out? From the linked issue we have decided not to go with this approach.

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