-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
cm: Remove legacy docker references #108007
cm: Remove legacy docker references #108007
Conversation
66303cd
to
5a33303
Compare
Dockershim and built-in Docker support are gone. Cleans up dead code references to them.
5a33303
to
c198062
Compare
/test pull-kubernetes-unit |
/lgtm |
/triage accepted |
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.
This is being already taken care by this PR, waiting approval https://github.com/kubernetes/kubernetes/pull/107594/files#diff-c07d343a8d459721656cf7ddacda71506bde01a5cac288d15a4aba6bfc9a1cae
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
// Temporarily export the function to be used by dockershim. | ||
// TODO(yujuhong): Move this function to dockershim once kubelet migrates to | ||
// dockershim as the default. | ||
func EnsureDockerInContainer(dockerAPIVersion *utilversion.Version, oomScoreAdj int, manager cgroups.Manager) error { |
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.
This appears to be totally unused, surprised a linter didn't catch this. https://cs.k8s.io/?q=EnsureDockerInContainer&i=nope&files=&excludeFiles=&repos=kubernetes/kubernetes
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.
Linters sadly seem to not catch public fns, in case they're being used across packages. But yeah couldn't find any real uses on all of github
@adisky the author can rebase; these changes aren't related to the ones happening in that PR, and they probably shouldn't have mixed cleanups like that as it makes things more difficult to review. |
/approve let's see if that kicks the bot |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ehashman, endocrimes, mrunalp 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 |
/hold |
Fwiw I don't mind whether this merges and that PR gets a rebase, or that merges and I'll close this. (I had dropped context of that PR from my brain, so was cleaning stuff up as I went, sorry @hakman) We mostly wanna get dockershim code out before 1.24 drops. |
@endocrimes I don't mind rebasing that one either. Leaving the decision to you. |
its upto you two @hakman @endocrimes, feel free to unhold as you like :) |
Cool - Gonna unhold (it'll simplify my life a lot as I work on unit testing CM, because this code is noise.) - But @hakman now I've seen your PR is also approved, rebase and I'll lgtm/unhold it for you when the timer expires tomorrow. /unhold |
/test pull-kubernetes-e2e-kind-ipv6 |
What type of PR is this?
/kind cleanup
/sig node
(because containermanager)
/cc @klueska
What this PR does / why we need it:
Dockershim and built-in Docker support are gone. This removes some leftover references to them in the container manager.
Special notes for your reviewer:
Does this PR introduce a user-facing change?