OCPEDGE-2381: Verify dynamic revision bump for etcd during kernel crash#30915
OCPEDGE-2381: Verify dynamic revision bump for etcd during kernel crash#30915kasturinarra wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@kasturinarra: This pull request references OCPEDGE-2381 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 story to target the "4.22.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kasturinarra 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 |
WalkthroughAdds a new Ginkgo test that triggers a kernel panic on a node to validate etcd revision bump and recovery by parsing pacemaker journal and etcd config/revision files, and adds a utility to create the disruption pod that triggers the panic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ 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.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/two_node/tnf_recovery.go`:
- Around line 436-449: The cleanup currently assumes a fixed pod name
("disrupt-<node>-0") and deletes it late while ignoring errors; instead, after
calling TriggerKernelPanic (where createNodeDisruptionPod may have created a pod
with a bumped suffix), capture the actual pod name returned by
createNodeDisruptionPod (or lookup the pod by label/owner) and immediately
register g.DeferCleanup to delete that exact pod; replace the hard-coded
disruptPodName usage with the captured name, perform the Delete in the deferred
cleanup (not after validateEtcdRecoveryState), and propagate/log any delete
error instead of swallowing it to ensure deterministic, always-on cleanup
(reference createNodeDisruptionPod, TriggerKernelPanic, disruptPodName variable,
and use g.DeferCleanup).
- Line 415: The test suite is being focused by a Ginkgo focused spec; change the
focused call g.FIt used earlier (the g.FIt spec declaration) back to a normal
g.It so that the new test "should compute etcd revision bump after kernel panic
recovery" (the g.It at the bottom) will run; locate the g.FIt occurrence and
replace it with g.It in the test file (the Ginkgo spec registration function) to
remove the focus.
- Around line 451-486: The test currently only compares journalBump and
configBump (variables in tnf_recovery.go read via
exutil.DebugNodeRetryWithOptionsAndChroot) which can mirror a broken
compute_bump_revision(); add an independent verification by querying the
survived etcd member for its max raft index (e.g., via etcdctl endpoint status
or API on the same node using exutil.DebugNodeRetryWithOptionsAndChroot and
crashTimestamp context), compute expectedBump = floor(maxRaftIndex * 0.2) and
then assert both journalBump and configBump equal expectedBump (or alternatively
capture a pre-crash baseline maxRaftIndex and compute expected from that),
keeping the added logic near the existing journalBump/configBump checks and
referencing journalBump, configBump, compute_bump_revision(), and
exutil.DebugNodeRetryWithOptionsAndChroot.
- Around line 422-427: The test currently only logs the installed
resource-agents RPM (raVersion) and continues; instead gate the test by parsing
the rpm -q output obtained from exutil.DebugNodeRetryWithOptionsAndChroot(oc,
survivedNode.Name, "openshift-etcd", "bash", "-c", "rpm -q resource-agents"),
compare the trimmed version string against the minimums (>= "4.10.0-71.el9_6.13"
for RHEL9 and >= "4.16.0-33.el10" for RHEL10) and call e2eskipper.Skipf(...)
with a clear message when the installed version is lower. Implement robust
version comparison (normalize/compare numeric parts and release suffixes) and
place the skip before proceeding with the rest of the test so tests on older
payloads are skipped rather than failing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6de83e7d-b0d5-4cb1-aa24-4bc7f43063f2
📒 Files selected for processing (2)
test/extended/two_node/tnf_recovery.gotest/extended/util/nodes.go
|
Scheduling required tests: |
kasturinarra
force-pushed
the
automate_case_86078
branch
from
c2e89f3 to
c3dd53e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/extended/two_node/tnf_recovery.go (2)
415-415:⚠️ Potential issue | 🔴 CriticalThis new spec still won't run while Line 152 is
g.FIt.Ginkgo executes only focused specs, so this kernel-panic case is skipped entirely.
#!/bin/bash # Verify whether any focused Ginkgo specs remain in the repo. # Expected: no matches in merged code. rg -n --type=go '\b(g\.)?F(It|Describe|Context|When|Specify)\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/tnf_recovery.go` at line 415, The test contains a focused Ginkgo spec call using g.FIt (focused It) which prevents the new kernel-panic spec from running; change the focused call g.FIt to an unfocused g.It (or remove any leading "F" from focused constructs like g.FDescribe/g.FContext) so all specs, including the new "should compute etcd revision bump after kernel panic recovery" case, will execute normally; locate occurrences of g.FIt (and any g.FDescribe/g.FContext) in tnf_recovery.go and replace them with g.It (g.Describe/g.Context) to remove focus.
429-433:⚠️ Potential issue | 🟠 MajorLines 496-498 still allow an underestimated bump to pass.
The assertion only enforces
> 0and an upper bound, so a buggycompute_bump_revision()that returns any smaller positive value still passes. CapturemaxRaftIndexbefore the panic and bracket the result between pre- and post-crash/5bounds.Possible direction
g.By("Recording timestamp before crash") timestampStr, err := exutil.DebugNodeRetryWithOptionsAndChroot(oc, survivedNode.Name, "openshift-etcd", "bash", "-c", "date -u '+%Y-%m-%d %H:%M:%S'") o.Expect(err).To(o.BeNil()) crashTimestamp := strings.TrimSpace(timestampStr) + + preCrashRaftIndexStr, err := exutil.DebugNodeRetryWithOptionsAndChroot(oc, survivedNode.Name, "openshift-etcd", + "bash", "-c", "jq -r '.maxRaftIndex' /var/lib/etcd/revision.json") + o.Expect(err).To(o.BeNil()) + preCrashMaxRaftIndex, err := strconv.Atoi(strings.TrimSpace(preCrashRaftIndexStr)) + o.Expect(err).To(o.BeNil()) ... - o.Expect(configBump).To(o.BeNumerically(">", 0)) - o.Expect(configBump).To(o.BeNumerically("<=", int(float64(maxRaftIndex)*0.2)), - fmt.Sprintf("bump %d should be <= floor(%d * 0.2) = %d", configBump, maxRaftIndex, int(float64(maxRaftIndex)*0.2))) + lowerBound := preCrashMaxRaftIndex / 5 + upperBound := maxRaftIndex / 5 + o.Expect(configBump).To(o.BeNumerically(">=", lowerBound), + fmt.Sprintf("bump %d should be >= floor(%d * 0.2) = %d", configBump, preCrashMaxRaftIndex, lowerBound)) + o.Expect(configBump).To(o.BeNumerically("<=", upperBound), + fmt.Sprintf("bump %d should be <= floor(%d * 0.2) = %d", configBump, maxRaftIndex, upperBound))Also applies to: 488-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/tnf_recovery.go` around lines 429 - 433, Capture the Raft index before inducing the panic and use it to tightly bound the computed bump: run the same helper used elsewhere (e.g., exutil.DebugNodeRetryWithOptionsAndChroot or equivalent) on survivedNode to fetch and parse maxRaftIndex into preMaxRaftIndex before recording crashTimestamp, then after recovery fetch postMaxRaftIndex; compute the bump via compute_bump_revision() and assert bump >= preMaxRaftIndex/5 and bump <= postMaxRaftIndex/5 (use integer division/ceil consistent with other checks) instead of the loose >0 check so underestimated positive bumps fail. Ensure you reference the variables maxRaftIndex, crashTimestamp and the function compute_bump_revision() when updating the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/two_node/tnf_recovery.go`:
- Line 415: The test contains a focused Ginkgo spec call using g.FIt (focused
It) which prevents the new kernel-panic spec from running; change the focused
call g.FIt to an unfocused g.It (or remove any leading "F" from focused
constructs like g.FDescribe/g.FContext) so all specs, including the new "should
compute etcd revision bump after kernel panic recovery" case, will execute
normally; locate occurrences of g.FIt (and any g.FDescribe/g.FContext) in
tnf_recovery.go and replace them with g.It (g.Describe/g.Context) to remove
focus.
- Around line 429-433: Capture the Raft index before inducing the panic and use
it to tightly bound the computed bump: run the same helper used elsewhere (e.g.,
exutil.DebugNodeRetryWithOptionsAndChroot or equivalent) on survivedNode to
fetch and parse maxRaftIndex into preMaxRaftIndex before recording
crashTimestamp, then after recovery fetch postMaxRaftIndex; compute the bump via
compute_bump_revision() and assert bump >= preMaxRaftIndex/5 and bump <=
postMaxRaftIndex/5 (use integer division/ceil consistent with other checks)
instead of the loose >0 check so underestimated positive bumps fail. Ensure you
reference the variables maxRaftIndex, crashTimestamp and the function
compute_bump_revision() when updating the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3511893d-b1e7-4217-9acf-5b189c168edb
📒 Files selected for processing (2)
test/extended/two_node/tnf_recovery.gotest/extended/util/nodes.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended/util/nodes.go
|
Scheduling required tests: |
|
/retest |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-techpreview |
|
@kasturinarra: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7cbf10f0-25b9-11f1-8498-4e2cd9354376-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-recovery-techpreview |
|
@kasturinarra: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4860c090-269c-11f1-87a9-d789d209118b-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-recovery-techpreview |
|
@kasturinarra: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4d50ed30-26a3-11f1-8030-f644c2574005-0 |
kasturinarra
force-pushed
the
automate_case_86078
branch
from
c3dd53e to
ec5f331
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/two_node/tnf_recovery.go (1)
488-498:⚠️ Potential issue | 🟠 MajorThis only checks an upper bound, not the bump calculation.
Lines 496-498 assert
configBump > 0and<=a post-recoverymaxRaftIndex. Any undersized positive bump would still pass, so the test can miss the regression it is supposed to catch. Capture the exact inputmaxRaftIndexbefore Line 435, or extract it from the pacemaker log entry, and assertconfigBump/journalBumpagainst that expected bump.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/tnf_recovery.go` around lines 488 - 498, The test only checks an upper bound for configBump so add a deterministic check against the original pre-recovery maxRaftIndex: capture the raft index before recovery (the same place compute_bump_revision runs) by calling exutil.DebugNodeRetryWithOptionsAndChroot (or parsing the pacemaker log entry) to read '.maxRaftIndex' into originalMaxRaftIndex, compute expectedBump := int(float64(originalMaxRaftIndex)*0.2) (floor) and then assert configBump == expectedBump and journalBump == expectedBump (and still assert > 0); update references to configBump and journalBump in the test to use originalMaxRaftIndex-derived expectedBump so the test fails on undersized bumps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/two_node/tnf_recovery.go`:
- Around line 488-498: The test only checks an upper bound for configBump so add
a deterministic check against the original pre-recovery maxRaftIndex: capture
the raft index before recovery (the same place compute_bump_revision runs) by
calling exutil.DebugNodeRetryWithOptionsAndChroot (or parsing the pacemaker log
entry) to read '.maxRaftIndex' into originalMaxRaftIndex, compute expectedBump
:= int(float64(originalMaxRaftIndex)*0.2) (floor) and then assert configBump ==
expectedBump and journalBump == expectedBump (and still assert > 0); update
references to configBump and journalBump in the test to use
originalMaxRaftIndex-derived expectedBump so the test fails on undersized bumps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a987574-2deb-47e7-993d-9e2e41097947
📒 Files selected for processing (2)
test/extended/two_node/tnf_recovery.gotest/extended/util/nodes.go
|
Scheduling required tests: |
kasturinarra
force-pushed
the
automate_case_86078
branch
from
ec5f331 to
0f343b2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/extended/two_node/tnf_recovery.go (2)
441-441:⚠️ Potential issue | 🔴 CriticalUnfocus the existing spec or this new test never runs.
Because Line 152 still registers
g.FIt, Ginkgo will execute only that focused spec and skip this new kernel-panic scenario. The main coverage added in this PR is effectively dead until the focus is removed.#!/bin/bash # Verify that no focused Ginkgo specs remain in this file. # Expected result: no matches. rg -n --type=go '\b(g\.)?F(It|Describe|Context|When|Specify)\s*\(' test/extended/two_node/tnf_recovery.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/tnf_recovery.go` at line 441, The test file contains a focused Ginkgo spec (g.FIt) which prevents other specs — including the new "should compute etcd revision bump after kernel panic recovery" g.It — from running; remove or change the focused call by replacing g.FIt with g.It (or remove the leading F from any focused g.FDescribe/g.FContext/g.FIt) so no specs remain focused (search for g.FIt, g.FDescribe, g.FContext, g.FWhen, g.FSpecify) and re-run the provided rg check to confirm there are zero focused specs.
520-524:⚠️ Potential issue | 🟠 MajorThe bump assertion is still too weak to verify the formula.
At Line 523, the test only enforces an upper bound from a later
maxRaftIndex. Even a gross underestimate likeconfigBump == 1still passes once the raft index has advanced enough, so this does not actually verifyfloor(maxRaftIndex*0.2). Capture an exact baseline and assert equality against an independently computed expected value instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/tnf_recovery.go` around lines 520 - 524, The current test only checks an upper bound against a later maxRaftIndex so a tiny configBump can still pass; capture a baseline raft index before any growth (e.g. record baselineMaxRaftIndex just after compute_bump_revision runs or immediately before the section that may advance raft index) and compute expectedBump := int(float64(baselineMaxRaftIndex)*0.2) (floor), then replace the two o.Expect checks with a strict equality/assertion that configBump == expectedBump and also assert expectedBump > 0 for safety, using the same context message format that references configBump and baselineMaxRaftIndex; reference variables configBump, maxRaftIndex (baselineMaxRaftIndex) and the compute_bump_revision checkpoint to locate where to capture the baseline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/two_node/tnf_recovery.go`:
- Line 441: The test file contains a focused Ginkgo spec (g.FIt) which prevents
other specs — including the new "should compute etcd revision bump after kernel
panic recovery" g.It — from running; remove or change the focused call by
replacing g.FIt with g.It (or remove the leading F from any focused
g.FDescribe/g.FContext/g.FIt) so no specs remain focused (search for g.FIt,
g.FDescribe, g.FContext, g.FWhen, g.FSpecify) and re-run the provided rg check
to confirm there are zero focused specs.
- Around line 520-524: The current test only checks an upper bound against a
later maxRaftIndex so a tiny configBump can still pass; capture a baseline raft
index before any growth (e.g. record baselineMaxRaftIndex just after
compute_bump_revision runs or immediately before the section that may advance
raft index) and compute expectedBump := int(float64(baselineMaxRaftIndex)*0.2)
(floor), then replace the two o.Expect checks with a strict equality/assertion
that configBump == expectedBump and also assert expectedBump > 0 for safety,
using the same context message format that references configBump and
baselineMaxRaftIndex; reference variables configBump, maxRaftIndex
(baselineMaxRaftIndex) and the compute_bump_revision checkpoint to locate where
to capture the baseline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1338c186-7f95-40d0-a7eb-9645149bcc91
📒 Files selected for processing (2)
test/extended/two_node/tnf_recovery.gotest/extended/util/nodes.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended/util/nodes.go
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-techpreview |
|
@kasturinarra: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/346b6fd0-274c-11f1-8041-051b3757f1e2-0 |
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-degraded-techpreview |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-degraded-techpreview |
|
@kasturinarra: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/de9e9590-2783-11f1-9d76-70d9db5574c0-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-recovery-techpreview |
|
@kasturinarra: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/73a200c0-27a5-11f1-9202-21065d6ba1da-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-dualstack-recovery-techpreview |
|
@eggfoobar: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/2eca2060-27ad-11f1-88a2-24845e4c396f-0 |
|
/test e2e-aws-etcd-certrotation |
|
@kasturinarra: 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. |
4 participants
No description provided.