Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes needed for IPv6 #107

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Changes needed for IPv6 #107

merged 1 commit into from
Nov 21, 2019

Conversation

stbenjam
Copy link
Member

@stbenjam stbenjam commented Oct 17, 2019

This commit adds support for IPv6. When $PROVISIONING_IP is specified,
which may be an IPv6 address, the various containers will wait for that
IP to become available on an interface.

If the IP is IPv6, then we use an IPv6-specific configurations. Note,
IPv6 hosts are expected to be using UEFI boot, as we use snponly.efi.
snponly.efi uses the UEFI network stack instead of the iPXE drivers.
When using EDK2 OVMF + iPXE + ipxe.efi, we have seen lock-ups in
initialization of the hardware devices.

As neither CentOS nor RHEL distribute iPXE builds with IPv6 support, we
build them from source as part of the container build.

@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 17, 2019
@hardys
Copy link
Member

hardys commented Oct 18, 2019

#108 is a possible alternative approach which avoids the new variable, wdyt?

@stbenjam
Copy link
Member Author

stbenjam commented Oct 18, 2019

I liked the idea of being explicit that we want IPv6 so there’s no surprises. Could we have dual stack environments where the host could get IPv4 and 6 addresses?

@hardys
Copy link
Member

hardys commented Oct 18, 2019

Could we have dual stack environments where the host might be IPv4 and 6 addresses?

In the future yes, but I think we'll need further changes to enable that?

I guess we could try to parse both IPs - @derekhiggins what are your thoughts here?

@stbenjam
Copy link
Member Author

At least initially I was worried if a nic gets both IPv4 and IPv6 addresses we don’t actually know which one we’ll pick. Maybe that’s not a concern but I’ve usually been in environments where you get both rather than just IPv6.

@stbenjam
Copy link
Member Author

Although for provisioning we have control so maybe I’m overthinking the problem.

@hardys
Copy link
Member

hardys commented Oct 18, 2019

At least initially I was worried if a nic gets both IPv4 and IPv6 addresses we don’t actually know which one we’ll pick

That is a valid concern, particularly if/when we lock down the service bind IPs to a specific interface/IP e.g #56 - in that case I don't think we can tell Ironic to listen on more than one protocol, only a wildcard or a specific IP?

My main worry with adding an explicit interface here is that we then have to wire in that new value via the installer and MAO, e.g like openshift/installer#2449 and openshift/machine-api-operator#402 - if we were to consider only the single-stack case initially we can just fix the validation in the installer to allow ipv6 addresses, whereas for dual-stack the interface changes are much more invasive, e.g duplicate configuration for all-the-things.

My vote is to detect the IP without any new flag, and to be safe we could make container startup fail if we detect more than one (non link-local) IP configured on the nic?

@hardys
Copy link
Member

hardys commented Oct 18, 2019

Another option would be to wire in the PROVISIONING_IP (or the entire configmap) to all containers, then we wouldn't have to detect the IP, but the disadvantage there is we could have a race between the static-ip-manager container and those running the services?

@dhellmann
Copy link
Member

Another option would be to wire in the PROVISIONING_IP (or the entire configmap) to all containers, then we wouldn't have to detect the IP, but the disadvantage there is we could have a race between the static-ip-manager container and those running the services?

We could have the start scripts wait for that exact IP, though, instead of waiting anything on the interface.

@hardys
Copy link
Member

hardys commented Oct 18, 2019

We could have the start scripts wait for that exact IP, though, instead of waiting anything on the interface.

That's a good point, and I guess we could also minimise the interface changes if we started using envFrom to pass the entire configmap into all containers vs specific keys e.g https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variables

@derekhiggins
Copy link
Member

Could we have dual stack environments where the host might be IPv4 and 6 addresses?

In the future yes, but I think we'll need further changes to enable that?

I guess we could try to parse both IPs - @derekhiggins what are your thoughts here?

