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
Fix typo in Containerd configuration #8206
Fix typo in Containerd configuration #8206
Conversation
@@ -13,7 +13,7 @@ containerd_runc_runtime: | |||
engine: "" | |||
root: "" | |||
options: | |||
systemCgroup: "true" | |||
systemdCgroup: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose something better:
systemdCgroup: "true" | |
systemdCgroup: "{{ (kubelet_cgroup_driver == 'systemd') | ternary('true', 'false', omit) }}" |
As it stands, just fixing the typo breaks kubelet_cgroup_driver: cgroupfs
scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pasqualet ok @cristicalin was also doing this on an other PR but fine by me if it's fine by him
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: floryut, pasqualet 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 |
Forget my recommendation above as it is broken, the proper way would be to set systemdCgroup: "{{ containerd_use_systemd_cgroup | ternary('true', 'false') }}" ... which is what I'm testing in the other PR. I'm fine with this PR addressing the issue but in a proper way as pointed above, since the scope of #8175 is not to fix containerd support but switch to it as the default in CI, I'm actually pulling out unrelated changes into separate PRs to keep the git history clean. |
f07df7d
to
b2973bc
Compare
@@ -2,5 +2,5 @@ | |||
|
|||
- name: set kubelet_config_extra_args options when cgroupfs is used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would ask you to remove this task al together in favour of modifying the options in a single place in default.yml, it is easier to grep for values and understand where they are coming from and how they depend on each other.
@cristicalin You're right, the ternary solution is better to understand and visualize, but here I was just updating the facts because it was already in place. Previously, this cgroup option was developed in this way to follow the same pattern of other cgroups options in |
b2973bc
to
d660555
Compare
Thank you @pasqualet for the fix! /lgtm |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fix typo introduced in #8123
Which issue(s) this PR fixes:
Fixes #8186
Does this PR introduce a user-facing change?: