Skip to content

fix: run systemctl daemon-reload before Containerd restart #842

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

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

dkoshkin
Copy link
Contributor

@dkoshkin dkoshkin commented Aug 5, 2024

What problem does this PR solve?:
During Machine bootstrapping Contianerd drop in files may be added.
These files are written to the disk before preKubeadmCommmands are run.
Always run systemctl daemon-reload along with systemctl restart containerd to pick up these dropins.

I saw a warning about the drop-ins when cloud init runs the Containerd restart script, so figured it makes sense to add it here instead of a separate script.

[2024-08-05 16:35:46] /tmp/tmp.MzIe5jTIdG
[2024-08-05 16:35:46] Warning: The unit file, source configuration file or drop-ins of containerd.service changed on disk. Run 'systemctl daemon-reload' to reload units.
[2024-08-05 16:35:46] /usr/bin/crictl
[2024-08-05 16:35:46] {
...

Which issue(s) this PR fixes:
Fixes #

How Has This Been Tested?:

Special notes for your reviewer:

During Machine bootstrapping Contianerd drop in files may be added.
These files are written to the disk before preKubeadmCommmands are run.
Always run systemctl daemon-reload along with systemctl restart containerd
to pick up these drop-ins.
Copy link
Member

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

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

Are there no tests to update? 🫣

@github-actions github-actions bot added fix and removed fix labels Aug 5, 2024
@dlipovetsky
Copy link
Contributor

dlipovetsky commented Aug 5, 2024

This systemctl behavior is unfortunate.

Systemctl knows when you need to run daemon-reload, and in fact, warns you. I think it should do it in your behalf automatically. That's been requested once in the past, and denied. But I think it's worth reexamining upstream.

Because daemon-reload affects all configuration, so it's best to run at a known "synchronization point," i.e. when you're done adding all drop-ins, not just the ones for containerd. It a relatively expensive operation, so it's best to run it once, if possible.

We can also avoid running it by first checking if it's required for any service whose configuration we've updated. Example:

if [ $(systemctl show containerd -p NeedDaemonReload --value) == "yes" ]; then
    systemctl daemon-reload
fi
systemctl restart containerd

@dlipovetsky
Copy link
Contributor

For long-term maintenance, I think we might want to factor this out, so that we have this general structure:

  1. Make any handler that updates the configuration of a systemd service implement ttwo methods: one that updates configuration, and another that restarts the service.
  2. Call the first method of all handlers.
  3. Run daemon-reload (to reload configuration possibly updated in the previous step).
  4. Call the second method of all handlers.

@dkoshkin
Copy link
Contributor Author

dkoshkin commented Aug 5, 2024

For long-term maintenance, I think we might want to factor this out, so that we have this general structure:

  1. Make any handler that updates the configuration of a systemd service implement ttwo methods: one that updates configuration, and another that restarts the service.
  2. Call the first method of all handlers.
  3. Run daemon-reload (to reload configuration possibly updated in the previous step).
  4. Call the second method of all handlers.

Great idea @dlipovetsky, let me create an issue in this repo. We've gone through a couple of iterations now of this common Contianerd command that's needed by different handlers and moving it out to somewhere more generic makes sense to me.

Copy link
Contributor

@dlipovetsky dlipovetsky left a comment

Choose a reason for hiding this comment

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

Good as a short-term implementation. We can revisit in the future.

@dkoshkin
Copy link
Contributor Author

dkoshkin commented Aug 5, 2024

This systemctl behavior is unfortunate.

Systemctl knows when you need to run daemon-reload, and in fact, warns you. I think it should do it in your behalf automatically. That's been requested once in the past, and denied. But I think it's worth reexamining upstream.

Because daemon-reload affects all configuration, so it's best to run at a known "synchronization point," i.e. when you're done adding all drop-ins, not just the ones for containerd. It a relatively expensive operation, so it's best to run it once, if possible.

We can also avoid running it by first checking if it's required for any service whose configuration we've updated. Example:

if [ $(systemctl show containerd -p NeedDaemonReload --value) == "yes" ]; then
    systemctl daemon-reload
fi
systemctl restart containerd

Doing a test with this suggestion.

@dkoshkin dkoshkin force-pushed the dkoshkin/fix-containerd-daemon-reload branch from f6701bd to 5969a52 Compare August 5, 2024 18:45
@dkoshkin dkoshkin enabled auto-merge (squash) August 5, 2024 19:30
@dkoshkin dkoshkin merged commit 2d30d2c into main Aug 5, 2024
15 checks passed
@dkoshkin dkoshkin deleted the dkoshkin/fix-containerd-daemon-reload branch August 5, 2024 20:18
@dkoshkin dkoshkin mentioned this pull request Aug 5, 2024
dkoshkin added a commit that referenced this pull request Aug 5, 2024
🤖 I have created a release *beep* *boop*
---


## 0.13.6 (2024-08-05)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

## What's Changed
### Fixes 🔧
* fix: run systemctl daemon-reload before Containerd restart by
@dkoshkin in
#842


**Full Changelog**:
v0.13.5...v0.13.6

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants