Repository navigation
cluster: don't wait for storage pods in a maintenance - #2717
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/operator/factory/vtcluster/storage.go">
<violation number="1" location="internal/controller/operator/factory/vtcluster/storage.go:189">
P2: With the `RollingUpdate` wait path, out-of-range maintenance IDs still reduce the required Ready replicas because reconciliation subtracts the set length. Filter these IDs to the active replica range; stale IDs can otherwise let the StatefulSet reconcile while live pods remain unready.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| cr.Spec.Storage.ReplicaCount = existingSpec.Replicas | ||
| } | ||
| }, | ||
| IgnoreNotReadyPodOrdinals: cr.StorageNodeIDsUnderMaintenance(), |
There was a problem hiding this comment.
P2: With the RollingUpdate wait path, out-of-range maintenance IDs still reduce the required Ready replicas because reconciliation subtracts the set length. Filter these IDs to the active replica range; stale IDs can otherwise let the StatefulSet reconcile while live pods remain unready.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/controller/operator/factory/vtcluster/storage.go, line 189:
<comment>With the `RollingUpdate` wait path, out-of-range maintenance IDs still reduce the required Ready replicas because reconciliation subtracts the set length. Filter these IDs to the active replica range; stale IDs can otherwise let the StatefulSet reconcile while live pods remain unready.</comment>
<file context>
@@ -186,6 +186,7 @@ func createOrUpdateVTStorageSTS(ctx context.Context, rclient client.Client, cr,
cr.Spec.Storage.ReplicaCount = existingSpec.Replicas
}
},
+ IgnoreNotReadyPodOrdinals: cr.StorageNodeIDsUnderMaintenance(),
}
owner := cr.AsOwner()
</file context>
bf24ed7 to
4874def
Compare
|
@cubic-ai-dev |
@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
4874def to
51fee1b
Compare
| return true, nil | ||
| } | ||
| if *targetReplicas != existingObj.Status.ReadyReplicas || *targetReplicas != existingObj.Status.UpdatedReplicas { | ||
| if *targetReplicas != existingObj.Status.UpdatedReplicas { |
There was a problem hiding this comment.
Would UpdatedReplicas have expected value? When some pods are on maintenance and replicaCount has changed it would probably get stuck?
There was a problem hiding this comment.
Good catch — only an issue under rollingUpdateStrategy: RollingUpdate. The default OnDelete path already defers maintenance pods. Under RollingUpdate, Kubernetes' own controller won't advance past a not-Ready pod, regardless of replica changes — that's native OrderedReady behavior and can't be bypassed without replacing it with our own maintenance-aware update path. Documented the limitation in the meantime.
|
Could you please add a new section to all relevant docs about this new behavior, including all the limitations? |
51fee1b to
5868241
Compare
5868241 to
8b14dc0
Compare
8b14dc0 to
18c28f7
Compare
|
I added two more things in 4bc910b
Please let me know if you see any issue. |
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Turn on auto-fix | Re-trigger cubic
8d83c34 to
385cd3b
Compare
There was a problem hiding this comment.
All reported issues were addressed across 19 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
385cd3b to
18e4506
Compare
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
3 participants
fixes #2665
Operator leaves a pod listed in both
vmstorage.maintenanceInsertNodeIDsandvmstorage.maintenanceSelectNodeIDsfields alone during reconciliation, since it receives no reads and no writes:PendingorTerminatingno longer blocksreconciliation of
vminsertandvmselect;vmstoragespec changes to the pod or its volumes, e.g. a new image or a bigger volume size.A pod listed in only one of the two fields still receives the other kind of traffic, so the Operator handles it as usual.