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

Bind Host Volume to Container on .run() #57

Closed
sreuter opened this issue Apr 22, 2014 · 8 comments · Fixed by #61
Closed

Bind Host Volume to Container on .run() #57

sreuter opened this issue Apr 22, 2014 · 8 comments · Fixed by #61

Comments

@sreuter
Copy link

sreuter commented Apr 22, 2014

I can't figure out how to bind a host volume into a container. According to the Remote API documentation this can be specified by setting 'Binds' like this...

docker.run('image', ['ls', '-al', '/tmp/'], null, {Binds: ["/tmp:/tmp:rw"]}, cb)

Unfortunately, this doesn't seem to work. (shows contents of original container /tmp)

Can somebody point me into the right direction here? I wonder if this is a bug in the Remote API...

Btw, it works perfectly fine using the docker cli tool!

@sreuter
Copy link
Author

sreuter commented Apr 22, 2014

I just digged a little bit deeper here. Turns out the problem is originates at the .run() implementation of dockerode, as the optional options object is passed to the container creation, but not to the actual container start call.

@apocas: Any proposal on how to allow users setting options for both calls? As .run() is some kind of helper function (and not a direct endpoint in docker's REST API), why don't map given options accordingly like:

.run() with...

options = {
  'Binds': ['/hostdir:/containerdir:rw']
}

results in

create_options = {
  'Volumes': {
    '/containerdir': {}
  }
}

start_options = {
  'Binds': ['/hostdir:/containerdir:rw']
}

@apocas
Copy link
Owner

apocas commented Apr 23, 2014

Interesting how "run" functionality keeps growing :)

Hummm, I see two options:

  • Expose another argument for the start options. (not pretty, verbose)
  • Automagically map options like you said. (possibly high maintenance)

Exposing another argument is indeed a bit ugly and verbose, but it gives a lot of freedom.
What do you think?

@sreuter
Copy link
Author

sreuter commented Apr 23, 2014

How about something in between...

options = {
  'Binds': ['/hostdir:/containerdir:rw'], // will be translated into necessary start & creation options
  'start_options': {
    .. // extends start options object
  },
  'create_options': {
    .. // extends create options object
  },
}

This gives us the possibility to implement high-level (helper) functions, while providing freedom on setting create & start options "manually".

I know this may result in a breaking change though, but think it may be worth it.

@sreuter
Copy link
Author

sreuter commented Apr 28, 2014

@apocas Any further thoughts on this one? :-)

@apocas
Copy link
Owner

apocas commented Apr 28, 2014

Yeah, I like that one :)

But we should throw an error if there's an option that can't be translated.
Basically I want to avoid people using some new option in a Remote API future version and it being silently ignored. If we throw an error saying something like: "I don't know what to do with XPTO option, try using start_options or create_options instead." should avoid this and give an alternate route.

Want to throw a PR for this? :)

srijs added a commit to srijs/dockerode that referenced this issue Apr 29, 2014
In order to be able to set default options for common operations on a container,
introduce a default options object per operation that can be overwritten from the outside.

Together with apocas#60 this resolves apocas#57, because you can now do:

docker.run(..., function (...) {
  ...
}).on('container', function (container) {
  container.defaultOptions.start.Binds = ["/tmp:/tmp:rw"];
});
@apocas
Copy link
Owner

apocas commented Apr 29, 2014

Let's go @srijs way for now, I think is solves this use case pretty well.

Will deprecate the current "options" parameter, since it doesn't give complete functionality. (it's needed for container creation)

@sreuter
Copy link
Author

sreuter commented Apr 29, 2014

Works like a charm guys!

Btw, did I mention @srijs is sitting right next to me? ;-)

@apocas
Copy link
Owner

apocas commented Apr 29, 2014

Ahah, No :-P

And I was worried about hurting your feelings for choosing one of the solutions lol

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

Successfully merging a pull request may close this issue.

2 participants