We could parse both IP's if present but I guess we need a way to decide which to use if both are present, on the bootstrap node I'm doing this by testing which gateway I can ping but finding someway to pass IPv4 or 6 to be explicit is probably better.

-    nmcli c add type ethernet ifname $PROVISIONING_NIC con-name provisioning ip4 172.22.0.2/24 gw4 172.22.0.1
+    nmcli c add type ethernet ifname $PROVISIONING_NIC con-name provisioning ip4 172.22.0.2/24 gw4 172.22.0.1 ip6 fd2e:6f44:5dd8:b856::2/64 gw6 fd2e:6f44:5dd8:b856::1
...
+IP=$(ip r | grep $PROVISIONING_NIC | awk '/default/ {print $3};')
+if ! ping -c 1 $IP ; then
+    IP="[$(ip -6 r | grep $PROVISIONING_NIC | awk '/default/ {print $3};')]"
+fi

@hardys
Copy link
Member

hardys commented Oct 18, 2019

Ok so it sounds like the conclusion is to add an explicit PROVISIONING_IP interface, and if that exists it will override PROVISIONING_INTERFACE and wait for an interface to be up with that IP, at which point we then configure the service to use the specified IP.

This keeps things explicit but also avoids adding any new interfaces to the openshift integration (we already require provisioning_ip for some other containers in the BMO pod), although it will require some adjustment to the sample ConfigMap in the BMO.

@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 18, 2019
@stbenjam
Copy link
Member Author

I pushed some changes that reflect the discussion, what do you think? It prefers PROVISIONING_IP over _INTERFACE. I updated metal3-io/baremetal-operator#324 to use envFrom as well, so everything just gets made available.

Both instances of httpd come up and are accessible over IPv6:

[notstack@master-2 metal3-dev-env]$ curl -qs http://\[fd2e:6f44:5dd8:b856::1\]/inspector.ipxe 2>&1>/dev/null && echo "success!"
success!
[notstack@master-2 metal3-dev-env]$ curl -qs http://\[fd2e:6f44:5dd8:b856::2\]:6180/inspector.ipxe 2>&1>/dev/null && echo "success!"
success!

I agree we should try to bring over the LOCAL_IMAGE stuff from dev-scripts instead of relying on my quay.io builds, I can look at that on Monday if no one else gets to it before then.

@stbenjam stbenjam changed the title Listen IPv6 addresses for HTTPD Changes needed for IPv6 Oct 21, 2019
runhttpd.sh Show resolved Hide resolved
@hardys hardys added the CI label Oct 22, 2019
@metal3ci
Copy link

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1260/

@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2019
runhealthcheck.sh Outdated Show resolved Hide resolved
@metal3ci
Copy link

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1262/

grub.cfg Outdated Show resolved Hide resolved
grub.cfg Outdated Show resolved Hide resolved
dnsmasq.conf.ipv6 Outdated Show resolved Hide resolved
dnsmasq.conf.ipv6 Outdated Show resolved Hide resolved
runhttpd.sh Show resolved Hide resolved
@metal3ci
Copy link

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1264/

@metal3-io-bot metal3-io-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Nov 7, 2019
@metal3ci
Copy link

metal3ci commented Nov 7, 2019

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1298/

Dockerfile Outdated Show resolved Hide resolved
rundnsmasq.sh Outdated Show resolved Hide resolved
Copy link
Member

@hardys hardys left a comment

Choose a reason for hiding this comment

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

Looks good, just one rebase issue, and a question about the kernel/ramdisk symlinks

@hardys
Copy link
Member

hardys commented Nov 8, 2019

@maelk FYI this looks nearly ready - if we push a WIP pr to metal3-dev-env with IRONIC_IMAGE_LOCAL_IMAGE set, will the integration tests pick that up, or does the ironic image always get baked into the pre-built minikube?

