8000 fix(chi): gate aborted auto-recovery on operator ready label by Tyagiquamar · Pull Request #2108 · Altinity/clickhouse-operator · GitHub
[go: up one dir, main page]
Skip to content

fix(chi): gate aborted auto-recovery on operator ready label - #2108

Open
Tyagiquamar wants to merge 6 commits into
Altinity:0.28.0from
Tyagiquamar:fix/recovery-ready-label-2106
Open

fix(chi): gate aborted auto-recovery on operator ready label#2108
Tyagiquamar wants to merge 6 commits into
Altinity:0.28.0from
Tyagiquamar:fix/recovery-ready-label-2106

Conversation

@Tyagiquamar
@Tyagiquamar Tyagiquamar commented Oct 18, 2026
Copy link
Copy Markdown

Summary

  • Re-enqueue aborted CHI reconciles when the clickhouse.altinity.com/ready label transitions to yes, instead of on kubelet container readiness alone.
  • Add unit coverage for the service-label transition gate.

Test plan

  • go test ./pkg/controller/chi -run 'TestIsPodOperatorReadyLabelNotReadyToReadyTransition|TestShouldTriggerAutoRecovery' (not run locally; CI-first A1)
  • Monitor gh pr checks on this PR

Fixes #2106

@Robin1987China
fix(chi): gate aborted auto-recovery on operator ready label
8f18983 
Re-enqueue aborted CHIs when the clickhouse.altinity.com/ready label
transitions to yes, not on kubelet container readiness alone.

Fixes Altinity#2106
@alex-zaitsev
alex-zaitsev changed the base branch from master to 0.28.0 October 19, 2026 13:20
@alex-zaitsev
alex-zaitsev commented Oct 19, 2026
Copy link
Copy Markdown
Member

PR #2108 — Code Review

PR: #2108
Commit reviewed: 8f1898387308
Base branch: 0.28.0
Related issue: #2106

Verdict: Changes requested

The PR changes two files and attempts to fix issue #2106 by triggering recovery when the operator's clickhouse.altinity.com/ready label changes to yes, rather than when Kubernetes reports that a Pod's containers have become Ready.

There are two significant issues with this approach.

Finding 1 — [P1] The new trigger can prevent recovery indefinitely

File: pkg/controller/chi/worker-pod-retry.go

The PR replaces the Pod NotReady → Ready trigger with an operator-label no → yes trigger:

if !isPodOperatorReadyLabelNotReadyToReadyTransition(oldPod, newPod, cr) {
    return
}

This creates a circular dependency.

The operator normally sets the label to yes only after successfully reconciling a host and including it back into the Service.

Consider the exact failure described in issue #2106:

  1. Replica 0-1 is excluded from the Service, setting ready=no.
  2. Its reconciliation times out, leaving the CHI Aborted.
  3. Replica 0-1's containers recover and become Ready.
  4. The new trigger ignores that event because the operator label remains no.
  5. No recovery reconcile is scheduled to perform the work that would eventually restore ready=yes.

Unless another event initiates reconciliation, the CHI can remain Aborted and replica 0-1 can remain excluded indefinitely.

This is supported by the existing call chain:

includeHost() → includeHostIntoService() → SetReadyMarkOnPodAndService()

Required change: Do not make automatic recovery depend exclusively on a label that recovery itself must restore.

Finding 2 — [P1] The original Service-outage risk remains

File: pkg/controller/chi/worker-status-helpers.go

PR #2108 changes only the recovery trigger. It does not change how the reconciler decides whether another replica is safe to 8000 rely on.

isHostHealthyForReconcile() still returns true for a running, Kubernetes-Ready Pod whose operator label is ready=no.

That predicate is used by:

  • reconcileShardWithHosts() to order recovery and rollout.
  • isShardSafeToDisruptHost() to authorize disruption of another replica.

Therefore, if a reconcile is triggered by a CHI update or another event, the original sequence can still occur:

Replica Kubernetes Ready Operator label Service eligible
0-0 Yes yes Yes
0-1 Yes no No

The safety check can incorrectly count 0-1 as a healthy peer, authorize exclusion of 0-0, and leave the Service without eligible replicas.

Required change: Ensure the peer-safety and recovery-ordering decisions account for clickhouse.altinity.com/ready=yes, in addition to Pod health.

A dedicated Service-eligibility predicate may be preferable to changing every use of isHostHealthyForReconcile().

Tests need to cover the actual failure

The new unit tests correctly exercise the label-transition helper, but they encode the problematic behavior:

{"kubelet ready but label still no", pod(false), pod(true), false},

That is exactly the event that needs to initiate recovery in issue #2106.

PR #2108 — Code Review

PR: #2108
Commit reviewed: 8f1898387308
Base branch: 0.28.0
Related issue: #2106

Verdict: Changes requested

The PR changes two files and attempts to fix issue #2106 by triggering recovery when the operator's clickhouse.altinity.com/ready label changes to yes, rather than when Kubernetes reports that a Pod's containers have become Ready.

I found two significant issues with this approach.

Finding 1 — [P1] The new trigger can prevent recovery indefinitely

File: pkg/controller/chi/worker-pod-retry.go

The PR replaces the Pod NotReady → Ready trigger with an operator-label no → yes trigger:

