Skip to content
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

Create extpoint for graphdrivers #13777

Merged
merged 1 commit into from
Oct 8, 2015

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Jun 5, 2015

Allows people to create out-of-process graphdrivers that can be used
with Docker.

Extensions must be started before Docker otherwise Docker will fail to
start.

For simplicity, uses in-process NaiveDiffDriver for diffing.

@duglin
Copy link
Contributor

duglin commented Jun 5, 2015

testcases?

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jun 6, 2015

Yeah, tests are coming.

One issue is it seems that if I use the vfs driver for the "remote" driver, it gets frozen when it hits chrootarchive.CopyWithTar.
Changing to archive.CopyWithTar instead, it works fine.
And of course, overlay and aufs work just fine.

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jun 6, 2015

Pushed tests with just using the overlay driver as the external driver for now.
Added exported function to the vfs driver which allows injecting a copying function to use.
I'd prefer to:

  1. make sure the test is able to create a real container
  2. not implement a working graphdriver just for tests.

Thoughts?

@icecrime
Copy link
Contributor

@cpuguy83 What are the use cases for this? I don't feel we have a need for this right now (it's not like we get dozens of PR about adding graphdriver that we refuse because we don't want them compiled in), so I'm a weak no.

@cpuguy83
Copy link
Member Author

we've already had requests for gluster and ceph... Ceph being one that can't be linked statically either.
It also allows experimenting with new drivers without having to build in.

Also consider drivers for enterprise storage devices like NetApp, EMC, and I could name a few others that would prefer not to be named.
These can all implement their own layering, CoW, etc.

@unclejack
Copy link
Contributor

@icecrime Please allow me to explain why this would be useful and why my vote would be an aye.

You've said that we don't get many PRs for adding new graph drivers and that this might not be needed now. While I do agree that we don't receive many PRs for new graph drivers, we also need to think about the reasons.

Companies like EMC, NetApp and others will most likely not be sending pull requests to add their product specific graph driver to the Docker repository. That can be because of a multitude of reasons: they want to keep it closed source, they want to put some proprietary stuff in there, they'd want to be able to change it, update it separately from Docker releases and so on.

The Ceph driver has been requested before and I've also rejected that idea because we'd have to tie it into Docker by linking to Ceph libraries which are version specific. The last thing we need is to link against more foreign C/C++ libraries and produce multiple Docker binaries for all those systems with all the different Ceph versions. It's obvious that we won't get a Ceph PR because we've turned it down for these reasons, so Ceph support can't happen without plugin support for graph drivers.

These graph driver plugins could live in a separate repository and they could be packaged separately from Docker. The Ceph graph driver would probably be the first one.

@cpuguy83 Perhaps making the necessary changes to allow custom Diff implementations would be a good idea. I also anticipate some changes in the graph driver API and having some kind of versions for this API would certainly help with that.

@icecrime
Copy link
Contributor

Sorry, I missed the comment here.

I see, makes sense then.

@hustcat
Copy link

hustcat commented Jul 22, 2015

@cpuguy83 I write ceph rbd storage driver plugins refer to your interface, see here. I wish your PR be merged ASAP.

@cpuguy83
Copy link
Member Author

@hustcat Really cool! Could I recommend to change your default graph store dir to not be /var/lib/docker? This should be pretty much private to the docker daemon and you should not rely on its existence.

@hustcat
Copy link

hustcat commented Jul 22, 2015

Yea,Thanks for your suggestion. I will continue to improve it:)

@thaJeztah
Copy link
Member

@hustcat are you still working on this?

@thaJeztah
Copy link
Member

Sorry, wrong ping, GitHub autocomplete... meant to type @cpuguy83 😊

@cpuguy83
Copy link
Member Author

cpuguy83 commented Aug 9, 2015

This is in design review.
The implementation is done.

@thaJeztah
Copy link
Member

Doh! Looks like I'm not having my day, LOL, should not be doing late-night browsing through issues 😉

@mpatlasov
Copy link

Brian, I'd like to use your implementation of graphdriver extpoint to resolve issues I attempted to solve by #15594, but this would require a minor improvement of what you have done -- it would be great not to loose the knowledge of "home" and "options" that GetDriver() has as arguments. For example, we could implement mux.HandleFunc("/Plugin.Init", ...) and call it from LookupExtPoint() after plugins.Get(). What do you think?

@cpuguy83
Copy link
Member Author

@mpatlasov This would make sense if Docker is going to initialize the plugin, but if the plugin is already running, it really shouldn't be doing this, and also is a bit weird in cases where maybe this is a multi-host solution.

@cpuguy83
Copy link
Member Author

cpuguy83 commented Sep 7, 2015

Ok, this is now fully functional, figured out the issue with chrootarchive in test.

In writing this, I noticed that there is no code path that actually calls graphdriver.Diff, and only 1 code path (which seems to never be called) which calls graphdriver.DiffSize.

@cpuguy83 cpuguy83 force-pushed the graphdriver_extpoints branch 3 times, most recently from 204e906 to c9d6d3a Compare September 8, 2015 20:02
Allows people to create out-of-process graphdrivers that can be used
with Docker.

Extensions must be started before Docker otherwise Docker will fail to
start.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@unclejack
Copy link
Contributor

Does anyone think we should have a versioning system for this API? This graphdriver plugins API will most likely get improved once people start building graph drivers and that means we'll have to version it.

@cpuguy83
Copy link
Member Author

@unclejack Right now the overall plugin API is bumped when there are changes to anything, and we don't stay backwards compatible.

@calavera
Copy link
Contributor

We can change the content type to be specific about the API endpoint. We already have a version number there. We don't need another versioning system.

@cpuguy83
Copy link
Member Author

Is there something that should change in this PR?

@kolyshkin kolyshkin mentioned this pull request Sep 22, 2015
@kolyshkin
Copy link
Contributor

Sorry for the noise, but is this going to be merged or not? Is there anything I can help with?

@LK4D4
Copy link
Contributor

LK4D4 commented Oct 8, 2015

@kolyshkin Yup, thanks for ping.

@LK4D4
Copy link
Contributor

LK4D4 commented Oct 8, 2015

LGTM
ping @thaJeztah @moxiegirl for docs too

@thaJeztah
Copy link
Member

docs LGTM

@moxiegirl
Copy link
Contributor

@cpuguy83 LGTM -- just an FYI I generally don't review experimental until we move it into the docs dir.

@LK4D4
Copy link
Contributor

LK4D4 commented Oct 8, 2015

ping @docker/core-maintainers

@jessfraz
Copy link
Contributor

jessfraz commented Oct 8, 2015

ok LGTM, shall we fire ze missiles on this one

jessfraz pushed a commit that referenced this pull request Oct 8, 2015
@jessfraz jessfraz merged commit 4c55464 into moby:master Oct 8, 2015
@unclejack unclejack mentioned this pull request Nov 28, 2015
@mx2323
Copy link

mx2323 commented Feb 5, 2016

@cpuguy83 is there any ability to pass options to the external graph driver? with the volume driver, you can pass in volume specific options that can be used by the driver to customize behavior.. i think that would be awesome to have here as well.

@cpuguy83
Copy link
Member Author

cpuguy83 commented Feb 5, 2016

@mx2323 No, the graphdriver is not user-facing.
We may add something like --storage-opt to docker run, in which case that will be possible, but it is not how graphdrivers work right now.

@cpuguy83 cpuguy83 deleted the graphdriver_extpoints branch February 5, 2016 00:47
@mx2323
Copy link

mx2323 commented Feb 5, 2016

@cpuguy83 thanks for the quick reply. sounds like it probably wouldn't be that hard to add. i can look into doing it and sending a diff out... thnx

@cpuguy83
Copy link
Member Author

cpuguy83 commented Feb 5, 2016

@mx2323 There's an open PR already: #19661

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet