Shell script handlers by freq

[chrislalos]

New PR to replace PR #745

Handlers for per-boot/per-instance/per-once multipart MIME

Add handlers for adding scripts to userdata that can be run at various
frequencies. Scripts of type x-shellscript-per-boot,
x-shellscript-per-instance, or x-shellscript-per-once can be added
to a multipart MIME userdata message as part of instance userdata.
These scripts will then be added to the appropriate per-boot,
per-instance, or per-once directory in /var/lib/cloud/scripts/
during processing of userdata.

Additional Context

Merge of upstream/main + fixes to pass integration tests.

This commit, my first since May '21, merges in all changes on upstream since then, including the master -> main branch name change. I also included the last suggestions made by Mr. Falcon, which I thought were great.

There are some unit tests failures with some unit tests I'd written that used to pass, but no longer do. The failure does not appear functional in nature and happens far deep down in the call hierarchy. I don't feel great checking code with a unittest failure but I believe I will need help in solving the issue. Thanks!

Test Steps

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]

When I dropped from 3 handler classes to 1 handler class, etc, it was based on a conversation we have back on Mar 5 21, on the old PR (first commit for first cloud-init PR #745). I had proposed the idea and you thought it sounded good. If it's just an idea that sounded good in the moment, but then turned out not to work, then no worries. I just wanted to make sure we weren't missing anything.

No, not missing anything in particular.

Sort of similarly to the above, it seems like the current code is using a class variable (prefixes) to customize the properties on objects. Sort of like a separate Person class per every common name, with a class var for name, instead of one class and an ivar. If my understanding is correct, then hypothetically that seems like a decent thing to fix, and among other things get back down to one class. But I don't have much enthusiasm to look at that now tbh.

Heh, funny you mention it. I was looking at doing that before I posted my comment, so went back to it and doing it that way should work too. This might be preferable to the diff I posted above:

diff --git a/cloudinit/handlers/shell_script_by_frequency.py b/cloudinit/handlers/shell_script_by_frequency.py
index 71d8031d..c820bf4c 100644
--- a/cloudinit/handlers/shell_script_by_frequency.py
+++ b/cloudinit/handlers/shell_script_by_frequency.py
@@ -42,25 +42,18 @@ def write_script_by_frequency(script_path, payload, frequency, scripts_dir):
 class ShellScriptByFreqPartHandler(Handler):
     """Common base class for the frequency-specific script handlers."""
 
-    prefixes = [
-        "text/x-shellscript-per-boot",
-        "text/x-shellscript-per-instance",
-        "text/x-shellscript-per-once",
-    ]
-
-    def __init__(self, freq, paths, **_kwargs):
-        print("ShellScriptByFreqPartHandler: c'tor()")
-        Handler.__init__(self, freq)
+    def __init__(self, script_frequency, paths, **_kwargs):
+        Handler.__init__(self, PER_ALWAYS)
+        self.prefixes = [f"text/x-shellscript-{pathMap[script_frequency]}"]
+        self.script_frequency = script_frequency
         self.scripts_dir = paths.get_cpath("scripts")
         if "script_path" in _kwargs:
             self.scripts_dir = paths.get_cpath(_kwargs["script_path"])
 
     def handle_part(self, data, ctype, script_path, payload, frequency):
-        print("ShellScriptByFreqPartHandler.handle_part()")
-        print("script_path=%s", script_path)
         if script_path is not None:
             filename = os.path.basename(script_path)
             filename = util.clean_filename(filename)
             write_script_by_frequency(
-                script_path, payload, self.frequency, self.scripts_dir
+                script_path, payload, self.script_frequency, self.scripts_dir
             )
diff --git a/cloudinit/stages.py b/cloudinit/stages.py
index 899b4e5e..3f17294b 100644
--- a/cloudinit/stages.py
+++ b/cloudinit/stages.py
@@ -520,21 +520,12 @@ class Init(object):
         # TODO(harlowja) Hmmm, should we dynamically import these??
         cloudconfig_handler = CloudConfigPartHandler(**opts)
         shellscript_handler = ShellScriptPartHandler(**opts)
-        shellscript_per_boot_handler = ShellScriptByFreqPartHandler(
-            PER_ALWAYS, **opts
-        )
-        shellscript_per_instance_handler = ShellScriptByFreqPartHandler(
-            PER_INSTANCE, **opts
-        )
-        shellscript_per_once_handler = ShellScriptByFreqPartHandler(
-            PER_ONCE, **opts
-        )
         def_handlers = [
             cloudconfig_handler,
             shellscript_handler,
-            shellscript_per_boot_handler,
-            shellscript_per_instance_handler,
-            shellscript_per_once_handler,
+            ShellScriptByFreqPartHandler(PER_ALWAYS, **opts),
+            ShellScriptByFreqPartHandler(PER_INSTANCE, **opts),
+            ShellScriptByFreqPartHandler(PER_ONCE, **opts),
             BootHookPartHandler(**opts),
             UpstartJobPartHandler(**opts),
         ]

Regarding this comment of yours: "even though we want the scripts to run per-whatever, the part handler itself should run per-always (similar to the existing shell script handler). On every boot, we should write the file to the correct directory, and then based what directory it is in, it runs as it should." ... am I correct that this is current behavior, and I don't actually have to do anything? Or ... actually, on second thought ... it means my handler does in fact need to write a script to the freq-specific folder and it needs to do so on every boot, irrespective of frequency. And then current behavior manages the script invocation once written, at the right time based on subfolder.

If you apply either of my patches, this should be correct behavior and you shouldn't have to do anything.

[chrislalos]

@TheRealFalcon Glad to see my idea was not so strange after all, and thanks for the implementation. I actually wasn't able to apply your initial patch per se, simply because I had already deleted print() statements before trying the patch, and I couldn't coerce the patch to match the file in its current form. I did faithfully recreate it by hand, and I could do the same with your above patch as well. Alternately, if you consider it much better git-fu and etiquette to revert back to the file(s) as they were when you made your patches, and then apply the patches against those versions, I can try that as well. I'd need to figure out just how to do that in git, but it seems straightforward.

One thing I'd feel better about, is if I followed your integration testing ideas and tested using a persistent lxd container on a Linux VM (I don't run Linux locally), and then shelled in after the testing and looked at everything for myself. I did have a VM set up for that on a discount cloud provider, but that VMs having unrelated issues and I can't use it at the moment. Hopefully I can get to it soon.

[chrislalos]

Incidentally, I've been surprised a few times that my code's passed CI, because there was something in my integration tests that seemed like it wouldn't work. But, I figured if CI passed than I guess I'm wrong.

However, I just checked the travis log for the integration tests step, and I could not find any mention of my tests or many other tests. The tests that did appear in there all seem to have a specific attribute: @pytest.mark.ci. Which sounds like marking the test so that it runs as part of CI.

Am I expected to mark my test with @pytest.mark.ci? Or should I leave that alone, and run integration tests myself some other way?

[TheRealFalcon]

I did faithfully recreate it by hand, and I could do the same with your above patch as well. Alternately, if you consider it much better git-fu and etiquette to revert back to the file(s) as they were when you made your patches, and then apply the patches against those versions, I can try that as well. I'd need to figure out just how to do that in git, but it seems straightforward.

They both have the same end result, so either way is fine by me. I answered your other question about ci inline, and left a few other minor comments.

[TheRealFalcon]

Now we have an extra newline here 🙂 .

There should be a newline at the end of line 37, but not on line 38. You probably weren't seeing it locally because you had already modified it locally but hadn't commited/pushed it to github.

I'm fine leaving it as-is though. The extra newline won't hurt anything.

[TheRealFalcon]

We can remove this comment

[TheRealFalcon]

This line isn't necessary

[chrislalos]

Done ... and I definitely saw the extra newline this time :) It's been removed.

Thanks for the clarity on pytest.mark.ci. In the future I will plan on running integration tests myself if I feel I need to.

[TheRealFalcon]

@chrislalos , I think we're there!

Two final things (I promise!):
Please provide a commit message. When I merge this, all of the commits will be squashed into a single commit. Guidelines for commit messages are near the bottom of this section (this PR has no accompanying Launchpad bug). You can put the commit message in the PR description.

Also, can you apply this patch to update the documentation? I tried pushing it to your branch but am unable to. Sorry for not thinking of it sooner.

diff --git a/doc/rtd/topics/format.rst b/doc/rtd/topics/format.rst
index 05580804..31fdcbba 100644
--- a/doc/rtd/topics/format.rst
+++ b/doc/rtd/topics/format.rst
@@ -38,6 +38,9 @@ Supported content-types are listed from the cloud-init subcommand make-mime:
     x-include-once-url
     x-include-url
     x-shellscript
+    x-shellscript-per-boot
+    x-shellscript-per-instance
+    x-shellscript-per-once
 
 
 Helper subcommand to generate mime messages
@@ -47,13 +50,26 @@ The cloud-init subcommand can generate MIME multi-part files: `make-mime`_.
 
 ``make-mime`` subcommand takes pairs of (filename, "text/" mime subtype)
 separated by a colon (e.g. ``config.yaml:cloud-config``) and emits a MIME
-multipart message to stdout.  An example invocation, assuming you have your
-cloud config in ``config.yaml`` and a shell script in ``script.sh`` and want
-to store the multipart message in ``user-data``:
+multipart message to stdout.
+
+Examples
+--------
+Create userdata containing both a cloud-config (``config.yaml``)
+and a shell script (``script.sh``)
+
+.. code-block:: shell-session
+
+    $ cloud-init devel make-mime -a config.yaml:cloud-config -a script.sh:x-shellscript > userdata
+
+Create userdata containing 3 shell scripts:
+
+- ``always.sh`` - Run every boot
+- ``instance.sh`` - Run once per instance
+- ``once.sh`` - Run once
 
 .. code-block:: shell-session
 
-    $ cloud-init devel make-mime -a config.yaml:cloud-config -a script.sh:x-shellscript > user-data
+    $ cloud-init devel make-mime -a always.sh:x-shellscript-per-boot -a instance.sh:x-shellscript-per-instance -a once.sh:x-shellscript-per-once
 
 .. _make-mime: https://github.com/canonical/cloud-init/blob/main/cloudinit/cmd/devel/make_mime.py
 

Also, I might have accidentally pinged you on another PR (accidentally intending to respond to this one). You can ignore that. 🙂

[github-actions]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

[TheRealFalcon]

@chrislalos , I'd rather not let this PR get closed again from going stale, so I'm going to merge it as-is and then propose the doc change in a separate PR.

Thanks for all your hard work on this and getting it to the finish line!

[beantaxi]

I could have sworn I made the docs changes and added the commit comment! I definitely had the commit comment ready to go in giithub ... but maybe I restarted for an OSX upgrade before I submitted. In any case I'm very pleased this is in. Thank you so much for your patience and guidance! I learned quite a bit. What's next ... wait for the next clout-init release? I seem to recall seeing mid Feb on the release calendar, would this PR be part of that/ Thanks! C

…

On Thu, Feb 10, 2022 at 3:49 PM James Falcon ***@***.***> wrote: Merged #1166 <#1166> into main. — Reply to this email directly, view it on GitHub <#1166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4R3QTJWUCLLYCDHYE5X5TU2QXH3ANCNFSM5LIHXTMQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[chrislalos]

I found the docs changes to doc/rtd/topics/format.rst ... looks like I applied your PATCH on main and not the PR branch. Shall I turn around and create a new PR for those containing just the one file?

[TheRealFalcon]

@chrislalos no worries, I added the patch as a separate PR already.

What's next ... wait for the next clout-init release? I seem to recall
seeing mid Feb on the release calendar, would this PR be part of that

This PR will go into the next upstream release, yes. We have an upstream release scheduled for tomorrow, Feb 15th. It will still take more time for the release to trickle down into distro packages though. For Ubuntu, it will likely be a couple more weeks for the release to go through the SRU process.

[chrislalos]

Been a while :)

I was just thinking about this PR ... I notice it's Merged, but not Closed, and tagged as stale. According to the release linked above (22.1), this PR was included in that release.

Should this simply be closed? Or is there another step or so required from me?

[TheRealFalcon]

Nope, nothing left to do. "Closed" really means "Closed without merging". Once a PR is merged, it is done.

If you opened a separate issue/bug for this, you could make sure that it is closed, but otherwise we're good here.