Merge ~chad.smith/cloud-init:config-modules-allow-distros-all into cloud-init:master

[Chad Smith]

cloud-config modules: honor distros definitions in each module

Modules can optionally define a list of supported distros on which they can run by declaring a distros attribute in the cc_*py module. This branch fixes handling of cloudinit.stages.Modules.run_section. The behavior of run_section is now the following:
 - always run a module if the module doesn't declare a distros attribute
 - always run a module if the module declares distros = [ALL_DISTROS]
 - skip a module if the distribution on which we run isn't in module.distros
 - force a run of a skipped module if unverified_modules configuration contains the module name

LP: #1715738
LP: #1715690

cloud-config modules: honor distros definitions in each module

Modules can optionally define a list of supported distros on which they can run by declaring a distros attribute in the cc_*py module. This branch fixes handling of cloudinit.stages.Modules.run_section. The behavior of run_section is now the following:
 - always run a module if the module doesn't declare a distros attribute
 - always run a module if the module declares distros = [CONFIG_MODULE_SUPPORT_ALL_DISTROS]
 - skip a module if the distribution on which we run isn't in module.distros
 - force a run of a skipped module if unverified_modules configuration contains the module name

LP: #1715738
LP: #1715690

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:547c29e3b8a05d769046bd4575984bfb9d972434
https://jenkins.ubuntu.com/server/job/cloud-init-ci/271/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/271/rebuild

review: Approve (continuous-integration)

95560e3... by Chad Smith on 2017-09-08

drop patchOS patchUtils from test_none_ds_forces_run_via_unverified_modules as it is done in setUp

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:f847f5b869e5eed51bc0502ab98d8fec7cd322b2
https://jenkins.ubuntu.com/server/job/cloud-init-ci/272/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/272/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Thanks for tackling this.

I do worry that we're changing existing behavior here: The skipped and forced were mostly advertising; cloud-init didn't not run modules just because they didn't declare their distro support.

If we're going to change that we should be careful about how we introduce the change.

Also, I do think we'd like a subcommand:

cloud-init devel list-modules [--mode=[config|final]] [--distro=[all|distro1|...]]

Maybe we could do something cleaner?

all_modules = find_all_modules
allowed_modules = [mod for mod in all_modules if d_name in mod.distros or mod.distros == 'all']
skipped = set(all_modules).difference(set(allowed_modules))
forced = cfg.get('unverified_modules', [])

[Scott Moser (smoser)]

I'm generally ok with this.
it does sure seem like we can clean up 'run_section'.

but this isnt making anything any worse.

[Ryan Harper (raharper)]

I'm ok with the changes, though I would much prefer not using a string that's 11 times longer than 'all';

if we can shorten that sanely, I'm +1

930181c... by Chad Smith on 2017-09-13

address raharper's review comments: -CONFIG_MODULE_SUPPORT_ALL_DISTROS -> ALL_DISTROS, drop empty conditional check worked_distros

1d890d0... by Chad Smith on 2017-09-13

condense multi-line params in unit test method calls

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:1d890d0d6cc4cdb8488e8a6ada068492e9e5eccb
https://jenkins.ubuntu.com/server/job/cloud-init-ci/292/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/292/rebuild

review: Needs Fixing (continuous-integration)

9a6a0ca... by Chad Smith on 2017-09-14

need to check that the module actually defines distros before deciding to skip

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:9a6a0ca79c108e432a3e2c2e009ff5464642563a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/293/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/293/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

review: Approve

diff --git a/cloudinit/config/cc_runcmd.py b/cloudinit/config/cc_runcmd.py
index 7f99569..449872f 100644
--- a/cloudinit/config/cc_runcmd.py
+++ b/cloudinit/config/cc_runcmd.py
@@ -10,6 +10,7 @@
 
 from cloudinit.config.schema import (
     get_schema_doc, validate_cloudconfig_schema)
+from cloudinit.distros import ALL_DISTROS
 from cloudinit.settings import PER_INSTANCE
 from cloudinit import util
 
@@ -23,7 +24,7 @@ from textwrap import dedent
 # configuration options before actually attempting to deploy with said
 # configuration.
 
-distros = ['all']
+distros = [ALL_DISTROS]
 
 schema = {
     'id': 'cc_runcmd',
diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index b714b9a..d5becd1 100755
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -30,6 +30,10 @@ from cloudinit import util
 from cloudinit.distros.parsers import hosts
 
 
+# Used when a cloud-config module can be run on all cloud-init distibutions.
+# The value 'all' is surfaced in module documentation for distro support.
+ALL_DISTROS = 'all'
+
 OSFAMILIES = {
     'debian': ['debian', 'ubuntu'],
     'redhat': ['centos', 'fedora', 'rhel'],
diff --git a/cloudinit/stages.py b/cloudinit/stages.py
index a1c4a51..d045268 100644
--- a/cloudinit/stages.py
+++ b/cloudinit/stages.py
@@ -821,28 +821,35 @@ class Modules(object):
         skipped = []
         forced = []
         overridden = self.cfg.get('unverified_modules', [])
+        active_mods = []
+        all_distros = set([distros.ALL_DISTROS])
         for (mod, name, _freq, _args) in mostly_mods:
-            worked_distros = set(mod.distros)
+            worked_distros = set(mod.distros)  # Minimally [] per fixup_modules
             worked_distros.update(
                 distros.Distro.expand_osfamily(mod.osfamilies))
 
-            # module does not declare 'distros' or lists this distro
-            if not worked_distros or d_name in worked_distros:
-                continue
-
-            if name in overridden:
-                forced.append(name)
-            else:
-                skipped.append(name)
+            # Skip only when the following conditions are all met:
+            #  - distros are defined in the module != ALL_DISTROS
+            #  - the current d_name isn't in distros
+            #  - and the module is unverified and not in the unverified_modules
+            #    override list
+            if worked_distros and worked_distros != all_distros:
+                if d_name not in worked_distros:
+                    if name not in overridden:
+                        skipped.append(name)
+                        continue
+                    forced.append(name)
+            active_mods.append([mod, name, _freq, _args])
 
         if skipped:
-            LOG.info("Skipping modules %s because they are not verified "
+            LOG.info("Skipping modules '%s' because they are not verified "
                      "on distro '%s'.  To run anyway, add them to "
-                     "'unverified_modules' in config.", skipped, d_name)
+                     "'unverified_modules' in config.",
+                     ','.join(skipped), d_name)
         if forced:
-            LOG.info("running unverified_modules: %s", forced)
+            LOG.info("running unverified_modules: '%s'", ', '.join(forced))
 
-        return self._run_modules(mostly_mods)
+        return self._run_modules(active_mods)
 
 
 def read_runtime_config():
diff --git a/tests/unittests/test_runs/test_simple_run.py b/tests/unittests/test_runs/test_simple_run.py
index 5cf666f..b8fb479 100644
--- a/tests/unittests/test_runs/test_simple_run.py
+++ b/tests/unittests/test_runs/test_simple_run.py
@@ -1,8 +1,6 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 import os
-import shutil
-import tempfile
 
 from cloudinit.tests import helpers
 
@@ -12,16 +10,19 @@ from cloudinit import util
 
 
 class TestSimpleRun(helpers.FilesystemMockingTestCase):
-    def _patchIn(self, root):
-        self.patchOS(root)
-        self.patchUtils(root)
-
-    def test_none_ds(self):
-        new_root = tempfile.mkdtemp()
-        self.addCleanup(shutil.rmtree, new_root)
-        self.replicateTestRoot('simple_ubuntu', new_root)
-        cfg = {
+
+    with_logs = True
+
+    def setUp(self):
+        super(TestSimpleRun, self).setUp()
+        self.new_root = self.tmp_dir()
+        self.replicateTestRoot('simple_ubuntu', self.new_root)
+
+        # Seed cloud.cfg file for our tests
+        self.cfg = {
             'datasource_list': ['None'],
+            'runcmd': ['ls /etc'],  # test ALL_DISTROS
+            'spacewalk': {},  # test non-ubuntu distros module definition
             'write_files': [
                 {
                     'path': '/etc/blah.ini',
@@ -29,14 +30,17 @@ class TestSimpleRun(helpers.FilesystemMockingTestCase):
                     'permissions': 0o755,
                 },
             ],
-            'cloud_init_modules': ['write-files'],
+            'cloud_init_modules': ['write-files', 'spacewalk', 'runcmd'],
         }
-        cloud_cfg = util.yaml_dumps(cfg)
-        util.ensure_dir(os.path.join(new_root, 'etc', 'cloud'))
-        util.write_file(os.path.join(new_root, 'etc',
+        cloud_cfg = util.yaml_dumps(self.cfg)
+        util.ensure_dir(os.path.join(self.new_root, 'etc', 'cloud'))
+        util.write_file(os.path.join(self.new_root, 'etc',
                                      'cloud', 'cloud.cfg'), cloud_cfg)
-        self._patchIn(new_root)
+        self.patchOS(self.new_root)
+        self.patchUtils(self.new_root)
 
+    def test_none_ds_populates_var_lib_cloud(self):
+        """Init and run_section default behavior creates appropriate dirs."""
         # Now start verifying whats created
         initer = stages.Init()
         initer.read_cfg()
@@ -51,10 +55,16 @@ class TestSimpleRun(helpers.FilesystemMockingTestCase):
         initer.update()
         self.assertTrue(os.path.islink("var/lib/cloud/instance"))
 
-        initer.cloudify().run('consume_data',
-                              initer.consume_data,
-                              args=[PER_INSTANCE],
-                              freq=PER_INSTANCE)
+    def test_none_ds_runs_modules_which_do_not_define_distros(self):
+        """Any modules which do not define a distros attribute are run."""
+        initer = stages.Init()
+        initer.read_cfg()
+        initer.initialize()
+        initer.fetch()
+        initer.instancify()
+        initer.update()
+        initer.cloudify().run('consume_data', initer.consume_data,
+                              args=[PER_INSTANCE], freq=PER_INSTANCE)
 
         mods = stages.Modules(initer)
         (which_ran, failures) = mods.run_section('cloud_init_modules')
@@ -63,5 +73,80 @@ class TestSimpleRun(helpers.FilesystemMockingTestCase):
         self.assertIn('write-files', which_ran)
         contents = util.load_file('/etc/blah.ini')
         self.assertEqual(contents, 'blah')
+        self.assertNotIn(
+            "Skipping modules ['write-files'] because they are not verified on"
+            " distro 'ubuntu'",
+            self.logs.getvalue())
+
+    def test_none_ds_skips_modules_which_define_unmatched_distros(self):
+        """Skip modules which define distros which don't match the current."""
+        initer = stages.Init()
+        initer.read_cfg()
+        initer.initialize()
+        initer.fetch()
+        initer.instancify()
+        initer.update()
+        initer.cloudify().run('consume_data', initer.consume_data,
+                              args=[PER_INSTANCE], freq=PER_INSTANCE)
+
+        mods = stages.Modules(initer)
+        (which_ran, failures) = mods.run_section('cloud_init_modules')
+        self.assertTrue(len(failures) == 0)
+        self.assertIn(
+            "Skipping modules 'spacewalk' because they are not verified on"
+            " distro 'ubuntu'",
+            self.logs.getvalue())
+        self.assertNotIn('spacewalk', which_ran)
+
+    def test_none_ds_runs_modules_which_distros_all(self):
+        """Skip modules which define distros attribute as supporting 'all'.
+
+        This is done in the module with the declaration:
+        distros = [ALL_DISTROS]. runcmd is an example.
+        """
+        initer = stages.Init()
+        initer.read_cfg()
+        initer.initialize()
+        initer.fetch()
+        initer.instancify()
+        initer.update()
+        initer.cloudify().run('consume_data', initer.consume_data,
+                              args=[PER_INSTANCE], freq=PER_INSTANCE)
+
+        mods = stages.Modules(initer)
+        (which_ran, failures) = mods.run_section('cloud_init_modules')
+        self.assertTrue(len(failures) == 0)
+        self.assertIn('runcmd', which_ran)
+        self.assertNotIn(
+            "Skipping modules 'runcmd' because they are not verified on"
+            " distro 'ubuntu'",
+            self.logs.getvalue())
+
+    def test_none_ds_forces_run_via_unverified_modules(self):
+        """run_section forced skipped modules by using unverified_modules."""
+
+        # re-write cloud.cfg with unverified_modules override
+        self.cfg['unverified_modules'] = ['spacewalk']  # Would have skipped
+        cloud_cfg = util.yaml_dumps(self.cfg)
+        util.ensure_dir(os.path.join(self.new_root, 'etc', 'cloud'))
+        util.write_file(os.path.join(self.new_root, 'etc',
+                                     'cloud', 'cloud.cfg'), cloud_cfg)
+
+        initer = stages.Init()
+        initer.read_cfg()
+        initer.initialize()
+        initer.fetch()
+        initer.instancify()
+        initer.update()
+        initer.cloudify().run('consume_data', initer.consume_data,
+                              args=[PER_INSTANCE], freq=PER_INSTANCE)
+
+        mods = stages.Modules(initer)
+        (which_ran, failures) = mods.run_section('cloud_init_modules')
+        self.assertTrue(len(failures) == 0)
+        self.assertIn('spacewalk', which_ran)
+        self.assertIn(
+            "running unverified_modules: 'spacewalk'",
+            self.logs.getvalue())
 
 # vi: ts=4 expandtab