OCPEDGE-2307: Update Arbiter Node tests based on platform type#30823
OCPEDGE-2307: Update Arbiter Node tests based on platform type#30823vimauro wants to merge 4 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@vimauro: This pull request references OCPEDGE-2307 which is a valid jira issue. 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. |
WalkthroughReworked arbiter topology test to initialize platform-specific pod expectations at runtime. Added Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@vimauro: This pull request references OCPEDGE-2307 which is a valid jira issue. 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.
Actionable comments posted: 1
🤖 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/arbiter_topology.go`:
- Around line 56-64: The BeforeSuite reads infra.Status.PlatformStatus.Type
without checking for nil; update the g.BeforeSuite initialization to guard
against a nil PlatformStatus by using infra.Status.PlatformStatus.Type when
infra.Status.PlatformStatus != nil, otherwise fall back to infra.Status.Platform
(or infra.Status.PlatformType) to derive platformType, then continue to
lowercase it and lookup expectedPodsByPlatform; reference the
variables/functions infra, PlatformStatus, Platform, platformType,
expectedPodsByPlatform and expectedPods to locate and apply the change.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/two_node/arbiter_topology.go
|
@vimauro: This pull request references OCPEDGE-2307 which is a valid jira issue. 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/two_node/arbiter_topology.go (1)
58-67:⚠️ Potential issue | 🔴 CriticalGuard
PlatformStatusbefore dereferencingTypeto avoid panic.At Line 62,
infra.Status.PlatformStatus.Typeis accessed without checking whetherPlatformStatusis nil. That can panic and fail the spec setup path. Please fall back toinfra.Status.PlatformwhenPlatformStatusis nil.Proposed fix
func initExpectedPods(oc *exutil.CLI) { expectedPodsOnce.Do(func() { infra, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{}) o.Expect(err).To(o.BeNil(), "Expected to retrieve infrastructure details without error") - platformType := strings.ToLower(string(infra.Status.PlatformStatus.Type)) + platformType := strings.ToLower(string(infra.Status.Platform)) + if infra.Status.PlatformStatus != nil { + platformType = strings.ToLower(string(infra.Status.PlatformStatus.Type)) + } var ok bool expectedPods, ok = expectedPodsByPlatform[platformType] o.Expect(ok).To(o.BeTrue(), "Expected to find expected pods for platform %s", platformType) }) }#!/bin/bash set -euo pipefail # Verify InfrastructureStatus fields and pointer semantics. rg -n --type go 'type InfrastructureStatus struct|PlatformStatus\s+\*PlatformStatus|Platform\s+PlatformType' # Verify the dereference site in this file. rg -n --type go -C2 'PlatformStatus\.Type' test/extended/two_node/arbiter_topology.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/arbiter_topology.go` around lines 58 - 67, The code in initExpectedPods dereferences infra.Status.PlatformStatus.Type without guarding PlatformStatus; update initExpectedPods to check if infra.Status.PlatformStatus != nil and use infra.Status.PlatformStatus.Type when present, otherwise fall back to infra.Status.Platform, then normalize to lowercase as before and lookup expectedPodsByPlatform; ensure you still call expectedPodsOnce.Do and keep the same expectations (o.Expect) around the lookup (referencing initExpectedPods, expectedPodsOnce, infra.Status.PlatformStatus.Type and infra.Status.Platform).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/two_node/arbiter_topology.go`:
- Around line 58-67: The code in initExpectedPods dereferences
infra.Status.PlatformStatus.Type without guarding PlatformStatus; update
initExpectedPods to check if infra.Status.PlatformStatus != nil and use
infra.Status.PlatformStatus.Type when present, otherwise fall back to
infra.Status.Platform, then normalize to lowercase as before and lookup
expectedPodsByPlatform; ensure you still call expectedPodsOnce.Do and keep the
same expectations (o.Expect) around the lookup (referencing initExpectedPods,
expectedPodsOnce, infra.Status.PlatformStatus.Type and infra.Status.Platform).
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/two_node/arbiter_topology.go
|
Scheduling required tests: |
eggfoobar
left a comment
eggfoobar
left a comment
There was a problem hiding this comment.
Looks great, small suggestion on which keys to use
|
@vimauro: This pull request references OCPEDGE-2307 which is a valid jira issue. 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/two_node/arbiter_topology.go (1)
58-66:⚠️ Potential issue | 🔴 CriticalCritical:
platformTypeis undefined andPlatformStatusis dereferenced unsafely.Line 65 won’t compile (
platformTypeis not declared), and Line 64 can panic wheninfra.Status.PlatformStatus == nil.🔧 Proposed fix
func initExpectedPods(oc *exutil.CLI) { expectedPodsOnce.Do(func() { infra, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{}) o.Expect(err).To(o.BeNil(), "Expected to retrieve infrastructure details without error") + platformType := infra.Status.Platform + if infra.Status.PlatformStatus != nil { + platformType = infra.Status.PlatformStatus.Type + } + var ok bool - expectedPods, ok = expectedPodsByPlatform[infra.Status.PlatformStatus.Type] + expectedPods, ok = expectedPodsByPlatform[platformType] o.Expect(ok).To(o.BeTrue(), "Expected to find expected pods for platform %s", platformType) }) }#!/bin/bash # Read-only verification: confirm unsafe dereference and undefined identifier usage in initializer. rg -n -C3 'func initExpectedPods|PlatformStatus\.Type|platformType' test/extended/two_node/arbiter_topology.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/arbiter_topology.go` around lines 58 - 66, The initExpectedPods function references an undefined platformType and unsafely dereferences infra.Status.PlatformStatus; update initExpectedPods to first check that infra.Status.PlatformStatus is non-nil, then assign platformType := infra.Status.PlatformStatus.Type, and use that variable in the lookup expectedPods, ok = expectedPodsByPlatform[platformType] and in the o.Expect message; if PlatformStatus is nil, fail the expectation with a clear error (e.g., o.Expect(infra.Status.PlatformStatus).ToNot(o.BeNil(), "Expected Infrastructure PlatformStatus to be set")) before accessing Type.
🧹 Nitpick comments (2)
test/extended/two_node/arbiter_topology.go (2)
26-51: Reduce duplication in per-platform expected pod maps.The two platform maps are nearly identical; extracting a shared base and applying per-platform overrides will lower drift risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/arbiter_topology.go` around lines 26 - 51, The two maps under expectedPodsByPlatform are almost identical; define a single base map (e.g., baseExpectedPods map[string]int) containing the common entries, then build expectedPodsByPlatform using copies of that base for v1.BareMetalPlatformType and v1.NonePlatformType, applying per-platform overrides (for example add "openshift-kni-infra": 1 to the BareMetal copy). Ensure you copy the base map when assigning to each platform (do not reuse the same map reference) so subsequent mutations won't affect other entries; update references to expectedPodsByPlatform accordingly.
59-66: Avoido.Expect(...)insidesync.Once.Do(...)initializer.If an assertion panics once,
sync.Oncecan still be marked done, leavingexpectedPodsunset for subsequent specs in the same process.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/arbiter_topology.go` around lines 59 - 66, The current sync.Once initializer (expectedPodsOnce.Do) performs test assertions via o.Expect which can panic and leave the Once marked done without setting expectedPods; change the initializer to only perform non-panic assignments and capture any error into a local variable (e.g. initErr) and the platform type into a local variable, then after expectedPodsOnce.Do(...) run o.Expect(initErr).To(o.BeNil(), ...) and assert that expectedPods was found (using the captured platform) outside the Do block; specifically update the expectedPodsOnce.Do closure to set infra, initErr, platformType and expectedPods from expectedPodsByPlatform (setting an ok flag) but do not call o.Expect inside the closure, then perform the o.Expect checks for err and ok immediately after the Do call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/two_node/arbiter_topology.go`:
- Around line 58-66: The initExpectedPods function references an undefined
platformType and unsafely dereferences infra.Status.PlatformStatus; update
initExpectedPods to first check that infra.Status.PlatformStatus is non-nil,
then assign platformType := infra.Status.PlatformStatus.Type, and use that
variable in the lookup expectedPods, ok = expectedPodsByPlatform[platformType]
and in the o.Expect message; if PlatformStatus is nil, fail the expectation with
a clear error (e.g., o.Expect(infra.Status.PlatformStatus).ToNot(o.BeNil(),
"Expected Infrastructure PlatformStatus to be set")) before accessing Type.
---
Nitpick comments:
In `@test/extended/two_node/arbiter_topology.go`:
- Around line 26-51: The two maps under expectedPodsByPlatform are almost
identical; define a single base map (e.g., baseExpectedPods map[string]int)
containing the common entries, then build expectedPodsByPlatform using copies of
that base for v1.BareMetalPlatformType and v1.NonePlatformType, applying
per-platform overrides (for example add "openshift-kni-infra": 1 to the
BareMetal copy). Ensure you copy the base map when assigning to each platform
(do not reuse the same map reference) so subsequent mutations won't affect other
entries; update references to expectedPodsByPlatform accordingly.
- Around line 59-66: The current sync.Once initializer (expectedPodsOnce.Do)
performs test assertions via o.Expect which can panic and leave the Once marked
done without setting expectedPods; change the initializer to only perform
non-panic assignments and capture any error into a local variable (e.g. initErr)
and the platform type into a local variable, then after expectedPodsOnce.Do(...)
run o.Expect(initErr).To(o.BeNil(), ...) and assert that expectedPods was found
(using the captured platform) outside the Do block; specifically update the
expectedPodsOnce.Do closure to set infra, initErr, platformType and expectedPods
from expectedPodsByPlatform (setting an ok flag) but do not call o.Expect inside
the closure, then perform the o.Expect checks for err and ok immediately after
the Do call.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/two_node/arbiter_topology.go
|
/lgtm |
openshift-ci
bot
added
lgtm
|
/retest |
|
@vimauro: This pull request references OCPEDGE-2307 which is a valid jira issue. 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/two_node/arbiter_topology.go (1)
58-66:⚠️ Potential issue | 🔴 CriticalGuard
PlatformStatusbefore dereferencingType(panic risk).At Line 64,
infra.Status.PlatformStatus.Typeis dereferenced unconditionally.PlatformStatuscan be nil, which can panic during setup and fail the suite.🔧 Proposed fix
func initExpectedPods(oc *exutil.CLI) { expectedPodsOnce.Do(func() { infra, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{}) o.Expect(err).To(o.BeNil(), "Expected to retrieve infrastructure details without error") + platformType := infra.Status.Platform + if infra.Status.PlatformStatus != nil { + platformType = infra.Status.PlatformStatus.Type + } + var ok bool - expectedPods, ok = expectedPodsByPlatform[infra.Status.PlatformStatus.Type] - o.Expect(ok).To(o.BeTrue(), "Expected to find expected pods for platform %s", infra.Status.PlatformStatus.Type) + expectedPods, ok = expectedPodsByPlatform[platformType] + o.Expect(ok).To(o.BeTrue(), "Expected to find expected pods for platform %s", platformType) }) }#!/bin/bash # Verify InfrastructureStatus field types and optionality, and current dereference site. set -euo pipefail echo "== InfrastructureStatus / PlatformStatus definitions ==" rg -n -C3 'type InfrastructureStatus struct|PlatformStatus \*PlatformStatus|Platform\s+PlatformType' --type go echo echo "== Current dereference in arbiter_topology.go ==" rg -n -C2 'PlatformStatus\.Type|expectedPodsByPlatform\[' test/extended/two_node/arbiter_topology.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/arbiter_topology.go` around lines 58 - 66, The code dereferences infra.Status.PlatformStatus.Type inside initExpectedPods without guarding for a nil PlatformStatus which can panic; update initExpectedPods to first check infra.Status.PlatformStatus for nil and fail the test with a clear o.Expect message if it's nil, otherwise read the platform into a local variable (e.g. platform := infra.Status.PlatformStatus.Type) and use that key when looking up expectedPodsByPlatform and assigning expectedPods; ensure the error message in the o.Expect(ok) includes the platform variable and that the nil-PlatformStatus case is explicitly handled to avoid a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/two_node/arbiter_topology.go`:
- Around line 58-66: The code dereferences infra.Status.PlatformStatus.Type
inside initExpectedPods without guarding for a nil PlatformStatus which can
panic; update initExpectedPods to first check infra.Status.PlatformStatus for
nil and fail the test with a clear o.Expect message if it's nil, otherwise read
the platform into a local variable (e.g. platform :=
infra.Status.PlatformStatus.Type) and use that key when looking up
expectedPodsByPlatform and assigning expectedPods; ensure the error message in
the o.Expect(ok) includes the platform variable and that the nil-PlatformStatus
case is explicitly handled to avoid a panic.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/extended/two_node/arbiter_topology.go
|
/label tide/merge-method-squash |
openshift-ci
bot
added
tide/merge-method-squash
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eggfoobar, vimauro The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Scheduling required tests: |
|
/retest |
|
@vimauro: The 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. |
|
/verified by @vimauro via CI |
openshift-ci-robot
added
the
verified
|
@vimauro: 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. |
|
/retest |
|
looks like there is currently a known issue in CI preventing the last test to succeeds. This has been already reported on Slack.
|
|
/test images |
|
@vimauro: The following test 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. |
3 participants
Update Arbiter Node tests based on platform type since we are adding a CI lane for
platform:nonehere openshift/release#75055Summary by CodeRabbit