10000 Remove 'docker-' prefix for containerd and runc binaries by tiborvass · Pull Request #37907 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove 'docker-' prefix for containerd and runc binaries #37907

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 26, 2018
Merged

Remove 'docker-' prefix for containerd and runc binaries #37907

merged 3 commits into from
Sep 26, 2018

Conversation

tiborvass
Copy link
Contributor

This allows to run the daemon in environments that have upstream containerd installed.

Signed-off-by: Tibor Vass tibor@docker.com

@tiborvass tiborvass changed the title Remove 'docker-' prefix for containerd and runc binaries [WIP] Remove 'docker-' prefix for containerd and runc binaries Sep 24, 2018
@tiborvass tiborvass force-pushed the remove-docker-prefix-containerd branch from d077cbb to a417216 Compare September 24, 2018 21:03
@codecov
Copy link
codecov bot commented Sep 24, 2018 •
edited
Loading

Codecov Report

Merging #37907 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #37907      +/-   ##
==========================================
- Coverage    36.1%   36.09%   -0.02%     
==========================================
  Files         610      610              
  Lines       45115    45121       +6     
==========================================
- Hits        16291    16288       -3     
- Misses      26584    26593       +9     
  Partials     2240     2240

@tiborvass tiborvass changed the title [WIP] Remove 'docker-' prefix for containerd and runc binaries Remove 'docker-' prefix for containerd and runc binaries Sep 24, 2018
@tiborvass tiborvass force-pushed the remove-docker-prefix-containerd branch from a417216 to d62e211 Compare September 24, 2018 21:39
Remove 'docker-' prefix for containerd and runc binaries
34eede0 
This allows to run the daemon in environments that have upstream containerd installed.

Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the remove-docker-prefix-containerd branch from d62e211 to 34eede0 Compare September 24, 2018 21:49
@tiborvass tiborvass mentioned this pull request Sep 24, 2018
Merged
seemethere
seemethere approved these changes Sep 25, 2018
View reviewed changes
Copy link
Contributor
@seemethere seemethere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

vdemeester
vdemeester approved these changes Sep 25, 2018
View reviewed changes
Copy link
Member
@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👼

Tibor Vass added 2 commits September 25, 2018 16:58
hack/make: remove 'docker-' prefix when copying binaries
361412c 
Signed-off-by: Tibor Vass <tibor@docker.com>
fix daemon tests that were using wrong containerd socket
52b60f7 
Signed-off-by: Tibor Vass <tibor@docker.com>
yongtang
yongtang approved these changes Sep 25, 2018
View reviewed changes
Copy link
Member
@yongtang yongtang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yongtang yongtang merged commit 472a528 into moby:master Sep 26, 2018
@seemethere seemethere mentioned this pull request Sep 26, 2018
Merged
@AkihiroSuda
Copy link
Member

I'm wondering this might cause packaging issues.
e.g. apt/dnf-installing both docker-ce and runc is no longer possible?

@thaJeztah
Copy link
Member

ping @seemethere ^^

@seemethere
Copy link
Contributor

docker-ce doesn't install runc anymore our containerd package does. But yeah our containerd package would essentially overwrite the runc binary installed with the runc distro provided package. Original intention was to have a docker distributed runc package as well but that probably won't happen until late Q4 for us.

@thaJeztah thaJeztah mentioned this pull request Nov 13, 2018
Closed
3 tasks
@sboeuf sboeuf mentioned this pull request Nov 20, 2018
Closed
@cyphar
Copy link
Contributor
cyphar commented Nov 29, 2018 •
edited
Loading

This is a very bad idea -- Docker has always been stuck on an older version of runc and now we have to do workarounds in order to be able to split the two in the distribution (because, for instance, cri-o uses a newer version).

There is no workaround for this change either! --default-runtime doesn't help because it saves the runtime in the RuntimeConfig (which then causes container to not be able to start on a daemon reboot where the runtime is no longer defined -- we found this out the hard way).

Speaking on the openSUSE side of things, we will carry a revert of this patch.

But yeah our containerd package would essentially overwrite the runc binary installed with the runc distro provided package.

This is not a good practice when packaging (and distributions won't accept such packages), not to mention that attempting to install such packages on most distributions will result in warnings due to file conflicts between packages.

@lifubang
Copy link
Contributor

Yes, it is. Another issue: When I use my own default containerd to start docker, once I kill containerd, then restart dockerd, dockerd will start his own contanerd, then the old containerd-shim is not stopped, and another containerd-shim is started, there are two containerd-shim for one container.

@axiqia axiqia mentioned this pull request Apr 6, 2019
Closed
@mattpag mattpag mentioned this pull request Jul 9, 2019
Merged
@tiborvass tiborvass deleted the remove-docker-prefix-containerd branch July 17, 2019 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@seemethere seemethere seemethere approved these changes

@yongtang yongtang yongtang approved these changes

@tianon tianon Awaiting requested review from tianon tianon is a code owner

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

@crosbymichael crosbymichael Awaiting requested review from crosbymichael

Assignees
No one assigned
Labels
impact/changelog status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@tiborvass @AkihiroSuda @thaJeztah @seemethere @cyphar @lifubang @vdemeest 34CB er @yongtang @GordonTheTurtle
0