Skip to content

Conversation

@honza
Copy link
Member

@honza honza commented Apr 8, 2022

Add new configuration fields to install-config.yaml that will enable the cluster operator to set up static networking for the bootstrap node

This is the implementation of the Bootstrap external IP OpenShift enhancement.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2022
@openshift-ci openshift-ci bot requested review from andfasano and elfosardo April 8, 2022 13:06
@honza honza force-pushed the bootstrap-static-ip branch 2 times, most recently from 4f30c1b to 0192704 Compare April 19, 2022 17:04
@honza honza changed the title WIP: baremetal: static IP for bootstrap node baremetal: static IP for bootstrap node Apr 19, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2022
@honza honza force-pushed the bootstrap-static-ip branch from 0192704 to 7cca21d Compare April 20, 2022 12:17
@zhouhao3
Copy link
Contributor

LGTM
Tested this PR locally by way of IPI and it works fine.

@sadasu
Copy link
Contributor

sadasu commented May 2, 2022

@honza would be great if you could add when the 2 new configs added to the install config should be set. Maybe here #5787 (comment) is a good place.
Also, as @zhouhao3 pointed out, the changes to install.openshift.io_installconfigs.yaml should include a description of the 2 new fields being added.

@honza honza force-pushed the bootstrap-static-ip branch from 7cca21d to 5591f67 Compare May 3, 2022 12:17
@honza
Copy link
Member Author

honza commented May 3, 2022

@sadasu The link in your comment is broken: where would you like me to add these comments?

@sadasu
Copy link
Contributor

sadasu commented May 3, 2022

@sadasu The link in your comment is broken: where would you like me to add these comments?

#5787 (comment)

@honza honza force-pushed the bootstrap-static-ip branch 2 times, most recently from bbee643 to a3535e2 Compare May 9, 2022 17:50
@zaneb zaneb added the platform/baremetal IPI bare metal hosts platform label May 9, 2022
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be checking that we pick the CIDR from the network that contains the BootstrapExternalStaticIP? In this implementation, we would be picking the 1st network's CIDR. Is that what we want?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a dual stack environment I think you'll always want the external IP to be the first (v4) network, although technically in the DHCP dual stack scenario I guess the NIC could get an IP on both networks

If we ever want IPI to support a case like https://issues.redhat.com/browse/METAL-163 where the BMCs are only accessible via v6, we'd need to revise this for the case where the provisioning network is disabled so the user can specify two IPs.

Considering that a follow-up seems reasonable to me, although we'd end up having to make the install-config interface a list like the networks if that becomes a requirement

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is what we're looking for. For a start, we're trying to configure the External interface, and this variable is for the Provisioning interface,
But also I don't know that you can just stick a MAC address as an interface name in NetworkManager? Nothing in https://networkmanager.dev/docs/api/latest/settings-connection.html suggests that to me.

In #5698 passing the MAC only worked because there is code that runs in the script that looks up the interface name for that MAC using NetworkManager https://github.com/openshift/installer/pull/5698/files#diff-6f24f49f3938f9f97f5429b1280c21ccb462b377705ad4f9a7f526316caa2784R31. What we have here is a much harder problem because we need networking to be set up in the initramfs, before we even get to run any code.

It looks from https://networkmanager.dev/docs/api/latest/NetworkManager.conf.html#connection-sections that there might be a way to set in the NetworkManager config which device a connection gets mapped to when interface-name doesn't appear It seems like either:

[connection-external]
match-device=mac:<external mac>

or

[device-external]
match-device=mac:<external mac>
allowed-connections=uuid:<connection UUID>

[device-provisioning]
match-device=mac:<provisioning mac>
allowed-connections=except:uuid:<connection UUID>

might possibly be the magic incantation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we have here is a much harder problem because we need networking to be set up in the initramfs, before we even get to run any code.

@zaneb I don't think that's the case on the bootstrap VM, unlike the other hosts we're not pulling ignition in the ramdisk via the network, because it's injected via the fw-cfg qemu interface instead.

I agree we need to use the ExternalMACAddress though, but I think we can just use mac-address in the [ethernet] section to specify the device, e.g similar to these docs (not tested this myself though)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we need to use the ExternalMACAddress though, but I think we can just use mac-address in the [ethernet] section to specify the device, e.g similar to these docs (not tested this myself though)

