Bug 1846093: Improve bootstrap reliability on heterogeneous UPI network configurations #384

ironcladlou · 2020-06-22T20:33:08Z

Before this change, bootstrap IP discovery assumed that the first address of the
unicast interface must be the bootstrap IP. This assumption doesn't always hold
in the face of user-defined interfaces and addresses whose ordering isn't
guaranteed. When the assumptions are broken and the incorrect bootstrap IP is
selected, bootstrapping fails because quorum cannot be established.

This change improves the accuracy of bootstrap IP discovery by more flexibly
accounting for a wider variety of possible network interface configurations.

An IP is now considered the bootstrap IP if all of the following are true.

For IPv4:

The IP is contained by the machine CIDR defined in the cluster configuration
On bare metal platforms, the IP is not the API or DNS VIP in the cluster configuration

For IPv6, the same must be true in addition to the following:

The IP is not deprecated
The IP is routable according at least one non-default route

This work is adapted from https://github.com/openshift/baremetal-runtimecfg/blob/master/pkg/utils/utils.go.

ironcladlou · 2020-06-22T20:34:28Z

Still need to play with this in a new cluster's bootstrapping environment, here's how it looks on an already solvent master ipv4 GCP master node:

I0622 20:29:00.242715      81 bootstrap_ip_linux.go:37] retrieved Address map map[0xc000262c30:[127.0.0.1/8 lo ::1/128] 0xc0005707e0:[10.129.0.74/23 eth0 fe80::7c73:6eff:fe59:f9ad/64]]
I0622 20:29:00.242841      81 bootstrap_ip_linux.go:114] Checking whether address 127.0.0.1/8 lo contains VIP 10.129.0.74
I0622 20:29:00.242929      81 bootstrap_ip_linux.go:56] Ignoring route non Router advertisement route {Ifindex: 3 Dst: fe80::/64 Src: <nil> Gw: <nil> Flags: [] Table: 254}
I0622 20:29:00.242953      81 bootstrap_ip_linux.go:66] Retrieved route map map[]
I0622 20:29:00.242976      81 bootstrap_ip_linux.go:114] Checking whether address 10.129.0.74/23 eth0 contains VIP 10.129.0.74
I0622 20:29:00.242986      81 bootstrap_ip_linux.go:116] Address 10.129.0.74/23 eth0 contains VIP 10.129.0.74
I0622 20:29:00.242995      81 bootstrap_ip_linux.go:114] Checking whether address fe80::7c73:6eff:fe59:f9ad/64 contains VIP 10.129.0.74
10.129.0.74

ironcladlou · 2020-06-22T20:39:00Z

What role should machineCIDR play in this discovery?

ironcladlou · 2020-06-23T17:42:34Z

@hexfusion @celebdor the latest commit here refactors the detection to add an additional filter that ensures candidate IPs are contained in the machine CIDR.

One caveat here is that I'm not sure if machineCIDR is always present. In the current code, I'm assuming that if machineCIDR is unavailable, no machine CIDR containment check will happen (but we do now produce a warning). The net effect in that case is that it's theoretically possible to select an IP which is routable but outside the unknown machine CIDR. I'm not entirely sure what the right assumption here should be.

Is machine CIDR truly optional? How should the product react when machine CIDR is unknown here?

hexfusion · 2020-06-23T18:09:16Z

great questions and ones that needs to be answered before this merges

hardys · 2020-06-24T09:04:33Z

@hexfusion @celebdor the latest commit here refactors the detection to add an additional filter that ensures candidate IPs are contained in the machine CIDR.

One caveat here is that I'm not sure if machineCIDR is always present. In the current code, I'm assuming that if machineCIDR is unavailable, no machine CIDR containment check will happen (but we do now produce a warning). The net effect in that case is that it's theoretically possible to select an IP which is routable but outside the unknown machine CIDR. I'm not entirely sure what the right assumption here should be.

Is machine CIDR truly optional? How should the product react when machine CIDR is unknown here?

Looking at the installer interfaces I think it's mandatory:

https://github.com/openshift/installer/blob/master/pkg/types/validation/installconfig.go#L249

It's marked as optional in the install-config schema, but AFAICS that's only to enable backwards compatibility with machineNetwork and the now-deprecated machineCIDR interface

https://github.com/openshift/installer/blob/master/pkg/types/installconfig.go#L213

https://github.com/openshift/installer/blob/master/pkg/types/installconfig.go#L234

ironcladlou · 2020-06-24T13:00:19Z

@hardys

@hexfusion @celebdor the latest commit here refactors the detection to add an additional filter that ensures candidate IPs are contained in the machine CIDR.
One caveat here is that I'm not sure if machineCIDR is always present. In the current code, I'm assuming that if machineCIDR is unavailable, no machine CIDR containment check will happen (but we do now produce a warning). The net effect in that case is that it's theoretically possible to select an IP which is routable but outside the unknown machine CIDR. I'm not entirely sure what the right assumption here should be.
Is machine CIDR truly optional? How should the product react when machine CIDR is unknown here?

Looking at the installer interfaces I think it's mandatory:

https://github.com/openshift/installer/blob/master/pkg/types/validation/installconfig.go#L249

It's marked as optional in the install-config schema, but AFAICS that's only to enable backwards compatibility with machineNetwork and the now-deprecated machineCIDR interface

https://github.com/openshift/installer/blob/master/pkg/types/installconfig.go#L213

https://github.com/openshift/installer/blob/master/pkg/types/installconfig.go#L234

Thank you for digging that up. Would it make sense then for us to fail fast and loudly if the machine CIDR is empty?

ironcladlou · 2020-06-24T14:42:35Z

/retest

ironcladlou · 2020-06-24T15:11:47Z

Latest revision disentangles render from all bootstrap IP determination by moving all that stuff into the bootstrap_* files. Also cleaned up that code and added a fail-fast check which asserts machine CIDR is required.

ironcladlou · 2020-07-01T20:10:39Z

I still really owe this work some unit tests, but it's ready for some detailed review. So far testing has been manual.

ironcladlou · 2020-07-01T20:32:14Z

cc @celebdor would love any feedback and fact checking you can spare on this one

hexfusion

@ironcladlou looks good in general, a few questions.

pkg/cmd/render/bootstrap_ip.go

pkg/cmd/render/bootstrap_ip_linux.go

pkg/cmd/render/render.go

pkg/cmd/render/bootstrap_ip.go

pkg/cmd/render/bootstrap_ip_linux.go

openshift-ci-robot · 2020-07-07T12:24:51Z

@ironcladlou: This pull request references Bugzilla bug 1846093, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

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

In response to this:

Bug 1846093: Improve bootstrap reliability on heterogeneous UPI network configurations

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.

ironcladlou · 2020-07-07T14:54:09Z

Added a couple of unit tests. During the course of that I had to do a little refactoring and also discovered a bit of brittle logic around ipv6 handling (using length check to detect the family can be flaky depending on how the address struct is allocated; I switched to using net.To4() which I think is more stable).

@hexfusion ptal

hexfusion · 2020-07-07T16:27:38Z

/retest

hexfusion · 2020-07-07T20:34:53Z

quick check of aws[1] failure etcd bootstrapped fine.

[1] https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-etcd-operator/384/pull-ci-openshift-cluster-etcd-operator-master-e2e-aws/1280539049492221952
/retest

hexfusion · 2020-07-07T23:26:29Z

hmm CI looks hosed :(

hexfusion · 2020-07-08T12:57:09Z

/retest

hexfusion · 2020-07-09T11:02:01Z

/test e2e-gcp-upgrade

ironcladlou · 2020-07-09T11:47:28Z

The metal failure could be something legit with ipv6, need to investigate

hexfusion · 2020-07-09T12:24:21Z

@stbenjam we are nervous to merge this with metal failing.

cc @romfreiman

ironcladlou · 2020-07-09T12:27:16Z

Okay, I had a chance to dig in to the metal-ipi failures, and the bootstrap IP seems to have been successfully identified and etcd is coming up — I didn't dig further into what might be the root cause of the overall failures. I need to rebase now, so I'll do that and squash again and go through another round of CI tests.

…ions Before this change, bootstrap IP discovery assumed that the first address of the unicast interface must be the bootstrap IP. This assumption doesn't always hold in the face of user-defined interfaces and addresses whose ordering isn't guaranteed. When the assumptions are broken and the incorrect bootstrap IP is selected, bootstrapping fails because quorum cannot be established. This change improves the accuracy of bootstrap IP discovery by more flexibly accounting for a wider variety of possible network interface configurations. An IP is now considered the bootstrap IP if all of the following are true. For IPv4: * The IP is contained by the machine CIDR defined in the cluster configuration * On bare metal platforms, the IP is not the API or DNS VIP in the cluster configuration For IPv6, the same must be true in addition to the following: * The IP is not deprecated * The IP is routable according at least one non-default route This work is adapted from https://github.com/openshift/baremetal-runtimecfg/blob/master/pkg/utils/utils.go.

hexfusion · 2020-07-09T12:54:22Z

/lgtm
/approve

openshift-ci-robot · 2020-07-09T12:54:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hexfusion, ironcladlou

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~~ [hexfusion]

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

openshift-bot · 2020-07-09T13:04:57Z

/retest