Repository navigation
feat(reconcile): manage per-pool PodDisruptionBudgets - #290
Conversation
GatewayJ
left a comment
There was a problem hiding this comment.
Per-pool PodDisruptionBudget management fits the existing reconcile flow. Please address the following correctness and resource-ownership cases before merging.
-
[P1] Tighten the budget when drive failures consume the remaining fault tolerance. In
src/reconcile/pod_disruption_budget.rs:104-113, an unhealthy drive causes the existing budget to be retained. With 8 Pods, 2 drives per Pod, one erasure set, and EC:4, the budget isminAvailable: 6. After one drive fails, all Pods can remain Ready while 15 drives are online. The unchanged budget still permits eviction of two healthy Pods, leaving 11 drives against a write quorum of 12. RustFS 1.0.0 readiness permits drive failures while quorum remains available, and Kubernetes counts Pod readiness rather than healthy drives. Please account for existing drive failures when calculating the remaining disruption allowance, and tighten the affected budget when a safe allowance cannot be established. The currentunhealthy_drive_keeps_the_pool_budget_and_retriestest preserves this unsafe behavior. References: RustFS readiness, Kubernetes PDB healthiness. -
[P1] Account for supported storage classes and existing object layouts. At
src/reconcile/pod_disruption_budget.rs:259-264, the allowance uses only STANDARD parity. For 8 Pods with one drive each and STANDARD EC:4,minAvailable: 5allows two Pods to be unavailable. An existing REDUCED_REDUNDANCY object written with the default EC:1 requires 7 data shards, so its GET fails with only 6 drives available even though the PDB is satisfied. Please define the storage classes covered by the availability guarantee and calculate the required quorum accordingly, including existing objects with lower parity. References: default RRS parity, persisted object quorum. -
[P2] Support heterogeneous per-pool parity in the actual admin response. At
src/reconcile/pod_disruption_budget.rs:157-161, a missing scalarstandardSCParityrejects the entire layout. RustFS 1.0.0 returns no scalar when pool parity values differ. For example, automatically configured sets of 4 and 8 drives use parity 2 and 4. This valid configuration repeatedly requeues every 60 seconds without creating budgets for any pool. Please obtain each pool's effective parity and add a test using the real heterogeneous admin response shape. Reference: RustFS homogeneous parity calculation. -
[P2] Verify ownership before applying a same-name PDB. At
src/reconcile/pod_disruption_budget.rs:118-120,ctx.apply()can add a Tenant owner reference to an existing user-managed PDB when its name,minAvailable, and selector already match. SSA permits shared ownership of equal field values, so omittingforcedoes not prevent this. Subsequently settingdisablePodDisruptionBudget: true, or deleting the Tenant, can delete that user-managed PDB. This violates the documented promise to leave externally created budgets untouched. Please verify ownership before updating an existing object, handle creation races with the same ownership check, and test a same-name, matching-spec PDB without a Tenant owner. Reference: Kubernetes SSA field ownership.
Upgrade compatibility needs explicit attention. The optional CRD field preserves existing manifest parsing, while default enablement changes drain behavior for existing Tenants. A user-managed PDB with a different name and an overlapping selector causes Kubernetes to reject evictions. The user guide already mentions migration; please make the upgrade procedure explicit: update the CRD and RBAC, provide a way to retain user-managed budgets through the opt-out before automatic creation, and preserve external resource ownership. Please record this operational compatibility impact in the PR/release notes. Reference: overlapping PDB selectors.
Reviewed base 30f695a018f27dffefdedb52263a1876d0ee0c61 and head e44d1242d68b764641ab20e8a480ee15ed625be8, with server behavior checked against the default RustFS 1.0.0 image source. The 14 focused tests passed locally with cargo test --locked --lib pod_disruption_budget -j 6; git diff --check passed. GitHub Test and Lint passed; Docker Build was skipped. Real Kubernetes eviction and SSA integration scenarios were not executed. Correctness and compatibility findings are listed above; the overall module placement is appropriate. Regression coverage is needed for the reported cases.
Read the erasure layout through an injected future so the reconcile can run against a mocked API server, and document removing user-created budgets before upgrading.
Offline drives and an unreadable layout now reduce the allowance down to minAvailable equal to the server count. Pools without a reported standard parity use the automatic parity of their set size. A same-name budget the Tenant does not own is never patched, including one created concurrently; owned budgets are updated with a forced server-side apply. Docs state the STANDARD storage class scope and the upgrade procedure.
e44d124 to
10532dd
Compare
|
Addressed in 10532dd (rebased onto main):
Upgrade compatibility. The user guide, CHANGELOG and the PR Impact section now describe the procedure: upgrade the chart (CRD and RBAC) first; set
|
Type of Change
Related Issues
Closes #289
Summary of Changes
The operator now manages one PodDisruptionBudget per Tenant pool, so node drains and autoscaler scale-downs cannot evict more pool Pods than the pool's erasure sets can lose while keeping write quorum for the
STANDARDstorage class.pod_disruption_budgetreconcile step after the pool StatefulSets. The budget is named<tenant>-<pool>, owned by the Tenant, carries the pool labels and selects the pool Pods.minAvailable= servers minus the servers that can be down while every erasure set keeps write quorum. It is derived from the layout RustFS reports in/rustfs/admin/v3/info(drivepool_index,set_index, endpoint host,state) and the reported standard parity. When pools resolve different parities RustFS reports none, and each set uses the automatic parity for its size, as RustFS does.minAvailable= servers, and the layout is re-read every 60s.PodDisruptionBudgetConflictwarning event.spec.disablePodDisruptionBudget(default: budgets on). Setting it deletes only the budgets owned by the Tenant; budgets of removed pools are deleted as well.rustfs-admin:RustfsServerInfonow deserializesservers[].drives[].deploy/k8s-dev):policy/poddisruptionbudgetsget, list, watch, create, patch, delete. The controller watches owned budgets.Checklist
make pre-commit(fmt-check + clippy + test + console-lint + console-fmt-check)[Unreleased](if user-visible change)Impact
Upgrade procedure (also in the user guide and CHANGELOG):
disablePodDisruptionBudgetand the operator ClusterRole gains PodDisruptionBudget permissions.spec.disablePodDisruptionBudget: truebefore the new operator image starts.Verification
make pre-commit cargo test --lib pod_disruption_budgetAdditional Notes
STANDARDobjects at the current parity.REDUCED_REDUNDANCYobjects (EC:1 by default) and objects written with a lower standard parity before a configuration change are not covered; including them would allow at most one drive per set and leave most multi-drive layouts without a budget. This is documented in the user guide.standardSCParity), a pool without spare servers, disabling (only Tenant-owned budgets, uid precondition), a removed pool while not settled, an unreadable layout and an offline drive tightening the budget to the server count, a same-name budget without the Tenant owner, and a budget created concurrently by someone else.