Just added these changes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep, that looks correct, here is the doc: https://networkmanager.dev/docs/api/latest/settings-802-3-ethernet.html

If specified, this connection will only apply to the Ethernet device whose permanent MAC address matches. This property does not change the MAC address of the device (i.e. MAC spoofing).

That's exactly what we want.

@honza honza force-pushed the bootstrap-static-ip branch from a3535e2 to dae3fa0 Compare May 11, 2022 13:22
@honza honza force-pushed the bootstrap-static-ip branch from dae3fa0 to 2f3b1eb Compare May 11, 2022 14:14
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD cf42b3c and 7 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD cf42b3c and 6 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD cf42b3c and 5 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 2ca5014 and 4 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 2ca5014 and 3 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 2ca5014 and 2 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD e0fc0b2 and 1 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD e0fc0b2 and 0 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/hold

Revision 4e5a726 was retested 9 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2022
@bparees
Copy link
Contributor

bparees commented Jun 8, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 26ac88e and 8 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD ec566da and 7 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD ec566da and 6 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD ec566da and 5 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD e7724f2 and 4 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 190547f and 3 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 190547f and 2 for PR HEAD 4e5a726 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 190547f and 1 for PR HEAD 4e5a726 in total

@honza
Copy link
Member Author

honza commented Jun 13, 2022

/test e2e-metal-ipi-ovn-ipv6

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 13, 2022

@honza: 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/e2e-metal-ipi-ovn-ipv6-required 61bd0443a10868b2cf56ba0a90beb6d1f2cc8b4e link true /test e2e-metal-ipi-ovn-ipv6-required
ci/prow/openstack-manifests 61bd0443a10868b2cf56ba0a90beb6d1f2cc8b4e link true /test openstack-manifests
ci/prow/e2e-metal-ipi-swapped-hosts 4e5a726 link false /test e2e-metal-ipi-swapped-hosts
ci/prow/e2e-metal-ipi-ovn-dualstack 4e5a726 link false /test e2e-metal-ipi-ovn-dualstack
ci/prow/e2e-metal-ipi-virtualmedia 4e5a726 link false /test e2e-metal-ipi-virtualmedia
ci/prow/e2e-libvirt 4e5a726 link false /test e2e-libvirt
ci/prow/e2e-ibmcloud 4e5a726 link false /test e2e-ibmcloud
ci/prow/e2e-metal-ipi 4e5a726 link false /test e2e-metal-ipi
ci/prow/okd-e2e-aws 4e5a726 link false /test okd-e2e-aws
ci/prow/e2e-crc 4e5a726 link false /test e2e-crc

Full PR test history. Your PR dashboard.

Details

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.

@zaneb
Copy link
Member

zaneb commented Jun 13, 2022

The e2e-metal-ipi-ovn-ipv6 appears to be permafailing (not just here). Logs show that the cluster is being bootstrapped successfully, so the failures are not related to this patch.

@rna-afk
Copy link
Contributor

rna-afk commented Jun 14, 2022

/override ci/prow/e2e-metal-ipi-ovn-ipv6

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2022

@rna-afk: rna-afk unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight.

Details

In response to this:

/override ci/prow/e2e-metal-ipi-ovn-ipv6

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.

@sdodson
Copy link
Member

sdodson commented Jun 14, 2022

/override ci/prow/e2e-metal-ipi-ovn-ipv6

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2022

@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-metal-ipi-ovn-ipv6

Details

In response to this:

/override ci/prow/e2e-metal-ipi-ovn-ipv6

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 openshift-ci bot merged commit 5d096a6 into openshift:master Jun 14, 2022
hardys pushed a commit to hardys/dev-scripts that referenced this pull request Jul 18, 2022
This configures static IP for the bootstrap external NIC

Requires openshift/installer#5787
openshift-ci bot pushed a commit to openshift-metal3/dev-scripts that referenced this pull request Jul 19, 2022
This configures static IP for the bootstrap external NIC

Requires openshift/installer#5787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. platform/baremetal IPI bare metal hosts platform px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.