From 911d17e3ee2c5979eb26e50f5035b4892191b3ba Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Fri, 13 Sep 2019 11:24:58 -0400 Subject: [PATCH 1/9] docker: periodically reconcile containers When running at scale, it's possible that Docker Engine starts containers successfully but gets wedged in a way where API call fails. The Docker Engine may remain unavailable for arbitrary long time. Here, we introduce a periodic reconcilation process that ensures that any container started by nomad is tracked, and killed if is running unexpectedly. Basically, the periodic job inspects any container that isn't tracked in its handlers. A creation grace period is used to prevent killing newly created containers that aren't registered yet. Also, we aim to avoid killing unrelated containters started by host or through raw_exec drivers. The logic is to pattern against containers environment variables and mounts to infer if they are an alloc docker container. Lastly, the periodic job can be disabled to avoid any interference if need be. --- drivers/docker/config.go | 49 ++++++ drivers/docker/reconciler.go | 164 ++++++++++++++++++ drivers/docker/reconciler_test.go | 150 ++++++++++++++++ .../docker/reconciler_containers_list.json | 116 +++++++++++++ 4 files changed, 479 insertions(+) create mode 100644 drivers/docker/reconciler.go create mode 100644 drivers/docker/reconciler_test.go create mode 100644 drivers/docker/test-resources/docker/reconciler_containers_list.json diff --git a/drivers/docker/config.go b/drivers/docker/config.go index b18f00cbf4ed..08f7f208df46 100644 --- a/drivers/docker/config.go +++ b/drivers/docker/config.go @@ -134,6 +134,21 @@ var ( Name: pluginName, } + danglingContainersBlock = hclspec.NewObject(map[string]*hclspec.Spec{ + "enabled": hclspec.NewDefault( + hclspec.NewAttr("enabled", "bool", false), + hclspec.NewLiteral(`true`), + ), + "period": hclspec.NewDefault( + hclspec.NewAttr("period", "string", false), + hclspec.NewLiteral(`"5m"`), + ), + "creation_timeout": hclspec.NewDefault( + hclspec.NewAttr("creation_timeout", "string", false), + hclspec.NewLiteral(`"5m"`), + ), + }) + // configSpec is the hcl specification returned by the ConfigSchema RPC // and is used to parse the contents of the 'plugin "docker" {...}' block. // Example: @@ -195,6 +210,10 @@ var ( hclspec.NewAttr("container", "bool", false), hclspec.NewLiteral("true"), ), + "dangling_containers": hclspec.NewDefault( + hclspec.NewBlock("dangling_containers", false, danglingContainersBlock), + hclspec.NewLiteral("{}"), + ), })), hclspec.NewLiteral(`{ image = true container = true @@ -491,6 +510,16 @@ type DockerVolumeDriverConfig struct { Options hclutils.MapStrStr `codec:"options"` } +type ContainerGCConfig struct { + Enabled bool `codec:"enabled"` + + PeriodStr string `codec:"period"` + period time.Duration `codec:"-"` + + CreationTimeoutStr string `codec:"creation_timeout"` + creationTimeout time.Duration `codec:"-"` +} + type DriverConfig struct { Endpoint string `codec:"endpoint"` Auth AuthConfig `codec:"auth"` @@ -519,6 +548,8 @@ type GCConfig struct { ImageDelay string `codec:"image_delay"` imageDelayDuration time.Duration `codec:"-"` Container bool `codec:"container"` + + DanglingContainers ContainerGCConfig `codec:"dangling_containers"` } type VolumeConfig struct { @@ -551,6 +582,22 @@ func (d *Driver) SetConfig(c *base.Config) error { d.config.GC.imageDelayDuration = dur } + if len(d.config.GC.DanglingContainers.PeriodStr) > 0 { + dur, err := time.ParseDuration(d.config.GC.DanglingContainers.PeriodStr) + if err != nil { + return fmt.Errorf("failed to parse 'period' duration: %v", err) + } + d.config.GC.DanglingContainers.period = dur + } + + if len(d.config.GC.DanglingContainers.CreationTimeoutStr) > 0 { + dur, err := time.ParseDuration(d.config.GC.DanglingContainers.CreationTimeoutStr) + if err != nil { + return fmt.Errorf("failed to parse 'container_delay' duration: %v", err) + } + d.config.GC.DanglingContainers.creationTimeout = dur + } + if c.AgentConfig != nil { d.clientConfig = c.AgentConfig.Driver } @@ -568,6 +615,8 @@ func (d *Driver) SetConfig(c *base.Config) error { d.coordinator = newDockerCoordinator(coordinatorConfig) + go d.removeDanglingContainersGoroutine() + return nil } diff --git a/drivers/docker/reconciler.go b/drivers/docker/reconciler.go new file mode 100644 index 000000000000..0753c50ebc08 --- /dev/null +++ b/drivers/docker/reconciler.go @@ -0,0 +1,164 @@ +package docker + +import ( + "context" + "fmt" + "regexp" + "strings" + "time" + + docker "github.com/fsouza/go-dockerclient" +) + +func (d *Driver) removeDanglingContainersGoroutine() { + if !d.config.GC.DanglingContainers.Enabled { + d.logger.Debug("skipping dangling containers handling; is disabled") + return + } + + period := d.config.GC.DanglingContainers.period + + succeeded := true + + timer := time.NewTimer(period) + for { + select { + case <-timer.C: + if d.previouslyDetected() && d.fingerprintSuccessful() { + err := d.removeDanglingContainersIteration() + if err != nil && succeeded { + d.logger.Warn("failed to remove dangling containers", "error", err) + } + succeeded = (err == nil) + } + + timer.Reset(period) + case <-d.ctx.Done(): + return + } + } +} + +func (d *Driver) removeDanglingContainersIteration() error { + tracked := d.trackedContainers() + untracked, err := d.untrackedContainers(tracked, d.config.GC.DanglingContainers.creationTimeout) + if err != nil { + return fmt.Errorf("failed to find untracked containers: %v", err) + } + + for _, id := range untracked { + d.logger.Info("removing untracked container", "container_id", id) + err := client.RemoveContainer(docker.RemoveContainerOptions{ + ID: id, + Force: true, + }) + if err != nil { + d.logger.Warn("failed to remove untracked container", "container_id", id, "error", err) + } + } + + return nil +} + +// untrackedContainers returns the ids of containers that look +func (d *Driver) untrackedContainers(tracked map[string]bool, creationTimeout time.Duration) ([]string, error) { + result := []string{} + + cc, err := client.ListContainers(docker.ListContainersOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to list containers: %v", err) + } + + cutoff := time.Now().Add(-creationTimeout).Unix() + + for _, c := range cc { + if tracked[c.ID] { + continue + } + + if c.Created > cutoff { + continue + } + + if !d.isNomadContainer(c) { + continue + } + + result = append(result, c.ID) + } + + return result, nil +} + +func (d *Driver) isNomadContainer(c docker.APIContainers) bool { + if _, ok := c.Labels["com.hashicorp.nomad.alloc_id"]; ok { + return true + } + + // pre-0.10 containers aren't tagged or labeled in any way, + // so use cheap heauristic based on mount paths + // before inspecting container details + if !hasMount(c, "/alloc") || + !hasMount(c, "/local") || + !hasMount(c, "/secrets") || + !hasNomadName(c) { + return false + } + + // double check before killing process + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + ci, err := client.InspectContainerWithContext(c.ID, ctx) + if err != nil { + return false + } + + env := ci.Config.Env + return hasEnvVar(env, "NOMAD_ALLOC_ID") && + hasEnvVar(env, "NOMAD_GROUP_NAME") +} + +func hasMount(c docker.APIContainers, p string) bool { + for _, m := range c.Mounts { + if m.Destination == p { + return true + } + } + + return false +} + +var nomadContainerNamePattern = regexp.MustCompile(`\/.*-[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}`) + +func hasNomadName(c docker.APIContainers) bool { + for _, n := range c.Names { + if nomadContainerNamePattern.MatchString(n) { + return true + } + } + + return false +} + +func hasEnvVar(vars []string, key string) bool { + for _, v := range vars { + if strings.HasPrefix(v, key+"=") { + return true + } + } + + return false +} + +func (d *Driver) trackedContainers() map[string]bool { + d.tasks.lock.RLock() + defer d.tasks.lock.RUnlock() + + r := make(map[string]bool, len(d.tasks.store)) + for _, h := range d.tasks.store { + r[h.containerID] = true + } + + return r +} diff --git a/drivers/docker/reconciler_test.go b/drivers/docker/reconciler_test.go new file mode 100644 index 000000000000..582552f48b21 --- /dev/null +++ b/drivers/docker/reconciler_test.go @@ -0,0 +1,150 @@ +package docker + +import ( + "encoding/json" + "os" + "testing" + "time" + + docker "github.com/fsouza/go-dockerclient" + "github.com/hashicorp/nomad/client/testutil" + "github.com/hashicorp/nomad/helper/uuid" + tu "github.com/hashicorp/nomad/testutil" + "github.com/stretchr/testify/require" +) + +var sampleContainerList []docker.APIContainers +var sampleNomadContainerListItem docker.APIContainers +var sampleNonNomadContainerListItem docker.APIContainers + +func init() { + path := "./test-resources/docker/reconciler_containers_list.json" + + f, err := os.Open(path) + if err != nil { + return + } + + err = json.NewDecoder(f).Decode(&sampleContainerList) + if err != nil { + return + } + + sampleNomadContainerListItem = sampleContainerList[0] + sampleNonNomadContainerListItem = sampleContainerList[1] +} + +func Test_HasMount(t *testing.T) { + require.True(t, hasMount(sampleNomadContainerListItem, "/alloc")) + require.True(t, hasMount(sampleNomadContainerListItem, "/data")) + require.True(t, hasMount(sampleNomadContainerListItem, "/secrets")) + require.False(t, hasMount(sampleNomadContainerListItem, "/random")) + + require.False(t, hasMount(sampleNonNomadContainerListItem, "/alloc")) + require.False(t, hasMount(sampleNonNomadContainerListItem, "/data")) + require.False(t, hasMount(sampleNonNomadContainerListItem, "/secrets")) + require.False(t, hasMount(sampleNonNomadContainerListItem, "/random")) +} + +func Test_HasNomadName(t *testing.T) { + require.True(t, hasNomadName(sampleNomadContainerListItem)) + require.False(t, hasNomadName(sampleNonNomadContainerListItem)) +} + +func TestHasEnv(t *testing.T) { + envvars := []string{ + "NOMAD_ALLOC_DIR=/alloc", + "NOMAD_ALLOC_ID=72bfa388-024e-a903-45b8-2bc28b74ed69", + "NOMAD_ALLOC_INDEX=0", + "NOMAD_ALLOC_NAME=example.cache[0]", + "NOMAD_CPU_LIMIT=500", + "NOMAD_DC=dc1", + "NOMAD_GROUP_NAME=cache", + "NOMAD_JOB_NAME=example", + "NOMAD_MEMORY_LIMIT=256", + "NOMAD_NAMESPACE=default", + "NOMAD_REGION=global", + "NOMAD_SECRETS_DIR=/secrets", + "NOMAD_TASK_DIR=/local", + "NOMAD_TASK_NAME=redis", + "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + "GOSU_VERSION=1.10", + "REDIS_VERSION=3.2.12", + "REDIS_DOWNLOAD_URL=http://download.redis.io/releases/redis-3.2.12.tar.gz", + "REDIS_DOWNLOAD_SHA=98c4254ae1be4e452aa7884245471501c9aa657993e0318d88f048093e7f88fd", + } + + require.True(t, hasEnvVar(envvars, "NOMAD_ALLOC_ID")) + require.True(t, hasEnvVar(envvars, "NOMAD_ALLOC_DIR")) + require.True(t, hasEnvVar(envvars, "GOSU_VERSION")) + + require.False(t, hasEnvVar(envvars, "NOMAD_ALLOC_")) + require.False(t, hasEnvVar(envvars, "OTHER_VARIABLE")) +} + +func TestDanglingContainerRemoval(t *testing.T) { + if !tu.IsCI() { + t.Parallel() + } + testutil.DockerCompatible(t) + + task, cfg, _ := dockerTask(t) + require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + + client, d, handle, cleanup := dockerSetup(t, task) + defer cleanup() + require.NoError(t, d.WaitUntilStarted(task.ID, 5*time.Second)) + + c, err := client.CreateContainer(docker.CreateContainerOptions{ + Name: "mytest-image-" + uuid.Generate(), + Config: &docker.Config{ + Image: cfg.Image, + Cmd: append([]string{cfg.Command}, cfg.Args...), + }, + }) + require.NoError(t, err) + defer client.RemoveContainer(docker.RemoveContainerOptions{ + ID: c.ID, + Force: true, + }) + + err = client.StartContainer(c.ID, nil) + require.NoError(t, err) + + time.Sleep(1 * time.Second) + + dd := d.Impl().(*Driver) + trackedContainers := map[string]bool{handle.containerID: true} + + { + tf := dd.trackedContainers() + require.Contains(t, tf, handle.containerID) + require.NotContains(t, tf, c.ID) + } + + untracked, err := dd.untrackedContainers(trackedContainers, 1*time.Minute) + require.NoError(t, err) + require.NotContains(t, untracked, handle.containerID) + require.NotContains(t, untracked, c.ID) + + untracked, err = dd.untrackedContainers(map[string]bool{}, 0) + require.NoError(t, err) + require.Contains(t, untracked, handle.containerID) + require.NotContains(t, untracked, c.ID) + + // Actually try to kill hosts + prestineDriver := dockerDriverHarness(t, nil).Impl().(*Driver) + prestineDriver.config.GC.DanglingContainers = ContainerGCConfig{ + Enabled: true, + period: 1 * time.Second, + creationTimeout: 1 * time.Second, + } + require.NoError(t, prestineDriver.removeDanglingContainersIteration()) + + _, err = client.InspectContainer(c.ID) + require.NoError(t, err) + + _, err = client.InspectContainer(handle.containerID) + require.Error(t, err) + require.Contains(t, err.Error(), NoSuchContainerError) +} diff --git a/drivers/docker/test-resources/docker/reconciler_containers_list.json b/drivers/docker/test-resources/docker/reconciler_containers_list.json new file mode 100644 index 000000000000..50e3086b75f5 --- /dev/null +++ b/drivers/docker/test-resources/docker/reconciler_containers_list.json @@ -0,0 +1,116 @@ +[ + { + "Id": "eb23be71498c2dc0254c029f32b360a000caf33157d1c93e226f4c1a4c9d2218", + "Names": [ + "/redis-72bfa388-024e-a903-45b8-2bc28b74ed69" + ], + "Image": "redis:3.2", + "ImageID": "sha256:87856cc39862cec77541d68382e4867d7ccb29a85a17221446c857ddaebca916", + "Command": "docker-entrypoint.sh redis-server", + "Created": 1568383081, + "Ports": [ + { + "PrivatePort": 6379, + "Type": "tcp" + } + ], + "Labels": {}, + "State": "running", + "Status": "Up 9 seconds", + "HostConfig": { + "NetworkMode": "default" + }, + "NetworkSettings": { + "Networks": { + "bridge": { + "IPAMConfig": null, + "Links": null, + "Aliases": null, + "NetworkID": "6715ed501c1cef14545cd6680f54b4971373ee4441aec2300fff1031c8dbf3a4", + "EndpointID": "ed830b4f2f33ab4134aea941611b00b9e576b35a4325d52bacfedd1e2e1ba213", + "Gateway": "172.17.0.1", + "IPAddress": "172.17.0.3", + "IPPrefixLen": 16, + "IPv6Gateway": "", + "GlobalIPv6Address": "", + "GlobalIPv6PrefixLen": 0, + "MacAddress": "02:42:ac:11:00:03", + "DriverOpts": null + } + } + }, + "Mounts": [ + { + "Type": "bind", + "Source": "/private/var/folders/r6/346cfqyn76b_lx1nrcl5278c0000gp/T/NomadClient831122597/72bfa388-024e-a903-45b8-2bc28b74ed69/alloc", + "Destination": "/alloc", + "Mode": "", + "RW": true, + "Propagation": "rprivate" + }, + { + "Type": "volume", + "Name": "d5d7f0f9a3326414257c57cfca01db96c53a424b43e251516511694554309681", + "Source": "", + "Destination": "/data", + "Driver": "local", + "Mode": "", + "RW": true, + "Propagation": "" + }, + { + "Type": "bind", + "Source": "/private/var/folders/r6/346cfqyn76b_lx1nrcl5278c0000gp/T/NomadClient831122597/72bfa388-024e-a903-45b8-2bc28b74ed69/redis/local", + "Destination": "/local", + "Mode": "", + "RW": true, + "Propagation": "rprivate" + }, + { + "Type": "bind", + "Source": "/private/var/folders/r6/346cfqyn76b_lx1nrcl5278c0000gp/T/NomadClient831122597/72bfa388-024e-a903-45b8-2bc28b74ed69/redis/secrets", + "Destination": "/secrets", + "Mode": "", + "RW": true, + "Propagation": "rprivate" + } + ] + }, + { + "Id": "99c49fbe999f6df7b7d6a891d69fe57d7b771a30d5d2899a922b44698084e5c9", + "Names": [ + "/serene_keller" + ], + "Image": "ubuntu:16.04", + "ImageID": "sha256:9361ce633ff193349d54bed380a5afe86043b09fd6ea8da7549dbbedfc2a7077", + "Command": "/bin/bash", + "Created": 1567795217, + "Ports": [], + "Labels": {}, + "State": "running", + "Status": "Up 6 days", + "HostConfig": { + "NetworkMode": "default" + }, + "NetworkSettings": { + "Networks": { + "bridge": { + "IPAMConfig": null, + "Links": null, + "Aliases": null, + "NetworkID": "6715ed501c1cef14545cd6680f54b4971373ee4441aec2300fff1031c8dbf3a4", + "EndpointID": "fab83a0d4089ca9944ca53c882bdf40ad310c6fda30dda0092731feb9bc9fab6", + "Gateway": "172.17.0.1", + "IPAddress": "172.17.0.2", + "IPPrefixLen": 16, + "IPv6Gateway": "", + "GlobalIPv6Address": "", + "GlobalIPv6PrefixLen": 0, + "MacAddress": "02:42:ac:11:00:02", + "DriverOpts": null + } + } + }, + "Mounts": [] + } +] From 3bf0ae995adef552d6dc1ba6d8fbfd6269f1ca89 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Fri, 13 Sep 2019 13:59:36 -0400 Subject: [PATCH 2/9] docker: explicit grace period for initial container reconcilation Ensure we wait for some grace period before killing docker containers that may have launched in earlier nomad restore. --- drivers/docker/reconciler.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/docker/reconciler.go b/drivers/docker/reconciler.go index 0753c50ebc08..77d0eabb1df8 100644 --- a/drivers/docker/reconciler.go +++ b/drivers/docker/reconciler.go @@ -20,7 +20,17 @@ func (d *Driver) removeDanglingContainersGoroutine() { succeeded := true - timer := time.NewTimer(period) + // ensure that we wait for at least a period or creation timeout + // for first container GC iteration + // The initial period is a grace period for restore allocation + // before a driver may kill containers launched by an earlier nomad + // process. + initialDelay := period + if d.config.GC.DanglingContainers.creationTimeout > initialDelay { + initialDelay = d.config.GC.DanglingContainers.creationTimeout + } + + timer := time.NewTimer(initialDelay) for { select { case <-timer.C: From c8ba2d1b862ceb59914d0dce422a2b7d6e5dd928 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Mon, 16 Sep 2019 10:40:56 -0400 Subject: [PATCH 3/9] address code review comments --- drivers/docker/driver.go | 4 ++++ drivers/docker/reconciler.go | 5 +++-- drivers/docker/reconciler_test.go | 2 -- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 67b578bc3b68..8c900d5d9e75 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -309,6 +309,10 @@ CREATE: // the container is started runningContainer, err := client.InspectContainer(container.ID) if err != nil { + client.RemoveContainer(docker.RemoveContainerOptions{ + ID: container.ID, + Force: true, + }) msg := "failed to inspect started container" d.logger.Error(msg, "error", err) client.RemoveContainer(docker.RemoveContainerOptions{ diff --git a/drivers/docker/reconciler.go b/drivers/docker/reconciler.go index 77d0eabb1df8..23eb0bbc9dc4 100644 --- a/drivers/docker/reconciler.go +++ b/drivers/docker/reconciler.go @@ -70,7 +70,8 @@ func (d *Driver) removeDanglingContainersIteration() error { return nil } -// untrackedContainers returns the ids of containers that look +// untrackedContainers returns the ids of containers that suspected +// to have been started by Nomad but aren't tracked by this driver func (d *Driver) untrackedContainers(tracked map[string]bool, creationTimeout time.Duration) ([]string, error) { result := []string{} @@ -116,7 +117,7 @@ func (d *Driver) isNomadContainer(c docker.APIContainers) bool { } // double check before killing process - ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + ctx, cancel := context.WithTimeout(d.ctx, 20*time.Second) defer cancel() ci, err := client.InspectContainerWithContext(c.ID, ctx) diff --git a/drivers/docker/reconciler_test.go b/drivers/docker/reconciler_test.go index 582552f48b21..06ffbbe00683 100644 --- a/drivers/docker/reconciler_test.go +++ b/drivers/docker/reconciler_test.go @@ -111,8 +111,6 @@ func TestDanglingContainerRemoval(t *testing.T) { err = client.StartContainer(c.ID, nil) require.NoError(t, err) - time.Sleep(1 * time.Second) - dd := d.Impl().(*Driver) trackedContainers := map[string]bool{handle.containerID: true} From 24f6c2bf073d60ca8351facb54dd8aeb0fc244b0 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Thu, 17 Oct 2019 08:37:18 -0400 Subject: [PATCH 4/9] refactor reconciler code and address comments --- drivers/docker/config.go | 40 +++++-- drivers/docker/reconciler.go | 120 ++++++++++++--------- drivers/docker/reconciler_test.go | 168 ++++++++++++++++++------------ 3 files changed, 206 insertions(+), 122 deletions(-) diff --git a/drivers/docker/config.go b/drivers/docker/config.go index 08f7f208df46..bec051a351de 100644 --- a/drivers/docker/config.go +++ b/drivers/docker/config.go @@ -143,10 +143,14 @@ var ( hclspec.NewAttr("period", "string", false), hclspec.NewLiteral(`"5m"`), ), - "creation_timeout": hclspec.NewDefault( - hclspec.NewAttr("creation_timeout", "string", false), + "creation_grace": hclspec.NewDefault( + hclspec.NewAttr("creation_grace", "string", false), hclspec.NewLiteral(`"5m"`), ), + "dry_run": hclspec.NewDefault( + hclspec.NewAttr("dry_run", "bool", false), + hclspec.NewLiteral(`false`), + ), }) // configSpec is the hcl specification returned by the ConfigSchema RPC @@ -510,14 +514,26 @@ type DockerVolumeDriverConfig struct { Options hclutils.MapStrStr `codec:"options"` } +// ContainerGCConfig controls the behavior of the GC reconciler to detects +// dangling nomad containers that aren't tracked due to docker/nomad bugs type ContainerGCConfig struct { + // Enabled controls whether container reconciler is enabled Enabled bool `codec:"enabled"` + // DryRun indicates that reconciler should log unexpectedly running containers + // if found without actually killing them + DryRun bool `codec:"dry_run"` + + // PeriodStr controls the frequency of scanning containers PeriodStr string `codec:"period"` period time.Duration `codec:"-"` - CreationTimeoutStr string `codec:"creation_timeout"` - creationTimeout time.Duration `codec:"-"` + // CreationGraceStr is the duration allowed for a newly created container + // to live without being registered as a running task in nomad. + // A container is treated as leaked if it lived more than grace duration + // and haven't been registered in tasks. + CreationGraceStr string `codec:"creation_grace"` + CreationGrace time.Duration `codec:"-"` } type DriverConfig struct { @@ -565,6 +581,8 @@ func (d *Driver) ConfigSchema() (*hclspec.Spec, error) { return configSpec, nil } +const danglingContainersCreationGraceMinimum = 1 * time.Minute + func (d *Driver) SetConfig(c *base.Config) error { var config DriverConfig if len(c.PluginConfig) != 0 { @@ -590,12 +608,15 @@ func (d *Driver) SetConfig(c *base.Config) error { d.config.GC.DanglingContainers.period = dur } - if len(d.config.GC.DanglingContainers.CreationTimeoutStr) > 0 { - dur, err := time.ParseDuration(d.config.GC.DanglingContainers.CreationTimeoutStr) + if len(d.config.GC.DanglingContainers.CreationGraceStr) > 0 { + dur, err := time.ParseDuration(d.config.GC.DanglingContainers.CreationGraceStr) if err != nil { - return fmt.Errorf("failed to parse 'container_delay' duration: %v", err) + return fmt.Errorf("failed to parse 'creation_grace' duration: %v", err) + } + if dur < danglingContainersCreationGraceMinimum { + return fmt.Errorf("creation_grace is less than minimum, %v", danglingContainersCreationGraceMinimum) } - d.config.GC.DanglingContainers.creationTimeout = dur + d.config.GC.DanglingContainers.CreationGrace = dur } if c.AgentConfig != nil { @@ -615,7 +636,8 @@ func (d *Driver) SetConfig(c *base.Config) error { d.coordinator = newDockerCoordinator(coordinatorConfig) - go d.removeDanglingContainersGoroutine() + reconciler := newReconciler(d) + reconciler.Start() return nil } diff --git a/drivers/docker/reconciler.go b/drivers/docker/reconciler.go index 23eb0bbc9dc4..a97984628edd 100644 --- a/drivers/docker/reconciler.go +++ b/drivers/docker/reconciler.go @@ -4,21 +4,55 @@ import ( "context" "fmt" "regexp" - "strings" "time" docker "github.com/fsouza/go-dockerclient" + hclog "github.com/hashicorp/go-hclog" ) -func (d *Driver) removeDanglingContainersGoroutine() { - if !d.config.GC.DanglingContainers.Enabled { - d.logger.Debug("skipping dangling containers handling; is disabled") +// containerReconciler detects and kills unexpectedly running containers. +// +// Due to Docker architecture and network based communication, it is +// possible for Docker to start a container successfully, but have the +// creation API call fail with a network error. containerReconciler +// scans for these untracked containers and kill them. +type containerReconciler struct { + ctx context.Context + config *ContainerGCConfig + client *docker.Client + logger hclog.Logger + + isDriverHealthy func() bool + trackedContainers func() map[string]bool + isNomadContainer func(c docker.APIContainers) bool +} + +func newReconciler(d *Driver) *containerReconciler { + return &containerReconciler{ + ctx: d.ctx, + config: &d.config.GC.DanglingContainers, + client: client, + logger: d.logger, + + isDriverHealthy: func() bool { return d.previouslyDetected() && d.fingerprintSuccessful() }, + trackedContainers: d.trackedContainers, + isNomadContainer: isNomadContainer, + } +} + +func (r *containerReconciler) Start() { + if !r.config.Enabled { + r.logger.Debug("skipping dangling containers handling; is disabled") return } - period := d.config.GC.DanglingContainers.period + go r.removeDanglingContainersGoroutine() +} + +func (r *containerReconciler) removeDanglingContainersGoroutine() { + period := r.config.period - succeeded := true + lastIterSucceeded := true // ensure that we wait for at least a period or creation timeout // for first container GC iteration @@ -26,44 +60,55 @@ func (d *Driver) removeDanglingContainersGoroutine() { // before a driver may kill containers launched by an earlier nomad // process. initialDelay := period - if d.config.GC.DanglingContainers.creationTimeout > initialDelay { - initialDelay = d.config.GC.DanglingContainers.creationTimeout + if r.config.CreationGrace > initialDelay { + initialDelay = r.config.CreationGrace } timer := time.NewTimer(initialDelay) for { select { case <-timer.C: - if d.previouslyDetected() && d.fingerprintSuccessful() { - err := d.removeDanglingContainersIteration() - if err != nil && succeeded { - d.logger.Warn("failed to remove dangling containers", "error", err) + if r.isDriverHealthy() { + err := r.removeDanglingContainersIteration() + if err != nil && lastIterSucceeded { + r.logger.Warn("failed to remove dangling containers", "error", err) } - succeeded = (err == nil) + lastIterSucceeded = (err == nil) } timer.Reset(period) - case <-d.ctx.Done(): + case <-r.ctx.Done(): return } } } -func (d *Driver) removeDanglingContainersIteration() error { - tracked := d.trackedContainers() - untracked, err := d.untrackedContainers(tracked, d.config.GC.DanglingContainers.creationTimeout) +func (r *containerReconciler) removeDanglingContainersIteration() error { + cutoff := time.Now().Add(-r.config.CreationGrace) + tracked := r.trackedContainers() + untracked, err := r.untrackedContainers(tracked, cutoff) if err != nil { return fmt.Errorf("failed to find untracked containers: %v", err) } + if len(untracked) == 0 { + return nil + } + + if r.config.DryRun { + r.logger.Info("detected untracked containers", "container_ids", untracked) + return nil + } + for _, id := range untracked { - d.logger.Info("removing untracked container", "container_id", id) err := client.RemoveContainer(docker.RemoveContainerOptions{ ID: id, Force: true, }) if err != nil { - d.logger.Warn("failed to remove untracked container", "container_id", id, "error", err) + r.logger.Warn("failed to remove untracked container", "container_id", id, "error", err) + } else { + r.logger.Info("removed untracked container", "container_id", id) } } @@ -72,15 +117,17 @@ func (d *Driver) removeDanglingContainersIteration() error { // untrackedContainers returns the ids of containers that suspected // to have been started by Nomad but aren't tracked by this driver -func (d *Driver) untrackedContainers(tracked map[string]bool, creationTimeout time.Duration) ([]string, error) { +func (r *containerReconciler) untrackedContainers(tracked map[string]bool, cutoffTime time.Time) ([]string, error) { result := []string{} - cc, err := client.ListContainers(docker.ListContainersOptions{}) + cc, err := client.ListContainers(docker.ListContainersOptions{ + All: false, // only reconcile running containers + }) if err != nil { return nil, fmt.Errorf("failed to list containers: %v", err) } - cutoff := time.Now().Add(-creationTimeout).Unix() + cutoff := cutoffTime.Unix() for _, c := range cc { if tracked[c.ID] { @@ -91,7 +138,7 @@ func (d *Driver) untrackedContainers(tracked map[string]bool, creationTimeout ti continue } - if !d.isNomadContainer(c) { + if !r.isNomadContainer(c) { continue } @@ -101,13 +148,13 @@ func (d *Driver) untrackedContainers(tracked map[string]bool, creationTimeout ti return result, nil } -func (d *Driver) isNomadContainer(c docker.APIContainers) bool { +func isNomadContainer(c docker.APIContainers) bool { if _, ok := c.Labels["com.hashicorp.nomad.alloc_id"]; ok { return true } // pre-0.10 containers aren't tagged or labeled in any way, - // so use cheap heauristic based on mount paths + // so use cheap heuristic based on mount paths // before inspecting container details if !hasMount(c, "/alloc") || !hasMount(c, "/local") || @@ -116,18 +163,7 @@ func (d *Driver) isNomadContainer(c docker.APIContainers) bool { return false } - // double check before killing process - ctx, cancel := context.WithTimeout(d.ctx, 20*time.Second) - defer cancel() - - ci, err := client.InspectContainerWithContext(c.ID, ctx) - if err != nil { - return false - } - - env := ci.Config.Env - return hasEnvVar(env, "NOMAD_ALLOC_ID") && - hasEnvVar(env, "NOMAD_GROUP_NAME") + return true } func hasMount(c docker.APIContainers, p string) bool { @@ -152,16 +188,6 @@ func hasNomadName(c docker.APIContainers) bool { return false } -func hasEnvVar(vars []string, key string) bool { - for _, v := range vars { - if strings.HasPrefix(v, key+"=") { - return true - } - } - - return false -} - func (d *Driver) trackedContainers() map[string]bool { d.tasks.lock.RLock() defer d.tasks.lock.RUnlock() diff --git a/drivers/docker/reconciler_test.go b/drivers/docker/reconciler_test.go index 06ffbbe00683..66e7028e79a0 100644 --- a/drivers/docker/reconciler_test.go +++ b/drivers/docker/reconciler_test.go @@ -9,85 +9,53 @@ import ( docker "github.com/fsouza/go-dockerclient" "github.com/hashicorp/nomad/client/testutil" "github.com/hashicorp/nomad/helper/uuid" - tu "github.com/hashicorp/nomad/testutil" "github.com/stretchr/testify/require" ) -var sampleContainerList []docker.APIContainers -var sampleNomadContainerListItem docker.APIContainers -var sampleNonNomadContainerListItem docker.APIContainers - -func init() { +func fakeContainerList(t *testing.T) (nomadContainer, nonNomadContainer docker.APIContainers) { path := "./test-resources/docker/reconciler_containers_list.json" f, err := os.Open(path) if err != nil { - return + t.Fatalf("failed to open file: %v", err) } + var sampleContainerList []docker.APIContainers err = json.NewDecoder(f).Decode(&sampleContainerList) if err != nil { - return + t.Fatalf("failed to decode container list: %v", err) } - sampleNomadContainerListItem = sampleContainerList[0] - sampleNonNomadContainerListItem = sampleContainerList[1] + return sampleContainerList[0], sampleContainerList[1] } func Test_HasMount(t *testing.T) { - require.True(t, hasMount(sampleNomadContainerListItem, "/alloc")) - require.True(t, hasMount(sampleNomadContainerListItem, "/data")) - require.True(t, hasMount(sampleNomadContainerListItem, "/secrets")) - require.False(t, hasMount(sampleNomadContainerListItem, "/random")) - - require.False(t, hasMount(sampleNonNomadContainerListItem, "/alloc")) - require.False(t, hasMount(sampleNonNomadContainerListItem, "/data")) - require.False(t, hasMount(sampleNonNomadContainerListItem, "/secrets")) - require.False(t, hasMount(sampleNonNomadContainerListItem, "/random")) -} + nomadContainer, nonNomadContainer := fakeContainerList(t) -func Test_HasNomadName(t *testing.T) { - require.True(t, hasNomadName(sampleNomadContainerListItem)) - require.False(t, hasNomadName(sampleNonNomadContainerListItem)) -} + require.True(t, hasMount(nomadContainer, "/alloc")) + require.True(t, hasMount(nomadContainer, "/data")) + require.True(t, hasMount(nomadContainer, "/secrets")) + require.False(t, hasMount(nomadContainer, "/random")) -func TestHasEnv(t *testing.T) { - envvars := []string{ - "NOMAD_ALLOC_DIR=/alloc", - "NOMAD_ALLOC_ID=72bfa388-024e-a903-45b8-2bc28b74ed69", - "NOMAD_ALLOC_INDEX=0", - "NOMAD_ALLOC_NAME=example.cache[0]", - "NOMAD_CPU_LIMIT=500", - "NOMAD_DC=dc1", - "NOMAD_GROUP_NAME=cache", - "NOMAD_JOB_NAME=example", - "NOMAD_MEMORY_LIMIT=256", - "NOMAD_NAMESPACE=default", - "NOMAD_REGION=global", - "NOMAD_SECRETS_DIR=/secrets", - "NOMAD_TASK_DIR=/local", - "NOMAD_TASK_NAME=redis", - "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", - "GOSU_VERSION=1.10", - "REDIS_VERSION=3.2.12", - "REDIS_DOWNLOAD_URL=http://download.redis.io/releases/redis-3.2.12.tar.gz", - "REDIS_DOWNLOAD_SHA=98c4254ae1be4e452aa7884245471501c9aa657993e0318d88f048093e7f88fd", - } + require.False(t, hasMount(nonNomadContainer, "/alloc")) + require.False(t, hasMount(nonNomadContainer, "/data")) + require.False(t, hasMount(nonNomadContainer, "/secrets")) + require.False(t, hasMount(nonNomadContainer, "/random")) +} - require.True(t, hasEnvVar(envvars, "NOMAD_ALLOC_ID")) - require.True(t, hasEnvVar(envvars, "NOMAD_ALLOC_DIR")) - require.True(t, hasEnvVar(envvars, "GOSU_VERSION")) +func Test_HasNomadName(t *testing.T) { + nomadContainer, nonNomadContainer := fakeContainerList(t) - require.False(t, hasEnvVar(envvars, "NOMAD_ALLOC_")) - require.False(t, hasEnvVar(envvars, "OTHER_VARIABLE")) + require.True(t, hasNomadName(nomadContainer)) + require.False(t, hasNomadName(nonNomadContainer)) } +// TestDanglingContainerRemoval asserts containers without corresponding tasks +// are removed after the creation grace period. func TestDanglingContainerRemoval(t *testing.T) { - if !tu.IsCI() { - t.Parallel() - } testutil.DockerCompatible(t) + // start two containers: one tracked nomad container, and one unrelated container task, cfg, _ := dockerTask(t) require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) @@ -112,32 +80,42 @@ func TestDanglingContainerRemoval(t *testing.T) { require.NoError(t, err) dd := d.Impl().(*Driver) + + reconciler := newReconciler(dd) trackedContainers := map[string]bool{handle.containerID: true} - { - tf := dd.trackedContainers() - require.Contains(t, tf, handle.containerID) - require.NotContains(t, tf, c.ID) - } + tf := reconciler.trackedContainers() + require.Contains(t, tf, handle.containerID) + require.NotContains(t, tf, c.ID) - untracked, err := dd.untrackedContainers(trackedContainers, 1*time.Minute) + // assert tracked containers should never be untracked + untracked, err := reconciler.untrackedContainers(trackedContainers, time.Now()) require.NoError(t, err) require.NotContains(t, untracked, handle.containerID) require.NotContains(t, untracked, c.ID) - untracked, err = dd.untrackedContainers(map[string]bool{}, 0) + // assert we recognize nomad containers with appropriate cutoff + untracked, err = reconciler.untrackedContainers(map[string]bool{}, time.Now()) require.NoError(t, err) require.Contains(t, untracked, handle.containerID) require.NotContains(t, untracked, c.ID) - // Actually try to kill hosts + // but ignore if creation happened before cutoff + untracked, err = reconciler.untrackedContainers(map[string]bool{}, time.Now().Add(-1*time.Minute)) + require.NoError(t, err) + require.NotContains(t, untracked, handle.containerID) + require.NotContains(t, untracked, c.ID) + + // a full integration tests to assert that containers are removed prestineDriver := dockerDriverHarness(t, nil).Impl().(*Driver) prestineDriver.config.GC.DanglingContainers = ContainerGCConfig{ - Enabled: true, - period: 1 * time.Second, - creationTimeout: 1 * time.Second, + Enabled: true, + period: 1 * time.Second, + CreationGrace: 1 * time.Second, } - require.NoError(t, prestineDriver.removeDanglingContainersIteration()) + nReconciler := newReconciler(prestineDriver) + + require.NoError(t, nReconciler.removeDanglingContainersIteration()) _, err = client.InspectContainer(c.ID) require.NoError(t, err) @@ -146,3 +124,61 @@ func TestDanglingContainerRemoval(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), NoSuchContainerError) } + +// TestDanglingContainerRemoval_Stopped asserts stopped containers without +// corresponding tasks are not removed even if after creation grace period. +func TestDanglingContainerRemoval_Stopped(t *testing.T) { + testutil.DockerCompatible(t) + + task, cfg, _ := dockerTask(t) + task.Resources.NomadResources.Networks = nil + require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + + // Start two containers: one nomad container, and one stopped container + // that acts like a nomad one + client, d, handle, cleanup := dockerSetup(t, task) + defer cleanup() + require.NoError(t, d.WaitUntilStarted(task.ID, 5*time.Second)) + + inspected, err := client.InspectContainer(handle.containerID) + require.NoError(t, err) + + stoppedC, err := client.CreateContainer(docker.CreateContainerOptions{ + Name: "mytest-image-" + uuid.Generate(), + Config: inspected.Config, + HostConfig: inspected.HostConfig, + }) + require.NoError(t, err) + defer client.RemoveContainer(docker.RemoveContainerOptions{ + ID: stoppedC.ID, + Force: true, + }) + + err = client.StartContainer(stoppedC.ID, nil) + require.NoError(t, err) + + err = client.StopContainer(stoppedC.ID, 60) + require.NoError(t, err) + + dd := d.Impl().(*Driver) + reconciler := newReconciler(dd) + trackedContainers := map[string]bool{handle.containerID: true} + + // assert nomad container is tracked, and we ignore stopped one + tf := reconciler.trackedContainers() + require.Contains(t, tf, handle.containerID) + require.NotContains(t, tf, stoppedC.ID) + + untracked, err := reconciler.untrackedContainers(trackedContainers, time.Now()) + require.NoError(t, err) + require.NotContains(t, untracked, handle.containerID) + require.NotContains(t, untracked, stoppedC.ID) + + // if we start container again, it'll be marked as untracked + require.NoError(t, client.StartContainer(stoppedC.ID, nil)) + + untracked, err = reconciler.untrackedContainers(trackedContainers, time.Now()) + require.NoError(t, err) + require.NotContains(t, untracked, handle.containerID) + require.Contains(t, untracked, stoppedC.ID) +} From ef4465dfa495c047925608eca5be3ebd9efe3503 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Thu, 17 Oct 2019 09:53:46 -0400 Subject: [PATCH 5/9] add docker labels --- drivers/docker/driver.go | 19 ++++++++++++++++++- drivers/docker/driver_test.go | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 8c900d5d9e75..216fa4d99083 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -66,6 +66,13 @@ var ( nvidiaVisibleDevices = "NVIDIA_VISIBLE_DEVICES" ) +const ( + dockerLabelTaskID = "com.hashicorp.nomad.task_id" + dockerLabelTaskName = "com.hashicorp.nomad.task_name" + dockerLabelAllocID = "com.hashicorp.nomad.alloc_id" + dockerLabelJobName = "com.hashicorp.nomad.job_name" +) + type Driver struct { // eventer is used to handle multiplexing of TaskEvents calls such that an // event can be broadcast to all callers @@ -981,9 +988,19 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T if len(driverConfig.Labels) > 0 { config.Labels = driverConfig.Labels - logger.Debug("applied labels on the container", "labels", config.Labels) } + config.Labels = map[string]string{ + dockerLabelTaskID: task.ID, + dockerLabelTaskName: task.Name, + dockerLabelAllocID: task.AllocID, + dockerLabelJobName: task.JobName, + } + for k, v := range driverConfig.Labels { + config.Labels[k] = v + } + logger.Debug("applied labels on the container", "labels", config.Labels) + config.Env = task.EnvList() containerName := fmt.Sprintf("%s-%s", strings.Replace(task.Name, "/", "_", -1), task.AllocID) diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go index a100aa7431e2..c7fafe448909 100644 --- a/drivers/docker/driver_test.go +++ b/drivers/docker/driver_test.go @@ -905,7 +905,8 @@ func TestDockerDriver_Labels(t *testing.T) { t.Fatalf("err: %v", err) } - require.Equal(t, 2, len(container.Config.Labels)) + // expect to see 4 additional standard labels + require.Equal(t, len(cfg.Labels)+4, len(container.Config.Labels)) for k, v := range cfg.Labels { require.Equal(t, v, container.Config.Labels[k]) } @@ -1008,6 +1009,38 @@ func TestDockerDriver_CreateContainerConfig(t *testing.T) { require.Equal(t, containerName, c.Name) } +func TestDockerDriver_CreateContainerConfig_Labels(t *testing.T) { + t.Parallel() + + task, cfg, _ := dockerTask(t) + task.AllocID = uuid.Generate() + task.JobName = "redis-demo-job" + + cfg.Labels = map[string]string{ + "user_label": "user_value", + } + + require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + + dh := dockerDriverHarness(t, nil) + driver := dh.Impl().(*Driver) + + c, err := driver.createContainerConfig(task, cfg, "org/repo:0.1") + require.NoError(t, err) + + expectedLabels := map[string]string{ + // user provided labels + "user_label": "user_value", + // default labels + "com.hashicorp.nomad.alloc_id": task.AllocID, + "com.hashicorp.nomad.job_name": "redis-demo-job", + "com.hashicorp.nomad.task_id": task.ID, + "com.hashicorp.nomad.task_name": "redis-demo", + } + + require.Equal(t, expectedLabels, c.Config.Labels) +} + func TestDockerDriver_CreateContainerConfig_Logging(t *testing.T) { t.Parallel() From 8c3136a66615bceffcc43e24e814fd7e7e8070ff Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Thu, 17 Oct 2019 10:28:23 -0400 Subject: [PATCH 6/9] docker label refactoring and additional tests --- drivers/docker/reconciler.go | 2 +- drivers/docker/reconciler_test.go | 96 ++++++++++++++++++------------- 2 files changed, 58 insertions(+), 40 deletions(-) diff --git a/drivers/docker/reconciler.go b/drivers/docker/reconciler.go index a97984628edd..9b2454d16b65 100644 --- a/drivers/docker/reconciler.go +++ b/drivers/docker/reconciler.go @@ -149,7 +149,7 @@ func (r *containerReconciler) untrackedContainers(tracked map[string]bool, cutof } func isNomadContainer(c docker.APIContainers) bool { - if _, ok := c.Labels["com.hashicorp.nomad.alloc_id"]; ok { + if _, ok := c.Labels[dockerLabelAllocID]; ok { return true } diff --git a/drivers/docker/reconciler_test.go b/drivers/docker/reconciler_test.go index 66e7028e79a0..71221464a3a3 100644 --- a/drivers/docker/reconciler_test.go +++ b/drivers/docker/reconciler_test.go @@ -63,7 +63,7 @@ func TestDanglingContainerRemoval(t *testing.T) { defer cleanup() require.NoError(t, d.WaitUntilStarted(task.ID, 5*time.Second)) - c, err := client.CreateContainer(docker.CreateContainerOptions{ + nonNomadContainer, err := client.CreateContainer(docker.CreateContainerOptions{ Name: "mytest-image-" + uuid.Generate(), Config: &docker.Config{ Image: cfg.Image, @@ -72,11 +72,30 @@ func TestDanglingContainerRemoval(t *testing.T) { }) require.NoError(t, err) defer client.RemoveContainer(docker.RemoveContainerOptions{ - ID: c.ID, + ID: nonNomadContainer.ID, Force: true, }) - err = client.StartContainer(c.ID, nil) + err = client.StartContainer(nonNomadContainer.ID, nil) + require.NoError(t, err) + + untrackedNomadContainer, err := client.CreateContainer(docker.CreateContainerOptions{ + Name: "mytest-image-" + uuid.Generate(), + Config: &docker.Config{ + Image: cfg.Image, + Cmd: append([]string{cfg.Command}, cfg.Args...), + Labels: map[string]string{ + dockerLabelAllocID: uuid.Generate(), + }, + }, + }) + require.NoError(t, err) + defer client.RemoveContainer(docker.RemoveContainerOptions{ + ID: untrackedNomadContainer.ID, + Force: true, + }) + + err = client.StartContainer(untrackedNomadContainer.ID, nil) require.NoError(t, err) dd := d.Impl().(*Driver) @@ -86,43 +105,51 @@ func TestDanglingContainerRemoval(t *testing.T) { tf := reconciler.trackedContainers() require.Contains(t, tf, handle.containerID) - require.NotContains(t, tf, c.ID) + require.NotContains(t, tf, untrackedNomadContainer) + require.NotContains(t, tf, nonNomadContainer.ID) // assert tracked containers should never be untracked untracked, err := reconciler.untrackedContainers(trackedContainers, time.Now()) require.NoError(t, err) require.NotContains(t, untracked, handle.containerID) - require.NotContains(t, untracked, c.ID) + require.NotContains(t, untracked, nonNomadContainer.ID) + require.Contains(t, untracked, untrackedNomadContainer.ID) // assert we recognize nomad containers with appropriate cutoff untracked, err = reconciler.untrackedContainers(map[string]bool{}, time.Now()) require.NoError(t, err) require.Contains(t, untracked, handle.containerID) - require.NotContains(t, untracked, c.ID) + require.Contains(t, untracked, untrackedNomadContainer.ID) + require.NotContains(t, untracked, nonNomadContainer.ID) // but ignore if creation happened before cutoff untracked, err = reconciler.untrackedContainers(map[string]bool{}, time.Now().Add(-1*time.Minute)) require.NoError(t, err) require.NotContains(t, untracked, handle.containerID) - require.NotContains(t, untracked, c.ID) + require.NotContains(t, untracked, untrackedNomadContainer.ID) + require.NotContains(t, untracked, nonNomadContainer.ID) // a full integration tests to assert that containers are removed prestineDriver := dockerDriverHarness(t, nil).Impl().(*Driver) prestineDriver.config.GC.DanglingContainers = ContainerGCConfig{ Enabled: true, period: 1 * time.Second, - CreationGrace: 1 * time.Second, + CreationGrace: 0 * time.Second, } nReconciler := newReconciler(prestineDriver) require.NoError(t, nReconciler.removeDanglingContainersIteration()) - _, err = client.InspectContainer(c.ID) + _, err = client.InspectContainer(nonNomadContainer.ID) require.NoError(t, err) _, err = client.InspectContainer(handle.containerID) require.Error(t, err) require.Contains(t, err.Error(), NoSuchContainerError) + + _, err = client.InspectContainer(untrackedNomadContainer.ID) + require.Error(t, err) + require.Contains(t, err.Error(), NoSuchContainerError) } // TestDanglingContainerRemoval_Stopped asserts stopped containers without @@ -130,55 +157,46 @@ func TestDanglingContainerRemoval(t *testing.T) { func TestDanglingContainerRemoval_Stopped(t *testing.T) { testutil.DockerCompatible(t) - task, cfg, _ := dockerTask(t) - task.Resources.NomadResources.Networks = nil - require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) - - // Start two containers: one nomad container, and one stopped container - // that acts like a nomad one - client, d, handle, cleanup := dockerSetup(t, task) - defer cleanup() - require.NoError(t, d.WaitUntilStarted(task.ID, 5*time.Second)) + _, cfg, _ := dockerTask(t) - inspected, err := client.InspectContainer(handle.containerID) - require.NoError(t, err) - - stoppedC, err := client.CreateContainer(docker.CreateContainerOptions{ - Name: "mytest-image-" + uuid.Generate(), - Config: inspected.Config, - HostConfig: inspected.HostConfig, + client := newTestDockerClient(t) + container, err := client.CreateContainer(docker.CreateContainerOptions{ + Name: "mytest-image-" + uuid.Generate(), + Config: &docker.Config{ + Image: cfg.Image, + Cmd: append([]string{cfg.Command}, cfg.Args...), + Labels: map[string]string{ + dockerLabelAllocID: uuid.Generate(), + }, + }, }) require.NoError(t, err) defer client.RemoveContainer(docker.RemoveContainerOptions{ - ID: stoppedC.ID, + ID: container.ID, Force: true, }) - err = client.StartContainer(stoppedC.ID, nil) + err = client.StartContainer(container.ID, nil) require.NoError(t, err) - err = client.StopContainer(stoppedC.ID, 60) + err = client.StopContainer(container.ID, 60) require.NoError(t, err) - dd := d.Impl().(*Driver) + dd := dockerDriverHarness(t, nil).Impl().(*Driver) reconciler := newReconciler(dd) - trackedContainers := map[string]bool{handle.containerID: true} // assert nomad container is tracked, and we ignore stopped one tf := reconciler.trackedContainers() - require.Contains(t, tf, handle.containerID) - require.NotContains(t, tf, stoppedC.ID) + require.NotContains(t, tf, container.ID) - untracked, err := reconciler.untrackedContainers(trackedContainers, time.Now()) + untracked, err := reconciler.untrackedContainers(map[string]bool{}, time.Now()) require.NoError(t, err) - require.NotContains(t, untracked, handle.containerID) - require.NotContains(t, untracked, stoppedC.ID) + require.NotContains(t, untracked, container.ID) // if we start container again, it'll be marked as untracked - require.NoError(t, client.StartContainer(stoppedC.ID, nil)) + require.NoError(t, client.StartContainer(container.ID, nil)) - untracked, err = reconciler.untrackedContainers(trackedContainers, time.Now()) + untracked, err = reconciler.untrackedContainers(map[string]bool{}, time.Now()) require.NoError(t, err) - require.NotContains(t, untracked, handle.containerID) - require.Contains(t, untracked, stoppedC.ID) + require.Contains(t, untracked, container.ID) } From 487b0d8349fc509d09b76be937c9a829ca8ac9a6 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Fri, 18 Oct 2019 14:35:15 -0400 Subject: [PATCH 7/9] Only start reconciler once in main driver driver.SetConfig is not appropriate for starting up reconciler goroutine. Some ephemeral driver instances are created for validating config and we ought not to side-effecting goroutines for those. We currently lack a lifecycle hook to inject these, so I picked the `Fingerprinter` function for now, and reconciler should only run after fingerprinter started. Use `sync.Once` to ensure that we only start reconciler loop once. --- drivers/docker/config.go | 3 +-- drivers/docker/driver.go | 2 ++ drivers/docker/fingerprint.go | 4 ++++ drivers/docker/reconciler.go | 7 ++++++- 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/docker/config.go b/drivers/docker/config.go index bec051a351de..89f92df6f389 100644 --- a/drivers/docker/config.go +++ b/drivers/docker/config.go @@ -636,8 +636,7 @@ func (d *Driver) SetConfig(c *base.Config) error { d.coordinator = newDockerCoordinator(coordinatorConfig) - reconciler := newReconciler(d) - reconciler.Start() + d.reconciler = newReconciler(d) return nil } diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 216fa4d99083..974df0823df9 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -115,6 +115,8 @@ type Driver struct { // for use during fingerprinting. detected bool detectedLock sync.RWMutex + + reconciler *containerReconciler } // NewDockerDriver returns a docker implementation of a driver plugin diff --git a/drivers/docker/fingerprint.go b/drivers/docker/fingerprint.go index 694a8ce33199..e6ffabc5c91a 100644 --- a/drivers/docker/fingerprint.go +++ b/drivers/docker/fingerprint.go @@ -13,6 +13,10 @@ import ( ) func (d *Driver) Fingerprint(ctx context.Context) (<-chan *drivers.Fingerprint, error) { + // start reconciler when we start fingerprinting + // this is the only method called when driver is launched properly + d.reconciler.Start() + ch := make(chan *drivers.Fingerprint) go d.handleFingerprint(ctx, ch) return ch, nil diff --git a/drivers/docker/reconciler.go b/drivers/docker/reconciler.go index 9b2454d16b65..e6213be490c4 100644 --- a/drivers/docker/reconciler.go +++ b/drivers/docker/reconciler.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "regexp" + "sync" "time" docker "github.com/fsouza/go-dockerclient" @@ -25,6 +26,8 @@ type containerReconciler struct { isDriverHealthy func() bool trackedContainers func() map[string]bool isNomadContainer func(c docker.APIContainers) bool + + once sync.Once } func newReconciler(d *Driver) *containerReconciler { @@ -46,7 +49,9 @@ func (r *containerReconciler) Start() { return } - go r.removeDanglingContainersGoroutine() + r.once.Do(func() { + go r.removeDanglingContainersGoroutine() + }) } func (r *containerReconciler) removeDanglingContainersGoroutine() { From 04a2e059942dd3818396fe037793ff41ba729732 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Fri, 18 Oct 2019 14:45:45 -0400 Subject: [PATCH 8/9] only set a single label for now Other labels aren't strictly necessary here, and we may follow up with a better way to customize. --- drivers/docker/driver.go | 16 +++++----------- drivers/docker/driver_test.go | 13 +++++++------ 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 974df0823df9..a20c0942fa74 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -67,10 +67,7 @@ var ( ) const ( - dockerLabelTaskID = "com.hashicorp.nomad.task_id" - dockerLabelTaskName = "com.hashicorp.nomad.task_name" - dockerLabelAllocID = "com.hashicorp.nomad.alloc_id" - dockerLabelJobName = "com.hashicorp.nomad.job_name" + dockerLabelAllocID = "com.hashicorp.nomad.alloc_id" ) type Driver struct { @@ -992,15 +989,12 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T config.Labels = driverConfig.Labels } - config.Labels = map[string]string{ - dockerLabelTaskID: task.ID, - dockerLabelTaskName: task.Name, - dockerLabelAllocID: task.AllocID, - dockerLabelJobName: task.JobName, - } + labels := make(map[string]string, len(driverConfig.Labels)+1) for k, v := range driverConfig.Labels { - config.Labels[k] = v + labels[k] = v } + labels[dockerLabelAllocID] = task.AllocID + config.Labels = labels logger.Debug("applied labels on the container", "labels", config.Labels) config.Env = task.EnvList() diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go index c7fafe448909..aa1cbb16ccca 100644 --- a/drivers/docker/driver_test.go +++ b/drivers/docker/driver_test.go @@ -905,8 +905,8 @@ func TestDockerDriver_Labels(t *testing.T) { t.Fatalf("err: %v", err) } - // expect to see 4 additional standard labels - require.Equal(t, len(cfg.Labels)+4, len(container.Config.Labels)) + // expect to see 1 additional standard labels + require.Equal(t, len(cfg.Labels)+1, len(container.Config.Labels)) for k, v := range cfg.Labels { require.Equal(t, v, container.Config.Labels[k]) } @@ -1018,6 +1018,10 @@ func TestDockerDriver_CreateContainerConfig_Labels(t *testing.T) { cfg.Labels = map[string]string{ "user_label": "user_value", + + // com.hashicorp.nomad. labels are reserved and + // cannot be overridden + "com.hashicorp.nomad.alloc_id": "bad_value", } require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) @@ -1032,10 +1036,7 @@ func TestDockerDriver_CreateContainerConfig_Labels(t *testing.T) { // user provided labels "user_label": "user_value", // default labels - "com.hashicorp.nomad.alloc_id": task.AllocID, - "com.hashicorp.nomad.job_name": "redis-demo-job", - "com.hashicorp.nomad.task_id": task.ID, - "com.hashicorp.nomad.task_name": "redis-demo", + "com.hashicorp.nomad.alloc_id": task.AllocID, } require.Equal(t, expectedLabels, c.Config.Labels) From c64647c218bc8701e523c133ea2bbc8238ab670c Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Fri, 18 Oct 2019 15:03:58 -0400 Subject: [PATCH 9/9] add timeouts for docker reconciler docker calls --- drivers/docker/reconciler.go | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/drivers/docker/reconciler.go b/drivers/docker/reconciler.go index e6213be490c4..16750fde60d9 100644 --- a/drivers/docker/reconciler.go +++ b/drivers/docker/reconciler.go @@ -106,10 +106,13 @@ func (r *containerReconciler) removeDanglingContainersIteration() error { } for _, id := range untracked { + ctx, cancel := r.dockerAPIQueryContext() err := client.RemoveContainer(docker.RemoveContainerOptions{ - ID: id, - Force: true, + Context: ctx, + ID: id, + Force: true, }) + cancel() if err != nil { r.logger.Warn("failed to remove untracked container", "container_id", id, "error", err) } else { @@ -125,8 +128,12 @@ func (r *containerReconciler) removeDanglingContainersIteration() error { func (r *containerReconciler) untrackedContainers(tracked map[string]bool, cutoffTime time.Time) ([]string, error) { result := []string{} + ctx, cancel := r.dockerAPIQueryContext() + defer cancel() + cc, err := client.ListContainers(docker.ListContainersOptions{ - All: false, // only reconcile running containers + Context: ctx, + All: false, // only reconcile running containers }) if err != nil { return nil, fmt.Errorf("failed to list containers: %v", err) @@ -153,6 +160,21 @@ func (r *containerReconciler) untrackedContainers(tracked map[string]bool, cutof return result, nil } +// dockerAPIQueryTimeout returns a context for docker API response with an appropriate timeout +// to protect against wedged locked-up API call. +// +// We'll try hitting Docker API on subsequent iteration. +func (r *containerReconciler) dockerAPIQueryContext() (context.Context, context.CancelFunc) { + // use a reasoanble floor to avoid very small limit + timeout := 30 * time.Second + + if timeout < r.config.period { + timeout = r.config.period + } + + return context.WithTimeout(context.Background(), timeout) +} + func isNomadContainer(c docker.APIContainers) bool { if _, ok := c.Labels[dockerLabelAllocID]; ok { return true