[WIP][DNM]: OCPBUGS-105240: test-only delay MemberRemove to reproduce bootstrap race - #1673
[WIP][DNM]: OCPBUGS-105240: test-only delay MemberRemove to reproduce bootstrap race#1673Neilhamza wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@Neilhamza: This pull request references Jira Issue OCPBUGS-105240, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/invalid-bug
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe bootstrap teardown controller now uses a ten-minute sync timeout and adds a test-only five-minute delay before bootstrap completion validation and etcd bootstrap member removal. ChangesBootstrap teardown timing
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Neilhamza
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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 `@pkg/operator/bootstrapteardown/bootstrap_teardown_controller.go`:
- Around line 146-152: Gate the five-minute sleep in the bootstrap member
removal flow behind an explicit test-only hook or mechanism, using the
surrounding removal logic and TestRemoveBootstrap integration point. Ensure
production reconciliation never delays, while the test can still enable the
delay to reproduce the race deterministically; leave normal removal behavior
unchanged.
- Around line 146-152: Replace the five-minute time.Sleep in removeBootstrap
with a timer that exits when ctx.Done() fires, and use a dedicated context whose
deadline spans both the intentional delay and subsequent MemberRemove call.
Alternatively, remove the entire TEST ONLY/DO NOT MERGE delay block before
merging.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 55f970c0-43a9-49f8-b128-6b0199aa1c1e
📒 Files selected for processing (1)
pkg/operator/bootstrapteardown/bootstrap_teardown_controller.go
| // TEST ONLY (OCPBUGS-105240): delay bootstrap member removal by 5 minutes | ||
| // to hold the race window open between EtcdRunningInCluster=True and MemberRemove(). | ||
| // This forces the TNF pacemaker startup deadlock deterministically. | ||
| // DO NOT MERGE. | ||
| klog.Warningf("TEST ONLY (OCPBUGS-105240): delaying bootstrap member removal by 5 minutes to reproduce race condition") | ||
| time.Sleep(5 * time.Minute) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make the delay truly test-only.
The TEST ONLY comment does not gate this code. Normal bootstrap teardown now blocks for five minutes on every safe removal attempt. The supplied TestRemoveBootstrap test also adds at least ten minutes across its two complete-removal cases.
Inject the delay as a test hook or guard it with an explicit test-only mechanism. Keep the normal reconciliation path unchanged.
🤖 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 `@pkg/operator/bootstrapteardown/bootstrap_teardown_controller.go` around lines
146 - 152, Gate the five-minute sleep in the bootstrap member removal flow
behind an explicit test-only hook or mechanism, using the surrounding removal
logic and TestRemoveBootstrap integration point. Ensure production
reconciliation never delays, while the test can still enable the delay to
reproduce the race deterministically; leave normal removal behavior unchanged.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'WithTimeout|removeBootstrap|time\.Sleep|MemberRemove' \
pkg/operator/bootstrapteardown
rg -n -C 6 '\bMemberRemove\s*\(' --glob '*.go'Repository: openshift/cluster-etcd-operator
Length of output: 17857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- controller imports and methods ---'
sed -n '1,185p' pkg/operator/bootstrapteardown/bootstrap_teardown_controller.go
printf '%s\n' '--- relevant tests ---'
sed -n '330,410p' pkg/operator/bootstrapteardown/bootstrap_teardown_controller_test.go
printf '%s\n' '--- timeout and sleep usage in the package ---'
rg -n -C 4 'context\.WithTimeout|time\.Sleep|MemberRemove|removeBootstrap' pkg/operator/bootstrapteardown
printf '%s\n' '--- current change summary ---'
git status --short
git diff --stat
git diff -- pkg/operator/bootstrapteardown/bootstrap_teardown_controller.goRepository: openshift/cluster-etcd-operator
Length of output: 24248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- EtcdClient and MemberRemove implementations ---'
rg -n -C 8 'type EtcdClient|func .*MemberRemove|MemberRemove\s*\(' \
pkg --glob '*.go'
printf '%s\n' '--- timing probe ---'
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class Context:
deadline_seconds: int
ctx = Context(deadline_seconds=60)
sleep_seconds = 5 * 60
member_remove_start = sleep_seconds
print(f"context deadline: {ctx.deadline_seconds}s")
print(f"MemberRemove starts: {member_remove_start}s")
print(f"context expired before MemberRemove: {member_remove_start >= ctx.deadline_seconds}")
PYRepository: openshift/cluster-etcd-operator
Length of output: 15345
Make the test-only delay respect its context deadline.
sync passes a one-minute timeoutCtx to removeBootstrap, but this path blocks for five minutes with time.Sleep. MemberRemove then receives an expired context and returns context deadline exceeded. Replace time.Sleep with a timer that observes ctx.Done(), and use a dedicated context whose deadline covers the delay and member removal. Otherwise, remove this DO NOT MERGE block before merging.
🤖 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 `@pkg/operator/bootstrapteardown/bootstrap_teardown_controller.go` around lines
146 - 152, Replace the five-minute time.Sleep in removeBootstrap with a timer
that exits when ctx.Done() fires, and use a dedicated context whose deadline
spans both the intentional delay and subsequent MemberRemove call.
Alternatively, remove the entire TEST ONLY/DO NOT MERGE delay block before
merging.
Source: Path instructions
…rap race Inject a 5-minute sleep between EtcdRunningInCluster=True and MemberRemove() in removeBootstrap() to hold the race window open and force the TNF pacemaker startup deadlock deterministically. Also extend the sync() context timeout from 1m to 10m so the sleep doesn't cause a context deadline exceeded before MemberRemove() runs. DO NOT MERGE — this is a reproducer for testing only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6e8b52a to
1df0224
Compare
|
@Neilhamza: This pull request references Jira Issue OCPBUGS-105240. The bug has been updated to no longer refer to the pull request using the external bug tracker. 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. |
2 participants
DO NOT MERGE
Test-only reproducer for OCPBUGS-105240. Injects a 5-minute
time.SleepbetweenEtcdRunningInCluster=TrueandMemberRemove()inremoveBootstrap()to hold the race window open and force the TNF pacemaker startup deadlock deterministically.Usage with cluster-bot
Reproduce the deadlock (PR A alone):
Deploy the resulting payload on a TNF fencing cluster. Expected: deadlock every time — pacemaker sees 3 etcd members, logs "found 3 members, need 2".
Verify the fix (PR A + fix PR together):
Expected: TNF gate stays closed during the 5-minute delay, then proceeds to clean install after the bootstrap member is removed.
Related
/hold
Summary by CodeRabbit
Bug Fixes
Tests