operator: scale down if proxy rollout is stuck - #65
Conversation
📝 WalkthroughWalkthroughThe proxy reconciler detects unschedulable pods during scale-down rollouts and permits proxy scaling without the usual confirmation delay. The changelog documents the autoscaling and blocked-rollout behavior. ChangesProxy rollout autoscaling
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Sequence Diagram(s)sequenceDiagram
participant handleProxyState
participant hasUnschedulableProxyPods
participant KubernetesPodAPI
participant handleProxyScale
handleProxyState->>hasUnschedulableProxyPods: check proxy pods
hasUnschedulableProxyPods->>KubernetesPodAPI: list pods by selector
KubernetesPodAPI-->>hasUnschedulableProxyPods: return pod statuses
hasUnschedulableProxyPods-->>handleProxyState: return rolloutBlocked
handleProxyState->>handleProxyScale: scale blocked proxy rollout
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/CHANGELOG.md`:
- Line 31: Remove the duplicate “### Changed” heading from the changelog,
preserving the existing heading and its associated content.
- Line 29: Update the changelog entry describing the proxy rollout condition to
specify an unschedulable proxy pod rather than a generic pending pod, matching
the controller’s isPodUnschedulable gating behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0616e253-9d97-4cf6-9078-e87b6d66483e
📒 Files selected for processing (2)
operator/CHANGELOG.mdoperator/internal/controller/aistore/proxy_controller.go
29e21d1 to
2d7a9a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/internal/controller/aistore/proxy_controller.go`:
- Around line 180-182: Update the comment above the scaling condition in the
proxy controller to describe rollout blocking as allowing scaling when the
desired proxy size is lower, rather than attributing it only to user-initiated
scale-downs. Leave the scaling logic unchanged.
- Around line 218-223: Update Reconciler.hasUnschedulableProxyPods to return
(bool, error), propagate the ListPods error instead of returning false, and
update every caller to handle and bubble the error so recovery is not skipped
after proxy pod-list failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 01a0848a-2fec-463a-ae57-c07466b90a2b
📒 Files selected for processing (2)
operator/CHANGELOG.mdoperator/internal/controller/aistore/proxy_controller.go
Signed-off-by: eric-higgins-ai <erichiggins@applied.co>
2d7a9a4 to
4cfed1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/CHANGELOG.md`:
- Around line 27-29: Correct the misspelled “scheduable” term in the autoscaling
changelog bullet to “schedulable,” leaving the rest of the entry unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 02b50a8b-0572-41e7-85d2-846ca75d9763
📒 Files selected for processing (2)
operator/CHANGELOG.mdoperator/internal/controller/aistore/proxy_controller.go
|
This may work for If a proxy is truly unschedulable then I believe we could instead check ais.GetProxySize() < ss.Spec.Replicas (GetProxySize checks against expected nodes which already checks the count of valid hosts). Then we could say, if we are using emptyDir + autoscale, to skip past the rollout block and proceed to scale before rollout is complete. That bypasses doing a full second proxy pod list to check schedule status. One thing we may need to consider is how this could impact rollout partition/primary management. |
I think there are a few different cases, and this addresses the problem to varying degrees of completeness:
At a high level, while this PR only allows the rollout to continue fully autonomously in the
This is a good point, though we don't get the benefit to case 2. mentioned above. If you think it's worth losing that to get rid of the additional proxy pod list then I'm down to make the change.
Now that you mention it, the rollout primary management has been slightly broken since this change. It assumes proxy-0 is always the primary, which is now not the case. I don't think this breaks the primary behavior any more than it's already broken though. The flow would be like:
|
There was a problem hiding this comment.
My main issue is "scale down if proxy rollout is stuck" doesn't describe what we want in all scenarios. We've previously required rollout to succeed before scaling because autoscaling wasn't supported.
I think, to rephrase it, we really want rollout to not block scale down however it's triggered, regardless of pod status. Avoiding this bunch of lists, filters, and if statements will help us avoid races and edge cases. If that scaling is driven by autoscale then fine, otherwise we respect the spec. If we can support this then we don't need to have special cases for each type of deployment.
But the tricky part is managing setting the primary proxy. Requiring rollout to succeed first allowed us to rely on the statefulset status to know we could assign ordinal 0 as primary and avoid elections during scaling.
There is a catch here now because if we reach that last stage of rollout we'll have set the highest ordinal as primary, which is exactly what will get removed if scale down kicks in. So scale down will try to set a new primary with reassignPrimaryForScaledown which will likely pick proxy-0, right around when the statefulset updates it. So we need to tighten that guard by setting primary to a different eligible proxy. This is another thing we can fix in a smaller scoped patch.
statefulsetScalingNeeded also blocks on isRolloutInProgress so will need an update there as well.
|
I think the easy solution is:
Edit: I'll push an update first to reactor and clean up our scale-down flow. It's fairly convoluted and untested as is. |
Part 1 is implemented. Part 2 will require more than just removing the guard. We'll need to watch out for any temporary readiness drops from rollout making it look like we need to scale, guard around low-replica-count edge cases, and be careful about requeue loops and making sure we hit We'll potentially also need to make both rollout and scale prefer the same primary -- today rollout sets to highest and scale still wants lowest valid proxy. I'm thinking rollout may want to prefer resetting to partition + 1 instead of highest, so we simply don't have to re-assign primary often on scale. |
3 participants
Rollouts to the proxy statefulset currently become stuck if scaling becomes necessary in the middle of the rollout. For example, if a node is removed from the cluster during the rollout then a pod will be pending and the rollout will get stuck there.
This was fixed in v3.0.0 for target pods by not blocking the rollout on unschedulable pods, but this isn't possible for proxies because they use the default
RollingUpdateupdate strategy of StatefulSets. Instead, we detect the case where a pod is unschedulable during a rollout and scaling is needed and perform the scaling before continuing with the rollout.Summary by CodeRabbit