Upgrade with deprecation of interface-rename #249

minglumlu · 2025-05-14T11:35:16Z

This PR copes with PR xapi-project/xen-api#6466
The commit 5c4e192 is to resolve the problem when host install is setting up its own networking.
The commit e0ee1e3 is to transform the data of interface-rename to upgraded system.

Signed-off-by: Ming Lu <[email protected]>

minglumlu · 2025-05-19T01:44:05Z

Hi @liulinC @DeliZhangX @stephenchengCloud @robhoes @psafont @lindig @rosslagerwall
May I please ask your review on this PR? Thanks.

liulinC · 2025-05-19T01:56:44Z

netutil.py

@@ -371,3 +371,13 @@ def disable_ipv6_module(root):
    dv6fd.write("alias net-pf-10 off\n")
    dv6fd.close()

+
+from xcp.net.ifrename.dynamic import DynamicRules


Is it possible/complicated that you do not use DynamicRules here?
The reason why I ask this is because you are now the only user want to use this and we are going to drop this feature from the library.

This is the only source of the last order. So it would have to be parsed either by the existing code, or new code. The existing code can be dropped when the upgrade path is not possible.

=====
It's not only source. Another is xapi db file :)

rosslagerwall · 2025-05-19T09:21:21Z

product.py

+                    nics = [(name, netutil.getHWAddr(name)) for name in netutil.getNetifList()]
+                    for mac in hwaddrs.lower().split(','):
+                        net_admin_ifaces += [name for (name, hwaddr) in nics if mac == hwaddr]
+                    if len(net_admin_ifaces) == 0:


if not net_admin_ifaces: is preferred. See PEP8:

For sequences, (strings, lists, tuples), use the fact that empty sequences are false: # Correct: if not seq: if seq: # Wrong: if len(seq): if not len(seq):

rosslagerwall · 2025-05-19T09:23:09Z

product.py

+                    net_admin_ifaces = []
+                    nics = [(name, netutil.getHWAddr(name)) for name in netutil.getNetifList()]
+                    for mac in hwaddrs.lower().split(','):
+                        net_admin_ifaces += [name for (name, hwaddr) in nics if mac == hwaddr]


I think list.extend() is preferred.

rosslagerwall · 2025-05-19T09:35:36Z

upgrade.py

+        if os.path.exists(src_file_path):
+            if self.convert_interface_rename_data(src_file_path, dest_file_path):
+                logger.log(f'Removing {interface_rename_dir} ...')
+                shutil.rmtree(interface_rename_dir)


Can't you read the data directly from the backup instead of copying it to the new installation and then immediately removing it?

rosslagerwall · 2025-05-19T09:39:54Z

upgrade.py

+
+    def convert_interface_rename_data(self, src_file_path, dest_file_path):
+        """
+        Convert data of interface-rename to the data consumed by networkd.


Since we use both xcp-networkd and systemd-networkd you should be explicit here.

rosslagerwall · 2025-05-19T09:43:16Z

upgrade.py

+                    position = match.group(1)
+                    line = f"{position}:mac=\"{mac}\"\n"
+                    logger.log(f"Converted interface-rename data: {line}")
+                    lines += line


Does this actually work? lines is a list and line is a string and the result of adding them is unlikely to be what you want:

>>> x = [] >>> x += "hello" >>> x ['h', 'e', 'l', 'l', 'o']

OK. It seems this works (as I tested it as a whole upgrade) but not in expected way like this.
I can make it write line by line as you are suggesting in another comment.

rosslagerwall · 2025-05-19T09:45:29Z

upgrade.py

+                    return False
+
+            with open(dest_file_path, mode='w', encoding='utf-8') as f:
+                f.writelines(lines)


Instead of building up a list of lines and then writing them like this, maybe it would be better to just write the file line by line?

rosslagerwall · 2025-05-19T09:48:49Z

upgrade.py

+                    lines += line
+                else:
+                    logger.error(f'Network device name {name} is not like ethN.')
+                    return False


I don't think returning an error here is the right thing to do. If something failed to be named correctly during the previous boot, wouldn't it be better to write out the information we have and let the upgrade complete rather than failing it and leaving an unbootable system?

(Possibly for the above parse error too.)

rosslagerwall · 2025-05-19T09:55:37Z

I've left some mostly minor comments, but I have a question at a higher level. It seems like the only thing it retrieves from the dynamic rules is the mapping between ethX number and MAC address. Doesn't XAPI/xcp-networkd already have this information in the database and it can then do the upgrade itself?

minglumlu · 2025-05-19T13:37:04Z

I've left some mostly minor comments, but I have a question at a higher level. It seems like the only thing it retrieves from the dynamic rules is the mapping between ethX number and MAC address. Doesn't XAPI/xcp-networkd already have this information in the database and it can then do the upgrade itself?

The mac-number mappings are in XAPI DB also, not in networkd db. But under this circumstance, xapi and networkd don't run. It has to parse from a file anyway, either xapi DB, or dynamic rules. Parsing from dynamic rules seems simpler as there is existing code for it.

rosslagerwall · 2025-05-19T14:40:38Z

I've left some mostly minor comments, but I have a question at a higher level. It seems like the only thing it retrieves from the dynamic rules is the mapping between ethX number and MAC address. Doesn't XAPI/xcp-networkd already have this information in the database and it can then do the upgrade itself?

The mac-number mappings are in XAPI DB also, not in networkd db. But under this circumstance, xapi and networkd don't run. It has to parse from a file anyway, either xapi DB, or dynamic rules. Parsing from dynamic rules seems simpler as there is existing code for it.

I wasn't suggesting that the host-installer parses either DB.

The output of the code here is some config file which the toolstack is presumably going to parse on first boot. If the information is already available in a toolstack DB, why not let the toolstack use the information it already has on first boot and skip this intermediate step?

minglumlu · 2025-05-20T01:53:54Z

The output of the code here is some config file which the toolstack is presumably going to parse on first boot. If the information is already available in a toolstack DB, why not let the toolstack use the information it already has on first boot and skip this intermediate step?

The interface ordering shall be done by networkd which starts before the xapi. In other words, the xapi relies on the order established by the networkd. At least, the networkd can't depend on xapi.

rosslagerwall · 2025-05-20T09:08:32Z

upgrade.py

@@ -487,6 +524,7 @@ def buildRestoreList(self, src_base):
        return restore_list

    completeUpgradeArgs = ['mounts', 'installation-to-overwrite', 'primary-disk', 'backup-partnum', 'logs-partnum', 'net-admin-interface', 'net-admin-bridge', 'net-admin-configuration']
+


Not sure if this was intentional or accidental but none of the other functions have a blank line between funcArgs = ... and def func(...

It's accidental :(

rosslagerwall · 2025-05-20T09:14:49Z

upgrade.py

+            if os.path.exists(src_file_path):
+                net_devs = netutil.net_devs_of_last_boot(src_file_path)
+            else:
+                logger.log(f"The interface-rename data file {src_file_path} does not exist.")


Should the function return early here?

In future upgrades, there will never be any interface rename data so continuing to create the file below seems unnecessary.

rosslagerwall · 2025-05-20T09:16:22Z

upgrade.py

+                logger.log(f"The interface-rename data file {src_file_path} does not exist.")
+
+            # The sorting result of interface-rename is to rename the interfaces to eth<N>
+            pattern = r'^eth(\d+)$'


For regexps used more than once, it would be better to do:

pattern = re.compile(r'^eth(\d+)$') ... match = pattern.search(name)

rosslagerwall · 2025-05-20T09:17:27Z

upgrade.py

+                        match = re.search(pattern, name)
+                        if match:
+                            position = match.group(1)
+                            line = f"{position}:mac=\"{mac}\"\n"


You can simplify this slightly by using single quotes: line = f'{position}:mac="{mac}"\n'

MarkSymsCtx · 2025-05-20T09:56:54Z

Don't forget to squash the fixup commits

The interface-rename functionality will be deprecated. Consequently, the names of the network interfaces will not be renamed as eth<N>. To setup the host installer's own networking, it can not use the name of management interface in product since the name is returned from networkd_db command and will not present in the new host installer's running environment. The solution is to use MAC address to find the interface. This also requires the networkd_db change to return the MAC address. Signed-off-by: Ming Lu <[email protected]>

The interface-rename functionality will disappear after the upgrade. The networkd will replace the interface-rename to maintain the order of the host network devices without renaming them. The order generated and maintained by interface-rename before upgrade should be passed to the networkd during upgrade so that the networkd can still keep the order after upgrade. This commit is to transform the data during upgrade. Signed-off-by: Ming Lu <[email protected]>

liulinC · 2025-05-22T03:35:44Z

upgrade.py

+            else:
+                logger.log(f"The interface-rename data file {src_file_path} does not exist.")
+
+            if not net_devs:


you can just return in above else, but not a big deal.

liulinC · 2025-05-22T03:50:44Z

upgrade.py

+            # The sorting result of interface-rename is to rename the interfaces to eth<N>
+            pattern = re.compile(r'^eth(\d+)$')
+            try:
+                with open(dest_file_path, mode='w', encoding='utf-8') as f:


I am afraid you would like to write 😄

net_devs |> List.filter_map(fun name, mac -> match pattern.match with | xxx-> Some xxx | _ -> None) |> List.iter( fun mac -> f.write(mac))

In the upgrade scenario, net devices order has been converted to initial rule and pass to new system. See xenserver/host-installer#249 This PR is to handle the remaining things in networkd: - Recognize upgrade scenario to run sort for upgrade - Generate changed interfaces list to update networkd db - Get management interface and address type from `/var/tmp/.previousInventory` and write to inventory

CP-54444: Remove interface-rename logic for legacy upgrade paths

e86cdb7

Signed-off-by: Ming Lu <[email protected]>

minglumlu mentioned this pull request May 14, 2025

CP-54444: Return MAC addresses to host installer xapi-project/xen-api#6466

Merged

liulinC reviewed May 19, 2025

View reviewed changes

rosslagerwall reviewed May 19, 2025

View reviewed changes

rosslagerwall reviewed May 20, 2025

View reviewed changes

rosslagerwall approved these changes May 20, 2025

View reviewed changes

minglumlu added 2 commits May 20, 2025 17:59

minglumlu force-pushed the private/mingl/CP-54444 branch from 66bfd32 to d0f9d8c Compare May 20, 2025 10:00

liulinC reviewed May 22, 2025

View reviewed changes

liulinC approved these changes May 22, 2025

View reviewed changes

liulinC reviewed May 22, 2025

View reviewed changes

liulinC merged commit 5eacb5c into xenserver:feature/host-netdev-order May 22, 2025

changlei-li mentioned this pull request Jun 9, 2025

CP-308260: Handle networkd upgrade xapi-project/xen-api#6511

Merged

		@@ -487,6 +524,7 @@ def buildRestoreList(self, src_base):
		return restore_list

		completeUpgradeArgs = ['mounts', 'installation-to-overwrite', 'primary-disk', 'backup-partnum', 'logs-partnum', 'net-admin-interface', 'net-admin-bridge', 'net-admin-configuration']

Upgrade with deprecation of interface-rename #249

Upgrade with deprecation of interface-rename #249

Uh oh!

Conversation

minglumlu commented May 14, 2025

Uh oh!

minglumlu commented May 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minglumlu May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosslagerwall May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosslagerwall commented May 19, 2025

Uh oh!

minglumlu commented May 19, 2025

Uh oh!

rosslagerwall commented May 19, 2025

Uh oh!

minglumlu commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarkSymsCtx commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liulinC May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minglumlu May 19, 2025 •

edited

Loading

rosslagerwall May 19, 2025 •

edited

Loading

liulinC May 22, 2025 •

edited

Loading