-
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
feat: check & uninstall container engine if needed #8439
feat: check & uninstall container engine if needed #8439
Conversation
Hi @cyril-corbon. 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. |
I'm assuming you are pushing this to address the discussion on #8431 but I don't quite like the approach of removing another container manager in the role that is supposed to cover a different one. What about the scenario when someone wants to move from docker to cri-o or from cri-o to containerd; in this case we should add a different logic to detect the currently deployed container manager and check it against the desired one then perform a full clean up if necessary. The approach of uninstalling an unrelated container manager would leave unrelated configuration files (think /etc/docker) and state (think /var/lib/docker) behind which could go unnoticed and cause issues later in the cluster lifecycle. |
I thought only docker has containerd as dependency that's why I did that. Maybe we could add a role to check if a container manager is installed before an installation and removed it ? This role should remove the package and all his configurations file as you said |
Personally I would like to see a |
ok I will rename the pr and do that thanks for the feedback 🙇 |
b732932
to
e01cefc
Compare
e01cefc
to
6b56ffb
Compare
6b56ffb
to
c66b3f7
Compare
Signed-off-by: Cyril Corbon <corboncyril@gmail.com>
c66b3f7
to
6d4f41a
Compare
Looks good @cyril-corbon , thanks for doing this! /ok-to-test |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cristicalin, cyril-corbon 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 |
/lgtm |
Signed-off-by: Cyril Corbon <corboncyril@gmail.com>
Signed-off-by: Cyril Corbon <corboncyril@gmail.com>
Signed-off-by: Cyril Corbon <corboncyril@gmail.com>
What type of PR is this?
/kind bug
What this PR does / why we need it:
we should verify that docker / docker-ce is not installed when we install containerd
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: