cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

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', [])





Diff comments:

> diff --git a/cloudinit/config/cc_runcmd.py b/cloudinit/config/cc_runcmd.py
> index 7c3ccd4..0899d15 100644
> --- a/cloudinit/config/cc_runcmd.py
> +++ b/cloudinit/config/cc_runcmd.py
> @@ -22,7 +23,7 @@ from textwrap import dedent
>  # configuration options before actually attempting to deploy with said
>  # configuration.
>  
> -distros = ['all']
> +distros = [CONFIG_MODULE_SUPPORT_ALL_DISTROS]

That's really verbose...  maybe distros.ALL_DISTROS ?

>  
>  schema = {
>      'id': 'cc_runcmd',
> diff --git a/cloudinit/stages.py b/cloudinit/stages.py
> index a1c4a51..1eb92cd 100644
> --- a/cloudinit/stages.py
> +++ b/cloudinit/stages.py
> @@ -821,28 +821,34 @@ class Modules(object):
>          skipped = []
>          forced = []
>          overridden = self.cfg.get('unverified_modules', [])
> +        active_mods = []
> +        all_distros = set([distros.CONFIG_MODULE_SUPPORT_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 != SUPPORT_ALL_DISTROS
> +            #  - the current d_name isn't in distros
> +            #  - and the module name isn't overridden via unverified_modules

That comment is a bit unclear;  something like:

- and the module is unverified and not in the unverified_modules override list

> +            if worked_distros and worked_distros != all_distros:

I think you can drop empty check, all_distros is always going to be ['all'] so we won't take the path unless we've got something in worked_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():


-- 
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/330384
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:config-modules-allow-distros-all into cloud-init:master.