microcloud: Subnet sharing warning should check interfaces not IPs #522

gabrielmougard · 2024-12-02T14:18:05Z

closes #516

roosterfish

Thanks for moving the existing check from IPs to interfaces. Please have a look at my last comment, I think we should check the other two remaining ifaces as well.

service/lxd_config.go

cmd/microcloud/ask.go

roosterfish · 2024-12-03T08:51:29Z

cmd/microcloud/main_init.go

-		if subnet.Contains(underlayIP) {
-			fmt.Printf("Warning: OVN underlay IP (%s) is shared with the Ceph cluster network (%s)\n", underlayIP.String(), subnet.String())
+		if sys.OVNGeneveIface == sys.MicroCephInternalNetworkIface {
+			fmt.Printf("Warning: OVN underlay (IP: %s) is shared on the same network interface %q with the Ceph cluster network (IP: %s)\n", underlayIP.String(), sys.OVNGeneveIface, subnet.String())


subnet above seems to be redundant now as you can just use sys.MicroCephInternalNetworkSubnet instead of subnet.String().

cmd/microcloud/main_init.go

roosterfish · 2024-12-03T15:36:31Z

Not related to the PR but since we have switched the runners to the free ones this error seems to become peristent. It's probably caused by this so I am wondering if the test bed reset is just failing here.

masnax

Looks good, just one suggestion to reduce complexity & prevent having lots of extra fields & types.

masnax · 2024-12-04T01:20:57Z

cmd/microcloud/ask.go

+// ipWithIface is a helper struct to store an IP address and
+// its corresponding network interface.
+type ipWithIface struct {
+	ip        net.IP
+	ifaceName string
+}
+


Instead of maintaining both this and several fields for each interface on InitSystem, why not just introduce a new type Network with 3 fields: Interface, IP, Subnet which are populated by the corresponding values from net.Interface.

Then each of OVNGeneveAddr MicroCephInternalSubnet, and MicroCephPublicSubnet can be changed to OVNGeneveNetwork, CephInternalNetwork, CephPublicNetwork respectively.

And finally validateSystems you can check per-cluster member if any local interface names clash, and then across all cluster members if the subnets clash.

gabrielmougard · 2024-12-05T10:49:08Z

@masnax @roosterfish I reworked the approach based on #522 (comment)

roosterfish

Hi, just saw I never submitted the review. Please have a look when you find time.

cmd/microcloud/ask.go

roosterfish · 2024-12-05T11:30:17Z

cmd/microcloud/ask.go

 		c.systems[sh.Name] = bootstrapSystem

 		// This is to avoid the situation where the internal network for Ceph has been skipped, but the public network has been set.
 		// Ceph will automatically set the internal network to the public Ceph network if the internal network is not set, which is not what we want.
 		// Instead, we still want to keep the internal Ceph network to use the MicroCloud internal network as a default.
 		if internalCephSubnet == microCloudInternalNetworkAddrCIDR {
-			bootstrapSystem.MicroCephInternalNetworkSubnet = microCloudInternalNetworkAddrCIDR
+			microcloudNetworkInterface, err := lxd.FindInterfaceForSubnet(microCloudInternalNetworkAddrCIDR)


I wonder if InitSystem should grow another MicroCloudInternalNetwork *Network where we can store this information after the user has selected the internal address for MicroCloud?

Yes I had the same thinking I remember... For now, it seems that we don't need it for the current state of things but it might be needed in the future.

My thinking was if we now check for collisions in between the OVN and Ceph networks, wouldn't it be worth also including the MicroCloud internal network into those checks?

cmd/microcloud/preseed.go

roosterfish · 2024-12-09T13:49:55Z

cmd/microcloud/main_init.go

+		// If there are multiple subnet types on the same subnet, we have a collision.
+		if len(subnetTypeToPeers) > 1 {
+			var sb strings.Builder
+			sb.WriteString("WARNING: Subnet collision detected:\n")


As this PR will probably land after the "look & feel" I'll add a link here for reference on the note box #505 (comment)

roosterfish · 2025-01-07T08:40:36Z

Also requires a rebase now due to the recent CLI changes.

gabrielmougard · 2025-01-07T08:42:39Z

Thanks for the feedback!

gabrielmougard · 2025-01-07T09:39:55Z

@roosterfish updated

cmd/microcloud/main_init.go

roosterfish · 2025-01-08T11:09:17Z

cmd/microcloud/ask.go

 		c.systems[sh.Name] = bootstrapSystem

 		// This is to avoid the situation where the internal network for Ceph has been skipped, but the public network has been set.
 		// Ceph will automatically set the internal network to the public Ceph network if the internal network is not set, which is not what we want.
 		// Instead, we still want to keep the internal Ceph network to use the MicroCloud internal network as a default.
 		if internalCephSubnet == microCloudInternalNetworkAddrCIDR {
-			bootstrapSystem.MicroCephInternalNetworkSubnet = microCloudInternalNetworkAddrCIDR
+			microcloudNetworkInterface, err := lxd.FindInterfaceForSubnet(microCloudInternalNetworkAddrCIDR)


My thinking was if we now check for collisions in between the OVN and Ceph networks, wouldn't it be worth also including the MicroCloud internal network into those checks?

cmd/microcloud/ask.go

cmd/microcloud/main_init.go

roosterfish

Thanks for adding the test cases!
The validation still seems to be outputting something else in case of single node MicroCloud. Please have a look at my comments.

cmd/microcloud/ask.go

cmd/microcloud/main_init.go

roosterfish · 2025-01-30T15:12:39Z

cmd/microcloud/main_init_test.go

+			name: "single system interface collision",
+			systems: map[string]InitSystem{
+				"system1": {
+					MicroCephInternalNetwork:  newNetwork("eth0", "10.0.1.0/24"),


I was looking for this test!
Because when you try to deploy a single node MicroCloud and select the same network for all MicroCloud internal, Ceph internal/public and OVN geneve you only get this:

- MicroCloud internal network, OVN underlay sharing network interface "enp5s0

So it proves detectCollisions does the right thing it's only that in the actual code it's presumably called with the wrong inputs.

Sorry, I don't think I understand what you mean.. What do you think I should change in this test?

This type will be used to store a variety of subnet informations and which local network interface they will use. This will be used both for configuring MicroCloud but for running validation as well (e.g, interface collisions within a member, subnet collisions between members of a cluster) Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

This will be needed to get a network interface info from a CIDR subnet notation (e.g, mostly in the case of configuring MicroCloud internal and public networks) Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

…MicroCephPublicNetwork` and `MicroCephInternalNetwork` instead of `OVNGeneveAddr`, `MicroCephPublicNetworkSubnet` and `MicroCephInternalNetworkSubnet` Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

This function detect the local network interface collisions and the global subet collisions Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

gabrielmougard requested a review from roosterfish December 2, 2024 14:18

gabrielmougard force-pushed the fix/subnet-sharing-warning branch 5 times, most recently from 8e01972 to 3c0887c Compare December 2, 2024 18:36

roosterfish requested changes Dec 3, 2024

View reviewed changes

gabrielmougard force-pushed the fix/subnet-sharing-warning branch 2 times, most recently from 9cec54a to eb8c057 Compare December 3, 2024 11:49

gabrielmougard requested a review from roosterfish December 3, 2024 13:16

masnax suggested changes Dec 4, 2024

View reviewed changes

gabrielmougard force-pushed the fix/subnet-sharing-warning branch from eb8c057 to a9f6ca7 Compare December 5, 2024 10:48

roosterfish mentioned this pull request Dec 9, 2024

Update MicroCloud look and feel #505

Merged

roosterfish requested changes Jan 7, 2025

View reviewed changes

gabrielmougard force-pushed the fix/subnet-sharing-warning branch from a9f6ca7 to e93d076 Compare January 7, 2025 09:39

roosterfish requested changes Jan 8, 2025

View reviewed changes

gabrielmougard force-pushed the fix/subnet-sharing-warning branch 3 times, most recently from 9f57467 to 1a8b231 Compare January 9, 2025 15:10

gabrielmougard requested review from roosterfish and masnax January 10, 2025 11:39

gabrielmougard force-pushed the fix/subnet-sharing-warning branch from 1a8b231 to 94488b6 Compare January 27, 2025 15:55

roosterfish reviewed Jan 28, 2025

View reviewed changes

cmd/microcloud/ask.go Outdated Show resolved Hide resolved

cmd/microcloud/main_init.go Outdated Show resolved Hide resolved

gabrielmougard force-pushed the fix/subnet-sharing-warning branch from 94488b6 to a3b5fb2 Compare January 29, 2025 15:07

roosterfish requested changes Jan 30, 2025

View reviewed changes

gabrielmougard added 5 commits January 31, 2025 16:39

service: Introduce the 'FindInterfaceForSubnet' function

6af10b3

This will be needed to get a network interface info from a CIDR subnet notation (e.g, mostly in the case of configuring MicroCloud internal and public networks) Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

cmd/microcloud: refactoring to respectively use OVNGeneveNetwork, `…

cad4787

…MicroCephPublicNetwork` and `MicroCephInternalNetwork` instead of `OVNGeneveAddr`, `MicroCephPublicNetworkSubnet` and `MicroCephInternalNetworkSubnet` Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

cmd/microcloud: Add the detectCollisions function

395b848

This function detect the local network interface collisions and the global subet collisions Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

cmd/microcloud: Add unit tests for detectCollisions

c6ab1d7

Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>

gabrielmougard force-pushed the fix/subnet-sharing-warning branch from a3b5fb2 to c6ab1d7 Compare January 31, 2025 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

microcloud: Subnet sharing warning should check interfaces not IPs #522

microcloud: Subnet sharing warning should check interfaces not IPs #522

gabrielmougard commented Dec 2, 2024

roosterfish left a comment

roosterfish Dec 3, 2024

roosterfish commented Dec 3, 2024

masnax left a comment

masnax Dec 4, 2024 •

edited

Loading

gabrielmougard Dec 4, 2024

gabrielmougard commented Dec 5, 2024

roosterfish left a comment

roosterfish Dec 5, 2024

gabrielmougard Jan 7, 2025

roosterfish Jan 8, 2025

roosterfish Dec 9, 2024

roosterfish commented Jan 7, 2025

gabrielmougard commented Jan 7, 2025

gabrielmougard commented Jan 7, 2025

roosterfish Jan 8, 2025

roosterfish left a comment

roosterfish Jan 30, 2025

roosterfish Jan 30, 2025

gabrielmougard Jan 31, 2025

microcloud: Subnet sharing warning should check interfaces not IPs #522

Are you sure you want to change the base?

microcloud: Subnet sharing warning should check interfaces not IPs #522

Conversation

gabrielmougard commented Dec 2, 2024

roosterfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roosterfish commented Dec 3, 2024

masnax left a comment

Choose a reason for hiding this comment

masnax Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmougard commented Dec 5, 2024

roosterfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roosterfish commented Jan 7, 2025

gabrielmougard commented Jan 7, 2025

gabrielmougard commented Jan 7, 2025

Choose a reason for hiding this comment

roosterfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masnax Dec 4, 2024 •

edited

Loading