fix: ensure we wait for the right CRI service name #4115
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4115 +/- ##
=======================================
Coverage 73.23% 73.23%
=======================================
Files 135 135
Lines 20625 20625
=======================================
Hits 15104 15104
Misses 4547 4547
Partials 974 974
Continue to review full report at Codecov.
|
| @@ -1,6 +1,10 @@ | |||
| [Unit] | |||
| Description=a script that checks docker health and restarts if needed | |||
| {{- if NeedsContainerd}} | |||
| After=containerd.service | |||
There was a problem hiding this comment.
Wouldn't this be the same as writing:
After={{GetContainerRuntime}}.service
Or is there some additional disconnect between the two? Right now I see those as the same.
There was a problem hiding this comment.
It is. The likelihood of the containerRuntime allowed values changing (from "docker" and "containerd" to something else to indicate the same configuration) is 0%. The likelihood that the apt package would rename the current service names to something else is perhaps a mite larger, but also unlikely, so your more elegant suggestion will work.
In the event the coupling between the CRI name and the service name goes away in the future, we can refactor to the more verbose if/else expression.
There was a problem hiding this comment.
Worst case we could have GetContainerRuntimeService in addition to GetContainerRuntime
/lgtm
| @@ -90,24 +90,13 @@ write_files: | |||
| Slice={{- GetKubeReservedCgroup -}}.slice | |||
| #EOF | |||
|
|
|||
| {{if NeedsContainerd}} | |||
| - path: /etc/systemd/system/containerd.service.d/kubereserved-slice.conf | |||
| - path: /etc/systemd/system/{{GetContainerRuntime}}.service.d/kubereserved-slice.conf | |||
There was a problem hiding this comment.
Simplifying this CRI filepath declaration while I'm here...
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, Michael-Sinz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reason for Change:
This PR ensures we are waiting for the correctly named CRI service name in various systemd definitions.
Issue Fixed:
Credit Where Due:
Does this change contain code from or inspired by another project?
If "Yes," did you notify that project's maintainers and provide attribution?
Requirements:
Notes: