-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Ensure containerd service unmasking #8726
Conversation
|
Welcome @rickerc! |
Hi @rickerc. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
@rickerc: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@rickerc please see the related issue #8734 which appears to be related. I'm guessing the reset operation also causes similar symptoms to what you experienced so we should try to fix this for all CRIs, not just containerd. Secondly, due to the CI outage of the last couple of days you will need to close and re-open the PR so the new CI creates a new pipeline. |
/ok-to-test |
Unmasking added for a couple of other runtimes to help with #8734 At least with containerd the masking is specifically needed because of the combination of apt and the workflow of the containerd deployment ansible which does:
It won't be universal to all the runtimes because the runtime install ansible doesn't always follow the above flow, etc. I don't think this will be seen on rpm based distros either -- might vary with rpm version but at least in light testing I only see the above 5 steps fail with ubuntu not rhel |
Thanks for doing this @rickerc ! |
Force systemd to unmask and start service when adding containerd service
Switch to start instead of restart Move unmasking to restart handler
Thanks for doing this @rickerc ! /approve |
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.
/lgtm
Thank you @rickerc
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cristicalin, floryut, rickerc 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 |
* Force containerd service unmasking Force systemd to unmask and start service when adding containerd service * Eliminate restart and move unmasking step Switch to start instead of restart Move unmasking to restart handler * Add unmasking to similar container runtimes * Add missing service names
* Force containerd service unmasking Force systemd to unmask and start service when adding containerd service * Eliminate restart and move unmasking step Switch to start instead of restart Move unmasking to restart handler * Add unmasking to similar container runtimes * Add missing service names
Force systemd to unmask and start service when adding containerd systemd service
What type of PR is this?
/kind bug
What this PR does / why we need it:
Ensures that the systemd containerd service is not masked when kubespray adds it, regardless of the initial system state (without this, transitioning from OS-deployed systemd-managed containerd to kubespray deployed systemd-managed containerd can fail because the kubespray uninstall of the OS deployment of containerd leaves the service masked in systemd)
Which issue(s) this PR fixes:
Fixes #8725 #8734
Special notes for your reviewer:
Does this PR introduce a user-facing change?: