OCPBUGS-29975: Allow multiple machine networks #6071
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@zaneb: This pull request references Jira Issue OCPBUGS-29975, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
openshift-ci-robot
added
the
jira/invalid-bug
zaneb
changed the title
openshift-ci
bot
added
the
size/L
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@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
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
|
Infra issues with the CentOS repos |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6071 +/- ##
==========================================
- Coverage 68.19% 68.18% -0.02%
==========================================
Files 279 279
Lines 39282 39288 +6
==========================================
Hits 26789 26789
- Misses 10060 10068 +8
+ Partials 2433 2431 -2
|
zaneb
force-pushed
the
multiple-machine-networks
branch
from
68f0945 to
b996310
Compare
There was a problem hiding this comment.
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)?
| @@ -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) | |||
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Please do not change the errors unless necessary
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added a comment indicating that this is something we'll want to fix as part of OCPBUGS-30730.
| 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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ack. I did change them, you can see the change in the tests here: 51e76b1#diff-854f83029709261bb4e532a5a6839b51ddeb2beb2f6e921056016a74340d06a3
| 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 == "" { |
There was a problem hiding this comment.
This test is irrelevant. The machine network cannot be valid and empty.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So you don't need the test for valid-network
There was a problem hiding this comment.
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.
zaneb
force-pushed
the
multiple-machine-networks
branch
2 times, most recently
from
c6e01dc to
ff9be84
Compare
No.
No.
Yes (and in fact this is likely to be a common case for
I don't see why not. |
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.
None platform does not have machine-networks at all (At least how we implemented it).
Why is it needed? |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. In 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 kubernetes-sigs/prow repository. |
|
@zaneb: This pull request references Jira Issue OCPBUGS-29975. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. In 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. |
zaneb
force-pushed
the
multiple-machine-networks
branch
2 times, most recently
from
3be57c4 to
4a89db6
Compare
|
@ori-amizur I added a check in the VerifyVIPs that the machine network the API VIP is in is present on all master nodes, and the machine network the Ingress VIP is in is present on all worker nodes. Note that in ABI we always pass the MachineNetworks provided in the install-config. Also, for dual-stack the user must always pass the MachineNetworks explicitly to assisted-service. |
zaneb
force-pushed
the
multiple-machine-networks
branch
2 times, most recently
from
17570ee to
04cfd6d
Compare
zaneb
force-pushed
the
multiple-machine-networks
branch
from
04cfd6d to
e1878e0
Compare
|
Tests are working now, so I believe this is ready for another round of review. |
|
/cc @avishayt @ori-amizur could you take another look and comment if this is ready to go. |
zaneb
force-pushed
the
multiple-machine-networks
branch
from
e1878e0 to
924e8a1
Compare
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.
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. Check only that the VIP is a member of any MachineNetwork.
When doing cluster validations, check that all of the hosts that the VIP can point to (i.e. control plane hosts for the API VIP, workers for the Ingress VIP) are members of the VIP's MachineNetwork. Since at the time of adding the VIPs (and also during cluster validations) we only check that the VIP is a member of _some_ MachineNetwork, we need this additional check to ensure that it is one where the hosts are.
It doesn't appear this was ever used outside of its own unit tests.
zaneb
force-pushed
the
multiple-machine-networks
branch
from
924e8a1 to
eb9879e
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zaneb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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 network 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
force-pushed
the
multiple-machine-networks
branch
from
eb9879e to
b9ba27b
Compare
|
@zaneb: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
| Context("Cluster with two networks of same stack", func() { | ||
| It("only v4 in cluster networks rejected", func() { | ||
| errStr := "Second cluster network has to be IPv6 subnet" | ||
| params.NewClusterParams.ClusterNetworks = []*models.ClusterNetwork{ |
There was a problem hiding this comment.
Because ClusterNetworks is an array that can theoretically contain any number of entries.
Is the maximum network count we expect 2?
If so, then should we have a test to ensure that only 2 networks are provided.
If not, then should we be assuming that we are dealing with a "Second" cluster network or should this test in fact check that at least one of the cluster networks has an IPv6 subnet and at least one is IPv4?
| It("only v4 in service networks rejected", func() { | ||
| errStr := "Second service network has to be IPv6 subnet" | ||
| params.NewClusterParams.ClusterNetworks = common.TestDualStackNetworking.ClusterNetworks | ||
| params.NewClusterParams.ServiceNetworks = []*models.ServiceNetwork{ |
There was a problem hiding this comment.
Because ServiceNetworks is an array that can theoretically contain any number of entries.
Is the maximum network count we expect 2?
If so, then should we have a test to ensure that only 2 networks are provided.
If not, then should we be assuming that we are dealing with a "Second" service network or should this test in fact check that at least one of the service networks has an IPv6 subnet and at least one is IPv4?
| @@ -4234,7 +4234,7 @@ var _ = Describe("Refresh Host", func() { | |||
| ntpSources: defaultNTPSources, | |||
| role: models.HostRoleMaster, | |||
| statusInfoChecker: makeValueChecker(formatStatusInfoFailedValidation(statusInfoNotReadyForInstall, | |||
| "Host does not belong to machine network CIDRs. Verify that the host belongs to every CIDR listed under machine networks")), | |||
| "Host does not belong to machine network CIDRs. Verify that the host belongs to a listed machine network CIDR for each IP stack in use")), | |||
There was a problem hiding this comment.
Maybe adding something like for each IP stack (IPv4/IPv6) in use so that the user clearly understands the meaning of 'IP stack'
| func VerifyMachineNetworksDualStack(networks []*models.MachineNetwork, isDualStack bool) error { | ||
| if !isDualStack { | ||
| return nil | ||
| } | ||
| if len(networks) != 2 { | ||
| if len(networks) < 2 { | ||
| return errors.Errorf("Expected 2 machine networks, found %d", len(networks)) |
There was a problem hiding this comment.
"Expected at least 2 machine networks and at least one for each IP stack (IPv4, IPv6), found %d"
| vipsWrapper.Verification(i), v.log) | ||
| failed = failed || verification != models.VipVerificationSucceeded | ||
| if verification == models.VipVerificationSucceeded { |
There was a problem hiding this comment.
I think we need a comment to explain what is going on here, looks like you are checking host networks after failing to find the VIP in machine networks.
If so, I think we should have a comment.
| verification, err = network.ValidateVipInHostNetworks(c.cluster.Hosts, c.cluster.MachineNetworks, vipsWrapper.IP(i), vipsWrapper.Type(), v.log) | ||
| failed = failed || verification != models.VipVerificationSucceeded | ||
| } else { | ||
| failed = true |
There was a problem hiding this comment.
I think we should skip the else here and assume failure unless disproved in the machine network and cluster network checks.
Failed = true should be the default value before any checks have been performed and we should be aiming to set this false in subsequent tests.
| machineNetworks []*models.MachineNetwork, | ||
| serviceNetworks []*models.ServiceNetwork) error { | ||
| errs := []error{} | ||
| for imn, mn := range machineNetworks { |
There was a problem hiding this comment.
High time complexity here O(m*n)
There might be something clever we could do to reduce this.
Different language I know, but there are approaches using a data structure to memorize already searched ranges and to allow iteration across a set of networks in Order(n)
Note the use of a Radix tree in this implementation.
https://github.com/fgiuba/ipconflict/blob/master/ipconflict/subnet.py#L46-L61
Due to the limited number of networks, the consequences of not handling this may not be problematic. But it does look like something could be improved here!
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-grouphost 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?
Checklist
docs, README, etc)Reviewers Checklist