fix: uninstall custom signal handlers before shutdown #5913

holmanb · 2024-12-05T23:44:37Z

Proposed Commit Message

fix: uninstall custom signal handlers before shutdown

Fixes GH-5849

Additional Context

This might address the following bug reports:

https://bugs.launchpad.net/maas/+bug/2089185
#5849

Test Steps

I haven't been able to reproduce either bug, so testing is needed.

Here is are debs for testing (funny name because Github is unaware of the existence of debian files):

noble: cloud-init_24.3.1-1174-gf7bbd23d-1~bddeb_all.tar.gz
jammy: cloud-init_24.3.1-1016-ge3d0bcd4-1~bddeb_all.tar.gz

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 · 2024-12-10T20:10:38Z

@holmanb , I'm assuming we're looking for some confirmation that this approach works before reviewing?

…utdown

r00ta · 2024-12-17T13:19:06Z

We tested this patch in MAAS but it does not work. As per chat we decided to release a fix in MAAS directly.

So, if this patch was supposed to be used only for MAAS we can close it. Thanks!

github-actions · 2025-01-07T00:24:58Z

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

holmanb · 2025-01-07T19:04:48Z

tests/integration_tests/modules/test_power_state_change.py

@@ -64,7 +64,7 @@ class TestPowerChange:
        [
            ("poweroff", "now", "10", "will execute: shutdown -P now msg"),
            ("reboot", "now", "0", "will execute: shutdown -r now msg"),
-            ("halt", "+1", "0", "will execute: shutdown -H +1 msg"),
+            ("halt", "+1", "0", re.escape("will execute: shutdown -H +1 msg")),


Did this ever pass?

Perhaps something changed in the verify ordering utility.

There's a good likelihood it never passed

holmanb · 2025-01-07T19:09:01Z

cloudinit/signal_handler.py

+    return None
+
+
+def inspect_handler(sig: Union[int, Callable, None]) -> None:


Gather some data that would be useful for future debugging.

Note that we won't actually see these logs unless we manually call cloud-init with cloud-init --debug. This is currently getting called pre-log setup.

holmanb · 2025-01-07T19:10:43Z

cloudinit/config/cc_package_update_upgrade_install.py

@@ -106,8 +109,9 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
            break
    if (upgrade or pkglist) and reboot_if_required and reboot_fn_exists:
        try:
-            LOG.warning(
-                "Rebooting after upgrade or install per %s", reboot_marker
+            LOG.info(


This is expected, so the warnings log level doesn't make sense.

TheRealFalcon

General approach LGTM.

Can we get some unittests of the new context manager? We obviously need to mock the exit, but I think we could use the context manager and then just call the handler directly and check the logs.

Also, if we're concerned about all the places this would need to be plumbed, I'm wondering if it'd be possible to check the process tree and determine if whatever invoked the reboot was initiated by cloud-init and just ignore those. I'm not saying we should do that here, but it's an alternative to think about.

cloudinit/signal_handler.py

holmanb · 2025-01-08T21:09:54Z

determine if whatever invoked the reboot

Is that possible? Does systemd expose the PID/GID of the process that set a specific boot target? I can't think of a way to do this that wouldn't be completely broken.

TheRealFalcon · 2025-01-08T22:34:49Z

Is that possible?

It appears so, but none of the options presented there seem reasonable for this use case, so...just ignore me 😄

holmanb · 2025-01-08T22:40:47Z

Is that possible?

It appears so

I still don't think so.

If a user passes a shutdown command through a script, cloud-init doesn't receive the signal from its own child process. The process that sends the signal on reboot/shutdown is systemd. What we would need for this to be possible is to know which process set the systemd boot target to shutdown / reboot / etc.

TheRealFalcon

LGTM!

Not something I'm asking to change, but I did find the new tests a bit hard to grok. Since the test body contains an if condition for each of the 2nd parametrizations, I think that 3 separate tests might have been more appropriate, but that could just be personal preference.

TheRealFalcon · 2025-01-09T15:20:19Z

Hrm...my approval didn't see the CI failures. There appears to be some legitimate ones there, so let's not merge quite yet.

TheRealFalcon

comments inline

cloudinit/signal_handler.py

TheRealFalcon · 2025-01-10T15:56:55Z

cloudinit/signal_handler.py

+    return None
+
+
+def inspect_handler(sig: Union[int, Callable, None]) -> None:


Note that we won't actually see these logs unless we manually call cloud-init with cloud-init --debug. This is currently getting called pre-log setup.

TheRealFalcon

LGTM!

Fixes canonicalGH-5849

Fixes GH-5849

Fixes canonicalGH-5849

fix: uninstall custom signal handlers before shutdown

2dbcd06

Merge remote-tracking branch 'upstream/main' into holmanb/graceful_sh…

76ce977

…utdown

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jan 7, 2025

holmanb removed the stale-pr Pull request is stale; will be auto-closed soon label Jan 7, 2025

holmanb added 3 commits January 6, 2025 17:51

disable for runparts, bootcmd, ansible

d2bf5c9

update

6fed757

add power state change test

a3ae78e

holmanb commented Jan 7, 2025

View reviewed changes

update

0d289e0

holmanb commented Jan 7, 2025

View reviewed changes

TheRealFalcon reviewed Jan 8, 2025

View reviewed changes

cloudinit/signal_handler.py Outdated Show resolved Hide resolved

cloudinit/signal_handler.py Outdated Show resolved Hide resolved

comments

52610a6

TheRealFalcon reviewed Jan 8, 2025

View reviewed changes

cloudinit/signal_handler.py Outdated Show resolved Hide resolved

TheRealFalcon self-assigned this Jan 8, 2025

fixup! comments

12b7730

tests

033cecf

better testing and threadsafety

87b6c4c

TheRealFalcon approved these changes Jan 9, 2025

View reviewed changes

TheRealFalcon requested changes Jan 10, 2025

View reviewed changes

holmanb added 3 commits January 13, 2025 09:22

comments

d3a3759

test

54f5f69

tests

83a5fa7

TheRealFalcon approved these changes Jan 13, 2025

View reviewed changes

holmanb merged commit 6cdf51a into canonical:main Jan 13, 2025
22 checks passed

TheRealFalcon pushed a commit to TheRealFalcon/cloud-init that referenced this pull request Jan 13, 2025

fix: uninstall custom signal handlers before shutdown (canonical#5913)

4d559e7

Fixes canonicalGH-5849

TheRealFalcon pushed a commit that referenced this pull request Jan 13, 2025

fix: uninstall custom signal handlers before shutdown (#5913)

f6dadcf

Fixes GH-5849

holmanb added a commit to holmanb/cloud-init that referenced this pull request Feb 11, 2025

fix: uninstall custom signal handlers before shutdown (canonical#5913)

74059bd

Fixes canonicalGH-5849

holmanb added a commit to holmanb/cloud-init that referenced this pull request Feb 11, 2025

fix: uninstall custom signal handlers before shutdown (canonical#5913)

bf5d22f

Fixes canonicalGH-5849

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: uninstall custom signal handlers before shutdown #5913

fix: uninstall custom signal handlers before shutdown #5913

holmanb commented Dec 5, 2024 •

edited

Loading

TheRealFalcon commented Dec 10, 2024

r00ta commented Dec 17, 2024 •

edited

Loading

github-actions bot commented Jan 7, 2025

holmanb Jan 7, 2025

TheRealFalcon Jan 8, 2025

holmanb Jan 7, 2025

TheRealFalcon Jan 10, 2025

holmanb Jan 7, 2025

TheRealFalcon left a comment

holmanb commented Jan 8, 2025

TheRealFalcon commented Jan 8, 2025

holmanb commented Jan 8, 2025

TheRealFalcon left a comment

TheRealFalcon commented Jan 9, 2025 •

edited

Loading

TheRealFalcon left a comment

TheRealFalcon Jan 10, 2025

TheRealFalcon left a comment

		return None


		def inspect_handler(sig: Union[int, Callable, None]) -> None:

fix: uninstall custom signal handlers before shutdown #5913

fix: uninstall custom signal handlers before shutdown #5913

Conversation

holmanb commented Dec 5, 2024 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Merge type

TheRealFalcon commented Dec 10, 2024

r00ta commented Dec 17, 2024 • edited Loading

github-actions bot commented Jan 7, 2025

holmanb Jan 7, 2025

Choose a reason for hiding this comment

TheRealFalcon Jan 8, 2025

Choose a reason for hiding this comment

holmanb Jan 7, 2025

Choose a reason for hiding this comment

TheRealFalcon Jan 10, 2025

Choose a reason for hiding this comment

holmanb Jan 7, 2025

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

holmanb commented Jan 8, 2025

TheRealFalcon commented Jan 8, 2025

holmanb commented Jan 8, 2025

TheRealFalcon left a comment

Choose a reason for hiding this comment

TheRealFalcon commented Jan 9, 2025 • edited Loading

TheRealFalcon left a comment

Choose a reason for hiding this comment

TheRealFalcon Jan 10, 2025

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

holmanb commented Dec 5, 2024 •

edited

Loading

r00ta commented Dec 17, 2024 •

edited

Loading

TheRealFalcon commented Jan 9, 2025 •

edited

Loading