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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdded a process-wide, mutex-guarded cached helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eggfoobar 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 |
|
/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
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
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 |
|
@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. |
2 participants
currently for two node fencing we do not support extra workers, skipping these tests for DualReplica topology