Update ocdebug to ssh#31216
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughThe kernel-panic recovery test now uses hypervisor-mediated two-hop SSH for post-crash verification. It builds SSH known_hosts, queries survived-node pacemaker/etcd metadata via SSH, and performs SSH-based Eventually assertions on the target node's containers and pacemaker logs. ChangesSSH-based post-crash verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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: 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 `@test/extended/edge_topologies/tnf_recovery.go`:
- Around line 427-438: The test currently only checks that hypervisor config
text exists; add an explicit fast-fail validation after
exutil.GetHypervisorConfig() that verifies required SSH fields
(sshCfg.HypervisorIP, sshCfg.SSHUser, sshCfg.PrivateKeyPath) are non-empty and
that the PrivateKeyPath is readable (e.g., os.Stat or attempt to open) before
constructing core.SSHConfig and calling core.PrepareLocalKnownHostsFile; on
validation failure call g.Skip or o.Expect/fatal with a clear message so the
test fails fast instead of later inside Eventually loops.
🪄 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: 081ff095-daf5-4eea-9d2f-b98cd1acf06f
📒 Files selected for processing (1)
test/extended/edge_topologies/tnf_recovery.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/extended/edge_topologies/tnf_recovery.go (1)
573-585:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
sudoforpodmancommand.Consistent with the previous comment about line 560, this
podman pscommand likely needssudowhen running over SSH as thecoreuser to access system containers.🐛 Proposed fix to add sudo
prevOutput, _, err := core.ExecuteRemoteSSHCommand(targetNodeIP, - "podman ps -a --format '{{.Names}}' | grep -m1 etcd-previous", + "sudo podman ps -a --format '{{.Names}}' | grep -m1 etcd-previous", &hypervisorConfig, localKH, targetRemoteKH)🤖 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 `@test/extended/edge_topologies/tnf_recovery.go` around lines 573 - 585, The SSH command invoking podman in the Eventually block uses "podman ps -a ..." without sudo; update the command string passed to core.ExecuteRemoteSSHCommand (the call in the anonymous func used by o.Eventually) to prefix podman with sudo (e.g., "sudo podman ps -a --format ... | grep -m1 etcd-previous") so the core user can list system containers; keep the rest of the call and error handling (prevOutput, targetNodeIP, hypervisorConfig, localKH, targetRemoteKH, and the expectation message referencing targetNode.Name) unchanged.
🤖 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 `@test/extended/edge_topologies/tnf_recovery.go`:
- Around line 517-519: The SSH reads of /var/lib/etcd in the tnf_recovery test
are missing sudo and can fail with permission denied; update the commands passed
to core.ExecuteRemoteSSHCommand (the grep that sets bumpAmountStr and the jq
that reads revision.json) to prefix them with sudo (and keep the existing sudo
on journalctl) so the reads run as root when executed on RHCOS; ensure you
update both command strings that reference /var/lib/etcd/config.yaml and
/var/lib/etcd/revision.json where survivedNodeIP is used.
---
Duplicate comments:
In `@test/extended/edge_topologies/tnf_recovery.go`:
- Around line 573-585: The SSH command invoking podman in the Eventually block
uses "podman ps -a ..." without sudo; update the command string passed to
core.ExecuteRemoteSSHCommand (the call in the anonymous func used by
o.Eventually) to prefix podman with sudo (e.g., "sudo podman ps -a --format ...
| grep -m1 etcd-previous") so the core user can list system containers; keep the
rest of the call and error handling (prevOutput, targetNodeIP, hypervisorConfig,
localKH, targetRemoteKH, and the expectation message referencing
targetNode.Name) unchanged.
🪄 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: ba461195-f6ea-478f-8ea5-06193e78151a
📒 Files selected for processing (1)
test/extended/edge_topologies/tnf_recovery.go
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 `@test/extended/edge_topologies/tnf_recovery.go`:
- Around line 573-585: The grep used in the Eventually block can match
substrings causing flaky matches; update the command passed to
core.ExecuteRemoteSSHCommand to match the container name exactly (for example
use grep -x '^etcd-previous$' or an exact-name filter) so prevOutput will only
be "etcd-previous"; update the check around prevOutput and the error message in
that block (referencing core.ExecuteRemoteSSHCommand, targetNodeIP, prevOutput,
targetNode.Name) to rely on the exact-match output.
🪄 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: 310c0f00-29a6-4623-a28a-65ac1f84b85f
📒 Files selected for processing (1)
test/extended/edge_topologies/tnf_recovery.go
|
Scheduling required tests: Scheduling tests matching the |
openshift-ci
Bot
added
the
ready-for-human-review
|
@kasturinarra: The following tests 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. |
|
/test e2e-metal-ovn-two-node-fencing-recovery e2e-aws-ovn-fips |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-ipv6-recovery |
|
@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/79bf7300-59a2-11f1-92ea-10dc971041d4-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-recovery |
|
@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/52711730-59b2-11f1-84dc-0186a3c34230-0 |
|
/test e2e-metal-ovn-two-node-fencing-recovery |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-ipv6-recovery |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ovn-two-node-fencing-recovery |
|
@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/6bbea980-59eb-11f1-9890-9e3acead88fd-0 |
|
@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/744ff5e0-59eb-11f1-86c5-e96352f08f90-0 |
1 participant
Summary by CodeRabbit