AGENT-626: Allow UserManaged LoadBalancer on baremetal/vsphere platforms#10558
AGENT-626: Allow UserManaged LoadBalancer on baremetal/vsphere platforms#10558zaneb wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@zaneb: This pull request references AGENT-626 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughPropagate platform LoadBalancer into AgentClusterInstall for BareMetal and vSphere, add a mapping helper for load balancer types, remove the obsolete vSphere agent warning, and remove TechPreview notes from LoadBalancer field docs and CRD descriptions. ChangesLoadBalancer graduation and agent support
🎯 3 (Moderate) | ⏱️ ~20 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)
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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
CI jobs couldn't obtain a machine to run on. |
|
/retest |
|
/lgtm |
Map the LoadBalancer type from platform install-config (baremetal, vsphere) through to the AgentClusterInstall spec. The configv1 PlatformLoadBalancerType is converted to the hiveext LoadBalancerType (UserManaged -> UserManaged, OpenShiftManagedDefault -> ClusterManaged). Remove the "is ignored" warning for LoadBalancer on the vsphere platform, since the field is now plumbed through. Assisted-by: Claude Code
The LoadBalancer feature was promoted to GA for baremetal and vsphere in 4.16 (OPNET-476). The feature gate check was removed by f5811d9 but the "available in TechPreview" comments in the platform type definitions were left behind. Assisted-by: Claude Code
|
Now with unit tests. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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.
🧹 Nitpick comments (1)
pkg/asset/agent/manifests/agentclusterinstall_test.go (1)
165-193: ⚡ Quick winAdd an explicit
OpenShiftManagedDefault -> ClusterManagedmapping test case.Great coverage for
UserManaged. Since this PR also introducesOpenShiftManagedDefault -> ClusterManaged, add at least one explicit case (ideally for both BareMetal and vSphere) so both mapper branches are locked by tests.Proposed test extension
+ installConfigBaremetalClusterManagedLB := getValidOptionalInstallConfig() + installConfigBaremetalClusterManagedLB.Config.Platform.BareMetal.LoadBalancer = &configv1.BareMetalPlatformLoadBalancer{ + Type: configv1.LoadBalancerTypeOpenShiftManagedDefault, + } + + goodBaremetalClusterManagedLBACI := getGoodACI() + goodBaremetalClusterManagedLBACI.Spec.LoadBalancer = &hiveext.LoadBalancer{ + Type: hiveext.LoadBalancerTypeClusterManaged, + } ... + { + name: "valid configuration baremetal cluster-managed load balancer", + dependencies: []asset.Asset{ + &workflow.AgentWorkflow{Workflow: workflow.AgentWorkflowTypeInstall}, + installConfigBaremetalClusterManagedLB, + &agentconfig.AgentHosts{}, + &agentconfig.AgentConfig{}, + }, + expectedConfig: goodBaremetalClusterManagedLBACI, + },Also applies to: 370-389
🤖 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 `@pkg/asset/agent/manifests/agentclusterinstall_test.go` around lines 165 - 193, Add explicit test cases mirroring the existing UserManaged ones that exercise the new OpenShiftManagedDefault -> ClusterManaged mapping: create an install config via getValidOptionalInstallConfig and set its Platform.BareMetal.LoadBalancer.Type to the OpenShiftManagedDefault variant (and a separate one for VSphere.Platform.LoadBalancer.Type), then create corresponding ACI objects via getGoodACI but set expected fields (e.g., goodBaremetal...ACI and goodVSphere...ACI) so the test asserts the mapper converts OpenShiftManagedDefault to the hiveext.LoadBalancerTypeClusterManaged result; place these new cases alongside the existing goodBaremetalUserManagedLBACI and goodVSphereUserManagedLBACI cases to lock both mapper branches.
🤖 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.
Nitpick comments:
In `@pkg/asset/agent/manifests/agentclusterinstall_test.go`:
- Around line 165-193: Add explicit test cases mirroring the existing
UserManaged ones that exercise the new OpenShiftManagedDefault -> ClusterManaged
mapping: create an install config via getValidOptionalInstallConfig and set its
Platform.BareMetal.LoadBalancer.Type to the OpenShiftManagedDefault variant (and
a separate one for VSphere.Platform.LoadBalancer.Type), then create
corresponding ACI objects via getGoodACI but set expected fields (e.g.,
goodBaremetal...ACI and goodVSphere...ACI) so the test asserts the mapper
converts OpenShiftManagedDefault to the hiveext.LoadBalancerTypeClusterManaged
result; place these new cases alongside the existing
goodBaremetalUserManagedLBACI and goodVSphereUserManagedLBACI cases to lock both
mapper branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7179b50f-a44e-41fd-94a2-a72ef08b1304
📒 Files selected for processing (8)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/agent/installconfig.gopkg/asset/agent/manifests/agentclusterinstall.gopkg/asset/agent/manifests/agentclusterinstall_test.gopkg/types/baremetal/platform.gopkg/types/nutanix/platform.gopkg/types/ovirt/platform.gopkg/types/vsphere/platform.go
💤 Files with no reviewable changes (5)
- pkg/types/nutanix/platform.go
- pkg/types/vsphere/platform.go
- pkg/types/baremetal/platform.go
- pkg/types/ovirt/platform.go
- pkg/asset/agent/installconfig.go
✅ Files skipped from review due to trivial changes (1)
- data/data/install.openshift.io_installconfigs.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/asset/agent/manifests/agentclusterinstall.go
|
/lgtm |
|
@zaneb: 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. |
3 participants
Map the LoadBalancer type from platform install-config (baremetal,
vsphere) through to the AgentClusterInstall spec. The configv1
PlatformLoadBalancerType is converted to the hiveext
LoadBalancerType (UserManaged -> UserManaged,
OpenShiftManagedDefault -> ClusterManaged).
Remove the "is ignored" warning for LoadBalancer on the vsphere
platform, since the field is now plumbed through.
Note that in Assisted Service, the UserManaged LoadBalancer is not currently supported with Nutanix.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests