feat(azure): add apply_network_config_set_name option to disable renames#6807
feat(azure): add apply_network_config_set_name option to disable renames#6807cjp256 wants to merge 5 commits intocanonical:mainfrom
Conversation
Azure IMDS does not guarantee NIC ordering across reboots, which can cause interface name instability when using set-name to rename interfaces to ethX. Add a new datasource config option apply_network_config_set_name (default: True) that controls whether set-name directives are included in the generated network config. When disabled, interfaces are identified by MAC address using the naming format nicXXXXXXXXXXXX (e.g., nic000d3a047598) instead of ethX, allowing the kernel/udev to assign stable names. Changes: - Add apply_network_config_set_name to BUILTIN_DS_CONFIG - Set default via _unpickle for backward compatibility with pickled state - Toggle netplan outputs per configuration - Update azure.rst documentation with IMDS reference link - Add parametrized unit tests for both enabled/disabled paths Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
github-actions
Bot
added
the
documentation
|
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 blackboxsw, 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 blackboxsw to reopen it.) |
github-actions
Bot
added
the
stale-pr
|
Nope. I should not have let this age out. Sorry @cjp256 . Will get review on this shortly. |
|
@blackboxsw gentle ping, I know you're busy :D |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Test coverage, docs and behavior look good. I think we need a bit of a fix for unpickle handling and better test coverage there.
Additionally, if we can provide a set of logs establishing successfull behavior across reboot with set-name False that would be ideal. Thanks for the contributions Chris.
| self._system_uuid = None | ||
| self._vm_id = None | ||
| self._wireserver_endpoint = DEFAULT_WIRESERVER_ENDPOINT | ||
| self.ds_cfg.setdefault( |
There was a problem hiding this comment.
Good coverage of unpickle case across upgrade path && system reboot.
-
While we are here, shouldn't we also be setting apply_network_config_for_secondary_ips within
_unpickletoo to ensure those defaults under which the prior instance launch are preserved in the unpickled datasource? -
I think we also have a preexisting test coverage gap which should have alerted us to this potential _unpickle initialization issue w.r.t.
apply_network_config_for_secondary_ips
--- a/tests/unittests/test_upgrade.py
+++ b/tests/unittests/test_upgrade.py
@@ -294,6 +294,8 @@ class TestUpgrade:
missing_attrs = ds.__dict__.keys() - previous_obj_pkl.__dict__.keys()
for attr in missing_attrs:
assert attr in expected
+ missing_ds_cfg_attrs = ds.ds_cfg.keys() - previous_obj_pkl.ds_cfg.keys()
+ assert set() == missing_ds_cfg_attrs
def test_networking_set_on_distro(self, previous_obj_pkl):
"""We always expect to have ``.networking`` on ``Distro`` objects."""This test change above also found another disparity with experimental_skip_ready_report too.
| apply_network_config_for_secondary_ips=self.ds_cfg.get( | ||
| "apply_network_config_for_secondary_ips" | ||
| ), | ||
| apply_network_config_set_name=self.ds_cfg.get( | ||
| "apply_network_config_set_name" | ||
| ), |
There was a problem hiding this comment.
Given that _unpickle and __init__ should properly set both apply_network_config_for_secondary_ips and apply_network_config_set_name values to defaults (or overrides in system config files), do we need to use ds_cfg.get or can we instead just use the return the expected value at the key index with self.ds_cfg["apply_network_config_set_name"]?
| network_metadata: dict, | ||
| *, | ||
| apply_network_config_for_secondary_ips: bool, | ||
| apply_network_config_set_name: bool = True, |
There was a problem hiding this comment.
I don't think we need a default value on this mandatory param as we are setting it in both __init__ and _unpickle.
| apply_network_config_set_name: bool = True, | |
| apply_network_config_set_name: bool, |
| if apply_network_config_set_name: | ||
| nicname = "eth{idx}".format(idx=idx) | ||
| else: | ||
| nicname = "enx{mac}".format(mac=mac.replace(":", "")) |
There was a problem hiding this comment.
I confirm predictable interface naming convention prefix aligns with predictable naming expectations in systemd
There was a problem hiding this comment.
Pull request overview
Adds an Azure datasource configuration toggle to control whether generated network v2 config includes set-name directives, addressing NIC ordering instability in Azure IMDS that can cause ethX renames to change across reboots.
Changes:
- Introduces
apply_network_config_set_name(defaultTrue) in Azure datasource config and ensures backward compatibility for previously pickled datasource state. - Updates Azure IMDS network-config generation to optionally omit
set-nameand use MAC-derived identifiers when disabled. - Extends unit tests and updates Azure datasource documentation to describe the new option and motivation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cloudinit/sources/DataSourceAzure.py |
Adds new ds_cfg option and threads it into network-config generation to optionally omit set-name. |
doc/rtd/reference/datasources/azure.rst |
Documents apply_network_config_set_name and explains the IMDS NIC ordering issue; updates example config. |
tests/unittests/sources/test_azure.py |
Adds parametrized tests covering both enabled/disabled set-name paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| An example configuration with the default values is provided below: | ||
|
|
||
| .. code-block:: yaml | ||
|
|
||
| datasource: | ||
| Azure: | ||
| apply_network_config: true | ||
| apply_network_config_for_secondary_ips: true | ||
| apply_network_config_set_name: false | ||
| data_dir: /var/lib/waagent |
| mac = normalize_mac_address(intf["macAddress"]) | ||
| if apply_network_config_set_name: | ||
| nicname = "eth{idx}".format(idx=idx) | ||
| else: | ||
| nicname = "enx{mac}".format(mac=mac.replace(":", "")) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Azure IMDS does not guarantee NIC ordering across reboots, which can cause interface name instability when using set-name to rename interfaces to ethX. Add a new datasource config option apply_network_config_set_name (default: True) that controls whether set-name directives are included in the generated network config.
When disabled, interfaces are identified by MAC address using the naming format nicXXXXXXXXXXXX (e.g., nic000d3a047598) instead of ethX in configuration. Without set-name, the kernel/udev are able to assign stable names.
Changes:
Without this change:
With this change: