Revert ds identify optimization

[holmanb]

Additional Context

Reported in #4794, when an invalid configuration includes an invalid (empty) datasource_list, ds-identify will identify the wrong datasource.

While the datasource_list is invalid, it does represent a regression because previous ds-identify behavior was to silently ignore the invalid configuration and now it needlessly prevents cloud-init from running rather than identifying the correct datasource like it used to.

This issue was introduced in #4327.

Test Steps

$ tox -e py3 tests/unittests/test_ds_identify.py

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

[TheRealFalcon]

Not against a revert, but would it be better to add a warning/error upstream, and then patch this behavior downstream?

[holmanb]

Not against a revert, but would it be better to add a warning/error upstream, and then patch this behavior downstream?

We could do that, but by doing so we are choosing to break users when we don't have to in a common scenario of mistaken configuration (i.e.: "I thought cloud.cfg was a yaml file, what gives?").

I'm in agreement for complaining louder when user configurations are wrong, but I don't want to do that without actually making it reasonable for users to fix their issues. Most users aren't going to know to go digging ds-identify.log, they'll just see the following traceback in cloud-init.log, and assume that cloud-init is at fault. I suppose that we could make this more user friendly by checking for the invalid content in /run/cloud/cloud.cfg -> datasource_list: [ datasource_list:, None ] in Python and create a more user-friendly warning message in cloud-init.log.

However, if we only warn about this issue, that still leaves the part of this that amplifies #4796 unfixed (which I haven't mentioned directly yet). Notice the (non-xfail) MAAS tests. Do we really want cloud-init to run DataSourceMAAS._get_data() for every system that has the word MAAS appear in a cloud.cfg comment? That's just broken and very surprising for a user.

I think that I would be on board with just making a user-friendly warning in upstream releases and not making ds-identify silently ignore invalid configurations, however commit 816e05d introduced more broken behavior than just that (and it doesn't break invalid configurations in a user-friendly way), which is why I propose reverting - at least for now.

2024-01-18 10:27:12,441 - {}init{}.py[DEBUG]: Looking for data source in: [\{'datasource_list': None}, 'None'], via packages ['', 'cloudinit.sources'] that matches dependencies ['FILESYSTEM', 'NETWORK']
2024-01-18 10:27:12,441 - util.py[WARNING]: failed stage init
2024-01-18 10:27:12,441 - util.py[DEBUG]: failed stage init
Traceback (most recent call last):
File "/usr/lib/python3.6/site-packages/cloudinit/cmd/main.py", line 781, in status_wrapper
ret = functor(name, args)
File "/usr/lib/python3.6/site-packages/cloudinit/cmd/main.py", line 394, in main_init
init.fetch(existing=existing)
File "/usr/lib/python3.6/site-packages/cloudinit/stages.py", line 493, in fetch
return self.get_data_source(existing=existing)
File "/usr/lib/python3.6/site-packages/cloudinit/stages.py", line 367, in get_data_source
self.reporter,
File "/usr/lib/python3.6/site-packages/cloudinit/sources/{}init{}.py", line 1001, in find_source
ds_list = list_sources(cfg_list, ds_deps, pkg_list)
File "/usr/lib/python3.6/site-packages/cloudinit/sources/{}init{}.py", line 1047, in list_sources
ds_name = importer.match_case_insensitive_module_name(ds)
File "/usr/lib/python3.6/site-packages/cloudinit/importer.py", line 40, in match_case_insensitive_module_name
if "nocloud-net" == mod_name.lower():
AttributeError: 'dict' object has no attribute 'lower'

[TheRealFalcon]

Yeah, you raise good points. I was initially thinking we could have ds-identify fail more loudly if it can't parse something correctly, but that requires more smarts than ds-identify currently has (hence this issue).

[TheRealFalcon]

Think it's worth putting an issue/commit reference in all of these tests (not just the xfail)? They just feel a bit random without the underlying context and if approaching otherwise I would think "why are we only testing maas here and not every other single datasource?"

[holmanb]

Think it's worth putting an issue/commit reference in all of these tests

I think you missed the rest of the docstring?

    The bug reported in 4794 combined with the previously existing bug
    reported in 4796 made for very loose MAAS false-positives.

Or maybe I misunderstand what you are asking for.

[holmanb]

I've added more context to the tests.

[TheRealFalcon]

Why's this needed?

[holmanb]

black disagrees about how to format the docstring. If you turn that off, black will require """ at the end of the single-line string, which makes it longer than 79 characters long.

[holmanb]

@smoser fyi ^^