This commit adds support for IPv6. When $PROVISIONING_IP is specified,
which may be an IPv6 address, the various containers will wait for that
IP to become available on an interface.

If the IP is IPv6, then we use an IPv6-specific configurations. Note,
IPv6 hosts are expected to be using UEFI boot, as we use snponly.efi.
snponly.efi uses the UEFI network stack instead of the iPXE drivers.
When using EDK2 OVMF + iPXE + ipxe.efi, we have seen lock-ups in
initialization of the hardware devices.

As neither CentOS nor RHEL distribute iPXE builds with IPv6 support, we
build them from source as part of the container build.
@metal3ci
Copy link

metal3ci commented Nov 8, 2019

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1299/

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2019
@juliakreger
Copy link
Member

@hardys Can you update your review flag for this? It only has one approver so far and needs 2 plus the lgtm if it is ready.

enable-ra
ra-param=PROVISIONING_INTERFACE,10

dhcp-vendorclass=set:pxe6,enterprise:343,PXEClient
Copy link
Member

Choose a reason for hiding this comment

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

We sure this is right? I think we're doing does not offer and the arch matches logic in ironic. :\ I would be worried about enterprises differing across NICs

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'm not sure, is the concern that other vendors might not use this? I got this value from @derekhiggins initial investigations of getting IPv6 PXE working. There's a note here https://dox.ipxe.org/dhcpv6_8h.html#a24258ff3db7cdd24d6084d5950c87852:

00151 /** DHCPv6 PXE vendor class
00152  *
00153  * The DHCPv6 vendor class includes a field for an IANA enterprise
00154  * number.  The EDK2 codebase uses the value 343, with the comment:
00155  *
00156  *     TODO: IANA TBD: temporarily using Intel's
00157  *
00158  * Since this "temporarily" has applied since at least 2010, we assume
00159  * that it has become a de facto standard.
00160  */
00161 #define DHCPV6_VENDOR_CLASS_PXE 343

Copy link
Member

@juliakreger juliakreger left a comment

Choose a reason for hiding this comment

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

Overall looks good, I'd prefer shardy to lgtm.

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, elfosardo, juliakreger, stbenjam

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 [dtantsur,juliakreger]

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

@metal3ci
Copy link

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1325/

@hardys hardys self-requested a review November 21, 2019 16:06
@hardys
Copy link
Member

hardys commented Nov 21, 2019

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2019
@metal3-io-bot metal3-io-bot merged commit 695933f into metal3-io:master Nov 21, 2019
@stbenjam stbenjam deleted the ipv6 branch November 21, 2019 16:13
hardys pushed a commit to hardys/metal3-dev-env that referenced this pull request Nov 21, 2019
A new version was pushed to https://quay.io/repository/metal3-io/ironic
since ipv6 support was added in metal3-io/ironic-image#107

This PR can be used to verify the integration tests are still working OK
hardys pushed a commit to hardys/ironic-image that referenced this pull request Nov 26, 2019
This commit adds support for IPv6. When $PROVISIONING_IP is specified,
which may be an IPv6 address, the various containers will wait for that
IP to become available on an interface.

If the IP is IPv6, then we use an IPv6-specific configurations. Note,
IPv6 hosts are expected to be using UEFI boot, as we use snponly.efi.
snponly.efi uses the UEFI network stack instead of the iPXE drivers.
When using EDK2 OVMF + iPXE + ipxe.efi, we have seen lock-ups in
initialization of the hardware devices.

As neither CentOS nor RHEL distribute iPXE builds with IPv6 support, we
build them from source as part of the container build.

 Conflicts:
	Dockerfile
	configure-ironic.sh

Related metal3-io change metal3-io#107
(cherry picked from commit 2ad2b0d)
elfosardo pushed a commit to elfosardo/ironic-image that referenced this pull request Oct 9, 2020
Bug 1884155: Explicitly set json_rpc.auth_strategy to noauth
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. CI lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants