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 due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded a process-wide, mutex-guarded cached helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 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 |
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| if infra.Status.ControlPlaneTopology == configv1.DualReplicaTopologyMode { | ||
| operationalStatus = "detached" |
There was a problem hiding this comment.
I don't quite understand this part. We definitely don't expect BMHs to spontaneously get detached.
There was a problem hiding this comment.
Hey @dtantsur, so this is a unique state to two node with fencing, if we grab the result from our serial run for TNF lanes we set it up as detached
$ curl -sL https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/openshift-origin-30998-nightly-4.22-e2e-metal-ovn-two-node-fencing-serial-techpreview-2of3/2042673644550754304/artifacts/e2e-metal-ovn-two-node-fencing-serial-techpreview/gather-extra/artifacts/inspect/namespaces/openshift-machine-api/metal3.io/baremetalhosts/ostest-master-0.yaml | yq '.status.operationalStatus'
detached
There was a problem hiding this comment.
It would be more fun if it was spontaneous, but here it is in the bootstrap code:
https://github.com/openshift/installer/blob/075b809c328f2534fdecff79c03ec13e8aafd19a/data/data/bootstrap/baremetal/files/usr/local/bin/master-bmh-update.sh#L29
There was a problem hiding this comment.
Could a detailed explanation please be added here? It's already not great that we're going to see confused customers asking why BMH doesn't work for them... At the very least, let's add breadcrumps here.
There was a problem hiding this comment.
That's fair @dtantsur , I added the info as by statement so it's easier to see in our CI logs.
There was a problem hiding this comment.
@dtantsur if we ever support compute nodes, they won't be detached by default, so hopefully that's a richer value proposition. But yeah, we should be clear about this in the test.
|
/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: 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. |
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
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 |
4 participants
currently for two node fencing we do not support extra workers, skipping these tests for DualReplica topology
Summary by CodeRabbit