Skip to content

fix: net_convert.py: make some import failures not generate an error#6399

Merged
aciba90 merged 1 commit into
canonical:mainfrom
dermotbradley:make-net_convert.py-imports-optional
Sep 2, 2025
Merged

fix: net_convert.py: make some import failures not generate an error#6399
aciba90 merged 1 commit into
canonical:mainfrom
dermotbradley:make-net_convert.py-imports-optional

Conversation

@dermotbradley
Copy link
Copy Markdown
Contributor

@dermotbradley dermotbradley commented Aug 17, 2025

Proposed Commit Message

fix: net_convert.py: make some import failures not generate an error)

If cloud-init DataSources are separately packaged by a distro then it
is possible that running "cloud-init devel net-convert -h" (and trying
to convert between some network formats) can fail with errors if
Azure, Openstack, or VMware cloud-init code is not present.

I observed these errors when running the above command on an Alpine
system where cloud-init's various DataSources are separately packaged
and only the NoCloud DataSource was installed.

This PR prevents import failures of Azure, Openstack, and VMware
specific code from causing such errors. Instead the accepted list of
inputs ("-k") is dynamically adapted based on the presence/absence of
each of the Azure/Openstack/VMware related code.

NOTE: I realise that cloud-init does not currently officially support
the packaging of DataSources code separately. However I believe that
Ubuntu is currently in the process of developing/testing such separate
DataSources packaging and so this PR would be relevant to Ubuntu also.

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>)

@dermotbradley dermotbradley force-pushed the make-net_convert.py-imports-optional branch from 42317e1 to d5eb354 Compare August 17, 2025 20:27
Copy link
Copy Markdown
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement, @dermotbradley.

I would rather perform the imports where they are needed because:

  1. I think it is clearer for the reader to see them directly in the args.kind switch, in order to understand they are not needed universally
  2. If they are needed and not present, the error will still be an import error, conveying clearly what happened, as opposed to a more obscure error.
diff --git a/cloudinit/cmd/devel/net_convert.py b/cloudinit/cmd/devel/net_convert.py
index b3c2fa7c5..3e92c1661 100755
--- a/cloudinit/cmd/devel/net_convert.py
+++ b/cloudinit/cmd/devel/net_convert.py
@@ -20,9 +20,6 @@ from cloudinit.net import (
     networkd,
     sysconfig,
 )
-from cloudinit.sources import DataSourceAzure as azure
-from cloudinit.sources.helpers import openstack
-from cloudinit.sources.helpers.vmware.imc import guestcust_util
 
 NAME = "net-convert"
 
@@ -124,15 +121,21 @@ def handle_args(name, args):
                 "\n".join(["Input YAML", safeyaml.dumps(pre_ns), ""])
             )
     elif args.kind == "network_data.json":
+        from cloudinit.sources.helpers import openstack
+
         pre_ns = openstack.convert_net_json(
             json.loads(net_data), known_macs=known_macs
         )
     elif args.kind == "azure-imds":
+        from cloudinit.sources import DataSourceAzure as azure
+
         pre_ns = azure.generate_network_config_from_instance_network_metadata(
             json.loads(net_data)["network"],
             apply_network_config_for_secondary_ips=True,
         )
     elif args.kind == "vmware-imc":
+        from cloudinit.sources.helpers.vmware.imc import guestcust_util
+
         config = guestcust_util.Config(
             guestcust_util.ConfigFile(args.network_data.name)
         )

@dermotbradley dermotbradley force-pushed the make-net_convert.py-imports-optional branch from d5eb354 to 01c2764 Compare August 20, 2025 00:02
@dermotbradley
Copy link
Copy Markdown
Contributor Author

dermotbradley commented Aug 20, 2025

@aciba90

I would rather perform the imports where they are needed because:

1. I think it is clearer for the reader to see them directly in the `args.kind` switch, in order to understand they are not needed universally

2. If they are needed and not present, the error will still be an import error, conveying clearly what happened, as opposed to a more obscure error.

I took a different approach, I changed things so that the supported input kind list adapts based on whether the Azure, Openstack, and VMware code can be imported or not.

@dermotbradley dermotbradley force-pushed the make-net_convert.py-imports-optional branch 4 times, most recently from bb9155d to e89629c Compare August 20, 2025 00:55
Copy link
Copy Markdown
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

Thanks for refactoring this. The new approach is a bit more complex, and the justification is that we want the command to adjust its kinds depending on the available datasources, right?

Comment thread cloudinit/cmd/devel/net_convert.py
@dermotbradley dermotbradley force-pushed the make-net_convert.py-imports-optional branch 2 times, most recently from 01b11bc to d16e140 Compare August 22, 2025 00:33
@dermotbradley dermotbradley requested a review from aciba90 August 22, 2025 00:39
@dermotbradley
Copy link
Copy Markdown
Contributor Author

Thanks for refactoring this. The new approach is a bit more complex, and the justification is that we want the command to adjust its kinds depending on the available datasources, right?

Yupe, the Azure/Openstack/VMware inputs conditionally appear in the help output and are accepted/rejected as valid net-config input options depending on whether their Python code is present.

Copy link
Copy Markdown
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. One naming request.

In addition to that, could you please update the commit message to reflect the current new behavior (dynamic kinds)?

Comment thread cloudinit/cmd/devel/net_convert.py Outdated
If cloud-init DataSources are separately packaged by a distro then it
is possible that running "cloud-init devel net-convert -h" (and trying
to convert between some network formats) can fail with errors if
Azure, Openstack, or VMware cloud-init code is not present.

I observed these errors when running the above command on an Alpine
system where cloud-init's various DataSources are separately packaged
and only the NoCloud DataSource was installed.

This PR prevents import failures of Azure, Openstack, and VMware
specific code from causing such errors. Instead the accepted list of
inputs ("-k") is dynamically adapted based on the presence/absence of
each of the Azure/Openstack/VMware related code.
@dermotbradley dermotbradley force-pushed the make-net_convert.py-imports-optional branch from d16e140 to 4bdfbfa Compare August 22, 2025 19:11
@dermotbradley
Copy link
Copy Markdown
Contributor Author

Thanks for the fixes. One naming request.

In addition to that, could you please update the commit message to reflect the current new behavior (dynamic kinds)?

Done.

Copy link
Copy Markdown
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dermotbradley
Copy link
Copy Markdown
Contributor Author

Are there any outstanding actions remaining before this can be merged?

@aciba90 aciba90 merged commit ada8b0c into canonical:main Sep 2, 2025
22 checks passed
blackboxsw pushed a commit to blackboxsw/cloud-init that referenced this pull request Sep 3, 2025
…nical#6399)

If cloud-init DataSources are separately packaged by a distro then it
is possible that running "cloud-init devel net-convert -h" (and trying
to convert between some network formats) can fail with errors if
Azure, Openstack, or VMware cloud-init code is not present.

I observed these errors when running the above command on an Alpine
system where cloud-init's various DataSources are separately packaged
and only the NoCloud DataSource was installed.

This PR prevents import failures of Azure, Openstack, and VMware
specific code from causing such errors. Instead the accepted list of
inputs ("-k") is dynamically adapted based on the presence/absence of
each of the Azure/Openstack/VMware related code.

NOTE: I realise that cloud-init does not currently officially support
the packaging of DataSources code separately. However I believe that
Ubuntu is currently in the process of developing/testing such separate
DataSources packaging and so this PR would be relevant to Ubuntu also.
blackboxsw pushed a commit to blackboxsw/cloud-init that referenced this pull request Sep 3, 2025
…nical#6399)

If cloud-init DataSources are separately packaged by a distro then it
is possible that running "cloud-init devel net-convert -h" (and trying
to convert between some network formats) can fail with errors if
Azure, Openstack, or VMware cloud-init code is not present.

I observed these errors when running the above command on an Alpine
system where cloud-init's various DataSources are separately packaged
and only the NoCloud DataSource was installed.

This PR prevents import failures of Azure, Openstack, and VMware
specific code from causing such errors. Instead the accepted list of
inputs ("-k") is dynamically adapted based on the presence/absence of
each of the Azure/Openstack/VMware related code.

NOTE: I realise that cloud-init does not currently officially support
the packaging of DataSources code separately. However I believe that
Ubuntu is currently in the process of developing/testing such separate
DataSources packaging and so this PR would be relevant to Ubuntu also.
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.

2 participants