OCPBUGS-29975: Allow multiple machine networks #6071

zaneb · 2024-03-11T01:10:13Z

Until now, assisted-service has assumed that there would be either exactly one MachineNetwork specified for single-stack clusters, or for dual-stack clusters that there would be exactly one IPv4 and one IPv6 MachineNetwork specified (in that order).

None of these restrictions exist in OpenShift itself, which allows multiple MachineNetworks of each address family. This is necessary to support remote worker nodes on day 1, as well as distributed installations of a kind that are common in UPI deployments (where an external load balancer can easily balance across hosts in separate networks). (OCPBUGS-29975)

This change removes the assumptions about a single MachineNetwork per address family for clusters with UserManagedNetworking enabled, and reverts the change in #4867 that prevents users from specifying more networks at the API level.

(Clusters without UserManagedNetworking will still use Layer 2 reachability checks in the belongs-to-majority-group host validation, which effectively prevents using remote worker nodes when creating these clusters. See OCPBUGS-30730.)

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

It's confusing that GetMachineNetworksFromBootstrapHost() returns the existing MachineNetworks in the cluster (and does *not* get them from the bootstrap host) if they already exist there. Refactor to make the logic clearer.

The network type is set for all platforms in getBasicInstallConfig(). There is no need to set it again in the none platform provider.

openshift-ci-robot · 2024-03-11T01:10:17Z

@zaneb: This pull request references Jira Issue OCPBUGS-29975, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Until now, assisted-service has assumed that there would be either exactly one MachineNetwork specified for single-stack clusters, or for dual-stack clusters that there would be exactly one IPv4 and one IPv6 MachineNetwork specified (in that order).

None of these restrictions exist in OpenShift itself, which allows multiple MachineNetworks of each address family. This is necessary to support remote worker nodes on day 1, as well as distributed installations of a kind that are common in UPI deployments (where an external load balancer can easily balance across hosts in separate networks). (OCPBUGS-29975)

This change removes the assumptions about a single MachineNetwork per address family for clusters with UserManagedNetworking enabled, and reverts the change in #4867 that prevents users from specifying more networks at the API level.

(Clusters without UserManagedNetworking will still use Layer 2 reachability checks in the belongs-to-majority-group host validation, which effectively prevents using remote worker nodes when creating these clusters. See OCPBUGS-30730.)

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

Agent-based installer

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

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.

openshift-ci · 2024-03-11T01:10:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zaneb
Once this PR has been reviewed and has the lgtm label, please assign rccrdpccl for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

zaneb · 2024-03-11T01:11:37Z

/jira refresh

openshift-ci-robot · 2024-03-11T01:11:43Z

@zaneb: This pull request references Jira Issue OCPBUGS-29975, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @mhanss

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/jira refresh

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.

zaneb · 2024-03-11T03:00:11Z

Infra issues with the CentOS repos
/retest

codecov · 2024-03-11T03:24:14Z

Codecov Report

Attention: Patch coverage is 27.90698% with 62 lines in your changes are missing coverage. Please review.

Project coverage is 68.36%. Comparing base (ad3e304) to head (ff9be84).
Report is 17 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6071      +/-   ##
==========================================
- Coverage   68.40%   68.36%   -0.04%     
==========================================
  Files         239      239              
  Lines       35453    35474      +21     
==========================================
+ Hits        24251    24252       +1     
- Misses       9098     9119      +21     
+ Partials     2104     2103       -1

Files	Coverage Δ
internal/cluster/validator.go	`96.15% <100.00%> (+0.44%)`	⬆️
internal/host/validator.go	`81.73% <100.00%> (ø)`
internal/cluster/validations/validations.go	`26.07% <0.00%> (+0.20%)`	⬆️
internal/provider/external/external.go	`0.00% <0.00%> (ø)`
internal/provider/external/oci.go	`52.38% <0.00%> (ø)`
internal/provider/nutanix/installConfig.go	`0.00% <0.00%> (ø)`
internal/provider/vsphere/installConfig.go	`0.00% <0.00%> (ø)`
internal/bminventory/inventory.go	`71.29% <75.00%> (-0.03%)`	⬇️
internal/network/dual_stack_validations.go	`0.00% <0.00%> (ø)`
internal/network/machine_network_cidr.go	`56.88% <45.83%> (+1.21%)`	⬆️
... and 1 more

