OPNET-351: Extend infra CR to store VIPs and MachineNetwork #1593

mkowalski · 2023-09-20T13:36:52Z

In order to enable work around mutable VIPs we are extending the Infrastructure CR to hold VIPs in the spec and not only in the status.

In order to be able to perform proper validations (which currently only happen in o/installer as VIPs are immutable) we are storing machine networks alongside.

Implements: OPNET-351
Contributes-to: OPNET-80
Contributes-to: OPNET-340

openshift-ci-robot · 2023-09-20T13:36:56Z

@mkowalski: This pull request references OPNET-351 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 story to target the "4.15.0" version, but no target version was set.

In response to this:

In order to enable work around mutable VIPs we are extending the Infrastructure CR to hold VIPs in the spec and not only in the status.

In order to be able to perform proper validations (which currently only happen in o/installer as VIPs are immutable) we are storing machine networks alongside.

Implements: OPNET-351
Contributes-to: OPNET-80
Contributes-to: OPNET-340

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.

openshift-ci-robot · 2023-09-20T13:37:59Z

@mkowalski: This pull request references OPNET-351 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 story to target the "4.15.0" version, but no target version was set.

In response to this:

In order to enable work around mutable VIPs we are extending the Infrastructure CR to hold VIPs in the spec and not only in the status.

In order to be able to perform proper validations (which currently only happen in o/installer as VIPs are immutable) we are storing machine networks alongside.

Implements: OPNET-351
Contributes-to: OPNET-80
Contributes-to: OPNET-340

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.

openshift-ci · 2023-09-20T13:38:22Z

Hello @mkowalski! 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.

mkowalski · 2023-09-20T14:29:38Z

/cc @cybertron
Baremetal

/cc @MaysaMacedo
OpenStack

/cc @rvanderp3
VSphere

mkowalski · 2023-09-20T14:53:31Z

/retest

Prow is choking

JoelSpeed

IIUC these fields are copying from status fields? What happens if there's a diff between spec and status, you need a controller to reconcile the difference, validate and accept the change, what is going to do that?

config/v1/types_infrastructure.go

MaysaMacedo · 2023-10-20T19:13:35Z

config/v1/0000_10_config-operator_01_infrastructure-CustomNoUpgrade.crd.yaml

+                          maxItems: 2
+                          type: array
+                          x-kubernetes-list-type: set
+                        machineNetworks:


Is machineNetworks required once apiServerInternalIPs and ingressIPs are set? If yes, should that be documented/verified?

No it is not because we need to support upgraded clusters. As the only source of this information is install-config.yaml to which Cluster Network Operator has no access, it can be only o/installer who sets this field during installation time.

So for newly installed 4.15 clusters we will populate this field and for clusters that originated in 4.14 or older this field will remain empty.

Also I know about at least one platform which users do not know the concept of Machine Network even though they use API/Ingress VIPs. I do not know how it started, but at some point someone started setting a catch-all CIDR as machine network for them so for a while we had clusters with something like 0.0.0.0/0 as machine network. This never failed because no component in OCP really used this subnet, but it's to give you the idea of how people misuse or ignore this parameter.

mkowalski · 2023-10-24T08:04:23Z

Hey folks, can we get approve+lgtm on this unless there are more changes requested? That would unblock the works in o/installer and CNO

MaysaMacedo · 2023-10-24T17:11:59Z

/lgtm
For the OpenStack part

mkowalski · 2023-10-31T15:24:23Z

/retest-required

mkowalski · 2023-10-31T16:57:17Z

/test e2e-aws-ovn

Not my failure

level=info msg=Creating infrastructure resources...
level=error
level=error msg=Error: no matching EC2 Subnet found

mkowalski · 2023-11-01T08:53:49Z

/test e2e-aws-ovn

Failure of unrelated test

JoelSpeed · 2023-11-10T18:41:02Z

config/v1/types_infrastructure.go

@@ -1176,6 +1291,11 @@ type VSpherePlatformStatus struct {
 	// +openshift:enable:FeatureSets=CustomNoUpgrade;TechPreviewNoUpgrade
 	// +optional
 	LoadBalancer *VSpherePlatformLoadBalancer `json:"loadBalancer,omitempty"`
+
+	// machineNetworks are IP networks used to connect all the OpenShift cluster nodes.


How many networks are supported? We should add a limit to the length of this list and explain that limit in the godoc

Can we add a comment inline that explains that the values in the list should be either of ipv4 or ipv6 CIDR notation

There is no limit on machine networks supported today. There are no rules as for the order of IP stacks in the list of machine networks today. This one field already in install-config.yaml is a bit of wild west, mainly for the reason that it's not used anywhere for real

We have a comment explaining notation added to the Spec. In Status I am not doing this because it's read only and no one should modify it

Ok. so while I appreciate it's currently wild west, what do you think users are actually using? Do you think we could justify limiting this to 32 networks? This new API will only affect new clusters anyway right?

I am okay with limiting to 32 right now but we need to be aware there may be incoming request to increase this. Let me explain the use case...

We have this thingy called "remote worker nodes". While in the most basic setup it means you have masters in 1st subnet and workers in 2nd (separate) subnet, I can easily imagine a customer deploying cluster with 100 workers where each of them lives in its own subnet. I believe it's only a matter of time, but again, today limit of 32 should be okay and in the worst case we receive a bugzilla to increase it.

This new API will only affect new clusters anyway right?

Yes, this is correct. The new MachineNetwork fields will remain empty for already existing clusters (even those that are upgraded to 4.15) because we don't have today any means to backfill it

I'm gonna wager we won't get that request any time soon, but you may tell me you told me so if we do. It would be relaxing the API validation so is a compatible change given that we control the consumers of the field.

Yep, it will be a very easy change to do shall we ever need it

JoelSpeed · 2023-11-10T18:53:11Z

config/v1/types_infrastructure.go

+	// machineNetworks are IP networks used to connect all the OpenShift cluster
+	// nodes. Each network is provided in the CIDR format, e.g. "192.0.2.0/24".


You should explain that this supports ipv6 as well.

Is there a length limit that we can put on this list please?

Changed a comment to

// machineNetworks are IP networks used to connect all the OpenShift cluster // nodes. Each network is provided in the CIDR format and should be IPv4 or IPv6, // for example "10.0.0.0/8" or "fd00::/8".

We can not add length limit for the reasons explained above (i.e. the same field in install-config.yaml has no limits and in general in OCP we allow as many machine networks as you want)

config/v1/types_infrastructure.go

In order to enable work around mutable VIPs we are extending the Infrastructure CR to hold VIPs in the spec and not only in the status. In order to be able to perform proper validations (which currently only happen in o/installer as VIPs are immutable) we are storing machine networks alongside. Implements: OPNET-351 Contributes-to: OPNET-80 Contributes-to: OPNET-340

JoelSpeed · 2023-11-20T13:46:00Z

/lgtm

openshift-ci · 2023-11-20T13:47:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, MaysaMacedo, mkowalski

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 · 2023-11-20T18:36:08Z

@mkowalski: 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.

openshift-bot · 2023-11-21T00:48:48Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-config-api-container-v4.15.0-202311202349.p0.gf872698.assembly.stream for distgit ose-cluster-config-api.
All builds following this will include this PR.

This commit bumps openshift/api dependency in order to be able to use work done in openshift/api#1593. We want to be able to use new fields in the Infrastructure CR that have been added by the PR mentioned above.

This commit bumps openshift/api dependency in order to be able to use work done in openshift/api#1593. It bumps openshift/client-go in order to be able to interact with new fields implemented in the PR mentioned above.

In openshift/api#1593 OpenShift API got new fields related to the network configuration. PR bumps openshift/client-go in order to be able to interact with new fields implemented in the PR mentioned above.

mkowalski · 2023-12-04T15:04:42Z

/revert

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 20, 2023

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 20, 2023

openshift-ci bot requested review from deads2k and knobunc September 20, 2023 13:45

mkowalski force-pushed the OPNET-351 branch 2 times, most recently from 58a18cd to 8f95a94 Compare September 20, 2023 14:29

openshift-ci bot requested review from cybertron, MaysaMacedo and rvanderp3 September 20, 2023 14:30

mkowalski force-pushed the OPNET-351 branch from 8f95a94 to 9ba1ad7 Compare September 20, 2023 14:33

MaysaMacedo mentioned this pull request Sep 22, 2023

Mutable dual-stack VIPs openshift/enhancements#1463

Open

JoelSpeed reviewed Oct 16, 2023

View reviewed changes

mkowalski force-pushed the OPNET-351 branch 2 times, most recently from 2b0d5c4 to bbf3a91 Compare October 16, 2023 16:55

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2023

mkowalski force-pushed the OPNET-351 branch 3 times, most recently from 58b2953 to 657f306 Compare October 17, 2023 08:01

mkowalski mentioned this pull request Oct 18, 2023

OPNET-357: Populate network configuration at install-time openshift/installer#7604

Merged

MaysaMacedo reviewed Oct 20, 2023

View reviewed changes

openshift-ci bot assigned MaysaMacedo Oct 24, 2023

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

mkowalski force-pushed the OPNET-351 branch 2 times, most recently from 3a8e0d3 to 18e6f05 Compare October 30, 2023 16:59

JoelSpeed reviewed Nov 10, 2023

View reviewed changes

mkowalski force-pushed the OPNET-351 branch 2 times, most recently from 6cdc2e1 to 76bd105 Compare November 14, 2023 14:25

mkowalski force-pushed the OPNET-351 branch from 76bd105 to c904792 Compare November 20, 2023 13:02

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 20, 2023

openshift-ci bot assigned JoelSpeed Nov 20, 2023

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2023

openshift-merge-bot bot merged commit f872698 into openshift:master Nov 20, 2023
16 checks passed

mkowalski deleted the OPNET-351 branch November 20, 2023 18:40

This was referenced Dec 4, 2023

OPNET-414: Revert "Extend infra CR to store VIPs and MachineNetwork" #1694

Merged

OPNET-415: Revert the revert "Extend infra CR to store VIPs and MachineNetwork"" #1695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPNET-351: Extend infra CR to store VIPs and MachineNetwork #1593

OPNET-351: Extend infra CR to store VIPs and MachineNetwork #1593

mkowalski commented Sep 20, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Sep 20, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Sep 20, 2023 •

edited by openshift-ci bot

openshift-ci bot commented Sep 20, 2023

mkowalski commented Sep 20, 2023

mkowalski commented Sep 20, 2023

JoelSpeed left a comment

MaysaMacedo Oct 20, 2023

mkowalski Oct 20, 2023 •

edited

mkowalski commented Oct 24, 2023

MaysaMacedo commented Oct 24, 2023

mkowalski commented Oct 31, 2023

mkowalski commented Oct 31, 2023

mkowalski commented Nov 1, 2023

JoelSpeed Nov 10, 2023

mkowalski Nov 14, 2023

JoelSpeed Nov 20, 2023

mkowalski Nov 20, 2023

JoelSpeed Nov 20, 2023

mkowalski Nov 20, 2023

JoelSpeed Nov 10, 2023

mkowalski Nov 14, 2023 •

edited

JoelSpeed commented Nov 20, 2023

openshift-ci bot commented Nov 20, 2023

openshift-ci bot commented Nov 20, 2023

openshift-bot commented Nov 21, 2023

mkowalski commented Dec 4, 2023

		// machineNetworks are IP networks used to connect all the OpenShift cluster
		// nodes. Each network is provided in the CIDR format, e.g. "192.0.2.0/24".

OPNET-351: Extend infra CR to store VIPs and MachineNetwork #1593

OPNET-351: Extend infra CR to store VIPs and MachineNetwork #1593

Conversation

mkowalski commented Sep 20, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Sep 20, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Sep 20, 2023 • edited by openshift-ci bot

openshift-ci bot commented Sep 20, 2023

mkowalski commented Sep 20, 2023

mkowalski commented Sep 20, 2023

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkowalski Oct 20, 2023 • edited

Choose a reason for hiding this comment

mkowalski commented Oct 24, 2023

MaysaMacedo commented Oct 24, 2023

mkowalski commented Oct 31, 2023

mkowalski commented Oct 31, 2023

mkowalski commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkowalski Nov 14, 2023 • edited

Choose a reason for hiding this comment

JoelSpeed commented Nov 20, 2023

openshift-ci bot commented Nov 20, 2023

openshift-ci bot commented Nov 20, 2023

openshift-bot commented Nov 21, 2023

mkowalski commented Dec 4, 2023

mkowalski commented Sep 20, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Sep 20, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Sep 20, 2023 •

edited by openshift-ci bot

mkowalski Oct 20, 2023 •

edited

mkowalski Nov 14, 2023 •

edited