Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cc_* module meta defintion over hardcoded vars (SC-888) #1385

Merged
merged 9 commits into from
Apr 28, 2022

Conversation

TheRealFalcon
Copy link
Member

@TheRealFalcon TheRealFalcon commented Apr 15, 2022

Proposed Commit Message

Use cc_* module meta defintion over hardcoded vars

Previously, each cc_* module required a hardcoded 'distro' and
'frequency' variable to control the respective distro to run on and
run frequency. If undefined or invalid, we silently changed this to
a default value.

Instead, this commit will validate the MetaSchema definition and raise
an exception if invalid. This means every module MUST contain a
MetaSchema definition.

Additionally, the Modules class was moved out of stages.py into its
own module, along with some helper functions in config/__init__.py.

Additional Context

Review the commits in order. One of the commits is a big cut and paste (file moves) with no changes, so easiest to review the changes separately after that.

@TheRealFalcon
Copy link
Member Author

TheRealFalcon commented Apr 15, 2022

I need to add a test to ensure things fail when a module doesn't have the required MetaSchema, but otherwise this PR is reviewable as-is. DONE

Copy link
Collaborator

@blackboxsw blackboxsw left a comment

Choose a reason for hiding this comment

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

Generally looks great. I like actually creating a separate cohesive cloudinit.config.modules. Couple change suggeestions and supplements.

"title": "Ensure Network Manager is not managing IPv6 interface",
"description": __doc__,
"distros": [ALL_DISTROS],
"frequency": PER_ALWAYS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to still have the local 'frequency' variable in case someone changes that value and doesn't notice this one in the future.

Suggested change
"frequency": PER_ALWAYS,
"frequency": frequency,

cloudinit/config/modules.py Show resolved Hide resolved
f"Module '{mod}' with name '{name}' MUST have a 'meta' attribute "
"of type 'MetaSchema'."
)
if mod.meta["frequency"] not in FREQUENCIES:
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 noisy errors in misconfigured modules instead of silent DEFAULT to PER_INSTANCE. Strict behavior for the win.

@@ -129,6 +144,22 @@ def _read_modules(self, name):
return module_list

def _fixup_modules(self, raw_mods):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth typing here to give concrete validation of return values? Maybe a pain for this at the moment.

from cloudinit.settings import PER_ALWAYS

frequency = PER_ALWAYS

# This module is undocumented
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not true :)

I think we probably want to filter out cmdline docs for modules which have "meta" but don't have specific schema defined, or we want a "hidden" flag. Otherwise we get [docs for them on the commandline because the module contains a 'meta' attr](via https://github.com/canonical/cloud-init/blob/main/cloudinit/config/schema.py#L709).

+Write Files Deferred
+------------
+**Summary:** Defer writing certain files
+
+This module is based on `'Write Files' <write-files>`__, and
+will handle all files from the write_files list, that have been
+marked as deferred and thus are not being processed by the
+write-files module.
+
+*Please note that his module is not exposed to the user through
+its own dedicated top-level directive.*
+
+Reset RMC
+------------
+**Summary:** reset rsct node id
+
+Reset RMC module is IBM PowerVM Hypervisor specific
+
+Reliable Scalable Cluster Technology (RSCT) is a set of software components,
+that  together provide a comprehensive clustering environment (RAS features)
+for IBM PowerVM based virtual machines. RSCT includes the Resource monitoring
+and control (RMC) subsystem. RMC is a generalized framework used for managing,
+monitoring, and manipulating resources. RMC runs as a daemon process on
+individual machines and needs creation of unique node id and restarts
+during VM boot.
+More details refer
+https://www.ibm.com/support/knowledgecenter/en/SGVKBA_3.2/admin/bl503_ovrv.htm
+
+This module handles
+- creation of the unique RSCT node id to every instance/virtual machine
+  and ensure once set, it isn't changed subsequently by cloud-init.
+  In order to do so, it restarts RSCT service.
+
+Prerequisite of using this module is to install RSCT packages.
+
 Mounts
 ------
 **Summary:** Configure mount points and swap files
@@ -2844,6 +2879,27 @@
         maxsize: 2G
     
 
+Refresh IPv6 interface and RMC
+------------------------------
+**Summary:** Ensure Network Manager is not managing IPv6 interface
+
+This module is IBM PowerVM Hypervisor specific
+
+Reliable Scalable Cluster Technology (RSCT) is a set of software components
+that together provide a comprehensive clustering environment(RAS features)
+for IBM PowerVM based virtual machines. RSCT includes the Resource
+Monitoring and Control (RMC) subsystem. RMC is a generalized framework used
+for managing, monitoring, and manipulating resources. RMC runs as a daemon
+process on individual machines and needs creation of unique node id and
+restarts during VM boot.
+More details refer
+https://www.ibm.com/support/knowledgecenter/en/SGVKBA_3.2/admin/bl503_ovrv.htm
+
+This module handles
+- Refreshing RMC
+- Disabling NetworkManager from handling IPv6 interface, as IPv6 interface
+  is used for communication between RMC daemon and PowerVM hypervisor.
+
 Foo
 ---
 **Summary:** Example module

cloudinit/config/cc_write_files_deferred.py Show resolved Hide resolved
"name": "Write Files Deferred",
"title": "Defer writing certain files",
"description": __doc__,
"distros": [ALL_DISTROS],
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you feel excited about cleaning out cruft, we could also drop any distros= ["all"] module attributes use ALL_DISTROS instead

$ fgrep -r "distro" cloudinit/config/cc_* | grep '"all"'
cloudinit/config/cc_bootcmd.py:distros = ["all"]
cloudinit/config/cc_chef.py:distros = ["all"]
cloudinit/config/cc_keys_to_console.py:distros = ["all"]
cloudinit/config/cc_locale.py:distros = ["all"]
cloudinit/config/cc_mcollective.py:distros = ["all"]
cloudinit/config/cc_migrator.py:distros = ["all"]
cloudinit/config/cc_mounts.py:distros = ["all"]
cloudinit/config/cc_update_etc_hosts.py:distros = ["all"]
cloudinit/config/cc_update_hostname.py:distros = ["all"]
cloudinit/config/cc_users_groups.py:    "distros": ["all"],
cloudinit/config/cc_write_files.py:    "distros": ["all"],

@blackboxsw
Copy link
Collaborator

The only real fix I think we need here is making sure cloud-init devel schema --docs all doesn't render documentation for modules without schema.

@TheRealFalcon
Copy link
Member Author

Addressed the comments as a separate commit (but force pushed due to rebase).

I'm not a huge fan of setting __doc__ = "", but our schema docs are fundamentally based on __doc__ and I don't think we should change that in this branch. I'm wondering if it's worth documenting every single module, even if there's no configuration involved. We do this for a handful of other modules (e.g., scripts-per-boot ), so is it worth providing these modules in the docs as well?

Copy link
Collaborator

@blackboxsw blackboxsw 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 cleanup. LGTM. Docs are clean and migration to a separate module looks good.

@blackboxsw blackboxsw merged commit 8a6be66 into canonical:main Apr 28, 2022
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.

None yet

2 participants