Support Docker Volumes and Logging #1767
Conversation
|
On the logging front, we need to avoid starting the syslog server when the user specifies a docker logging driver. |
| @@ -58,6 +58,10 @@ const ( | |||
| // driver | |||
| dockerDriverAttr = "driver.docker" | |||
|
|
|||
| dockerSELinuxLabelConfigOption = "docker.volumes.selinuxlabel" | |||
| @@ -74,6 +78,12 @@ type DockerDriverAuth struct { | |||
| ServerAddress string `mapstructure:"server_address"` // server address of the registry | |||
| } | |||
|
|
|||
| type DockerLoggingOpts struct { | |||
|
|
||
| if len(driverConfig.Logging) == 0 { | ||
| d.logger.Printf("[DEBUG] driver.docker: Setting default logging options to syslog and %s", syslogAddr) | ||
| driverConfig.Logging = []DockerLoggingOpts{ |
There was a problem hiding this comment.
Move this to a factory method NewDockerLoggingOpts
| @@ -339,10 +363,17 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool | |||
|
|
|||
| node.Attributes[dockerDriverAttr] = "1" | |||
| node.Attributes["driver.docker.version"] = env.Get("Version") | |||
|
|
|||
| if d.config.ReadBoolDefault(dockerVolumesConfigOption, false) { | |||
There was a problem hiding this comment.
Comment on what you are doing here
| var merr multierror.Error | ||
| volumesEnabled := d.config.ReadBoolDefault(dockerVolumesConfigOption, false) | ||
| if len(driverConfig.Volumes) > 0 && !volumesEnabled { | ||
| merr.Errors = append(merr.Errors, fmt.Errorf(dockerVolumesConfigOption+" is false; cannot use Docker Volumes: %+q", driverConfig.Volumes)) |
There was a problem hiding this comment.
fmt.Errorf("%s is false...", dockerVolumesConfigOption, driverConfig.Volumes)
| @@ -97,6 +107,9 @@ type DockerDriverConfig struct { | |||
| Interactive bool `mapstructure:"interactive"` // Keep STDIN open even if not attached | |||
| ShmSize int64 `mapstructure:"shm_size"` // Size of /dev/shm of the container in bytes | |||
| WorkDir string `mapstructure:"work_dir"` // Working directory inside the container | |||
| Logging []DockerLoggingOpts `mapstructure:"logging"` // Logging options for syslog server | |||
| Volumes []string `mapstructure:"volumes"` // Host-Volumes to mount in, syntax: /path/to/host/directory:/destination/path/in/container | |||
| VolumesFrom []string `mapstructure:"volumes_from"` // List of volumes-from | |||
There was a problem hiding this comment.
Why are we supporting this? How will people use this?
There was a problem hiding this comment.
Removing as per out of band discussion.
For anyone following along: VolumesFrom would be basically impossible to use safely with nomad since nomad names containers with the randomly generated UUID (among other issues).
…u don't want to bake certificates into the container, you can mount them into the directory directly. Furthermore, I added support for volumes-from. Currently, there is no support to move the data from one container to another, hence: If a container spawns on another host, it is very likely, that the data will not be found.
Before this commit, if the Logging config did not contain a logging option "syslog-address", it would definitely insert this option. If then, you decide to take another logdriver than syslog, docker would fail because it received a wrong log option for the selected driver. Now, nomad will only insert the syslog address in a hard way if there are no logging options at all - this way it keeps the default nomad settings.
Also add tests and fix bug with logging driver configuration.
schmichael
force-pushed
the
f-docker-volumes-logging
branch
from
968f271 to
cc5b40e
Compare
|
All comments should be addressed. |
| "volumes": &fields.FieldSchema{ | ||
| Type: fields.TypeArray, | ||
| }, | ||
| "volumes_from": &fields.FieldSchema{ |
|
|
||
| // Only launch syslog server if we're going to use it! | ||
| syslogAddr := "" | ||
| if len(driverConfig.Logging) == 0 || driverConfig.Logging[0].Type == "syslog" { |
There was a problem hiding this comment.
The first condition will never be true right?
There was a problem hiding this comment.
At this point it can be. In the createContainer call a few lines down it adds the default logging option if this slice is empty.
Since containers are named with alloc ids it's difficult to use safely. Not to mention task scheduling ordering issues could break it as well.
schmichael
force-pushed
the
f-docker-volumes-logging
branch
from
ed4ddeb to
7d115a3
Compare
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Thanks to @Fluxxo for his initial work on this in PR #1736!
We on the Nomad team wanted to disable Docker volume support by default since allowing it enables any task to access any other task's data on a given node. We want our defaults to always securely isolate tasks, so users must opt-in explicitly.