feat(vmware): Convert imc network config to Networking config Version 2 #5937

PengpengSun · 2024-12-19T09:13:15Z

Proposed Commit Message

feat(vmware): Convert imc network config to Networking config Version 2

To support dhcp4-overrides and dhcp6-overrides, we need to convert network configuration read from imc configuration file to v2 instead of v1.
This PR also adds ipv6 routes support.

If you need to write multiple paragraphs, feel free.

Fixes LP: #1776452

Additional Context

Test Steps

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

…Version 2hhhhh

…ig-v2

github-actions · 2025-01-07T00:24:49Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

PengpengSun · 2025-01-07T01:59:28Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

tag @TheRealFalcon to reopen it, thanks~

TheRealFalcon · 2025-01-23T19:27:56Z

@sshedi , is this something you could review too?

TheRealFalcon

Overall the changes look good. I left a few inline comments.

TheRealFalcon · 2025-01-23T21:57:58Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

+            v4 = v4_addrs[0]
+            # Add the ipv4 default route
+            if nic.primary and v4.gateways:
+                route_list.append({"to": "default", "via": v4.gateways[0]})


default works on netplan, but isn't a standard v2 option. This won't work on OSes that don't use netplan.

Hi @TheRealFalcon , this converting code is to convert our customization config to network v2 config supported by cloud-init, so it will be a part of metadata but written to OS directly.

@PengpengSun , sorry if I'm not understanding something, but network v2 config does not support specifying to: "default". You could use something like 0.0.0.0/0, but to: "default" will result in an error on systems that don't use netplan. Is there something I'm missing about where this gets rendered to?

Thanks @TheRealFalcon , I get it now. Using 0.0.0.0/0 here.
And I need to test this on real guest os other than Ubuntu, maybe RHEL.

TheRealFalcon · 2025-01-23T21:58:36Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

+                v6_cidr = ipv6_mask_to_net_prefix(v6.netmask)
+                # Add the ipv6 default route
+                if nic.primary and v6.gateway:
+                    route_list.append({"to": "default", "via": v6.gateway})


same default comment here

Using ::/0 here

TheRealFalcon · 2025-01-23T22:00:47Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

+        nic_config_dict = {}
+        # match
+        match = self.gen_match(mac)
+        if match:


match will always be non-None so this check isn't necessary.

I think we can condense this function.

nic_config_dict = {} generators = [ self.gen_match(mac), self.gen_set_name(name), self.gen_wakeonlan(nic), self.gen_dhcp4(nic), self.gen_dhcp6(nic), self.gen_addresses(nic), self.gen_routes(nic), self.gen_nameservers() ] for value in generators: if value: nic_config_dict.update(value)

Updated this part as @sshedi suggested, thanks.

cloudinit/sources/helpers/vmware/imc/config_nic.py

TheRealFalcon · 2025-01-23T22:02:09Z

cloudinit/sources/helpers/vmware/imc/config_nic.py


+    def gen_dhcp4(self, nic):


Nit: for the rest of these methods, do you think it makes more sense to return {} rather than None? The typing becomes more consistent and doesn't require None checks then.

Yes, return {} now.

PengpengSun · 2025-01-26T02:45:29Z

Overall the changes look good. I left a few inline comments.

Thanks @TheRealFalcon for the review, I'm traveling during spring festival here, probably won't be able to address any comments until Feb.5.

sshedi

@PengpengSun I believe this change is opaque to user, right?

Need some changes, PTAL.

sshedi · 2025-01-28T17:45:23Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

+    def gen_set_name(self, name):
+        return {"set-name": name}
+
+    def gen_wakeonlan(self, nic):


def gen_wakeonlan(self, nic): return {"wakeonlan": nic.onboot}

Updated, thanks.

sshedi · 2025-01-28T17:46:44Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

-
-        return route_list
+            dhcp4.update({"dhcp4": False})
+        if dhcp4:


If you are okay with returning {} instead of None, you can just do return dhcp4 here

Yes, just return dhcp4 now.

sshedi · 2025-01-28T18:08:56Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

+        nic_config_dict = {}
+        # match
+        match = self.gen_match(mac)
+        if match:


I think we can condense this function.

nic_config_dict = {} generators = [ self.gen_match(mac), self.gen_set_name(name), self.gen_wakeonlan(nic), self.gen_dhcp4(nic), self.gen_dhcp6(nic), self.gen_addresses(nic), self.gen_routes(nic), self.gen_nameservers() ] for value in generators: if value: nic_config_dict.update(value)

sshedi · 2025-01-29T05:33:42Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

+    def gen_addresses(self, nic):
+        address_list = []
+        v4_cidr = 32
+        v6_cidr = 128


This value 128 never gets used.

Removed it.

sshedi · 2025-01-29T05:35:59Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

+        v4_addrs = nic.staticIpv4
+        if v4_addrs:
+            v4 = v4_addrs[0]
+            if v4.netmask:


Is specifying netmask is optional during static ip setting? please correct me if I'm wrong here.
If there is no netmask given this will result in <ip>/32 as netmask, is that fine?

I think netmask should be a mandatory field during static ip setting, also please add a test for the same.

Per our API definition, netmask is optional for ipv4, if no netmask given, <ip>/32 is the expected result.

see subnetMask in https://dp-downloads.broadcom.com/api-content/apis/API_VWSA_001/8.0U3/html/ReferenceGuides/vim.vm.customization.IPSettings.html

but it's mandatory if static ipv6 address included.

see subnetMask in https://dp-downloads.broadcom.com/api-content/apis/API_VWSA_001/8.0U3/html/ReferenceGuides/vim.vm.customization.FixedIpV6.html

I added 2 tests for both ipv4 and ipv6 cases.

sshedi · 2025-01-29T05:40:53Z

cloudinit/sources/helpers/vmware/imc/config_nic.py

-        # if not self._primaryNic:
-        #    route_list.extend(self._genIpv6Route(name, nic, addrs))
+        v4_cidr = 32
+        v6_cidr = 128


this is also unused

removed it.

…ig-v2

PengpengSun · 2025-02-05T08:44:06Z

@PengpengSun I believe this change is opaque to user, right?

Right, it converts customization config to network v2 config supported by cloud-init, not written network configuration to Guest OS directly.

Need some changes, PTAL.

Thanks @sshedi for the review, I will address the comments.

…ig-v2

sshedi

LGTM. Thanks @PengpengSun

TheRealFalcon

LGTM!

PengpengSun and others added 2 commits December 19, 2024 09:02

feat(vmware): Convert imc network configuration to Networking config …

9235947

…Version 2hhhhh

Merge branch 'canonical:main' into ds-vmware-imc-support-network-conf…

cef747e

…ig-v2

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 7, 2025

TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 7, 2025

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 22, 2025

TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 22, 2025

canonical deleted a comment from github-actions bot Jan 22, 2025

TheRealFalcon self-assigned this Jan 23, 2025

TheRealFalcon reviewed Jan 23, 2025

View reviewed changes

sshedi reviewed Jan 29, 2025

View reviewed changes

Merge branch 'canonical:main' into ds-vmware-imc-support-network-conf…

4f4a16c

…ig-v2

PengpengSun and others added 2 commits February 6, 2025 14:19

Merge branch 'canonical:main' into ds-vmware-imc-support-network-conf…

acbb251

…ig-v2

Address comments from TheRealFalcon and sshedi

52d7507

PengpengSun requested review from sshedi and TheRealFalcon February 6, 2025 08:15

sshedi approved these changes Feb 7, 2025

View reviewed changes

TheRealFalcon approved these changes Feb 7, 2025

View reviewed changes

TheRealFalcon merged commit b38ab5f into canonical:main Feb 7, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vmware): Convert imc network config to Networking config Version 2 #5937

feat(vmware): Convert imc network config to Networking config Version 2 #5937

PengpengSun commented Dec 19, 2024

github-actions bot commented Jan 7, 2025

PengpengSun commented Jan 7, 2025

TheRealFalcon commented Jan 23, 2025

TheRealFalcon left a comment

TheRealFalcon Jan 23, 2025

PengpengSun Feb 5, 2025

TheRealFalcon Feb 5, 2025 •

edited

Loading

PengpengSun Feb 6, 2025 •

edited

Loading

TheRealFalcon Jan 23, 2025

PengpengSun Feb 6, 2025

TheRealFalcon Jan 23, 2025

sshedi Jan 28, 2025

PengpengSun Feb 6, 2025

TheRealFalcon Jan 23, 2025

PengpengSun Feb 6, 2025

PengpengSun commented Jan 26, 2025

sshedi left a comment

sshedi Jan 28, 2025

PengpengSun Feb 6, 2025

sshedi Jan 28, 2025

PengpengSun Feb 6, 2025

sshedi Jan 28, 2025

sshedi Jan 29, 2025

PengpengSun Feb 6, 2025

sshedi Jan 29, 2025

PengpengSun Feb 6, 2025

sshedi Jan 29, 2025

PengpengSun Feb 6, 2025

PengpengSun commented Feb 5, 2025

sshedi left a comment

TheRealFalcon left a comment

feat(vmware): Convert imc network config to Networking config Version 2 #5937

feat(vmware): Convert imc network config to Networking config Version 2 #5937

Conversation

PengpengSun commented Dec 19, 2024

Proposed Commit Message

Additional Context

Test Steps

Merge type

github-actions bot commented Jan 7, 2025

PengpengSun commented Jan 7, 2025

TheRealFalcon commented Jan 23, 2025

TheRealFalcon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealFalcon Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

PengpengSun Feb 6, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PengpengSun commented Jan 26, 2025

sshedi 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PengpengSun commented Feb 5, 2025

sshedi left a comment

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

TheRealFalcon Feb 5, 2025 •

edited

Loading

PengpengSun Feb 6, 2025 •

edited

Loading