... and 7 files with indirect coverage changes

ori-amizur

It is not clear what are the restrictions here:
Do we allow multiple API VIPs per address family?
Do we have restriction per host-role to belong to a specific machine-network? If yes, this needs to be added as validation.
What if we have only 3 host cluster (only masters). Can such cluster have multiple IPv4 machine-networks?
Can a machine-network be stale (without hosts)?

ori-amizur · 2024-03-11T12:46:21Z

internal/network/cidr_validations.go

@@ -21,7 +22,7 @@ const MinSNOMachineMaskDelta = 1
 func parseCIDR(cidr string) (ip net.IP, ipnet *net.IPNet, err error) {
 	ip, ipnet, err = net.ParseCIDR(cidr)
 	if err != nil {
-		err = errors.Wrapf(err, "Failed to parse CIDR '%s'", cidr)
+		err = fmt.Errorf("Failed to parse CIDR '%s': %w", cidr, err)


Why this was changed?

Because it's a pain to import both the standard library "errors" package and the deprecated old-timey hack "github.com/pkg/errors" package in the same file. This is a trivial refactor, the result is the same.

ori-amizur · 2024-03-11T12:47:42Z

internal/network/cidr_validations.go

 	if err != nil {
 		return err
 	}

 	if overlap {
-		return errors.Errorf("CIDRS %s and %s overlap", aCidrStr, bCidrStr)
+		return fmt.Errorf("CIDRS %s and %s overlap", aCidrStr, bCidrStr)


Please do not change the errors unless necessary

ori-amizur · 2024-03-11T16:16:34Z

internal/bminventory/inventory.go

 					log.WithError(err).Warnf("Verify VIPs")
 					return common.NewApiError(http.StatusBadRequest, err)
 				}
 			}

 		} else {
-			primaryMachineNetworkCidr, err = network.CalculateMachineNetworkCIDR(network.GetApiVipById(&targetConfiguration, 0), network.GetIngressVipById(&targetConfiguration, 0), cluster.Hosts, matchRequired)
+			primaryMachineNetworkCidr, err := network.CalculateMachineNetworkCIDR(network.GetApiVipById(&targetConfiguration, 0), network.GetIngressVipById(&targetConfiguration, 0), cluster.Hosts, matchRequired)


This might be wrong according the concept presented by this PR, because we may have to machine-network - one per vip and one of both vips.

ori-amizur · 2024-03-11T16:27:07Z

internal/cluster/validator.go

-	if err := checkCidrsOverlapping(c.cluster); err != nil {
-		return ValidationFailure, fmt.Sprintf("CIDRS Overlapping: %s.", err.Error())
+	if err := network.VerifyNoNetworkCidrOverlaps(c.cluster.ClusterNetworks, c.cluster.MachineNetworks, c.cluster.ServiceNetworks); err != nil {
+		return ValidationFailure, err.Error()


The validation error messages need to be agreed upon. They are presented by UI. Here it is not clear what is expected as text of this error message

Ack. I did change them, you can see the change in the tests here: 51e76b1#diff-854f83029709261bb4e532a5a6839b51ddeb2beb2f6e921056016a74340d06a3

ori-amizur · 2024-03-11T16:42:05Z

internal/host/validator.go

@@ -600,7 +600,7 @@ func (v *validator) belongsToMachineCidr(c *validationContext) (ValidationStatus
 	if c.inventory == nil || !network.IsMachineCidrAvailable(c.cluster) {
 		return ValidationPending, "Missing inventory or machine network CIDR"
 	}
-	if !network.IsHostInPrimaryMachineNetCidr(v.log, c.cluster, c.host) {
+	if !network.IsHostInMachineNetCidrs(v.log, c.cluster, c.host) {
 		return ValidationFailure, "Host does not belong to machine network CIDRs. Verify that the host belongs to every CIDR listed under machine networks"


This error message is not true now. Right ?

Ah yep, last part is not.

Wording is awkward here, because we still have to belong to both IPv4 and IPv6 CIDRs if both appear in the list. What do you think of "Verify that the host belongs to a CIDR for every IP version listed under machine networks"?

That might be OK. @avishayt what do you think?

"Verify that the host belongs to a machine network CIDR for every used IP version"?

ori-amizur · 2024-03-11T16:47:20Z

internal/network/machine_network_cidr.go

 		return models.VipVerificationUnverified, errors.Errorf("%s <%s> cannot be set if Machine Network CIDR is empty", vipName, vip)
 	}
-	if !ipInCidr(vip, machineNetworkCidr) {
-		return models.VipVerificationFailed, errors.Errorf("%s <%s> does not belong to machine-network-cidr <%s>", vipName, vip, machineNetworkCidr)
+	if machineNetworkCidr == "" {


This test is irrelevant. The machine network cannot be valid and empty.

Yes it can - validMachineNetwork will be true if any machine networks are defined, but machineNetworkCidr will only be set if the VIP is in one of them.

So you don't need the test for valid-network

This is maintaining the previous behaviour. If there are no machineNetworks specified, we return VipVerificationUnverified. If there are machineNetworks specified and the VIP isn't in any of them, we return VipVerificationFailed.

I think what's confusing in the diff is that machineNetworkCidr was previously a parameter, and now it's a local variable.

None of the subnets specified in any of the machineNetworks, clusterNetworks, or serviceNetworks should overlap. Validate all of these combinations, as openshift-installer does, instead of making assumptions about indices being aligned to address families.

Don't make assumptions about a 1:1 mapping between MachineNetworks and VIPs.

In the belongs-to-machine-cidr validation, allow the host to be a member of any MachineNetwork. In a dual-stack cluster, require it to be a member of both an IPv4 and an IPv6 network. Previously it was assumed that the only reason for multiple MachineNetworks to appear was that a dual stack cluster could contain exactly one IPv4 and one IPv6 MachineNetwork.

Multiple MachineNetworks in the same address family and IPv6-primary dual-stack clusters are a thing, so relax the dual-stack validation requirements for machine networks to allow them.

Allow users to specify multiple machine networks of the same address family. This is a documented and supported feature of OpenShift. This reverts commit 873dd81.

Don't restrict ourselves to the first machine networking when looking for an interface on a machine network to set the BootMACAddress.

OpenShift has ~always supported having machines in multiple machineNetworks, so update the TODO comment to reflect that accounting for this is already something we need to do to fully support non-UserManagedNetworking clusters. (UserManagedNetworking clusters use only the L3 connectivity check.) See https://issues.redhat.com/browse/OCPBUGS-30730 for more details.

zaneb · 2024-03-12T10:57:59Z

Do we allow multiple API VIPs per address family?

No.

Do we have restriction per host-role to belong to a specific machine-network? If yes, this needs to be added as validation.

No.

What if we have only 3 host cluster (only masters). Can such cluster have multiple IPv4 machine-networks?

Yes (and in fact this is likely to be a common case for platform: none).

Can a machine-network be stale (without hosts)?

I don't see why not.

openshift-ci · 2024-03-12T13:42:17Z

@zaneb: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/edge-e2e-oci-assisted-4-14	`ff9be84`	link	false	`/test edge-e2e-oci-assisted-4-14`
ci/prow/edge-e2e-nutanix-assisted	`ff9be84`	link	false	`/test edge-e2e-nutanix-assisted`

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.

ori-amizur · 2024-03-13T09:05:44Z

Do we have restriction per host-role to belong to a specific machine-network? If yes, this needs to be added as validation.

No.

So workers do not have to belong to machine-networks of ingress-vips? Maybe we have to verify that we have at least 2 workers belonging to machine-network of ingress VIPs ? If not it will cause a mess. The logic is already complicated and if we don't set rules, users will be confused.

What if we have only 3 host cluster (only masters). Can such cluster have multiple IPv4 machine-networks?

Yes (and in fact this is likely to be a common case for platform: none).

None platform does not have machine-networks at all (At least how we implemented it).
Anyway if we have multiple machine-networks for masters, how can the API VIPs move between these hosts?

Can a machine-network be stale (without hosts)?

I don't see why not.

Why is it needed?

zaneb added 5 commits March 11, 2024 12:32

Fix typo in function name

715cc3e

Refactor GetMachineNetworkForUserManagedNetworking()

2503b6b

It's confusing that GetMachineNetworksFromBootstrapHost() returns the existing MachineNetworks in the cluster (and does *not* get them from the bootstrap host) if they already exist there. Refactor to make the logic clearer.

Use plural in function name

c409fe6

Remove redundant code

7f35e9f

The network type is set for all platforms in getBasicInstallConfig(). There is no need to set it again in the none platform provider.

Use fmt.Errorf() instead of deprecated errors package

1232119

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

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Mar 11, 2024

zaneb changed the title ~~OCPBUGS-29975: Support multiple machine networks~~ OCPBUGS-29975: Allow multiple machine networks Mar 11, 2024

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 11, 2024

openshift-ci bot requested review from adriengentil and tsorya March 11, 2024 01:10

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 11, 2024

openshift-ci bot requested a review from mhanss March 11, 2024 01:11

zaneb force-pushed the multiple-machine-networks branch from 68f0945 to b996310 Compare March 11, 2024 09:49

ori-amizur reviewed Mar 11, 2024

View reviewed changes

zaneb force-pushed the multiple-machine-networks branch from b996310 to c6e01dc Compare March 12, 2024 03:26

zaneb added 7 commits March 12, 2024 17:01

Validate VIPs against all MachineNetworks

7d1ac38

Don't make assumptions about a 1:1 mapping between MachineNetworks and VIPs.

Allow multiple MachineNetworks in dual stack

41e0cbd

Multiple MachineNetworks in the same address family and IPv6-primary dual-stack clusters are a thing, so relax the dual-stack validation requirements for machine networks to allow them.

Revert "Forbid multiple machine networks in single-stack clusters"

bae9303

Allow users to specify multiple machine networks of the same address family. This is a documented and supported feature of OpenShift. This reverts commit 873dd81.

Find BootMACAddress from any machine network

f8bcf77

Don't restrict ourselves to the first machine networking when looking for an interface on a machine network to set the BootMACAddress.

zaneb force-pushed the multiple-machine-networks branch from c6e01dc to ff9be84 Compare March 12, 2024 10:12

zaneb mentioned this pull request Mar 19, 2024

OCPBUGS-30233: Filter IPs in majority group validation #6094

Draft

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-29975: Allow multiple machine networks #6071

OCPBUGS-29975: Allow multiple machine networks #6071

zaneb commented Mar 11, 2024

openshift-ci-robot commented Mar 11, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Mar 11, 2024

zaneb commented Mar 11, 2024

openshift-ci-robot commented Mar 11, 2024

zaneb commented Mar 11, 2024

codecov bot commented Mar 11, 2024 •

edited

ori-amizur left a comment

ori-amizur Mar 11, 2024

zaneb Mar 12, 2024

ori-amizur Mar 11, 2024

ori-amizur Mar 11, 2024

ori-amizur Mar 11, 2024

zaneb Mar 12, 2024

ori-amizur Mar 11, 2024

zaneb Mar 12, 2024

zaneb Mar 13, 2024

ori-amizur Mar 13, 2024

avishayt Mar 13, 2024

ori-amizur Mar 11, 2024

zaneb Mar 12, 2024

ori-amizur Mar 13, 2024

zaneb Mar 13, 2024

zaneb commented Mar 12, 2024

openshift-ci bot commented Mar 12, 2024

ori-amizur commented Mar 13, 2024 •

edited

OCPBUGS-29975: Allow multiple machine networks #6071

Are you sure you want to change the base?

OCPBUGS-29975: Allow multiple machine networks #6071

Conversation

zaneb commented Mar 11, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Mar 11, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Mar 11, 2024

zaneb commented Mar 11, 2024

openshift-ci-robot commented Mar 11, 2024

zaneb commented Mar 11, 2024

codecov bot commented Mar 11, 2024 • edited

Codecov Report

ori-amizur left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaneb commented Mar 12, 2024

openshift-ci bot commented Mar 12, 2024

ori-amizur commented Mar 13, 2024 • edited

codecov bot commented Mar 11, 2024 •

edited

ori-amizur commented Mar 13, 2024 •

edited