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

Ensure that the docker graph directory exists #220

Closed
wants to merge 3 commits into from

Conversation

jontg
Copy link

@jontg jontg commented Oct 28, 2014

Clean re-submission of #216, with tasks from there here:

  • I just want to confirm that the graph directory is relevant only to upstart and not any of the other service management types.
  • There are multiple difference commits going on here. Can you remove the commits related to the upstream merge and cacheing the key. That should be addressed in a separate commit.
  • Can you put the check for the node attribute as a chef resource guard instead of a conditional check?
  • Please add a chefspec test case to test your behavior.

@jontg
Copy link
Author

jontg commented Oct 28, 2014

To address task 1 above, I did some digging and it appears to be relevant in a couple of places:

> grep -nHr -e daemon_cli_args *
libraries/helpers.rb:76:    def self.daemon_cli_args(node)
recipes/runit.rb:15:    'daemon_options' => Docker::Helpers.daemon_cli_args(node)
recipes/systemd.rb:19:    'daemon_options' => Docker::Helpers.daemon_cli_args(node)
recipes/sysv.rb:16:    'daemon_options' => Docker::Helpers.daemon_cli_args(node)
recipes/upstart.rb:18:    'daemon_options' => Docker::Helpers.daemon_cli_args(node)

and so I moved it to default.rb for now

@jontg
Copy link
Author

jontg commented Oct 28, 2014

To address task 3 above, I am not sure that there is a clean way to handle the if condition as a chef resource guard. If the value of node['docker']['graph'] is nil, then we would be defining a resource directory[nil] — I do not believe this plays nicely with other failure cases (if two people define the same resource name then all sorts of warnings come up as per https://tickets.opscode.com/browse/CHEF-3694).

Let me know which approach you prefer, the submitted resource as directory['docker-graph'], or wrapping the resource in an if-block.

@jontg
Copy link
Author

jontg commented Oct 28, 2014

Spec incoming.

@jontg
Copy link
Author

jontg commented Oct 28, 2014

To address task 4 above, see 4c54e29

@jontg
Copy link
Author

jontg commented Oct 28, 2014

Hey @tduffield, thank you for your comments on this PR (formerly 216) — I've addressed bullets 1, 2 and 4, and would appreciate some feedback on how to properly handle bullet point 3 (namely, putting directory creation under a chef resource guard rather than wrapping the resource in an if-block). In previous revisions, the change looked like:

if node['docker'].key?(['graph'])
  directory node['docker']['graph']
end

I've provided the alternative using a chef resource guard in this edition, but it's not pretty. Which do you prefer?

@tduffield
Copy link
Contributor

I prefer using the resource guard even if its not the prettiest. Also, does the graph directory need to belong to docker or can it belong to root?

@jontg
Copy link
Author

jontg commented Oct 28, 2014

Ok, fair enough. The graph directory belongs to root, and can actually be chmod'd to 700 so long as the docker daemon is run from root. The docker group only comes into play when deciding which clients can talk to the daemon.

@tduffield
Copy link
Contributor

Should the 700 mode be present in the resource declaration?

@jontg
Copy link
Author

jontg commented Oct 28, 2014

On deeper inspection it looks like docker creates some lower-permissioned directories underneath it, so the default mode of 755 looks right. Here's some output from an existing server:

> ls -la /var/lib/docker/
total 212
drwxr-xr-x  10 root root   4096 Oct 24 11:13 .
drwxr-xr-x   8 root root   4096 Jan 30  2014 ..
drwxr-xr-x   2 root root   4096 May 23 11:05 apparmor
drwxr-xr-x   6 root root   4096 Jan 30  2014 aufs
drwx------  15 root root   4096 Oct 24 11:13 containers
drwx------   3 root root   4096 May 23 11:05 execdriver
drwx------ 203 root root  32768 Oct 24 11:12 graph
drwx------   2 root root   4096 May 23 11:05 init
-rw-r--r--   1 root root 134144 Oct 24 11:13 linkgraph.db
lrwxrwxrwx   1 root root     18 Jan 30  2014 lxc-start-unconfined -> /usr/bin/lxc-start
-rw-------   1 root root   1361 Oct 24 11:13 repositories-aufs
drwx------   3 root root   4096 Jun  3 20:57 vfs
drwx------  13 root root   4096 Aug 13 01:29 volumes

@tduffield
Copy link
Contributor

Closed - Merged into 0.35-stable for release.

@tduffield tduffield closed this Oct 28, 2014
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 this pull request may close these issues.

None yet

2 participants