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

[Tyagiquamar]

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

[alex-zaitsev]

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 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