ARO-26557: Fix etcd bootstrap race when replicas join after quorum#8425
ARO-26557: Fix etcd bootstrap race when replicas join after quorum#8425patriksuba wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@patriksuba: This pull request references ARO-26557 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: patriksuba The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis change adds quorum-size helper functions and tests, surfaces ETCD_EXPECTED_MEMBER_COUNT and ETCD_QUORUM_MIN_MEMBERS from adaptStatefulSet into the reset-member initContainer, and modifies the reset-member script to gather MEMBER_LIST/MEMBER_ID and conditionally run Sequence Diagram(s)sequenceDiagram
participant HostedControlPlaneController
participant reset-member_initContainer
participant EtcdCluster
HostedControlPlaneController->>reset-member_initContainer: set env ETCD_EXPECTED_MEMBER_COUNT, ETCD_QUORUM_MIN_MEMBERS
reset-member_initContainer->>EtcdCluster: run etcdctl member list -> MEMBER_LIST
reset-member_initContainer->>reset-member_initContainer: compute MEMBER_ID, MEMBER_COUNT
alt MEMBER_COUNT >= ETCD_QUORUM_MIN_MEMBERS AND MEMBER_COUNT < ETCD_EXPECTED_MEMBER_COUNT
reset-member_initContainer->>EtcdCluster: etcdctl member add <peerURL>
else MEMBER_COUNT >= ETCD_EXPECTED_MEMBER_COUNT
reset-member_initContainer->>reset-member_initContainer: log "refusing to grow cluster"
else MEMBER_COUNT < ETCD_QUORUM_MIN_MEMBERS
reset-member_initContainer->>reset-member_initContainer: log "static bootstrap (new) path"
end
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8425 +/- ##
=======================================
Coverage 40.40% 40.40%
=======================================
Files 755 756 +1
Lines 93235 93257 +22
=======================================
+ Hits 37675 37685 +10
- Misses 52858 52870 +12
Partials 2702 2702
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yaml (1)
228-248:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid re-querying membership after the initial successful probe.
At Line 240, a second
etcdctl member listfailure is masked (|| true), which can setMEMBER_COUNT=0and force the Line 247 static"new"path even after Line 228 succeeded. That can reintroduce divergent bootstrap behavior during transient API/network blips.Use one captured
member listsnapshot from the successful call and derive bothMEMBER_IDandMEMBER_COUNTfrom it; if parsing fails, retry/fail init instead of falling back to"new".Proposed patch
- if etcdctl member list; then + if MEMBER_LIST="$(etcdctl member list -w simple)"; then echo "Cluster is functional" - MEMBER_ID=$(etcdctl member list -w simple | grep "${HOSTNAME}" | awk -F, '{ print $1 }') + MEMBER_ID=$(printf '%s\n' "${MEMBER_LIST}" | awk -F, -v host="${HOSTNAME}" '$2 == " name="host { print $1 }') if [[ -n "${MEMBER_ID}" ]]; then echo "A member with this name (${HOSTNAME}) already exists, removing" etcdctl member remove "${MEMBER_ID}" echo "Adding new member" etcdctl member add ${HOSTNAME} --peer-urls https://${HOSTNAME}.etcd-discovery.${NAMESPACE}.svc:2380 echo "existing" > /etc/etcd/clusterstate/state else echo "A member does not exist with name (${HOSTNAME}), evaluating straggler join" EXPECTED="${ETCD_EXPECTED_MEMBER_COUNT:-3}" - MEMBER_COUNT=$(etcdctl member list -w simple | grep -c . || true) + MEMBER_COUNT=$(printf '%s\n' "${MEMBER_LIST}" | grep -c .) QUORUM="${ETCD_QUORUM_MIN_MEMBERS:-$(( EXPECTED / 2 + 1 ))}" if [[ "${MEMBER_COUNT}" -ge "${QUORUM}" ]]; then🤖 Prompt for 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. In `@control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yaml` around lines 228 - 248, Capture the output of the successful etcdctl member list probe into a single variable and reuse it for both MEMBER_ID and MEMBER_COUNT instead of re-running etcdctl (replace the separate calls that set MEMBER_ID with a parse of that snapshot and replace MEMBER_COUNT=$(etcdctl member list -w simple | grep -c . || true) with a count derived from the same snapshot); if parsing the snapshot to obtain MEMBER_ID or MEMBER_COUNT fails, retry the probe a bounded number of times or fail initialization instead of falling back to the static "new" path; update the code paths that reference MEMBER_ID, MEMBER_COUNT, HOSTNAME, NAMESPACE, ETCD_EXPECTED_MEMBER_COUNT and ETCD_QUORUM_MIN_MEMBERS to read from the parsed snapshot and add appropriate retry/fail logic.
🤖 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.
Outside diff comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yaml`:
- Around line 228-248: Capture the output of the successful etcdctl member list
probe into a single variable and reuse it for both MEMBER_ID and MEMBER_COUNT
instead of re-running etcdctl (replace the separate calls that set MEMBER_ID
with a parse of that snapshot and replace MEMBER_COUNT=$(etcdctl member list -w
simple | grep -c . || true) with a count derived from the same snapshot); if
parsing the snapshot to obtain MEMBER_ID or MEMBER_COUNT fails, retry the probe
a bounded number of times or fail initialization instead of falling back to the
static "new" path; update the code paths that reference MEMBER_ID, MEMBER_COUNT,
HOSTNAME, NAMESPACE, ETCD_EXPECTED_MEMBER_COUNT and ETCD_QUORUM_MIN_MEMBERS to
read from the parsed snapshot and add appropriate retry/fail logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 17a637b3-3f78-48e6-910c-3710a4c6befe
⛔ Files ignored due to path filters (5)
control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/AROSwift/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/GCP/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/IBMCloud/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**
📒 Files selected for processing (4)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go
patriksuba
force-pushed
the
aro-26557-etcd-bootstrap-race
branch
from
60e67fe to
26b1ba3
Compare
|
/test all |
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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yaml`:
- Around line 240-247: The current logic calls etcdctl member add when
MEMBER_COUNT >= ETCD_QUORUM_MIN_MEMBERS, but it doesn't ensure the live cluster
is still smaller than the desired ETCD_EXPECTED_MEMBER_COUNT; update the
conditional that decides to run etcdctl member add (the block referencing
MEMBER_COUNT, ETCD_QUORUM_MIN_MEMBERS, ETCD_EXPECTED_MEMBER_COUNT and etcdctl
member add ${HOSTNAME} --peer-urls ...) to require both that MEMBER_COUNT >=
ETCD_QUORUM_MIN_MEMBERS and MEMBER_COUNT < ETCD_EXPECTED_MEMBER_COUNT before
adding this member, otherwise fall back to the static bootstrap path.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5dcf96e9-c079-427e-a61f-f8e24e391aa2
⛔ Files ignored due to path filters (5)
control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/AROSwift/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/GCP/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/IBMCloud/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**
📒 Files selected for processing (4)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go
🚧 Files skipped from review as they are similar to previous changes (3)
- control-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership.go
- control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go
patriksuba
force-pushed
the
aro-26557-etcd-bootstrap-race
branch
from
26b1ba3 to
eaeb257
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-azure-v2-self-managed |
|
@patriksuba: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Good — PR #8425 only touches etcd-related files. It has nothing to do with the missing Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth CI failures are not caused by PR #8425's code changes. The Prow job ( Root CauseProw job (
However, the source code for these binaries ( This means the Konflux job ( Recommendations
Evidence
|
Parallel etcd pods and delayed PVCs allow a majority to bootstrap before every replica starts. A late replica with an empty data dir must use member add and existing initial-cluster-state instead of new, or it can form a divergent cluster identity. Change the reset-member init to detect that case (member list shows at least quorum-min members but not this hostname) and add the member before starting etcd. Prevent adding etcd cluster members beyond the expected limit.
patriksuba
force-pushed
the
aro-26557-etcd-bootstrap-race
branch
from
eaeb257 to
efbc1d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership.go (1)
16-21: ⚡ Quick win
resetMemberStragglerJoinQuorumMethas no production caller — logic is duplicated in bash.This helper exists only to be exercised by
membership_test.go. The equivalent decision (MEMBER_COUNT >= ETCD_QUORUM_MIN_MEMBERS && MEMBER_COUNT < ETCD_EXPECTED_MEMBER_COUNT) is reimplemented inassets/etcd/statefulset.yaml. Testing a helper that no production code path uses gives false confidence: the bash branch can drift without any test failure.Two reasonable options:
- Drop this helper (and its tests) and add a bash-level test for the script instead, or
- Use it from
adaptStatefulSetto project the boolean (or use it to document/derive the env var contract), so the Go logic and bash logic stay tied to a single source of truth.🤖 Prompt for 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. In `@control-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership.go` around lines 16 - 21, The helper resetMemberStragglerJoinQuorumMet is unreferenced in production and duplicates logic in the bash manifest; either remove the helper and its membership_test.go tests and add a test that validates the bash script/manifest behavior, or (preferred) call resetMemberStragglerJoinQuorumMet from adaptStatefulSet so the code computes the same boolean used to set the ETCD env var/manifest value and membership_test.go stays as unit coverage for the real logic—update adaptStatefulSet to invoke resetMemberStragglerJoinQuorumMet(expectedClusterSize, memberCount) (or adjust parameter order to match) and use its return to derive/set the environment variable or field written to the StatefulSet, and remove the duplicated hard-coded expression from assets/etcd/statefulset.yaml generation so there is a single source of truth.
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yaml`:
- Around line 230-231: The current MEMBER_ID extraction uses grep "${HOSTNAME}"
which can match substrings/regex and return wrong rows; replace that grep-based
extraction by selecting the exact member-name column from MEMBER_LIST using awk
to compare field 3 to HOSTNAME (i.e., use MEMBER_LIST assignment and then
compute MEMBER_ID with awk -F, -v name="${HOSTNAME}" '$3 == name { print $1 }');
update the lines referencing MEMBER_LIST and MEMBER_ID (the two lines that set
MEMBER_LIST and MEMBER_ID) so MEMBER_ID is derived with the exact-match awk
approach to avoid partial/regex matches.
---
Nitpick comments:
In `@control-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership.go`:
- Around line 16-21: The helper resetMemberStragglerJoinQuorumMet is
unreferenced in production and duplicates logic in the bash manifest; either
remove the helper and its membership_test.go tests and add a test that validates
the bash script/manifest behavior, or (preferred) call
resetMemberStragglerJoinQuorumMet from adaptStatefulSet so the code computes the
same boolean used to set the ETCD env var/manifest value and membership_test.go
stays as unit coverage for the real logic—update adaptStatefulSet to invoke
resetMemberStragglerJoinQuorumMet(expectedClusterSize, memberCount) (or adjust
parameter order to match) and use its return to derive/set the environment
variable or field written to the StatefulSet, and remove the duplicated
hard-coded expression from assets/etcd/statefulset.yaml generation so there is a
single source of truth.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3dc7c258-03b8-4d2a-be4e-84061ec222e5
⛔ Files ignored due to path filters (5)
control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/AROSwift/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/GCP/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/IBMCloud/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**control-plane-operator/controllers/hostedcontrolplane/testdata/etcd/zz_fixture_TestControlPlaneComponents_etcd_statefulset.yamlis excluded by!**/testdata/**
📒 Files selected for processing (4)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/membership_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go
| MEMBER_LIST=$(etcdctl member list -w simple) | ||
| MEMBER_ID=$(echo "${MEMBER_LIST}" | grep "${HOSTNAME}" | awk -F, '{ print $1 }') |
There was a problem hiding this comment.
grep "${HOSTNAME}" is a substring/regex match across the member list.
grep "${HOSTNAME}" matches any line containing HOSTNAME, and treats it as a regex. For etcd member names sharing prefixes (e.g. etcd-1 would also match a line for etcd-10/etcd-11), this can pick up the wrong row and yield a bogus MEMBER_ID (or a multi-line value that breaks the subsequent etcdctl member remove). Today HyperShift caps etcd at a small replica count so the collision is unlikely in practice, but since this line is being modified it’s cheap to harden.
Two safer options:
- Anchor the match on the member-list column (with
-w simple, the member name is field 3):awk -F, -v name="${HOSTNAME}" '$3 == name { print $1 }' - Or use
grep -F -w -- "${HOSTNAME}"to disable regex and require a whole-word match (note:-wstill treats-as a word boundary, so prefer the awk form for full safety).
♻️ Proposed change
- MEMBER_LIST=$(etcdctl member list -w simple)
- MEMBER_ID=$(echo "${MEMBER_LIST}" | grep "${HOSTNAME}" | awk -F, '{ print $1 }')
+ MEMBER_LIST=$(etcdctl member list -w simple)
+ MEMBER_ID=$(echo "${MEMBER_LIST}" | awk -F, -v name="${HOSTNAME}" '$3 == name { print $1 }')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MEMBER_LIST=$(etcdctl member list -w simple) | |
| MEMBER_ID=$(echo "${MEMBER_LIST}" | grep "${HOSTNAME}" | awk -F, '{ print $1 }') | |
| MEMBER_LIST=$(etcdctl member list -w simple) | |
| MEMBER_ID=$(echo "${MEMBER_LIST}" | awk -F, -v name="${HOSTNAME}" '$3 == name { print $1 }') |
🤖 Prompt for 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.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/etcd/statefulset.yaml`
around lines 230 - 231, The current MEMBER_ID extraction uses grep "${HOSTNAME}"
which can match substrings/regex and return wrong rows; replace that grep-based
extraction by selecting the exact member-name column from MEMBER_LIST using awk
to compare field 3 to HOSTNAME (i.e., use MEMBER_LIST assignment and then
compute MEMBER_ID with awk -F, -v name="${HOSTNAME}" '$3 == name { print $1 }');
update the lines referencing MEMBER_LIST and MEMBER_ID (the two lines that set
MEMBER_LIST and MEMBER_ID) so MEMBER_ID is derived with the exact-match awk
approach to avoid partial/regex matches.
|
/assign @jparrill |
3 participants
What this PR does / why we need it:
Parallel etcd pods and delayed PVCs allow a majority to bootstrap before every replica starts. A late replica with an empty data dir must use member add and existing initial-cluster-state instead of new, or it can form a divergent cluster identity.
Change the reset-member init to detect that case (member list shows at least quorum-min members but not this hostname) and add the member before starting etcd.
Which issue(s) this PR fixes:
Fixes ARO-26557
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Improvements
Tests