if !isPodOperatorReadyLabelNotReadyToReadyTransition(oldPod, newPod, cr) {
    return
}

This creates a circular dependency.

The operator normally sets the label to yes only after successfully reconciling a host and including it back into the Service.

Consider the exact failure described in issue #2106:

  1. Replica 0-1 is excluded from the Service, setting ready=no.
  2. Its reconciliation times out, leaving the CHI Aborted.
  3. Replica 0-1's containers recover and become Ready.
  4. The new trigger ignores that event because the operator label remains no.
  5. No recovery reconcile is scheduled to perform the work that would eventually restore ready=yes.

Unless another event initiates reconciliation, the CHI can remain Aborted and replica 0-1 can remain excluded indefinitely.

This is supported by the existing call chain:

includeHost() → includeHostIntoService() → SetReadyMarkOnPodAndService()

Required change: Do not make automatic recovery depend exclusively on a label that recovery itself must restore.

Finding 2 — [P1] The original Service-outage risk remains

File: pkg/controller/chi/worker-status-helpers.go

PR #2108 changes only the recovery trigger. It does not change how the reconciler decides whether another replica is safe to rely on.

isHostHealthyForReconcile() still returns true for a running, Kubernetes-Ready Pod whose operator label is ready=no.

That predicate is used by:

  • reconcileShardWithHosts() to order recovery and rollout.
  • isShardSafeToDisruptHost() to authorize disruption of another replica.

Therefore, if a reconcile is triggered by a CHI update or another event, the original sequence can still occur:

Replica Kubernetes Ready Operator label Service eligible
0-0 Yes yes Yes
0-1 Yes no No

The safety check can incorrectly count 0-1 as a healthy peer, authorize exclusion of 0-0, and leave the Service without eligible replicas.

Required change: Ensure the peer-safety and recovery-ordering decisions account for clickhouse.altinity.com/ready=yes, in addition to Pod health.

A dedicated Service-eligibility predicate may be preferable to changing every use of isHostHealthyForReconcile().

Tests need to cover the actual failure

The new unit tests correctly exercise the label-transition helper, but they encode the problematic behavior:

{"kubelet ready but label still no", pod(false), pod(true), false},

That is exactly the event that needs to initiate recovery in issue #2106.

I would request a regression test with one shard and two replicas, asserting that:

  1. Replica 0-1 is Kubernetes Ready but has ready=no after an aborted reconcile.
  2. Recovery is initiated and 0-1 is processed first.
  3. Replica 0-0 remains Service-eligible until a safe replacement is available.
  4. The generated Service never reaches zero eligible replicas during recovery.

CI status

At the time of review:

  • check_helm: passed.
  • run_tests: hangs and aborts

@Tyagiquamar
Tyagiquamar commented Oct 20, 2026
Copy link
Copy Markdown
Author

Addressed the review feedback on 8f18983 / merge tip:

  1. Recovery trigger — restored kubelet NotReady→Ready (no longer depends on the operator ready label transitioning to yes, which recovery itself must set).
  2. Peer safety / ordering — isHostHealthyForReconcile now also requires clickhouse.altinity.com/ready=yes, so a container-Ready but Service-excluded replica is recovered first and cannot authorize disruption of its last eligible sibling.

Local check: docker run golang:1.27.2-bookworm go test -vet=off ./pkg/controller/chi/ -run 'TestIsHostHealthyForReconcile|TestIsPodNotReadyToReadyTransition|TestShouldTriggerAutoRecovery' → pass.

@Tyagiquamar
fix(chi): require service ready label for peer health (Altinity#2106)
49e0810 
Restore kubelet NotReady->Ready as the aborted auto-recovery trigger so recovery is not gated on a label that recovery itself restores. Count only clickhouse.altinity.com/ready=yes hosts as healthy peers for recovery ordering and disruption safety.
@alex-zaitsev
alex-zaitsev commented Oct 20, 2026
Copy link
Copy Markdown
Member

✘ [ Fail ] /regression/e2e.test_operator/test_010044. Schema and data propagation with slow replica (25m 14s)

Without the PR, once the slow replica is Kubernetes Ready, hasUnhealthyHosts() can become false. For a same-generation recovery event, the operator then returns gateNothingToDo and skips reconciliation.

With the PR, the missing Service-ready label keeps hasUnhealthyHosts() true, so that early exit no longer applies.

Recommended correction

For test_010044, explicitly disable automatic recovery in its operator configuration (test-044-chopconf.yaml)

spec:
  reconcile:
    recovery:
      onStatus:
        aborted:
          onPodReady: none

That preserves the original test's sequence: abort, wait for the Pod, verify schema is absent, force reconciliation, and verify schema appears.

@alex-zaitsev @cursoragent
test: disable auto-recovery in test_010044
77fb3f4 
The slow replica stays Service-ineligible after it becomes Ready, so recovery would reconcile before the test checks that the schema is absent.

Co-authored-by: Cursor <cursoragent@cursor.com>
@alex-zaitsev alex-zaitsev added the planned for review This feature is planned for review label Oct 21, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sunsingerus sunsingerus Awaiting requested review from sunsingerus

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

planned for review This feature is planned for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Recover reconciles from recoverAbortedReconcileOnPodReady ignores readiness of service selector labels

4 participants

@Tyagiquamar @alex-zaitsev @Robin1987China @Slach
0