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

Fix OVN leases not showing static IPs #12236

Merged
merged 4 commits into from
Sep 15, 2023

Conversation

MusicDin
Copy link
Member

This PR fixes OVN leases not showing static IPs. Due to this error, dynamic leases also disappeared from the list after instance restart.

Fixes #12177

lxd/device/nic_ovn.go Outdated Show resolved Hide resolved
@@ -996,7 +996,7 @@ func (d *nicOVN) State() (*api.InstanceStateNetwork, error) {
// OVN only supports dynamic IP allocation if neither IPv4 or IPv6 are statically set.
if d.config["ipv4.address"] == "" && d.config["ipv6.address"] == "" {
instanceUUID := d.inst.LocalConfig()["volatile.uuid"]
dynamicIPs, err := d.network.InstanceDevicePortDynamicIPs(instanceUUID, d.name)
dynamicIPs, err := d.network.InstanceDevicePortIPs(instanceUUID, d.name)
if err == nil {
for _, dynamicIP := range dynamicIPs {
Copy link
Member

Choose a reason for hiding this comment

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

logic here needs to exclude any ips that are specified in the static NIC config otherwise we'll end up with duplicates

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 was looking at this one, but this part of code is executed only if both ipv4.address and ipv6.address are empty (line 997), so there should be no need for that.

Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Can you test if that statement:

// OVN only supports dynamic IP allocation if neither IPv4 or IPv6 are statically set.

Is still true with sticky IPs in play?

Copy link
Member Author

Choose a reason for hiding this comment

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

Setting only one static IP (either IPv4 or IPv6) results in the IP of the other family to be dynamically set. So, this should no longer be true?

Copy link
Member

@tomponline tomponline Sep 11, 2023

Choose a reason for hiding this comment

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

Setting IPv4 to static also sets IPv6 to EUI64 statically in OVN:

lxc launch ubuntu:jammy c1 --network ovn1 -d eth0,ipv4.address=10.143.147.10

sudo ovn-nbctl list logical_switch_port
addresses           : ["00:16:3e:8b:45:8f 10.143.147.10 fd42:875e:7708:3d62:216:3eff:fe8b:458f"]

But setting IPv6 statically without IPv4 results in another bug:

lxc launch ubuntu:jammy c1 --network ovn1 -d eth0,ipv6.address=fd42:875e:7708:3d62:216:3eff:fe8b:458f
Creating c1
Starting c1                                 
Error: Failed to start device "eth0": Failed setting up OVN port: Insufficient dynamic addresses allocated
Try `lxc info --show-log local:c1` for more info

Please can you look into that also.

Copy link
Member Author

Choose a reason for hiding this comment

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

If ipv6.dhcp.stateful is enabled, setting static IPv4 results in IPv6 (EUI64) not being shown.

// OVN only supports dynamic IP allocation if neither IPv4 or IPv6 are statically set
if d.config["ipv4.address"] == "" && d.config["ipv6.address"] == "" {
   ...
}

Because it is listed under leases:

$ lxc launch ubuntu:22.04 c2 -n ovn -d eth0,ipv4.address=10.100.100.15

$ lxc ls
+------+---------+----------------------+------------------------------------------------+-----------+-----------+
| NAME |  STATE  |         IPV4         |                      IPV6                      |   TYPE    | SNAPSHOTS |
+------+---------+----------------------+------------------------------------------------+-----------+-----------+
| c2   | RUNNING | 10.100.100.15 (eth0) |                                                | CONTAINER | 0         |
+------+---------+----------------------+------------------------------------------------+-----------+-----------+


$ lxc network list-leases ovn
+----------+-------------------+-----------------------------------------+---------+
| HOSTNAME |    MAC ADDRESS    |               IP ADDRESS                |  TYPE   |
+----------+-------------------+-----------------------------------------+---------+
| c2       | 00:16:3e:86:d5:d5 | 10.100.100.15                           | STATIC  |
+----------+-------------------+-----------------------------------------+---------+
| c2       | 00:16:3e:86:d5:d5 | fd42:1000:1000:1000:216:3eff:fe86:d5d5  | DYNAMIC |
+----------+-------------------+-----------------------------------------+---------+
| ovn.gw   |                   | 10.100.100.1                            | GATEWAY |
+----------+-------------------+-----------------------------------------+---------+
| ovn.gw   |                   | fd42:1000:1000:1000::1                  | GATEWAY |
+----------+-------------------+-----------------------------------------+---------+

However, if static IPv6 is set (and IPv4 is not), OVN router does not provide any dynamic IP (even though there is enough IPs available).

Commenting out the error line results in only static IPv6 being generated:

lxc launch ubuntu:22.04 c1 -n ovn -d eth0,ipv6.address=fd42:1000:1000:1000:1234:1234:1234:1234

$ lxc ls
+------+---------+----------------------+------------------------------------------------+-----------+-----------+
| NAME |  STATE  |         IPV4         |                      IPV6                      |   TYPE    | SNAPSHOTS |
+------+---------+----------------------+------------------------------------------------+-----------+-----------+
| c1   | RUNNING |                      | fd42:1000:1000:1000:1234:1234:1234:1234 (eth0) | CONTAINER | 0         |
+------+---------+----------------------+------------------------------------------------+-----------+-----------+

$ lxc network list-leases ovn
+----------+-------------------+-----------------------------------------+---------+
| HOSTNAME |    MAC ADDRESS    |               IP ADDRESS                |  TYPE   |
+----------+-------------------+-----------------------------------------+---------+
| c1       | 00:16:3e:06:31:3d | fd42:1000:1000:1000:1234:1234:1234:1234 | STATIC  |
+----------+-------------------+-----------------------------------------+---------+
| ovn.gw   |                   | 10.100.100.1                            | GATEWAY |
+----------+-------------------+-----------------------------------------+---------+
| ovn.gw   |                   | fd42:1000:1000:1000::1                  | GATEWAY |
+----------+-------------------+-----------------------------------------+---------+

This is actually the case for both IPv6 and IPv4, but when IPv4 is set, EUI64 is generated which prevents the error.

I'll dive deeper into this after fixing other issues.

@MusicDin MusicDin force-pushed the fix/network-zone-records branch 2 times, most recently from 59ce5df to 64e0f0c Compare September 11, 2023 15:43
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

This is on the right track, but lets make sure the implementation of the new function is efficient.

@tomponline
Copy link
Member

@MusicDin once you're happy with this can you also run this script https://github.com/canonical/lxd-ci/blob/main/tests/network-ovn inside a fresh Focal VM, you'll need to modify it slightly so it doesn't remove the lxd snap first, so that you can sideload your custom lxd binary into it.

@tomponline tomponline marked this pull request as draft September 12, 2023 09:43
@tomponline
Copy link
Member

Converted to draft until testing confirmed.

@MusicDin
Copy link
Member Author

Test lxd-ci/tests/network-ovn is passing, however, I would still like to dive deeper into static IP issues before having it reviewed again.

…tch port IPs

Signed-off-by: Din Music <din.music@canonical.com>
Signed-off-by: Din Music <din.music@canonical.com>
…s not set for a device

Signed-off-by: Din Music <din.music@canonical.com>
… function

Signed-off-by: Din Music <din.music@canonical.com>
@tomponline tomponline marked this pull request as ready for review September 15, 2023 12:38
@tomponline tomponline merged commit 881163d into canonical:main Sep 15, 2023
26 checks passed
@tomponline
Copy link
Member

tomponline commented Sep 15, 2023

I would still like to dive deeper into static IP issues before having it reviewed again.

It LGTM, we should probably prevent setting ipv6.address if ipv4.address isnt also set.
But that can be in a separate PR.

@MusicDin
Copy link
Member Author

Okey, I'll do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Network zone records disappear after container restart when using OVN
2 participants