nutanix-cloud-native / cluster-api-runtime-extensions-nutanix

https://nutanix-cloud-native.github.io/cluster-api-runtime-extensions-nutanix/
Apache License 2.0
8 stars 4 forks source link

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

Closed dkoshkin closed 2 months ago

dkoshkin commented 2 months ago

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:

dlipovetsky commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

dkoshkin commented 2 months ago

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.