-
Notifications
You must be signed in to change notification settings - Fork 18.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
Remove LXC support. #17700
Remove LXC support. #17700
Conversation
LGTM! 👍 |
Think there's also some docs around lxc here https://github.com/docker/docker/blob/master/docs/reference/run.md#runtime-privilege-linux-capabilities-and-lxc-configuration (but we're not in docs review yet) |
runconfig/parse.go:65: flLxcOpts declared and not used |
thanks @cpuguy83 @thaJeztah yeah, I left a few things behind. I wanted to check if it actually worked :) |
@calavera no problem, just mentioned it so that we don't skip docs review on this one. I can help checking once we're there |
fed7b7c
to
d4bfe60
Compare
@docker/maintainers please, make sure you check this PR, because it's happening 😁 |
@@ -72,16 +72,6 @@ RUN cd /usr/local/lvm2 \ | |||
&& make install_device-mapper | |||
# see https://git.fedorahosted.org/cgit/lvm2.git/tree/INSTALL | |||
|
|||
# Install lxc |
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.
ugh i know it's silly but if we could merge this at the same time as #17701 to suffer only one cache bust :)
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.
we can put both changes in the same PR to merge them at the same time, although it's unusual.
@calavera are we going to keep the execdrivers since there is only one now ? |
@vieux that's a very good question. Maybe we can just remove that option all together. After all, windows and linux are already separated by build flags. |
I've also removed the global daemon option |
703737f
to
5be4701
Compare
Does this mean dockerinit goes away? Can we switch to building docker as dynbinary as default? |
@calavera the |
Yeah, thought abut this too. Ultimately I think it should fail and not just silently ignore the fact that the user was specifying LXC. |
@rhatdan dynbinary has been the default since 1.8. @vdemeester yes it goes away without deprecation notice. It doesn't make sense to keep it as deprecated when it doesn't do anything. @cpuguy83 it fails if you set the flag, because the parser doesn't recognize that option 😄 |
@calavera ok,seems fair. LGTM then :-) |
Conceptually, LGTM! Only glanced at the change itself, but nothing shocking. |
I'm not sure about removing Still, removing To complicate the matter further, I'd be interested in moving the |
@tiborvass That's one more reason to remove this flag from the daemon. It's a completely different feature. I'd rather not keep flags that we don't use only just in case. It will be easier to move forward with runc if we start with a clean slate and we don't try to keep the old idioms around only because we had them already. @crosbymichael I'd really appreciate your input in this. |
@calavera that's why it's the runc integration PR that should remove the flag. Anyway I won't fight it. |
Btw, libnetwork depends on |
LGTM |
Ah please leave --exec-driver for the moment. There will soon be two Windows drivers while I'm developing the libcontainer Windows one |
How is OCI exec driver going to be added? Note that native driver / libcontainer != OCI. There are three OCI complaint executors already: clr, runv, runc. With only one of them based on libcontainer bits. This kills clr exec-driver that uses VMs on Linux, which Highland team was about to be piloting to use. And which was requested to be merged to master... Given that imminently we will need oci exec driver to supplement and migrate from native driver on linux at least, removing this support adds no value, and makes current and future development match harder. Please revert? |
@xnox Might be better to open a separate issue rather than commenting on a closed pull request. I would also suggest you be more specific about why this affects OCI integration. |
LXC support has been deprecated and the related completion has been removed in moby#17700 but was added back in moby#17334. Signed-off-by: Vincent Bernat <vincent@bernat.im>
I just want to be clear, but does this PR mean that I cannot run Docker on LXC from docker 1.10? Or is there a way other than using In other words, what should I do if I want to keep using Docker on LXC? |
@kimh correct; are there specific reasons you cannot use the "native" driver? |
I work in CircleCI and we use LXC container to run builds. Therefore, we've used LXC exec-driver to run Docker.
Actually, I'm wondering if I can use libcontainer on LXC and made this SO question which led me to this PR. So repeating the same question, is it technically possible to use libcontainer driver on LXC container? |
@kimh There should be nothing stopping it. |
The LXC driver was deprecated in Docker 1.8.
Following the deprecation rules, we can remove a deprecated feature
after two major releases. LXC won't be supported anymore starting on Docker 1.10.
It also removes the global daemon option
--exec-driver
because it doesn't make sense anymore.Signed-off-by: David Calavera david.calavera@gmail.com