From 365ca68295f7d9373e93b054b28ccd4c81d78449 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Tue, 5 Sep 2017 14:02:57 -0700 Subject: [PATCH] Parse Docker mounts correctly (#3163) * Parse Docker mounts correctly This PR fixes the parsing of Docker mounts and adds testing to ensure no regressions. Fixes https://github.com/hashicorp/nomad/issues/3156 * Review feedback --- client/driver/docker.go | 84 ++++++++++------ client/driver/docker_test.go | 180 +++++++++++++++++++++++++++++++++++ 2 files changed, 237 insertions(+), 27 deletions(-) diff --git a/client/driver/docker.go b/client/driver/docker.go index 0287aad53d39..dab056d5f102 100644 --- a/client/driver/docker.go +++ b/client/driver/docker.go @@ -136,22 +136,22 @@ type DockerLoggingOpts struct { } type DockerMount struct { - Target string `mapstructure:"target"` - Source string `mapstructure:"source"` - ReadOnly bool `mapstructure:"readonly"` - VolumeOptions *DockerVolumeOptions `mapstructure:"volume_options"` + Target string `mapstructure:"target"` + Source string `mapstructure:"source"` + ReadOnly bool `mapstructure:"readonly"` + VolumeOptions []*DockerVolumeOptions `mapstructure:"volume_options"` } type DockerVolumeOptions struct { - NoCopy bool `mapstructure:"no_copy"` - Labels map[string]string `mapstructure:"labels"` - DriverConfig DockerVolumeDriverConfig `mapstructure:"driver_config"` + NoCopy bool `mapstructure:"no_copy"` + Labels []map[string]string `mapstructure:"labels"` + DriverConfig []DockerVolumeDriverConfig `mapstructure:"driver_config"` } // VolumeDriverConfig holds a map of volume driver specific options type DockerVolumeDriverConfig struct { - Name string `mapstructure:"name"` - Options map[string]string `mapstructure:"options"` + Name string `mapstructure:"name"` + Options []map[string]string `mapstructure:"options"` } type DockerDriverConfig struct { @@ -259,22 +259,43 @@ func NewDockerDriverConfig(task *structs.Task, env *env.TaskEnv) (*DockerDriverC for i, m := range dconf.Mounts { dconf.Mounts[i].Target = env.ReplaceEnv(m.Target) dconf.Mounts[i].Source = env.ReplaceEnv(m.Source) - if m.VolumeOptions != nil { - if m.VolumeOptions.Labels != nil { - for k, v := range m.VolumeOptions.Labels { + + if len(m.VolumeOptions) > 1 { + return nil, fmt.Errorf("Only one volume_options stanza allowed") + } + + if len(m.VolumeOptions) == 1 { + vo := m.VolumeOptions[0] + if len(vo.Labels) > 1 { + return nil, fmt.Errorf("labels may only be specified once in volume_options stanza") + } + + if len(vo.Labels) == 1 { + for k, v := range vo.Labels[0] { if k != env.ReplaceEnv(k) { - delete(dconf.Mounts[i].VolumeOptions.Labels, k) + delete(vo.Labels[0], k) } - dconf.Mounts[i].VolumeOptions.Labels[env.ReplaceEnv(k)] = env.ReplaceEnv(v) + vo.Labels[0][env.ReplaceEnv(k)] = env.ReplaceEnv(v) } } - dconf.Mounts[i].VolumeOptions.DriverConfig.Name = env.ReplaceEnv(m.VolumeOptions.DriverConfig.Name) - if m.VolumeOptions.DriverConfig.Options != nil { - for k, v := range m.VolumeOptions.DriverConfig.Options { - if k != env.ReplaceEnv(k) { - delete(dconf.Mounts[i].VolumeOptions.DriverConfig.Options, k) + + if len(vo.DriverConfig) > 1 { + return nil, fmt.Errorf("volume driver config may only be specified once") + } + if len(vo.DriverConfig) == 1 { + vo.DriverConfig[0].Name = env.ReplaceEnv(vo.DriverConfig[0].Name) + if len(vo.DriverConfig[0].Options) > 1 { + return nil, fmt.Errorf("volume driver options may only be specified once") + } + + if len(vo.DriverConfig[0].Options) == 1 { + options := vo.DriverConfig[0].Options[0] + for k, v := range options { + if k != env.ReplaceEnv(k) { + delete(options, k) + } + options[env.ReplaceEnv(k)] = env.ReplaceEnv(v) } - dconf.Mounts[i].VolumeOptions.DriverConfig.Options[env.ReplaceEnv(k)] = env.ReplaceEnv(v) } } } @@ -995,14 +1016,23 @@ func (d *DockerDriver) createContainerConfig(ctx *ExecContext, task *structs.Tas Type: "volume", // Only type supported ReadOnly: m.ReadOnly, } - if m.VolumeOptions != nil { + if len(m.VolumeOptions) == 1 { + vo := m.VolumeOptions[0] hm.VolumeOptions = &docker.VolumeOptions{ - NoCopy: m.VolumeOptions.NoCopy, - Labels: m.VolumeOptions.Labels, - DriverConfig: docker.VolumeDriverConfig{ - Name: m.VolumeOptions.DriverConfig.Name, - Options: m.VolumeOptions.DriverConfig.Options, - }, + NoCopy: vo.NoCopy, + } + + if len(vo.DriverConfig) == 1 { + dc := vo.DriverConfig[0] + hm.VolumeOptions.DriverConfig = docker.VolumeDriverConfig{ + Name: dc.Name, + } + if len(dc.Options) == 1 { + hm.VolumeOptions.DriverConfig.Options = dc.Options[0] + } + } + if len(vo.Labels) == 1 { + hm.VolumeOptions.Labels = vo.Labels[0] } } hostConfig.Mounts = append(hostConfig.Mounts, hm) diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index f9524a34630a..3a966f62a85d 100644 --- a/client/driver/docker_test.go +++ b/client/driver/docker_test.go @@ -1376,6 +1376,186 @@ func TestDockerDriver_VolumesEnabled(t *testing.T) { } } +func TestDockerDriver_Mounts(t *testing.T) { + if !tu.IsTravis() { + t.Parallel() + } + if !testutil.DockerIsConnected(t) { + t.SkipNow() + } + + goodMount := map[string]interface{}{ + "target": "/nomad", + "volume_options": []interface{}{ + map[string]interface{}{ + "labels": []interface{}{ + map[string]string{"foo": "bar"}, + }, + "driver_config": []interface{}{ + map[string]interface{}{ + "name": "local", + "options": []interface{}{ + map[string]interface{}{ + "foo": "bar", + }, + }, + }, + }, + }, + }, + "readonly": true, + "source": "test", + } + + cases := []struct { + Name string + Mounts []interface{} + Error string + }{ + { + Name: "good-one", + Error: "", + Mounts: []interface{}{goodMount}, + }, + { + Name: "good-many", + Error: "", + Mounts: []interface{}{goodMount, goodMount, goodMount}, + }, + { + Name: "multiple volume options", + Error: "Only one volume_options stanza allowed", + Mounts: []interface{}{ + map[string]interface{}{ + "target": "/nomad", + "volume_options": []interface{}{ + map[string]interface{}{ + "driver_config": []interface{}{ + map[string]interface{}{ + "name": "local", + }, + }, + }, + map[string]interface{}{ + "driver_config": []interface{}{ + map[string]interface{}{ + "name": "local", + }, + }, + }, + }, + }, + }, + }, + { + Name: "multiple driver configs", + Error: "volume driver config may only be specified once", + Mounts: []interface{}{ + map[string]interface{}{ + "target": "/nomad", + "volume_options": []interface{}{ + map[string]interface{}{ + "driver_config": []interface{}{ + map[string]interface{}{ + "name": "local", + }, + map[string]interface{}{ + "name": "local", + }, + }, + }, + }, + }, + }, + }, + { + Name: "multiple volume labels", + Error: "labels may only be", + Mounts: []interface{}{ + map[string]interface{}{ + "target": "/nomad", + "volume_options": []interface{}{ + map[string]interface{}{ + "labels": []interface{}{ + map[string]string{"foo": "bar"}, + map[string]string{"baz": "bam"}, + }, + }, + }, + }, + }, + }, + { + Name: "multiple driver options", + Error: "driver options may only", + Mounts: []interface{}{ + map[string]interface{}{ + "target": "/nomad", + "volume_options": []interface{}{ + map[string]interface{}{ + "driver_config": []interface{}{ + map[string]interface{}{ + "name": "local", + "options": []interface{}{ + map[string]interface{}{ + "foo": "bar", + }, + map[string]interface{}{ + "bam": "bar", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + task := &structs.Task{ + Name: "redis-demo", + Driver: "docker", + Config: map[string]interface{}{ + "image": "busybox", + "load": "busybox.tar", + "command": "/bin/sleep", + "args": []string{"10000"}, + }, + Resources: &structs.Resources{ + MemoryMB: 256, + CPU: 512, + }, + LogConfig: &structs.LogConfig{ + MaxFiles: 10, + MaxFileSizeMB: 10, + }, + } + + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + // Build the task + task.Config["mounts"] = c.Mounts + + ctx := testDockerDriverContexts(t, task) + driver := NewDockerDriver(ctx.DriverCtx) + copyImage(t, ctx.ExecCtx.TaskDir, "busybox.tar") + defer ctx.AllocDir.Destroy() + + _, err := driver.Prestart(ctx.ExecCtx, task) + if err == nil && c.Error != "" { + t.Fatalf("expected error: %v", c.Error) + } else if err != nil { + if c.Error == "" { + t.Fatalf("unexpected error in prestart: %v", err) + } else if !strings.Contains(err.Error(), c.Error) { + t.Fatalf("expected error %q; got %v", c.Error, err) + } + } + }) + } +} + // TestDockerDriver_Cleanup ensures Cleanup removes only downloaded images. func TestDockerDriver_Cleanup(t *testing.T) { if !tu.IsTravis() {