OCPBUGS-82072 OCPBUGS-82077: fix: skip unused tests for two node#30998
OCPBUGS-82072 OCPBUGS-82077: fix: skip unused tests for two node#30998eggfoobar 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
jira/valid-reference
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-82072, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-82077, 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughIntroduce an unexported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-82072, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-82077, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
![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 (2)
test/extended/baremetal/hosts.go (1)
166-175:⚠️ Potential issue | 🔴 CriticalGuard the cleanup against uninitialized helper.
When
skipIfTwoNode(oc)skips the test,helperis never initialized. Ginkgo v2 still executes the AfterEach hook in this case, which callshelper.DeleteAllExtraWorkers()on a nil receiver—causing a panic. SinceDeleteAllExtraWorkersdoesn't guard against nil receivers, either:
- Guard the cleanup:
if helper != nil { helper.DeleteAllExtraWorkers() }, or- Initialize
helperbefore skip checks.This pattern also appears in
test/extended/baremetal/high_availability.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/baremetal/hosts.go` around lines 166 - 175, The AfterEach cleanup calls helper.DeleteAllExtraWorkers() even when helper may be nil because skipIfTwoNode(oc) can skip BeforeEach; update the AfterEach block to guard the call (e.g., if helper != nil { helper.DeleteAllExtraWorkers() }) so DeleteAllExtraWorkers is only invoked on an initialized NewBaremetalTestHelper, and apply the same nil-check change to the corresponding AfterEach in high_availability.go to prevent a panic.test/extended/baremetal/high_availability.go (1)
40-49:⚠️ Potential issue | 🟠 MajorAdd nil-check guard in AfterEach cleanup.
DeleteAllExtraWorkers(helper.go:137) checksif b.extraWorkers == nilbut does not guard against a nil receiver. WhenskipIfTwoNode(oc)skips the test beforehelperis assigned, Ginkgo v2 still executes the AfterEach block, causinghelper.DeleteAllExtraWorkers()to panic on a nil receiver.Guard the cleanup call:
g.AfterEach(func() { if helper != nil { helper.DeleteAllExtraWorkers() } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/baremetal/high_availability.go` around lines 40 - 49, The AfterEach cleanup calls helper.DeleteAllExtraWorkers() without guarding against helper being nil which can panic if Setup was skipped (e.g., due to skipIfTwoNode); update the AfterEach to check the helper receiver before calling DeleteAllExtraWorkers (i.e., ensure helper != nil) so DeleteAllExtraWorkers is only invoked when NewBaremetalTestHelper successfully assigned helper and avoid nil receiver panics from Ginkgo v2 executing AfterEach even when BeforeEach skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/extended/baremetal/high_availability.go`:
- Around line 40-49: The AfterEach cleanup calls helper.DeleteAllExtraWorkers()
without guarding against helper being nil which can panic if Setup was skipped
(e.g., due to skipIfTwoNode); update the AfterEach to check the helper receiver
before calling DeleteAllExtraWorkers (i.e., ensure helper != nil) so
DeleteAllExtraWorkers is only invoked when NewBaremetalTestHelper successfully
assigned helper and avoid nil receiver panics from Ginkgo v2 executing AfterEach
even when BeforeEach skipped.
In `@test/extended/baremetal/hosts.go`:
- Around line 166-175: The AfterEach cleanup calls
helper.DeleteAllExtraWorkers() even when helper may be nil because
skipIfTwoNode(oc) can skip BeforeEach; update the AfterEach block to guard the
call (e.g., if helper != nil { helper.DeleteAllExtraWorkers() }) so
DeleteAllExtraWorkers is only invoked on an initialized NewBaremetalTestHelper,
and apply the same nil-check change to the corresponding AfterEach in
high_availability.go to prevent a panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2c27ace-b83b-4b86-a6db-63b0d16ca46e
📒 Files selected for processing (3)
test/extended/baremetal/common.gotest/extended/baremetal/high_availability.gotest/extended/baremetal/hosts.go
eggfoobar
force-pushed
the
skip-baremetal-worker-checks-on-twonode
branch
from
ddd95d6 to
b9669ac
Compare
|
/payload-job |
|
@eggfoobar: trigger 3 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/7df390b0-350c-11f1-8c63-6c1fd6462c30-0 |
|
Scheduling required tests: Scheduling tests matching the |
|
/verified by https://prow.ci.openshift.org/view/gs/test-platform-results/logs/openshift-origin-30998-nightly-4.22-e2e-metal-ovn-two-node-fencing-serial-techpreview-1of3/2042673644018077696 and https://prow.ci.openshift.org/view/gs/test-platform-results/logs/openshift-origin-30998-nightly-4.22-e2e-metal-ovn-two-node-fencing-serial-techpreview-2of3/2042673644550754304 Verified by CI lanes on TNF clusters 1of3 shows the skip 2of3 shows success |
openshift-ci-robot
added
the
verified
|
@eggfoobar: This PR has been marked as verified by 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eggfoobar, jaypoulz 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 |
|
/retest |
eggfoobar
force-pushed
the
skip-baremetal-worker-checks-on-twonode
branch
from
b9669ac to
c1405f1
Compare
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
/payload-job |
openshift-ci-robot
added
jira/invalid-bug
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-82072, which is invalid:
Comment This pull request references Jira Issue OCPBUGS-82077, which is invalid:
Comment 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. |
|
@eggfoobar: trigger 3 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/5dbfd570-3a6f-11f1-818d-e8389f86f951-0 |
|
/jira refresh |
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-82072, which is invalid:
Comment This pull request references Jira Issue OCPBUGS-82077, which is invalid:
Comment 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. |
|
/jira refresh |
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-82072, which is valid. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-82077, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@eggfoobar: This pull request references Jira Issue OCPBUGS-82072, which is valid. 3 validation(s) were run on this bug
This pull request references Jira Issue OCPBUGS-82077, which is valid. 3 validation(s) were run on this bug
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. |
|
Scheduling required tests: Scheduling tests matching the |
|
/retest |
|
Job Failure Risk Analysis for sha: c1405f1
|
|
/retest |
|
|
||
| // clusterInfrastructure returns the cluster Infrastructure object. A successful fetch is cached for | ||
| // the rest of the process; if Get fails, nothing is cached and the next call retries. | ||
| func clusterInfrastructure(oc *exutil.CLI) (*configv1.Infrastructure, error) { |
There was a problem hiding this comment.
Why do you need such approach? It looks pretty fragile and overcomplicating/invalidating the underlying cache. I don't have all the details, but it looks like a different problem was maybe found? If so, I'd suggest reviewing it and avoiding locks to introduce a potentially stale behavior, and fetch the required resource via oc as usul
There was a problem hiding this comment.
Oh sure thing, this was just an optimization step since it seemed redundant to make the call 3 times for the same data that does not change during the run of the suite. Removing
currently for two node fencing we do not support extra workers, skipping these tests for DualReplica topology updated test to account for correct baremetal operator state during tnf deployments Signed-off-by: ehila <ehila@redhat.com>
eggfoobar
force-pushed
the
skip-baremetal-worker-checks-on-twonode
branch
from
c1405f1 to
e100fab
Compare
|
/payload-job |
|
@eggfoobar: trigger 3 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/9853c8a0-3da2-11f1-999b-95d30e32b69f-0 |
|
Scheduling required tests: Scheduling tests matching the |
|
@eggfoobar: 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. |
|
Job Failure Risk Analysis for sha: e100fab
|
5 participants
currently for two node fencing we do not support extra workers, skipping these tests for DualReplica topology
Summary by CodeRabbit