cloudinit/importer.py: print a meaningful message if import fails

[esposem]

Proposed Commit Message

cloudinit/importer.py: print a meaningful message if import fails

Sometimes an import might fail for different reasons: the string
is wrongly typed, or the module has a dependency that is not
installed in python.

We should print that there is an import error, otherwise it might be
really difficult to understand what is the root cause of this
issue. Currently, cloud-init just ignores the error and continues.
This can have fatal consequences, especially when used to pick
the datasource to use.

For example, we can have a missing/misspelled datasource file:
$ ls cloudinit/sources/DataSourceInexistent.py
ls: cannot access 'cloudinit/sources/DataSourceInexistent.py': No such file or directory
$ python
>>> __import__("cloudinit.sources.DataSourceInexistent")

and in this case cloud-init should also hint that the error is
"No module named 'cloudinit.sources.DataSourceInexistent'"

If instead the datasource exists but uses a dependency that
is not installed (for example netifaces for DataSourceVMware):
$ ls cloudinit/sources/DataSourceVMWare.py
cloudinit/sources/DataSourceVMWare.py
$ python
>>> __import__("cloudinit.sources.DataSourceVMware")

then cloud-init should hint that the error is
"No module named 'netifaces'"

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[TheRealFalcon]

Hey @esposem , theoretically, I agree that it's better to log when there's a failure, but with this enabled, we get about 50 or so lines about failed import. Those failed imports aren't a problem, but all of the log messages about them would be misleading. It looks like in the past logging around this was intentionally silenced: 141caf7#diff-5ad5eef05a628e49ee49dfa5c2128663ea6dd587e5af03d44a230aee9f7bdf8cR48-R49

Is it possible to add additional logging around where the find_import is being called?

[esposem]

Hi @TheRealFalcon, yes I also was not too happy about the tests I had to modify because of this additional log, but I did not expect it would be so many honestly.

The problem is that list_sources in cloudinit/sources/__init.py__ calls the find_module function, that then returns an empty list. And we don't know why the list is empty: it might be that the attributes of the datasource seached do not match, that the import fails, that the name is wrong (or file is missing) and so on. Do you have a better idea on how to handle this?

I don't really know how to handle this better (Update: see below), but I agree that this might not be the optimal solution, even though I wonder why are there all these imported modules missing, mainly the ubuntu. If they are missing, they should not be imported maybe?

[esposem]

@TheRealFalcon See if this is better: add the log as optional, and print only in cloudinit/sources/__init__.py, which is where I would like to have the log reported 😄

[TheRealFalcon]

@esposem , this still results in some false negatives as it can search for a datasource in multiple places.

Would something like this (against main) work?

diff --git a/cloudinit/sources/__init__.py b/cloudinit/sources/__init__.py
index 9083f3995..d55997bab 100644
--- a/cloudinit/sources/__init__.py
+++ b/cloudinit/sources/__init__.py
@@ -929,6 +929,11 @@ def list_sources(cfg_list, depends, pkg_list):
         m_locs, _looked_locs = importer.find_module(
             ds_name, pkg_list, ["get_datasource_list"]
         )
+        if not m_locs:
+            LOG.error(
+                f"Could not import {ds_name}. Does the DataSource exist and "
+                "is it importable?"
+            )
         for m_loc in m_locs:
             mod = importer.import_module(m_loc)
             lister = getattr(mod, "get_datasource_list")

[esposem]

Not exactly what I was hoping for, as the exact cause of the empty list is still not explained, but I guess there is nothing else we can do about it, isn't it?
I will change the PR with the suggested change.

Thank you!
Emanuele

[TheRealFalcon]

Thanks! I'm open to refactoring the import logic so we can get some more granularity without additional failures, but that would obviously be a larger undertaking.