OSASINFRA-3089 - Add loadBalancerType to Infrastructure API object #1391

EmilienM · 2023-01-26T17:02:59Z

This will add support for external LB in OpenShift API in
the PlatformStatus, for all on-prem platforms which already have
fields for API & Ingress VIPs in PlatformStatus.

See openshift/enhancements#1322

openshift-ci · 2023-01-26T17:03:08Z

Hello @EmilienM! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

EmilienM · 2023-01-26T17:06:02Z

/uncc derekwaynecarr mfojtik

EmilienM · 2023-01-26T17:06:45Z

/cc JoelSpeed jcpowermac cybertron

EmilienM · 2023-01-26T17:16:25Z

/cc pierreprinetti mdbooth

EmilienM · 2023-01-26T17:16:51Z

/assigh JoelSpeed

config/v1/types_infrastructure.go

JoelSpeed · 2023-01-26T17:59:37Z

config/v1/types_infrastructure.go

@@ -123,6 +123,20 @@ const (
 	ExternalTopologyMode TopologyMode = "External"
 )

+// PlatformLoadBalancerType defines the type of load balancer used by the cluster.
+// +kubebuilder:validation:Enum:="";"Internal";"External"


If you are keeping omitempty, you don't need "" in this enum. You should be able to add an integration test to check these validations work as expected, you can add tests in https://github.com/openshift/api/blob/master/config/v1/stable.infrastructure.testsuite.yaml based on the docs https://github.com/openshift/api/tree/master/tests#testing-status

you can add tests

ack but before working on tests I want to make this is a valid API so I don't spend too much time in case this needs a major change.

config/v1/types_infrastructure.go

EmilienM · 2023-01-30T20:15:46Z

/assign @deads2k

Hi David, please take a look at this and our enhancement, as we need this for 4.13 TP.

Thanks

config/v1/types_infrastructure.go

JoelSpeed · 2023-02-07T18:21:00Z

config/v1/techpreview.infrastructure.testsuite.yaml

@@ -12,3 +12,137 @@ tests:
      apiVersion: config.openshift.io/v1
      kind: Infrastructure
      spec: {}
+  onUpdate:


Can you add a test case that checks, from an initial with the UserManaged case, what happens if I try to remove the loadBalancer and it's subkeys from the object? I suspect right now that will be accepted and we will need an additional validation to make sure the field cannot be unset (will need the same for the type within the load balancer as well)

I added this test and... it worked 🤔

To the struct, you'll need to add a validation along the lines of !has(oldSelf.loadBalancer) && !has(self.loadBalancer) || has(oldSelf.loadBalancer) && has(self.loadBalancer) with a message of loadBalancer may only be configured during installation

This means that when the baremetal platform is added for the first time, if it has loadbalancers, great, that's ok, if it doesn't, that's also ok, but once it exists, this validation will run and you won't be able to add loadBalancers to an existing install, and won't be able to remove it (test cases for both of those would be great 🙏 )

I've addressed the comment on the validation. However I think the testsuite is ok, unless I missed something?

eurijon · 2023-02-08T16:10:36Z

/label qe-approved
Checked with @EmilienM that the change is backward compatible by using this install-config:

...
platform:
  openstack:
    cloud: --
    machinesSubnet: --
    apiVIPs:
    - yy
    ingressVIPs:
    - xx
featureSet: TechPreviewNoUpgrade
...

(without setting loadBalancer section)

haproxy and keepalived are correctly deployed in master nodes and the API is fully functional

JoelSpeed · 2023-02-08T17:16:50Z

config/v1/types_infrastructure.go

 // BareMetalPlatformSpec holds the desired state of the BareMetal infrastructure provider.
 // This only includes fields that can be modified in the cluster.
 type BareMetalPlatformSpec struct{}

 // BareMetalPlatformStatus holds the current status of the BareMetal infrastructure provider.
 // For more information about the network architecture used with the BareMetal platform type, see:
 // https://github.com/openshift/installer/blob/master/docs/design/baremetal/networking-infrastructure.md
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.loadBalancer) && !has(self.loadBalancer) || has(oldSelf.loadBalancer) && has(self.loadBalancer)",message="loadBalancer may only be configured during installation"


So reviewing the test cases, I think this may not actually be needed, it seems the defaulting is happening before the validation, so if you remove the entire loadBalancer struct, it then causes the default to kick in, which then is passing validation, that's a little irritating as it means if the value is the default, and someone tries to unset loadBalancer, we can't error and tell them no, you can't do that, the only reason it works is because the default is the same

Can you try a test case with the default value, and unsetting that, what happens? If it passes, and doesn't error, then I can't see a way for this validation to actually be hit, so we may as well remove it

config/v1/types_infrastructure.go

JoelSpeed · 2023-02-08T18:02:25Z

/lgtm

Please get someone from QE to add the qe-approved label

/label docs-approved
/label px-approved

This will add support for Tech Preview external LB in OpenShift API in the PlatformStatus, for all on-prem platforms which already have fields for API & Ingress VIPs in PlatformStatus. See openshift/enhancements#1322

openshift-ci · 2023-02-08T18:34:52Z

@EmilienM: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

JoelSpeed · 2023-02-08T19:28:16Z

/lgtm

openshift-ci · 2023-02-08T19:29:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM, JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from derekwaynecarr and mfojtik January 26, 2023 17:03

openshift-ci bot removed request for mfojtik and derekwaynecarr January 26, 2023 17:06

openshift-ci bot requested review from cybertron, jcpowermac and JoelSpeed January 26, 2023 17:06

openshift-ci bot requested review from mdbooth and pierreprinetti January 26, 2023 17:16

pierreprinetti reviewed Jan 26, 2023

View reviewed changes

config/v1/types_infrastructure.go Show resolved Hide resolved

pierreprinetti reviewed Jan 26, 2023

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

EmilienM commented Jan 26, 2023

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

EmilienM force-pushed the osp-external-lb branch from c1bc951 to f7c9828 Compare January 26, 2023 17:42

EmilienM commented Jan 26, 2023

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

JoelSpeed reviewed Jan 26, 2023

View reviewed changes

EmilienM commented Jan 26, 2023

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

EmilienM force-pushed the osp-external-lb branch from f7c9828 to 403c5da Compare January 26, 2023 18:59

JoelSpeed reviewed Jan 27, 2023

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

mdbooth reviewed Jan 27, 2023

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

EmilienM force-pushed the osp-external-lb branch 4 times, most recently from 05f9b80 to 072a8d0 Compare January 30, 2023 20:14

openshift-ci bot assigned deads2k Jan 30, 2023

JoelSpeed reviewed Feb 7, 2023

View reviewed changes

EmilienM force-pushed the osp-external-lb branch 2 times, most recently from 084a41e to f439128 Compare February 8, 2023 13:59

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2023

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 8, 2023

EmilienM force-pushed the osp-external-lb branch 2 times, most recently from ee72e48 to 9734f98 Compare February 8, 2023 17:10

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2023

JoelSpeed reviewed Feb 8, 2023

View reviewed changes

EmilienM force-pushed the osp-external-lb branch from 9734f98 to 8594d9f Compare February 8, 2023 17:52

JoelSpeed reviewed Feb 8, 2023

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

EmilienM force-pushed the osp-external-lb branch from 8594d9f to 045880c Compare February 8, 2023 18:01

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Feb 8, 2023

openshift-ci bot assigned JoelSpeed Feb 8, 2023

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 8, 2023

OSASINFRA-3089 - Add loadBalancer to Infrastructure API object

a660f60

This will add support for Tech Preview external LB in OpenShift API in the PlatformStatus, for all on-prem platforms which already have fields for API & Ingress VIPs in PlatformStatus. See openshift/enhancements#1322

EmilienM force-pushed the osp-external-lb branch from 045880c to a660f60 Compare February 8, 2023 18:20

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2023

openshift-merge-robot merged commit 068b2ae into openshift:master Feb 8, 2023

EmilienM deleted the osp-external-lb branch February 8, 2023 19:54

patrickdillon mentioned this pull request Jul 17, 2023

Add source of DNS configuration to AWSPlatformStatus #1397

Closed

mkowalski mentioned this pull request Feb 27, 2024

OPNET-466: PlatformLoadBalancer becomes GA for On-Prem #1757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSASINFRA-3089 - Add loadBalancerType to Infrastructure API object #1391

OSASINFRA-3089 - Add loadBalancerType to Infrastructure API object #1391

EmilienM commented Jan 26, 2023 •

edited

openshift-ci bot commented Jan 26, 2023

EmilienM commented Jan 26, 2023

EmilienM commented Jan 26, 2023

EmilienM commented Jan 26, 2023

EmilienM commented Jan 26, 2023

JoelSpeed Jan 26, 2023

EmilienM Jan 26, 2023 •

edited

EmilienM commented Jan 30, 2023

JoelSpeed Feb 7, 2023

EmilienM Feb 7, 2023

JoelSpeed Feb 8, 2023

EmilienM Feb 8, 2023

eurijon commented Feb 8, 2023 •

edited

JoelSpeed Feb 8, 2023

JoelSpeed commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

JoelSpeed commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

OSASINFRA-3089 - Add loadBalancerType to Infrastructure API object #1391

OSASINFRA-3089 - Add loadBalancerType to Infrastructure API object #1391

Conversation

EmilienM commented Jan 26, 2023 • edited

openshift-ci bot commented Jan 26, 2023

EmilienM commented Jan 26, 2023

EmilienM commented Jan 26, 2023

EmilienM commented Jan 26, 2023

EmilienM commented Jan 26, 2023

JoelSpeed Jan 26, 2023

Choose a reason for hiding this comment

EmilienM Jan 26, 2023 • edited

Choose a reason for hiding this comment

EmilienM commented Jan 30, 2023

JoelSpeed Feb 7, 2023

Choose a reason for hiding this comment

EmilienM Feb 7, 2023

Choose a reason for hiding this comment

JoelSpeed Feb 8, 2023

Choose a reason for hiding this comment

EmilienM Feb 8, 2023

Choose a reason for hiding this comment

eurijon commented Feb 8, 2023 • edited

JoelSpeed Feb 8, 2023

Choose a reason for hiding this comment

JoelSpeed commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

JoelSpeed commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

EmilienM commented Jan 26, 2023 •

edited

EmilienM Jan 26, 2023 •

edited

eurijon commented Feb 8, 2023 •

edited