OCPEDGE-2381: Add test to verify for backup container exists when etcd crashes#30922
OCPEDGE-2381: Add test to verify for backup container exists when etcd crashes#30922kasturinarra wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds a disruptive Ginkgo test suite that simulates ungraceful reboot recovery on a dual-replica cluster: it removes the etcd pod manifest, reboots the target node, validates etcd membership transitions (learner → learner started → promoted to voting), checks etcd container/runtime artifacts, and inspects pacemaker logs for pod recreation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 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 |
kasturinarra
changed the title
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/extended/two_node/tnf_recovery.go (1)
475-480: Normalize shell output before asserting container-running state.Line 479 compares exact raw output to
"'true'". This is fragile against trailing newlines/format changes from shell commands.Stabilize assertion
- o.Expect(got).To(o.Equal("'true'"), fmt.Sprintf("expected etcd container running on %s", targetNode.Name)) + o.Expect(strings.TrimSpace(got)).To( + o.Or(o.Equal("true"), o.Equal("'true'")), + fmt.Sprintf("expected etcd container running on %s", targetNode.Name), + )🤖 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 475 - 480, The test is asserting raw shell output equals "'true'" which is brittle; update the check around the call to exutil.DebugNodeRetryWithOptionsAndChroot (and the got variable from ensurePodmanEtcdContainerIsRunning) to normalize output first—apply strings.TrimSpace and strip any surrounding quotes (e.g., strings.Trim(got, `"'`)) and then assert the normalized value equals "true" so trailing newlines or quote variations won't break the assertion.
🤖 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 442-446: The current approach captures a human-readable timestamp
via exutil.DebugNodeRetryWithOptionsAndChroot into timestampStr/rebootTimestamp
and then lexically filters pacemaker logs, which is brittle; change the
timestamp capture to epoch seconds (e.g., use date +%s via
DebugNodeRetryWithOptionsAndChroot to populate a numeric rebootEpoch variable)
and then filter pacemaker/journal logs numerically (e.g., use journalctl
--since=@<epoch> or compare numeric epoch fields in awk) instead of doing string
comparisons against full log lines; update all uses that reference
rebootTimestamp (and the later awk-based filter logic) to use the epoch-based
numeric filter so events remain correctly ordered across time boundaries.
- Around line 447-450: The test currently uses rm -f which masks a missing
/var/lib/etcd/pod.yaml and weakens the precondition; update the removal step in
tnf_recovery.go to first assert the file exists on the target node (use
exutil.DebugNodeRetryWithOptionsAndChroot with a "test -f
/var/lib/etcd/pod.yaml" or "stat" check against targetNode.Name) and fail the
test if it is missing, then run the remove command (without -f) to delete it and
keep the existing o.Expect check (or adjust the error message) so the test
verifies recreation after an actual deletion.
- Around line 414-436: The new duplicate Describe block titled "Two Node with
Fencing etcd recovery" must be removed and its It(...) test moved into the
existing Describe block that already contains the BeforeEach setup to avoid
duplicated setup and missing feature-gate labeling; update the target Describe
(the existing var _ = g.Describe(...) that includes BeforeEach and
utils.SkipIfNotTopology) to include the new It(...) and ensure the Describe
retains the OCPFeatureGate:DualReplica label so the test keeps the same label
set and topology gating (locate the duplicate Describe, the BeforeEach function,
and the new It(...) and merge the It into the original Describe rather than
creating a second Describe).
---
Nitpick comments:
In `@test/extended/two_node/tnf_recovery.go`:
- Around line 475-480: The test is asserting raw shell output equals "'true'"
which is brittle; update the check around the call to
exutil.DebugNodeRetryWithOptionsAndChroot (and the got variable from
ensurePodmanEtcdContainerIsRunning) to normalize output first—apply
strings.TrimSpace and strip any surrounding quotes (e.g., strings.Trim(got,
`"'`)) and then assert the normalized value equals "true" so trailing newlines
or quote variations won't break the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4369e033-00f8-494d-b09f-a9506ad9e10c
📒 Files selected for processing (1)
test/extended/two_node/tnf_recovery.go
|
Scheduling required tests: |
kasturinarra
force-pushed
the
automate_case_86079
branch
from
5d285bf to
560c591
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/two_node/tnf_recovery.go (1)
477-479: Fragile command argument parsing withstrings.Split.Using
strings.Split(ensurePodmanEtcdContainerIsRunning, " ")...to construct command arguments is fragile. If the command contains quoted strings, paths with spaces, or complex shell expressions, splitting by space will produce incorrect arguments.Consider defining the command as a slice directly or using
bash -cwith the full command string (as done elsewhere in this file).♻️ Suggested alternative approach
- got, err := exutil.DebugNodeRetryWithOptionsAndChroot(oc, targetNode.Name, "openshift-etcd", - strings.Split(ensurePodmanEtcdContainerIsRunning, " ")...) + got, err := exutil.DebugNodeRetryWithOptionsAndChroot(oc, targetNode.Name, "openshift-etcd", + "bash", "-c", "podman inspect --format '{{.State.Running}}' etcd")🤖 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 477 - 479, The command argument construction using strings.Split(ensurePodmanEtcdContainerIsRunning, " ") is fragile; update the call site that invokes ensurePodmanEtcdContainerIsRunning so it either (a) passes a pre-built []string of arguments instead of splitting the string, or (b) executes the full string via a shell wrapper like "bash -c" with the entire ensurePodmanEtcdContainerIsRunning string as a single argument; locate the usage around the Expect(got).To(o.Equal("'true'")) assertion and replace the strings.Split(...) spread with one of these safer approaches (refer to the ensurePodmanEtcdContainerIsRunning symbol and the surrounding command execution code to apply the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/two_node/tnf_recovery.go`:
- Around line 477-479: The command argument construction using
strings.Split(ensurePodmanEtcdContainerIsRunning, " ") is fragile; update the
call site that invokes ensurePodmanEtcdContainerIsRunning so it either (a)
passes a pre-built []string of arguments instead of splitting the string, or (b)
executes the full string via a shell wrapper like "bash -c" with the entire
ensurePodmanEtcdContainerIsRunning string as a single argument; locate the usage
around the Expect(got).To(o.Equal("'true'")) assertion and replace the
strings.Split(...) spread with one of these safer approaches (refer to the
ensurePodmanEtcdContainerIsRunning symbol and the surrounding command execution
code to apply the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbe21470-c466-4c79-9601-acec05174456
📒 Files selected for processing (1)
test/extended/two_node/tnf_recovery.go
No update needed. The strings.Split(ensurePodmanEtcdContainerIsRunning, " ") pattern is the established convention in this codebase — tnf_topology.go:134 uses the exact |
|
Scheduling required tests: |
|
@kasturinarra: all tests passed! 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. |
|
/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/e97cdf30-2783-11f1-8365-3f97cbfc1ca9-0 |
2 participants
No description provided.