From 496df868a389e5f09464a499e912658de2070d47 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 8 Feb 2017 16:15:59 -0800 Subject: [PATCH 1/9] api/task.go: Fix many metalinter warnings --- agent/acs/handler/acs_handler_test.go | 2 +- agent/api/container_test.go | 4 +- agent/api/ecsclient/client.go | 2 +- agent/api/ecsclient/client_test.go | 6 +- agent/api/port_binding.go | 2 +- agent/api/port_binding_test.go | 6 +- agent/api/task.go | 14 +- agent/api/task_test.go | 16 +- agent/api/testutils/container_equal.go | 2 +- agent/api/testutils/container_equal_test.go | 4 +- agent/api/types.go | 170 ++++++++++++++---- agent/engine/docker_container_engine_test.go | 4 +- .../engine/docker_image_manager_integ_test.go | 12 +- agent/engine/docker_task_engine.go | 18 +- agent/engine/dockerauth/ecr.go | 2 +- agent/engine/dockerauth/ecr_test.go | 22 +-- .../dockerstate/docker_task_engine_state.go | 12 +- agent/engine/dockerstate/dockerstate_test.go | 10 +- .../engine/dockerstate/testutils/json_test.go | 2 +- agent/engine/engine_unix_integ_test.go | 6 +- agent/engine/engine_windows_integ_test.go | 2 +- agent/handlers/v1_handlers.go | 2 +- agent/handlers/v1_handlers_test.go | 2 +- agent/stats/container_test.go | 4 +- agent/stats/engine_integ_test.go | 6 +- 25 files changed, 215 insertions(+), 117 deletions(-) diff --git a/agent/acs/handler/acs_handler_test.go b/agent/acs/handler/acs_handler_test.go index 12726677f2b..2ca46863bea 100644 --- a/agent/acs/handler/acs_handler_test.go +++ b/agent/acs/handler/acs_handler_test.go @@ -1103,7 +1103,7 @@ func validateAddedContainer(expectedContainer *api.Container, addedContainer *ap // fields that we are intrested in for comparison containerToCompareFromAdded := &api.Container{ Name: addedContainer.Name, - Cpu: addedContainer.Cpu, + CPU: addedContainer.CPU, Essential: addedContainer.Essential, Memory: addedContainer.Memory, Image: addedContainer.Image, diff --git a/agent/api/container_test.go b/agent/api/container_test.go index 7123e0fbdc9..68387fe6c84 100644 --- a/agent/api/container_test.go +++ b/agent/api/container_test.go @@ -26,7 +26,7 @@ func TestOverridden(t *testing.T) { Name: "name", Image: "image", Command: []string{"foo", "bar"}, - Cpu: 1, + CPU: 1, Memory: 1, Links: []string{}, Ports: []PortBinding{PortBinding{10, 10, "", TransportProtocolTCP}}, @@ -63,7 +63,7 @@ func (pair configPair) Equal() bool { if (conf.Memory / 1024 / 1024) != int64(cont.Memory) { return false } - if conf.CPUShares != int64(cont.Cpu) { + if conf.CPUShares != int64(cont.CPU) { return false } if conf.Image != cont.Image { diff --git a/agent/api/ecsclient/client.go b/agent/api/ecsclient/client.go index 7cb3b78a917..c61570ee36a 100644 --- a/agent/api/ecsclient/client.go +++ b/agent/api/ecsclient/client.go @@ -336,7 +336,7 @@ func (client *APIECSClient) SubmitContainerStateChange(change api.ContainerState for i, binding := range change.PortBindings { hostPort := int64(binding.HostPort) containerPort := int64(binding.ContainerPort) - bindIP := binding.BindIp + bindIP := binding.BindIP protocol := binding.Protocol.String() networkBindings[i] = &ecs.NetworkBinding{ BindIP: &bindIP, diff --git a/agent/api/ecsclient/client_test.go b/agent/api/ecsclient/client_test.go index 725cff112a1..651465fa7c2 100644 --- a/agent/api/ecsclient/client_test.go +++ b/agent/api/ecsclient/client_test.go @@ -116,12 +116,12 @@ func TestSubmitContainerStateChange(t *testing.T) { Status: api.ContainerRunning, PortBindings: []api.PortBinding{ api.PortBinding{ - BindIp: "1.2.3.4", + BindIP: "1.2.3.4", ContainerPort: 1, HostPort: 2, }, api.PortBinding{ - BindIp: "2.2.3.4", + BindIP: "2.2.3.4", ContainerPort: 3, HostPort: 4, Protocol: api.TransportProtocolUDP, @@ -247,7 +247,7 @@ func buildAttributeList(capabilities []string, attributes map[string]string) []* func TestReRegisterContainerInstance(t *testing.T) { additionalAttributes := map[string]string{"my_custom_attribute": "Custom_Value1", - "my_other_custom_attribute": "Custom_Value2", + "my_other_custom_attribute": "Custom_Value2", "attribute_name_with_no_value": "", } diff --git a/agent/api/port_binding.go b/agent/api/port_binding.go index 531f93dc54c..29da63e115e 100644 --- a/agent/api/port_binding.go +++ b/agent/api/port_binding.go @@ -47,7 +47,7 @@ func PortBindingFromDockerPortBinding(dockerPortBindings map[docker.Port][]docke portBindings = append(portBindings, PortBinding{ ContainerPort: uint16(containerPort), HostPort: uint16(hostPort), - BindIp: binding.HostIP, + BindIP: binding.HostIP, Protocol: protocol, }) } diff --git a/agent/api/port_binding_test.go b/agent/api/port_binding_test.go index 9b9657e2fa6..6da97b252b8 100644 --- a/agent/api/port_binding_test.go +++ b/agent/api/port_binding_test.go @@ -31,7 +31,7 @@ func TestPortBindingFromDockerPortBinding(t *testing.T) { }, []PortBinding{ PortBinding{ - BindIp: "1.2.3.4", + BindIP: "1.2.3.4", HostPort: 55, ContainerPort: 53, Protocol: TransportProtocolUDP, @@ -47,13 +47,13 @@ func TestPortBindingFromDockerPortBinding(t *testing.T) { }, []PortBinding{ PortBinding{ - BindIp: "2.3.4.5", + BindIP: "2.3.4.5", HostPort: 8080, ContainerPort: 80, Protocol: TransportProtocolTCP, }, PortBinding{ - BindIp: "5.6.7.8", + BindIP: "5.6.7.8", HostPort: 80, ContainerPort: 80, Protocol: TransportProtocolTCP, diff --git a/agent/api/task.go b/agent/api/task.go index 5504ad1f935..4b727bcb651 100644 --- a/agent/api/task.go +++ b/agent/api/task.go @@ -265,7 +265,7 @@ func (task *Task) dockerConfig(container *Container) (*docker.Config, *DockerCli Volumes: dockerVolumes, Env: dockerEnv, Memory: dockerMem, - CPUShares: task.dockerCpuShares(container.Cpu), + CPUShares: task.dockerCpuShares(container.CPU), } if container.DockerConfig.Config != nil { @@ -545,17 +545,17 @@ func (task *Task) updateKnownStatusTime() { } func (task *Task) SetCredentialsId(id string) { - task.credentialsIdLock.Lock() - defer task.credentialsIdLock.Unlock() + task.credentialsIDLock.Lock() + defer task.credentialsIDLock.Unlock() - task.credentialsId = id + task.credentialsID = id } func (task *Task) GetCredentialsId() string { - task.credentialsIdLock.RLock() - defer task.credentialsIdLock.RUnlock() + task.credentialsIDLock.RLock() + defer task.credentialsIDLock.RUnlock() - return task.credentialsId + return task.credentialsID } func (task *Task) GetDesiredStatus() TaskStatus { diff --git a/agent/api/task_test.go b/agent/api/task_test.go index c9ea54262f2..32c3007a26f 100644 --- a/agent/api/task_test.go +++ b/agent/api/task_test.go @@ -33,7 +33,7 @@ func strptr(s string) *string { return &s } func dockerMap(task *Task) map[string]*DockerContainer { m := make(map[string]*DockerContainer) for _, c := range task.Containers { - m[c.Name] = &DockerContainer{DockerId: "dockerid-" + c.Name, DockerName: "dockername-" + c.Name, Container: c} + m[c.Name] = &DockerContainer{DockerID: "dockerid-" + c.Name, DockerName: "dockername-" + c.Name, Container: c} } return m } @@ -84,7 +84,7 @@ func TestDockerConfigCPUShareZero(t *testing.T) { Containers: []*Container{ &Container{ Name: "c1", - Cpu: 0, + CPU: 0, }, }, } @@ -104,7 +104,7 @@ func TestDockerConfigCPUShareMinimum(t *testing.T) { Containers: []*Container{ &Container{ Name: "c1", - Cpu: 1, + CPU: 1, }, }, } @@ -124,7 +124,7 @@ func TestDockerConfigCPUShareUnchanged(t *testing.T) { Containers: []*Container{ &Container{ Name: "c1", - Cpu: 100, + CPU: 100, }, }, } @@ -257,7 +257,7 @@ func TestDockerHostConfigRawConfigMerging(t *testing.T) { &Container{ Name: "c1", Image: "image", - Cpu: 50, + CPU: 50, Memory: 100, VolumesFrom: []VolumeFrom{VolumeFrom{SourceContainer: "c2"}}, DockerConfig: DockerConfig{ @@ -394,7 +394,7 @@ func TestDockerConfigRawConfigMerging(t *testing.T) { &Container{ Name: "c1", Image: "image", - Cpu: 50, + CPU: 50, Memory: 100, DockerConfig: DockerConfig{ Config: strptr(string(rawConfig)), @@ -456,7 +456,7 @@ func TestGetCredentialsEndpointWhenCredentialsAreSet(t *testing.T) { Name: "c2", Environment: make(map[string]string), }}, - credentialsId: credentialsIDInTask, + credentialsID: credentialsIDInTask, } taskCredentials := &credentials.TaskIAMRoleCredentials{ @@ -652,7 +652,7 @@ func TestTaskFromACS(t *testing.T) { EntryPoint: &[]string{"sh", "-c"}, Essential: true, Environment: map[string]string{"key": "value"}, - Cpu: 10, + CPU: 10, Memory: 100, MountPoints: []MountPoint{ MountPoint{ diff --git a/agent/api/testutils/container_equal.go b/agent/api/testutils/container_equal.go index 9bc5147e17a..aeea7e3344e 100644 --- a/agent/api/testutils/container_equal.go +++ b/agent/api/testutils/container_equal.go @@ -38,7 +38,7 @@ func ContainersEqual(lhs, rhs *api.Container) bool { if !utils.StrSliceEqual(lhs.Command, rhs.Command) { return false } - if lhs.Cpu != rhs.Cpu || lhs.Memory != rhs.Memory { + if lhs.CPU != rhs.CPU || lhs.Memory != rhs.Memory { return false } // Order doesn't matter diff --git a/agent/api/testutils/container_equal_test.go b/agent/api/testutils/container_equal_test.go index c533dd4f2de..dd2cfa690c3 100644 --- a/agent/api/testutils/container_equal_test.go +++ b/agent/api/testutils/container_equal_test.go @@ -30,7 +30,7 @@ func TestContainerEqual(t *testing.T) { Container{Name: "name"}, Container{Name: "name"}, Container{Image: "nginx"}, Container{Image: "nginx"}, Container{Command: []string{"c"}}, Container{Command: []string{"c"}}, - Container{Cpu: 1}, Container{Cpu: 1}, + Container{CPU: 1}, Container{CPU: 1}, Container{Memory: 1}, Container{Memory: 1}, Container{Links: []string{"1", "2"}}, Container{Links: []string{"1", "2"}}, Container{Links: []string{"1", "2"}}, Container{Links: []string{"2", "1"}}, @@ -53,7 +53,7 @@ func TestContainerEqual(t *testing.T) { Container{Image: "nginx"}, Container{Image: "えんじんえっくす"}, Container{Command: []string{"c"}}, Container{Command: []string{"し"}}, Container{Command: []string{"c", "b"}}, Container{Command: []string{"b", "c"}}, - Container{Cpu: 1}, Container{Cpu: 2e2}, + Container{CPU: 1}, Container{CPU: 2e2}, Container{Memory: 1}, Container{Memory: 2e2}, Container{Links: []string{"1", "2"}}, Container{Links: []string{"1", "二"}}, Container{VolumesFrom: []VolumeFrom{VolumeFrom{"1", false}, VolumeFrom{"2", true}}}, Container{VolumesFrom: []VolumeFrom{VolumeFrom{"1", false}, VolumeFrom{"二", false}}}, diff --git a/agent/api/types.go b/agent/api/types.go index b1e468b1ed9..a4a22f4d545 100644 --- a/agent/api/types.go +++ b/agent/api/types.go @@ -21,40 +21,62 @@ import ( "time" ) +// TaskStatus is an enumeration of valid states in the task lifecycle type TaskStatus int32 const ( + // TaskStatusNone is the zero state of a task; this task has been received but no further progress has completed TaskStatusNone TaskStatus = iota + // TaskPulled represents a task which has had all its container images pulled, but not all have yet progressed passed pull TaskPulled + // TaskCreated represents a task which has had all its containers created TaskCreated + // TaskRunning represents a task which has had all its containers started TaskRunning + // TaskStopped represents a task in which all containers are stopped TaskStopped ) +// ContainerStatus is an enumeration of valid states in the container lifecycle type ContainerStatus int32 const ( + // ContainerStatusNone is the zero state of a container; this container has not completed pull ContainerStatusNone ContainerStatus = iota + // ContainerPulled represents a container which has had the image pulled ContainerPulled + // ContainerCreated represents a container that has been created ContainerCreated + // ContainerRunning represents a container that has started ContainerRunning + // ContainerStopped represents a container that has stopped ContainerStopped - ContainerZombie // Impossible status to use as a virtual 'max' + // ContainerZombie is an "impossible" state that is used as the maximum + ContainerZombie ) +// TransportProtocol is an enumeration of valid transport protocols type TransportProtocol int32 const ( + // TransportProtocolTCP represents TCP TransportProtocolTCP TransportProtocol = iota + // TransportProtocolUDP represents UDP TransportProtocolUDP ) +const ( + tcp = "tcp" + udp = "udp" +) + +// NewTransportProtocol returns a TransportProtocol from a string in the task func NewTransportProtocol(protocol string) (TransportProtocol, error) { switch protocol { - case "tcp": + case tcp: return TransportProtocolTCP, nil - case "udp": + case udp: return TransportProtocolUDP, nil default: return TransportProtocolTCP, errors.New(protocol + " is not a recognized transport protocol") @@ -63,54 +85,85 @@ func NewTransportProtocol(protocol string) (TransportProtocol, error) { func (tp *TransportProtocol) String() string { if tp == nil { - return "tcp" + return tcp } switch *tp { case TransportProtocolUDP: - return "udp" + return udp case TransportProtocolTCP: - return "tcp" + return tcp default: log.Crit("Unknown TransportProtocol type!") - return "tcp" + return tcp } } +// PortBinding represents a port binding for a container type PortBinding struct { + // ContainerPort is the port inside the container ContainerPort uint16 - HostPort uint16 - BindIp string - Protocol TransportProtocol + // HostPort is the port exposed on the host + HostPort uint16 + // BindIP is the IP address to which the port is bound + BindIP string `json:"BindIp"` + // Protocol is the protocol of the port + Protocol TransportProtocol } +// TaskOverrides are the overrides applied to a task type TaskOverrides struct{} +// Task is the internal representation of a task in the ECS agent type Task struct { - Arn string - Overrides TaskOverrides `json:"-"` - Family string - Version string + // Arn is the unique identifer for the task + Arn string + // Overrides are the overrides applied to a task + Overrides TaskOverrides `json:"-"` + // Family is the name of the task definition family + Family string + // Version is the version of the task definition + Version string + // Containers are the containers for the task Containers []*Container - Volumes []TaskVolume `json:"volumes"` - + // Volumes are the volumes for the task + Volumes []TaskVolume `json:"volumes"` + + // DesiredStatus represents the state where the task should go. Generally the desired status is informed by the ECS + // backend as a result of either API calls made to ECS or decisions made by the ECS service scheduler. The + // DesiredStatus is almost always either TaskRunning or TaskStopped. Do not access DesiredStatus directly. Instead, + // use `UpdateStatus`, `UpdateDesiredStatus`, `SetDesiredStatus`, and `SetDesiredStatus`. + // TODO DesiredStatus should probably be private with appropriately written setter/getter. When this is done, we need + // to ensure that the UnmarshalJSON is handled properly so that the state storage continues to work. DesiredStatus TaskStatus desiredStatusLock sync.RWMutex - KnownStatus TaskStatus - knownStatusLock sync.RWMutex + // KnownStatus represents the state where the task is. This is generally the minimum of equivalent status types for + // the containers in the task; if one container is at ContainerRunning and another is at ContainerPulled, the task + // KnownStatus would be TaskPulled. Do not access KnownStatus directly. Instead, use `UpdateStatus`, + // `UpdateKnownStatusAndTime`, and `GetKnownStatus`. + // TODO KnownStatus should probably be private with appropriately written setter/getter. When this is done, we need + // to ensure that the UnmarshalJSON is handled properly so that the state storage continues to work. + KnownStatus TaskStatus + knownStatusLock sync.RWMutex + // KnownStatusTime captures the time when the KnownStatus was last updated. Do not access KnownStatusTime directly, + // instead use `GetKnownStatusTime`. KnownStatusTime time.Time `json:"KnownTime"` knownStatusTimeLock sync.RWMutex + // SentStatus represents the last KnownStatus that was sent to the ECS SubmitTaskStateChange API. + // TODO(samuelkarp) SentStatus needs a lock and setters/getters. + // TODO SentStatus should probably be private with appropriately written setter/getter. When this is done, we need + // to ensure that the UnmarshalJSON is handled properly so that the state storage continues to work. SentStatus TaskStatus StartSequenceNumber int64 StopSequenceNumber int64 - // credentialsId is used to set the CredentialsId field for the + // credentialsID is used to set the CredentialsId field for the // IAMRoleCredentials object associated with the task. This id can be // used to look up the credentials for task in the credentials manager - credentialsId string - credentialsIdLock sync.RWMutex + credentialsID string + credentialsIDLock sync.RWMutex } // TaskVolume is a definition of all the volumes available for containers to @@ -145,26 +198,38 @@ func (fs *FSHostVolume) SourcePath() string { return fs.FSSourcePath } +// EmptyHostVolume represents a volume without a specified host path type EmptyHostVolume struct { HostPath string `json:"hostPath"` } +// SourcePath returns the generated host path for the volume func (e *EmptyHostVolume) SourcePath() string { return e.HostPath } +// ContainerStateChange represents a state change that needs to be sent to the +// SubmitContainerStateChange API type ContainerStateChange struct { - TaskArn string + // TaskArn is the unique identifier for the task + TaskArn string + // ContainerName is the name of the container ContainerName string - Status ContainerStatus - - Reason string - ExitCode *int + // Status is the status to send + Status ContainerStatus + + // Reason may contain details of why the container stopped + Reason string + // ExitCode is the exit code of the container, if available + ExitCode *int + // PortBindings are the details of the host ports picked for the specified + // container ports PortBindings []PortBinding // This bit is a little hacky; a pointer to the container's sentstatus which // may be updated to indicate what status was sent. This is used to ensure // the same event is handled only once. + // TODO(samuelkarp) Change this to expose a *Container and use container.UpdateSentStatus SentStatus *ContainerStatus } @@ -185,15 +250,21 @@ func (c *ContainerStateChange) String() string { return res } +// TaskStateChange represents a state change that needs to be sent to the +// SubmitTaskStateChange API type TaskStateChange struct { + // TaskArn is the unique identifier for the task TaskArn string - Status TaskStatus - Reason string + // Status is the status to send + Status TaskStatus + // Reason may contain details of why the task stopped + Reason string // As above, this is the same sort of hacky. // This is a pointer to the task's sent-status that gives the event handler a // hook into storing metadata about the task on the task such that it follows // the lifecycle of the task and so on. + // TODO(samuelkarp) Change this to expose a *Task and use task.UpdateSentStatus SentStatus *TaskStatus } @@ -214,16 +285,22 @@ func (t *Task) String() string { return res + "]" } +// ContainerOverrides are overrides applied to the container type ContainerOverrides struct { Command *[]string `json:"command"` } +// Container is the internal representation of a container in the ECS agent type Container struct { - Name string - Image string - ImageID string + // Name is the name of the container specified in the task definition + Name string + // Image is the image name specified in the task definition + Image string + // ImageID is the local ID of the image used in the container + ImageID string + Command []string - Cpu uint + CPU uint `json:"Cpu"` Memory uint Links []string VolumesFrom []VolumeFrom `json:"volumesFrom"` @@ -236,9 +313,20 @@ type Container struct { DockerConfig DockerConfig `json:"dockerConfig"` RegistryAuthentication *RegistryAuthenticationData `json:"registryAuthentication"` + // DesiredStatus represents the state where the container should go. Generally the desired status is informed by the + // ECS backend as a result of either API calls made to ECS or decisions made by the ECS service scheduler, though the + // agent may also set the DesiredStatus if a different "essential" container in the task exits. The DesiredStatus is + // almost always either ContainerRunning or ContainerStopped. Do not access DesiredStatus directly. Instead, + // use `GetDesiredStatus` and `SetDesiredStatus`. + // TODO DesiredStatus should probably be private with appropriately written setter/getter. When this is done, we need + // to ensure that the UnmarshalJSON is handled properly so that the state storage continues to work. DesiredStatus ContainerStatus `json:"desiredStatus"` desiredStatusLock sync.RWMutex + // KnownStatus represents the state where the container is. Do not access KnownStatus directly. Instead, use + // `GetKnownStatus` and `SetKnownStatus`. + // TODO KnownStatus should probably be private with appropriately written setter/getter. When this is done, we need + // to ensure that the UnmarshalJSON is handled properly so that the state storage continues to work. KnownStatus ContainerStatus knownStatusLock sync.RWMutex @@ -248,11 +336,17 @@ type Container struct { // 'Internal' containers are ones that are not directly specified by task definitions, but created by the agent IsInternal bool + // AppliedStatus is the status that has been "applied" (e.g., we've called Pull, Create, Start, or Stop) but we don't + // yet know that the application was successful. AppliedStatus ContainerStatus // ApplyingError is an error that occured trying to transition the container to its desired state // It is propagated to the backend in the form 'Name: ErrorString' as the 'reason' field. ApplyingError *DefaultNamedError + // SentStatus represents the last KnownStatus that was sent to the ECS SubmitContainerStateChange API. + // TODO(samuelkarp) SentStatus needs a lock and setters/getters. + // TODO SentStatus should probably be private with appropriately written setter/getter. When this is done, we need + // to ensure that the UnmarshalJSON is handled properly so that the state storage continues to work. SentStatus ContainerStatus KnownExitCode *int @@ -277,15 +371,18 @@ type VolumeFrom struct { ReadOnly bool `json:"readOnly"` } +// RegistryAuthenticationData is the authentication data sent by the ECS backend. Currently, the only supported +// authentication data is for ECR. type RegistryAuthenticationData struct { Type string `json:"type"` ECRAuthData *ECRAuthData `json:"ecrAuthData"` } +// ECRAuthData is the authentication details for ECR specifying the region, registryID, and possible endpoint override type ECRAuthData struct { EndpointOverride string `json:"endpointOverride"` Region string `json:"region"` - RegistryId string `json:"registryId"` + RegistryID string `json:"registryId"` } func (c *Container) String() string { @@ -296,6 +393,7 @@ func (c *Container) String() string { return ret } +// Resource is an on-host resource type Resource struct { Name string Type string @@ -303,12 +401,12 @@ type Resource struct { LongValue int64 } -// This is a mapping between containers-as-docker-knows-them and +// DockerContainer is a mapping between containers-as-docker-knows-them and // containers-as-we-know-them. // This is primarily used in DockerState, but lives here such that tasks and // containers know how to convert themselves into Docker's desired config format type DockerContainer struct { - DockerId string + DockerID string `json:"DockerId"` DockerName string // needed for linking Container *Container @@ -318,5 +416,5 @@ func (dc *DockerContainer) String() string { if dc == nil { return "nil" } - return fmt.Sprintf("Id: %s, Name: %s, Container: %s", dc.DockerId, dc.DockerName, dc.Container.String()) + return fmt.Sprintf("Id: %s, Name: %s, Container: %s", dc.DockerID, dc.DockerName, dc.Container.String()) } diff --git a/agent/engine/docker_container_engine_test.go b/agent/engine/docker_container_engine_test.go index 5d9b87c3db5..b1e827dc939 100644 --- a/agent/engine/docker_container_engine_test.go +++ b/agent/engine/docker_container_engine_test.go @@ -257,7 +257,7 @@ func TestPullImageECRSuccess(t *testing.T) { authData := &api.RegistryAuthenticationData{ Type: "ecr", ECRAuthData: &api.ECRAuthData{ - RegistryId: registryID, + RegistryID: registryID, Region: region, EndpointOverride: endpointOverride, }, @@ -313,7 +313,7 @@ func TestPullImageECRAuthFail(t *testing.T) { authData := &api.RegistryAuthenticationData{ Type: "ecr", ECRAuthData: &api.ECRAuthData{ - RegistryId: registryID, + RegistryID: registryID, Region: region, EndpointOverride: endpointOverride, }, diff --git a/agent/engine/docker_image_manager_integ_test.go b/agent/engine/docker_image_manager_integ_test.go index ad194326b18..1726dd199a8 100644 --- a/agent/engine/docker_image_manager_integ_test.go +++ b/agent/engine/docker_image_manager_integ_test.go @@ -555,7 +555,7 @@ func createImageCleanupHappyTestTask(taskName string) *api.Task { Image: test1Image1Name, Essential: false, DesiredStatus: api.ContainerRunning, - Cpu: 10, + CPU: 10, Memory: 10, }, &api.Container{ @@ -563,7 +563,7 @@ func createImageCleanupHappyTestTask(taskName string) *api.Task { Image: test1Image2Name, Essential: false, DesiredStatus: api.ContainerRunning, - Cpu: 10, + CPU: 10, Memory: 10, }, &api.Container{ @@ -571,7 +571,7 @@ func createImageCleanupHappyTestTask(taskName string) *api.Task { Image: test1Image3Name, Essential: false, DesiredStatus: api.ContainerRunning, - Cpu: 10, + CPU: 10, Memory: 10, }, }, @@ -590,7 +590,7 @@ func createImageCleanupThresholdTestTask(taskName string) *api.Task { Image: test2Image1Name, Essential: false, DesiredStatus: api.ContainerRunning, - Cpu: 10, + CPU: 10, Memory: 10, }, &api.Container{ @@ -598,7 +598,7 @@ func createImageCleanupThresholdTestTask(taskName string) *api.Task { Image: test2Image2Name, Essential: false, DesiredStatus: api.ContainerRunning, - Cpu: 10, + CPU: 10, Memory: 10, }, &api.Container{ @@ -606,7 +606,7 @@ func createImageCleanupThresholdTestTask(taskName string) *api.Task { Image: test2Image3Name, Essential: false, DesiredStatus: api.ContainerRunning, - Cpu: 10, + CPU: 10, Memory: 10, }, }, diff --git a/agent/engine/docker_task_engine.go b/agent/engine/docker_task_engine.go index 9a9f3a339b2..382f3bb296a 100644 --- a/agent/engine/docker_task_engine.go +++ b/agent/engine/docker_task_engine.go @@ -226,26 +226,26 @@ func (engine *DockerTaskEngine) synchronizeState() { continue } for _, cont := range conts { - if cont.DockerId == "" { + if cont.DockerID == "" { log.Debug("Found container potentially created while we were down", "name", cont.DockerName) // Figure out the dockerid describedCont, err := engine.client.InspectContainer(cont.DockerName, inspectContainerTimeout) if err != nil { log.Warn("Could not find matching container for expected", "name", cont.DockerName) } else { - cont.DockerId = describedCont.ID + cont.DockerID = describedCont.ID // update mappings that need dockerid engine.state.AddContainer(cont, task) engine.imageManager.RecordContainerReference(cont.Container) } } - if cont.DockerId != "" { - currentState, metadata := engine.client.DescribeContainer(cont.DockerId) + if cont.DockerID != "" { + currentState, metadata := engine.client.DescribeContainer(cont.DockerID) if metadata.Error != nil { currentState = api.ContainerStopped if !cont.Container.KnownTerminal() { cont.Container.ApplyingError = api.NewNamedError(&ContainerVanishedError{}) - log.Warn("Could not describe previously known container; assuming dead", "err", metadata.Error, "id", cont.DockerId, "name", cont.DockerName) + log.Warn("Could not describe previously known container; assuming dead", "err", metadata.Error, "id", cont.DockerID, "name", cont.DockerName) engine.imageManager.RemoveContainerReferenceFromImageState(cont.Container) } } else { @@ -274,7 +274,7 @@ func (engine *DockerTaskEngine) CheckTaskState(task *api.Task) { if !ok { continue } - status, metadata := engine.client.DescribeContainer(dockerContainer.DockerId) + status, metadata := engine.client.DescribeContainer(dockerContainer.DockerID) engine.processTasks.RLock() managedTask, ok := engine.managedTasks[task.Arn] engine.processTasks.RUnlock() @@ -578,7 +578,7 @@ func (engine *DockerTaskEngine) createContainer(task *api.Task, container *api.C metadata := client.CreateContainer(config, hostConfig, containerName, createContainerTimeout) if metadata.DockerID != "" { - engine.state.AddContainer(&api.DockerContainer{DockerId: metadata.DockerID, DockerName: containerName, Container: container}, task) + engine.state.AddContainer(&api.DockerContainer{DockerID: metadata.DockerID, DockerName: containerName, Container: container}, task) } seelog.Infof("Created docker container for task %s: %s -> %s", task, container, metadata.DockerID) return metadata @@ -604,7 +604,7 @@ func (engine *DockerTaskEngine) startContainer(task *api.Task, container *api.Co Error: CannotStartContainerError{fmt.Errorf("Container not recorded as created")}, } } - return client.StartContainer(dockerContainer.DockerId, startContainerTimeout) + return client.StartContainer(dockerContainer.DockerID, startContainerTimeout) } func (engine *DockerTaskEngine) stopContainer(task *api.Task, container *api.Container) DockerContainerMetadata { @@ -623,7 +623,7 @@ func (engine *DockerTaskEngine) stopContainer(task *api.Task, container *api.Con } } - return engine.client.StopContainer(dockerContainer.DockerId, stopContainerTimeout) + return engine.client.StopContainer(dockerContainer.DockerID, stopContainerTimeout) } func (engine *DockerTaskEngine) removeContainer(task *api.Task, container *api.Container) error { diff --git a/agent/engine/dockerauth/ecr.go b/agent/engine/dockerauth/ecr.go index 1a94b51f0db..a144e99798c 100644 --- a/agent/engine/dockerauth/ecr.go +++ b/agent/engine/dockerauth/ecr.go @@ -52,7 +52,7 @@ func (authProvider *ecrAuthProvider) GetAuthconfig(image string) (docker.AuthCon return docker.AuthConfiguration{}, fmt.Errorf("ecrAuthProvider cannot be used without AuthData") } log.Debugf("Calling ECR.GetAuthorizationToken for %s", image) - authData, err := authProvider.client.GetAuthorizationToken(authProvider.authData.RegistryId) + authData, err := authProvider.client.GetAuthorizationToken(authProvider.authData.RegistryID) if err != nil { return docker.AuthConfiguration{}, err } diff --git a/agent/engine/dockerauth/ecr_test.go b/agent/engine/dockerauth/ecr_test.go index 233ba7fc906..f47c2e7430f 100644 --- a/agent/engine/dockerauth/ecr_test.go +++ b/agent/engine/dockerauth/ecr_test.go @@ -46,7 +46,7 @@ func TestNewAuthProviderECRAuth(t *testing.T) { authData := &api.ECRAuthData{ Region: "us-west-2", - RegistryId: "0123456789012", + RegistryID: "0123456789012", EndpointOverride: "my.endpoint", } @@ -66,7 +66,7 @@ func TestGetAuthConfigSuccess(t *testing.T) { authData := &api.ECRAuthData{ Region: "us-west-2", - RegistryId: "0123456789012", + RegistryID: "0123456789012", EndpointOverride: "my.endpoint", } proxyEndpoint := "proxy" @@ -78,7 +78,7 @@ func TestGetAuthConfigSuccess(t *testing.T) { authData: authData, } - client.EXPECT().GetAuthorizationToken(authData.RegistryId).Return(&ecrapi.AuthorizationData{ + client.EXPECT().GetAuthorizationToken(authData.RegistryID).Return(&ecrapi.AuthorizationData{ ProxyEndpoint: aws.String(proxyEndpointScheme + proxyEndpoint), AuthorizationToken: aws.String(base64.StdEncoding.EncodeToString([]byte(username + ":" + password))), }, nil) @@ -105,7 +105,7 @@ func TestGetAuthConfigNoMatchAuthorizationToken(t *testing.T) { authData := &api.ECRAuthData{ Region: "us-west-2", - RegistryId: "0123456789012", + RegistryID: "0123456789012", EndpointOverride: "my.endpoint", } proxyEndpoint := "proxy" @@ -117,7 +117,7 @@ func TestGetAuthConfigNoMatchAuthorizationToken(t *testing.T) { authData: authData, } - client.EXPECT().GetAuthorizationToken(authData.RegistryId).Return(&ecrapi.AuthorizationData{ + client.EXPECT().GetAuthorizationToken(authData.RegistryID).Return(&ecrapi.AuthorizationData{ ProxyEndpoint: aws.String(proxyEndpointScheme + "notproxy"), AuthorizationToken: aws.String(base64.StdEncoding.EncodeToString([]byte(username + ":" + password))), }, nil) @@ -139,7 +139,7 @@ func TestGetAuthConfigBadBase64(t *testing.T) { authData := &api.ECRAuthData{ Region: "us-west-2", - RegistryId: "0123456789012", + RegistryID: "0123456789012", EndpointOverride: "my.endpoint", } proxyEndpoint := "proxy" @@ -151,7 +151,7 @@ func TestGetAuthConfigBadBase64(t *testing.T) { authData: authData, } - client.EXPECT().GetAuthorizationToken(authData.RegistryId).Return(&ecrapi.AuthorizationData{ + client.EXPECT().GetAuthorizationToken(authData.RegistryID).Return(&ecrapi.AuthorizationData{ ProxyEndpoint: aws.String(proxyEndpointScheme + "notproxy"), AuthorizationToken: aws.String((username + ":" + password)), }, nil) @@ -173,7 +173,7 @@ func TestGetAuthConfigMissingResponse(t *testing.T) { authData := &api.ECRAuthData{ Region: "us-west-2", - RegistryId: "0123456789012", + RegistryID: "0123456789012", EndpointOverride: "my.endpoint", } proxyEndpoint := "proxy" @@ -183,7 +183,7 @@ func TestGetAuthConfigMissingResponse(t *testing.T) { authData: authData, } - client.EXPECT().GetAuthorizationToken(authData.RegistryId) + client.EXPECT().GetAuthorizationToken(authData.RegistryID) authconfig, err := provider.GetAuthconfig(proxyEndpoint + "/myimage") if err == nil { @@ -202,7 +202,7 @@ func TestGetAuthConfigECRError(t *testing.T) { authData := &api.ECRAuthData{ Region: "us-west-2", - RegistryId: "0123456789012", + RegistryID: "0123456789012", EndpointOverride: "my.endpoint", } proxyEndpoint := "proxy" @@ -212,7 +212,7 @@ func TestGetAuthConfigECRError(t *testing.T) { authData: authData, } - client.EXPECT().GetAuthorizationToken(authData.RegistryId).Return(nil, errors.New("test error")) + client.EXPECT().GetAuthorizationToken(authData.RegistryID).Return(nil, errors.New("test error")) authconfig, err := provider.GetAuthconfig(proxyEndpoint + "/myimage") if err == nil { diff --git a/agent/engine/dockerstate/docker_task_engine_state.go b/agent/engine/dockerstate/docker_task_engine_state.go index a1b11850035..f5cfe583fc6 100644 --- a/agent/engine/dockerstate/docker_task_engine_state.go +++ b/agent/engine/dockerstate/docker_task_engine_state.go @@ -125,8 +125,8 @@ func (state *DockerTaskEngineState) RemoveTask(task *api.Task) { delete(state.taskToId, task.Arn) for _, dockerContainer := range containerMap { - delete(state.idToTask, dockerContainer.DockerId) - delete(state.idToContainer, dockerContainer.DockerId) + delete(state.idToTask, dockerContainer.DockerID) + delete(state.idToContainer, dockerContainer.DockerID) } } @@ -163,8 +163,8 @@ func (state *DockerTaskEngineState) AddContainer(container *api.DockerContainer, state.tasks[task.Arn] = task } - if container.DockerId != "" { - state.idToTask[container.DockerId] = task.Arn + if container.DockerID != "" { + state.idToTask[container.DockerID] = task.Arn } existingMap, exists := state.taskToId[task.Arn] if !exists { @@ -173,8 +173,8 @@ func (state *DockerTaskEngineState) AddContainer(container *api.DockerContainer, } existingMap[container.Container.Name] = container - if container.DockerId != "" { - state.idToContainer[container.DockerId] = container + if container.DockerID != "" { + state.idToContainer[container.DockerID] = container } } diff --git a/agent/engine/dockerstate/dockerstate_test.go b/agent/engine/dockerstate/dockerstate_test.go index efcacddd48c..d1c719cd2d2 100644 --- a/agent/engine/dockerstate/dockerstate_test.go +++ b/agent/engine/dockerstate/dockerstate_test.go @@ -97,11 +97,11 @@ func TestTwophaseAddContainer(t *testing.T) { if container.DockerName != "dockerName" { t.Fatal("Incorrect docker name") } - if container.DockerId != "" { + if container.DockerID != "" { t.Fatal("DockerID Should be blank") } - state.AddContainer(&api.DockerContainer{DockerName: "dockerName", Container: testTask.Containers[0], DockerId: "did"}, testTask) + state.AddContainer(&api.DockerContainer{DockerName: "dockerName", Container: testTask.Containers[0], DockerID: "did"}, testTask) containerMap, ok = state.ContainerMapByArn("test") if !ok { @@ -115,7 +115,7 @@ func TestTwophaseAddContainer(t *testing.T) { if container.DockerName != "dockerName" { t.Fatal("Incorrect docker name") } - if container.DockerId != "did" { + if container.DockerID != "did" { t.Fatal("DockerID should have been updated") } @@ -123,7 +123,7 @@ func TestTwophaseAddContainer(t *testing.T) { if !ok { t.Fatal("Could not get container by id") } - if container.DockerName != "dockerName" || container.DockerId != "did" { + if container.DockerName != "dockerName" || container.DockerID != "did" { t.Fatal("Incorrect container fetched") } } @@ -134,7 +134,7 @@ func TestRemoveTask(t *testing.T) { Name: "c1", } testDockerContainer := &api.DockerContainer{ - DockerId: "did", + DockerID: "did", Container: testContainer, } testTask := &api.Task{ diff --git a/agent/engine/dockerstate/testutils/json_test.go b/agent/engine/dockerstate/testutils/json_test.go index 66e8fb72bfa..40d3dad9329 100644 --- a/agent/engine/dockerstate/testutils/json_test.go +++ b/agent/engine/dockerstate/testutils/json_test.go @@ -73,7 +73,7 @@ func TestJsonEncoding(t *testing.T) { testTask := createTestTask("test1", 1) testState.AddTask(testTask) for i, cont := range testTask.Containers { - testState.AddContainer(&api.DockerContainer{DockerId: "docker" + strconv.Itoa(i), DockerName: "someName", Container: cont}, testTask) + testState.AddContainer(&api.DockerContainer{DockerID: "docker" + strconv.Itoa(i), DockerName: "someName", Container: cont}, testTask) } other := decodeEqual(t, testState) _, ok := other.ContainerMapByArn("test1") diff --git a/agent/engine/engine_unix_integ_test.go b/agent/engine/engine_unix_integ_test.go index dbfa08e4e2f..636bdf55558 100644 --- a/agent/engine/engine_unix_integ_test.go +++ b/agent/engine/engine_unix_integ_test.go @@ -62,7 +62,7 @@ func createTestContainer() *api.Container { Command: []string{}, Essential: true, DesiredStatus: api.ContainerRunning, - Cpu: 100, + CPU: 100, Memory: 80, } } @@ -188,7 +188,7 @@ func TestPortForward(t *testing.T) { // Kill the existing container now to make the test run more quickly. containerMap, _ := taskEngine.(*DockerTaskEngine).state.ContainerMapByArn(testTask.Arn) - cid := containerMap[testTask.Containers[0].Name].DockerId + cid := containerMap[testTask.Containers[0].Name].DockerID client, _ := docker.NewClient(endpoint) err = client.KillContainer(docker.KillContainerOptions{ID: cid}) if err != nil { @@ -816,7 +816,7 @@ func TestSignalEvent(t *testing.T) { // Signal the container now containerMap, _ := taskEngine.(*DockerTaskEngine).state.ContainerMapByArn(testTask.Arn) - cid := containerMap[testTask.Containers[0].Name].DockerId + cid := containerMap[testTask.Containers[0].Name].DockerID client, _ := docker.NewClient(endpoint) err := client.KillContainer(docker.KillContainerOptions{ID: cid, Signal: docker.Signal(int(syscall.SIGUSR1))}) if err != nil { diff --git a/agent/engine/engine_windows_integ_test.go b/agent/engine/engine_windows_integ_test.go index 5fd3f9e79bc..193565790d9 100644 --- a/agent/engine/engine_windows_integ_test.go +++ b/agent/engine/engine_windows_integ_test.go @@ -31,7 +31,7 @@ func createTestContainer() *api.Container { Image: "microsoft/windowsservercore:latest", Essential: true, DesiredStatus: api.ContainerRunning, - Cpu: 100, + CPU: 100, Memory: 80, } } diff --git a/agent/handlers/v1_handlers.go b/agent/handlers/v1_handlers.go index 8f328ae9ef7..a74763054b7 100644 --- a/agent/handlers/v1_handlers.go +++ b/agent/handlers/v1_handlers.go @@ -68,7 +68,7 @@ func newTaskResponse(task *api.Task, containerMap map[string]*api.DockerContaine if container.Container.IsInternal { continue } - containers = append(containers, ContainerResponse{container.DockerId, container.DockerName, containerName}) + containers = append(containers, ContainerResponse{container.DockerID, container.DockerName, containerName}) } knownStatus := task.GetKnownStatus() diff --git a/agent/handlers/v1_handlers_test.go b/agent/handlers/v1_handlers_test.go index b051fe8277a..d13962e32ad 100644 --- a/agent/handlers/v1_handlers_test.go +++ b/agent/handlers/v1_handlers_test.go @@ -272,7 +272,7 @@ func stateSetupHelper(state *dockerstate.DockerTaskEngineState, tasks []*api.Tas for _, container := range task.Containers { state.AddContainer(&api.DockerContainer{ Container: container, - DockerId: "dockerid-" + task.Arn + "-" + container.Name, + DockerID: "dockerid-" + task.Arn + "-" + container.Name, DockerName: "dockername-" + task.Arn + "-" + container.Name, }, task) } diff --git a/agent/stats/container_test.go b/agent/stats/container_test.go index 5f672d8e650..28271537547 100644 --- a/agent/stats/container_test.go +++ b/agent/stats/container_test.go @@ -137,7 +137,7 @@ func TestContainerStatsCollectionReconnection(t *testing.T) { close(closedChan) mockContainer := &api.DockerContainer{ - DockerId: dockerID, + DockerID: dockerID, Container: &api.Container{ KnownStatus: api.ContainerRunning, }, @@ -178,7 +178,7 @@ func TestContainerStatsCollectionStopsIfContainerIsTerminal(t *testing.T) { statsErr := fmt.Errorf("test error") mockContainer := &api.DockerContainer{ - DockerId: dockerID, + DockerID: dockerID, Container: &api.Container{ KnownStatus: api.ContainerStopped, }, diff --git a/agent/stats/engine_integ_test.go b/agent/stats/engine_integ_test.go index 8241d88742b..e891d8e10df 100644 --- a/agent/stats/engine_integ_test.go +++ b/agent/stats/engine_integ_test.go @@ -33,7 +33,7 @@ func (resolver *IntegContainerMetadataResolver) addToMap(containerID string) { Version: taskDefinitionVersion, } resolver.containerIDToDockerContainer[containerID] = &api.DockerContainer{ - DockerId: containerID, + DockerID: containerID, Container: &api.Container{}, } } @@ -281,7 +281,7 @@ func TestStatsEngineWithDockerTaskEngine(t *testing.T) { dockerTaskEngine.State().AddTask(&testTask) dockerTaskEngine.State().AddContainer( &api.DockerContainer{ - DockerId: container.ID, + DockerID: container.ID, DockerName: "gremlin", Container: containers[0], }, @@ -405,7 +405,7 @@ func TestStatsEngineWithDockerTaskEngineMissingRemoveEvent(t *testing.T) { dockerTaskEngine.State().AddTask(&testTask) dockerTaskEngine.State().AddContainer( &api.DockerContainer{ - DockerId: container.ID, + DockerID: container.ID, DockerName: "gremlin", Container: containers[0], }, From 0bd64f32c8c7e9d4aeaf84ed91daaea302e20509 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 8 Feb 2017 16:58:44 -0800 Subject: [PATCH 2/9] api: Handle SentStatus more safely --- agent/api/container.go | 16 ++++++++++++ agent/api/task.go | 16 ++++++++++++ agent/api/types.go | 32 +++++++++++------------- agent/engine/docker_task_engine.go | 10 ++++---- agent/eventhandler/handler_test.go | 15 +++++------ agent/eventhandler/task_handler.go | 8 +++--- agent/eventhandler/task_handler_types.go | 4 +-- 7 files changed, 63 insertions(+), 38 deletions(-) diff --git a/agent/api/container.go b/agent/api/container.go index 796fce7ed5f..6907cdcc149 100644 --- a/agent/api/container.go +++ b/agent/api/container.go @@ -62,3 +62,19 @@ func (c *Container) SetDesiredStatus(status ContainerStatus) { c.DesiredStatus = status } + +// GetSentStatus safely returns the SentStatus of the container +func (c *Container) GetSentStatus() ContainerStatus { + c.sentStatusLock.RLock() + defer c.sentStatusLock.RUnlock() + + return c.SentStatus +} + +// SetSentStatus safely sets the SentStatus of the container +func (c *Container) SetSentStatus(status ContainerStatus) { + c.sentStatusLock.Lock() + defer c.sentStatusLock.Unlock() + + c.SentStatus = status +} diff --git a/agent/api/task.go b/agent/api/task.go index 4b727bcb651..b5d93bd0091 100644 --- a/agent/api/task.go +++ b/agent/api/task.go @@ -571,3 +571,19 @@ func (task *Task) SetDesiredStatus(status TaskStatus) { task.DesiredStatus = status } + +// GetSentStatus safely returns the SentStatus of the task +func (task *Task) GetSentStatus() TaskStatus { + task.sentStatusLock.RLock() + defer task.sentStatusLock.RUnlock() + + return task.SentStatus +} + +// SetSentStatus safely sets the SentStatus of the task +func (task *Task) SetSentStatus(status TaskStatus) { + task.sentStatusLock.Lock() + defer task.sentStatusLock.Unlock() + + task.SentStatus = status +} diff --git a/agent/api/types.go b/agent/api/types.go index a4a22f4d545..0a29d8f2924 100644 --- a/agent/api/types.go +++ b/agent/api/types.go @@ -154,7 +154,8 @@ type Task struct { // TODO(samuelkarp) SentStatus needs a lock and setters/getters. // TODO SentStatus should probably be private with appropriately written setter/getter. When this is done, we need // to ensure that the UnmarshalJSON is handled properly so that the state storage continues to work. - SentStatus TaskStatus + SentStatus TaskStatus + sentStatusLock sync.RWMutex StartSequenceNumber int64 StopSequenceNumber int64 @@ -226,11 +227,9 @@ type ContainerStateChange struct { // container ports PortBindings []PortBinding - // This bit is a little hacky; a pointer to the container's sentstatus which - // may be updated to indicate what status was sent. This is used to ensure - // the same event is handled only once. - // TODO(samuelkarp) Change this to expose a *Container and use container.UpdateSentStatus - SentStatus *ContainerStatus + // Container is a pointer to the container involved in the state change that gives the event handler a hook into + // storing what status was sent. This is used to ensure the same event is handled only once. + Container *Container } func (c *ContainerStateChange) String() string { @@ -244,8 +243,8 @@ func (c *ContainerStateChange) String() string { if len(c.PortBindings) != 0 { res += fmt.Sprintf(", Ports %v", c.PortBindings) } - if c.SentStatus != nil { - res += ", Known Sent: " + c.SentStatus.String() + if c.Container != nil { + res += ", Known Sent: " + c.Container.GetSentStatus().String() } return res } @@ -260,18 +259,15 @@ type TaskStateChange struct { // Reason may contain details of why the task stopped Reason string - // As above, this is the same sort of hacky. - // This is a pointer to the task's sent-status that gives the event handler a - // hook into storing metadata about the task on the task such that it follows - // the lifecycle of the task and so on. - // TODO(samuelkarp) Change this to expose a *Task and use task.UpdateSentStatus - SentStatus *TaskStatus + // Task is a pointer to the task involved in the state change that gives the event handler a hook into storing + // what status was sent. This is used to ensure the same event is handled only once. + Task *Task } func (t *TaskStateChange) String() string { res := fmt.Sprintf("%s -> %s", t.TaskArn, t.Status.String()) - if t.SentStatus != nil { - res += ", Known Sent: " + t.SentStatus.String() + if t.Task != nil { + res += ", Known Sent: " + t.Task.GetSentStatus().String() } return res } @@ -344,10 +340,10 @@ type Container struct { ApplyingError *DefaultNamedError // SentStatus represents the last KnownStatus that was sent to the ECS SubmitContainerStateChange API. - // TODO(samuelkarp) SentStatus needs a lock and setters/getters. // TODO SentStatus should probably be private with appropriately written setter/getter. When this is done, we need // to ensure that the UnmarshalJSON is handled properly so that the state storage continues to work. - SentStatus ContainerStatus + SentStatus ContainerStatus + sentStatusLock sync.RWMutex KnownExitCode *int KnownPortBindings []PortBinding diff --git a/agent/engine/docker_task_engine.go b/agent/engine/docker_task_engine.go index 382f3bb296a..d1f8254f6ef 100644 --- a/agent/engine/docker_task_engine.go +++ b/agent/engine/docker_task_engine.go @@ -316,10 +316,10 @@ func (engine *DockerTaskEngine) emitTaskEvent(task *api.Task, reason string) { return } event := api.TaskStateChange{ - TaskArn: task.Arn, - Status: taskKnownStatus, - Reason: reason, - SentStatus: &task.SentStatus, + TaskArn: task.Arn, + Status: taskKnownStatus, + Reason: reason, + Task: task, } log.Info("Task change event", "event", event) engine.taskEvents <- event @@ -374,7 +374,7 @@ func (engine *DockerTaskEngine) emitContainerEvent(task *api.Task, cont *api.Con ExitCode: cont.KnownExitCode, PortBindings: cont.KnownPortBindings, Reason: reason, - SentStatus: &cont.SentStatus, + Container: cont, } log.Debug("Container change event", "event", event) engine.containerEvents <- event diff --git a/agent/eventhandler/handler_test.go b/agent/eventhandler/handler_test.go index debe8c9d685..3a27e0cb625 100644 --- a/agent/eventhandler/handler_test.go +++ b/agent/eventhandler/handler_test.go @@ -28,10 +28,10 @@ import ( ) func contEvent(arn string) api.ContainerStateChange { - return api.ContainerStateChange{TaskArn: arn, ContainerName: "containerName", Status: api.ContainerRunning} + return api.ContainerStateChange{TaskArn: arn, ContainerName: "containerName", Status: api.ContainerRunning, Container: &api.Container{}} } func taskEvent(arn string) api.TaskStateChange { - return api.TaskStateChange{TaskArn: arn, Status: api.TaskRunning} + return api.TaskStateChange{TaskArn: arn, Status: api.TaskRunning, Task: &api.Task{}} } func TestSendsEventsOneContainer(t *testing.T) { @@ -175,20 +175,17 @@ func TestSendsEventsDedupe(t *testing.T) { // Verify that a task doesn't get sent if we already have 'sent' it task1 := taskEvent("alreadySent") - taskRunning := api.TaskRunning - task1.SentStatus = &taskRunning + task1.Task.SetSentStatus(api.TaskRunning) cont1 := contEvent("alreadySent") - containerRunning := api.ContainerRunning - cont1.SentStatus = &containerRunning + cont1.Container.SetSentStatus(api.ContainerRunning) AddContainerEvent(cont1, client) AddTaskEvent(task1, client) task2 := taskEvent("containerSent") - taskNone := api.TaskStatusNone - task2.SentStatus = &taskNone + task2.Task.SetSentStatus(api.TaskStatusNone) cont2 := contEvent("containerSent") - cont2.SentStatus = &containerRunning + cont2.Container.SetSentStatus(api.ContainerRunning) // Expect to send a task status but not a container status called := make(chan struct{}) diff --git a/agent/eventhandler/task_handler.go b/agent/eventhandler/task_handler.go index 28b0da451c7..9ef3fac05bb 100644 --- a/agent/eventhandler/task_handler.go +++ b/agent/eventhandler/task_handler.go @@ -114,8 +114,8 @@ func SubmitTaskEvents(events *eventList, client api.ECSClient) { if err == nil { // submitted; ensure we don't retry it event.containerSent = true - if event.containerChange.SentStatus != nil { - *event.containerChange.SentStatus = event.containerChange.Status + if event.containerChange.Container != nil { + event.containerChange.Container.SetSentStatus(event.containerChange.Status) } statesaver.Save() llog.Debug("Submitted container state change") @@ -130,8 +130,8 @@ func SubmitTaskEvents(events *eventList, client api.ECSClient) { if err == nil { // submitted or can't be retried; ensure we don't retry it event.taskSent = true - if event.taskChange.SentStatus != nil { - *event.taskChange.SentStatus = event.taskChange.Status + if event.taskChange.Task != nil { + event.taskChange.Task.SetSentStatus(event.taskChange.Status) } statesaver.Save() llog.Debug("Submitted task state change") diff --git a/agent/eventhandler/task_handler_types.go b/agent/eventhandler/task_handler_types.go index 2481738575e..f6119c7a688 100644 --- a/agent/eventhandler/task_handler_types.go +++ b/agent/eventhandler/task_handler_types.go @@ -76,7 +76,7 @@ func (event *sendableEvent) taskShouldBeSent() bool { if tevent.Status == api.TaskStatusNone { return false // defensive programming :) } - if event.taskSent || (tevent.SentStatus != nil && *tevent.SentStatus >= tevent.Status) { + if event.taskSent || (tevent.Task != nil && tevent.Task.GetSentStatus() >= tevent.Status) { return false // redundant event } return true @@ -87,7 +87,7 @@ func (event *sendableEvent) containerShouldBeSent() bool { return false } cevent := event.containerChange - if event.containerSent || (cevent.SentStatus != nil && *cevent.SentStatus >= cevent.Status) { + if event.containerSent || (cevent.Container != nil && cevent.Container.GetSentStatus() >= cevent.Status) { return false } return true From 57b21c7ada633c61bf1b11e2918a0e5177940ce9 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Thu, 9 Feb 2017 15:26:14 -0800 Subject: [PATCH 3/9] engine: Report task status prior to removal Prior to this change, a race condition exists between reporting task status and task cleanup in the agent. If reporting task status took an excessively long time and ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION is set very short, the containers and associated task metadata could be removed before the ECS backend is informed that the task has stopped. When the task is especially short-lived, it's also possible that the cleanup could occur before the ECS backend is even informed that the task is running. In this situation, it's possible for the ECS backend to re-transmit the task to the agent (assuming that it hadn't started yet) and the de-duplication logic in the agent can break. With this change, we ensure that the task status is reported prior to cleanup actually starting. --- .../engine/docker_image_manager_integ_test.go | 8 +++++++ agent/engine/docker_task_engine_test.go | 2 ++ agent/engine/engine_integ_test.go | 8 ++++++- agent/engine/task_manager.go | 24 +++++++++++++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/agent/engine/docker_image_manager_integ_test.go b/agent/engine/docker_image_manager_integ_test.go index 1726dd199a8..30bbb936f23 100644 --- a/agent/engine/docker_image_manager_integ_test.go +++ b/agent/engine/docker_image_manager_integ_test.go @@ -112,6 +112,7 @@ func TestIntegImageCleanupHappyCase(t *testing.T) { // Verify Task is stopped. verifyTaskIsStopped(taskEvents, testTask) + testTask.SetSentStatus(api.TaskStopped) // Allow Task cleanup to occur time.Sleep(5 * time.Second) @@ -230,6 +231,7 @@ func TestIntegImageCleanupThreshold(t *testing.T) { // Verify Task is stopped verifyTaskIsStopped(taskEvents, testTask) + testTask.SetSentStatus(api.TaskStopped) // Allow Task cleanup to occur time.Sleep(2 * time.Second) @@ -378,6 +380,9 @@ func TestImageWithSameNameAndDifferentID(t *testing.T) { // Verify Task is stopped verifyTaskIsStopped(taskEvents, task1, task2, task3) + task1.SetSentStatus(api.TaskStopped) + task2.SetSentStatus(api.TaskStopped) + task3.SetSentStatus(api.TaskStopped) // Allow Task cleanup to occur time.Sleep(2 * time.Second) @@ -501,6 +506,9 @@ func TestImageWithSameIDAndDifferentNames(t *testing.T) { // Verify Task is stopped verifyTaskIsStopped(taskEvents, task1, task2, task3) + task1.SetSentStatus(api.TaskStopped) + task2.SetSentStatus(api.TaskStopped) + task3.SetSentStatus(api.TaskStopped) // Allow Task cleanup to occur time.Sleep(2 * time.Second) diff --git a/agent/engine/docker_task_engine_test.go b/agent/engine/docker_task_engine_test.go index 2f9693adba4..3ce775a0165 100644 --- a/agent/engine/docker_task_engine_test.go +++ b/agent/engine/docker_task_engine_test.go @@ -187,6 +187,7 @@ func TestBatchContainerHappyPath(t *testing.T) { assert.Equal(t, *cont.ExitCode, 0, "Exit code should be present") } assert.Equal(t, (<-taskEvents).Status, api.TaskStopped, "Task is not in STOPPED state") + sleepTask.SetSentStatus(api.TaskStopped) // Extra events should not block forever; duplicate acs and docker events are possible go func() { eventStream <- createDockerEvent(api.ContainerStopped) }() @@ -317,6 +318,7 @@ func TestRemoveEvents(t *testing.T) { }).Return(nil) taskEngine.AddTask(sleepTaskStop) + sleepTask.SetSentStatus(api.TaskStopped) imageManager.EXPECT().RemoveContainerReferenceFromImageState(gomock.Any()) // trigger cleanup cleanup <- time.Now() diff --git a/agent/engine/engine_integ_test.go b/agent/engine/engine_integ_test.go index e81158d66b8..1e73d4582cf 100644 --- a/agent/engine/engine_integ_test.go +++ b/agent/engine/engine_integ_test.go @@ -41,6 +41,11 @@ const ( credentialsIDIntegTest = "credsid" ) +func init() { + // Set this very low for integ tests only + _stoppedSentWaitInterval = 1 * time.Second +} + func createTestTask(arn string) *api.Task { return &api.Task{ Arn: arn, @@ -183,8 +188,9 @@ func TestSweepContainer(t *testing.T) { defer discardEvents(taskEvents)() // Should be stopped, let's verify it's still listed... - _, ok := taskEngine.(*DockerTaskEngine).State().TaskByArn("testSweepContainer") + task, ok := taskEngine.(*DockerTaskEngine).State().TaskByArn("testSweepContainer") assert.True(t, ok, "Expected task to be present still, but wasn't") + task.SetSentStatus(api.TaskStopped) // cleanupTask waits for TaskStopped to be sent before cleaning time.Sleep(1 * time.Minute) for i := 0; i < 60; i++ { _, ok = taskEngine.(*DockerTaskEngine).State().TaskByArn("testSweepContainer") diff --git a/agent/engine/task_manager.go b/agent/engine/task_manager.go index c829f0c42d6..c9af9050941 100644 --- a/agent/engine/task_manager.go +++ b/agent/engine/task_manager.go @@ -26,6 +26,8 @@ import ( const ( steadyStateTaskVerifyInterval = 10 * time.Minute + stoppedSentWaitInterval = 30 * time.Second + maxStoppedWaitTimes = 72 * time.Hour / stoppedSentWaitInterval ) type acsTaskUpdate struct { @@ -474,6 +476,9 @@ func (mtask *managedTask) time() ttime.Time { return mtask._time } +var _stoppedSentWaitInterval = stoppedSentWaitInterval +var _maxStoppedWaitTimes = int(maxStoppedWaitTimes) + func (mtask *managedTask) cleanupTask(taskStoppedDuration time.Duration) { cleanupTimeDuration := mtask.GetKnownStatusTime().Add(taskStoppedDuration).Sub(ttime.Now()) // There is a potential deadlock here if cleanupTime is negative. Ignore the computed @@ -489,8 +494,27 @@ func (mtask *managedTask) cleanupTask(taskStoppedDuration time.Duration) { cleanupTimeBool <- true close(cleanupTimeBool) }() + // wait for the cleanup time to elapse, signalled by cleanupTimeBool for !mtask.waitEvent(cleanupTimeBool) { } + stoppedSentBool := make(chan bool) + go func() { + for i := 0; i < _maxStoppedWaitTimes; i++ { + // ensure that we block until api.TaskStopped is actually sent + sentStatus := mtask.GetSentStatus() + if sentStatus >= api.TaskStopped { + stoppedSentBool <- true + close(stoppedSentBool) + return + } + seelog.Warnf("Blocking cleanup for task %v until the task has been reported stopped. SentStatus: %v (%d/%d)", mtask, sentStatus, i, _maxStoppedWaitTimes) + mtask._time.Sleep(_stoppedSentWaitInterval) + } + }() + // wait for api.TaskStopped to be sent + for !mtask.waitEvent(stoppedSentBool) { + } + log.Info("Cleaning up task's containers and data", "task", mtask.Task) // For the duration of this, simply discard any task events; this ensures the From eb987248a62396375e2b7b3b80114c75b5212274 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Thu, 9 Feb 2017 16:56:30 -0800 Subject: [PATCH 4/9] dockerstate: Create TaskEngineState interface --- agent/agent.go | 2 +- agent/engine/default.go | 2 +- agent/engine/docker_image_manager.go | 4 +- agent/engine/docker_image_manager_test.go | 60 ++--- agent/engine/docker_task_engine.go | 10 +- agent/engine/docker_task_engine_test.go | 2 +- .../dockerstate/docker_task_engine_state.go | 214 +++++++++++------- agent/engine/dockerstate/dockerstate_test.go | 26 +-- agent/engine/dockerstate/generate_mocks.go | 16 ++ agent/engine/dockerstate/json.go | 6 +- .../dockerstate/mocks/dockerstate_mocks.go | 169 ++++++++++++++ .../testutils/docker_state_equal.go | 2 +- .../engine/dockerstate/testutils/json_test.go | 8 +- agent/engine/engine_integ_test.go | 2 +- agent/engine/engine_mocks.go | 2 +- agent/handlers/mocks/handlers_mocks.go | 6 +- agent/handlers/mocks/http/handlers_mocks.go | 2 +- agent/handlers/types.go | 2 +- agent/handlers/v1_handlers.go | 10 +- agent/handlers/v1_handlers_test.go | 6 +- agent/httpclient/mock/httpclient.go | 2 +- agent/logger/audit/mocks/audit_log_mocks.go | 2 +- .../statemanager/mocks/statemanager_mocks.go | 2 +- agent/statemanager/state_manager_test.go | 2 +- agent/statemanager/state_manager_unix_test.go | 4 +- agent/stats/engine.go | 4 +- scripts/generate/mockgen.go | 4 +- 27 files changed, 399 insertions(+), 172 deletions(-) create mode 100644 agent/engine/dockerstate/generate_mocks.go create mode 100644 agent/engine/dockerstate/mocks/dockerstate_mocks.go diff --git a/agent/agent.go b/agent/agent.go index 13b76caad6a..09fd95b3a35 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -115,7 +115,7 @@ func _main() int { // the credentials handler credentialsManager := credentials.NewManager() // Create image manager. This will be used by the task engine for saving image states - state := dockerstate.NewDockerTaskEngineState() + state := dockerstate.NewTaskEngineState() imageManager := engine.NewImageManager(cfg, dockerClient, state) if *versionFlag { versionableEngine := engine.NewTaskEngine(cfg, dockerClient, credentialsManager, containerChangeEventStream, imageManager, state) diff --git a/agent/engine/default.go b/agent/engine/default.go index a6169b43590..fa3f00f87c6 100644 --- a/agent/engine/default.go +++ b/agent/engine/default.go @@ -26,6 +26,6 @@ import ( var log = logger.ForModule("TaskEngine") // NewTaskEngine returns a default TaskEngine -func NewTaskEngine(cfg *config.Config, client DockerClient, credentialsManager credentials.Manager, containerChangeEventStream *eventstream.EventStream, imageManager ImageManager, state *dockerstate.DockerTaskEngineState) TaskEngine { +func NewTaskEngine(cfg *config.Config, client DockerClient, credentialsManager credentials.Manager, containerChangeEventStream *eventstream.EventStream, imageManager ImageManager, state dockerstate.TaskEngineState) TaskEngine { return NewDockerTaskEngine(cfg, client, credentialsManager, containerChangeEventStream, imageManager, state) } diff --git a/agent/engine/docker_image_manager.go b/agent/engine/docker_image_manager.go index 921e73b9957..e9dad1e9969 100644 --- a/agent/engine/docker_image_manager.go +++ b/agent/engine/docker_image_manager.go @@ -50,7 +50,7 @@ type dockerImageManager struct { client DockerClient updateLock sync.RWMutex imageCleanupTicker *time.Ticker - state *dockerstate.DockerTaskEngineState + state dockerstate.TaskEngineState saver statemanager.Saver imageStatesConsideredForDeletion map[string]*image.ImageState minimumAgeBeforeDeletion time.Duration @@ -62,7 +62,7 @@ type dockerImageManager struct { type ImageStatesForDeletion []*image.ImageState // NewImageManager returns a new ImageManager -func NewImageManager(cfg *config.Config, client DockerClient, state *dockerstate.DockerTaskEngineState) ImageManager { +func NewImageManager(cfg *config.Config, client DockerClient, state dockerstate.TaskEngineState) ImageManager { return &dockerImageManager{ client: client, state: state, diff --git a/agent/engine/docker_image_manager_test.go b/agent/engine/docker_image_manager_test.go index 35ff3653bc9..faa5a8f81a6 100644 --- a/agent/engine/docker_image_manager_test.go +++ b/agent/engine/docker_image_manager_test.go @@ -35,7 +35,7 @@ func TestAddAndRemoveContainerToImageStateReferenceHappyPath(t *testing.T) { defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewDockerTaskEngineState()) + imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewTaskEngineState()) imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ @@ -83,7 +83,7 @@ func TestRecordContainerReferenceInspectError(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -117,7 +117,7 @@ func TestRecordContainerReferenceWithNoImageName(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -160,7 +160,7 @@ func TestAddInvalidContainerReferenceToImageState(t *testing.T) { defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewDockerTaskEngineState()) + imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewTaskEngineState()) imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ @@ -176,7 +176,7 @@ func TestAddContainerReferenceToExistingImageState(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageID := "sha256:qwerty" container := &api.Container{ Name: "testContainer", @@ -213,7 +213,7 @@ func TestAddContainerReferenceToExistingImageStateNoState(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} container := &api.Container{ Name: "testContainer", Image: "testContainerImage", @@ -228,7 +228,7 @@ func TestAddContainerReferenceToNewImageState(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageID := "sha256:qwerty" var imageSize int64 imageSize = 18767 @@ -248,7 +248,7 @@ func TestAddContainerReferenceToNewImageStateAddedState(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageID := "sha256:qwerty" var imageSize int64 imageSize = 18767 @@ -286,7 +286,7 @@ func TestRemoveContainerReferenceFromInvalidImageState(t *testing.T) { defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewDockerTaskEngineState()) + imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewTaskEngineState()) imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ @@ -307,7 +307,7 @@ func TestRemoveInvalidContainerReferenceFromImageState(t *testing.T) { defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewDockerTaskEngineState()) + imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewTaskEngineState()) imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ @@ -324,7 +324,7 @@ func TestRemoveContainerReferenceFromImageStateInspectError(t *testing.T) { defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewDockerTaskEngineState()) + imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewTaskEngineState()) imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ @@ -344,7 +344,7 @@ func TestRemoveContainerReferenceFromImageStateWithNoReference(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -380,7 +380,7 @@ func TestGetCandidateImagesForDeletionImageNoImageState(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -400,7 +400,7 @@ func TestGetCandidateImagesForDeletionImageJustPulled(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -425,7 +425,7 @@ func TestGetCandidateImagesForDeletionImageHasContainerReference(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -466,7 +466,7 @@ func TestGetCandidateImagesForDeletionImageHasMoreContainerReferences(t *testing imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -519,7 +519,7 @@ func TestGetLeastRecentlyUsedImages(t *testing.T) { defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewDockerTaskEngineState()) + imageManager := NewImageManager(defaultTestConfig(), client, dockerstate.NewTaskEngineState()) imageStateA := &image.ImageState{ LastUsedAt: time.Now().AddDate(0, -5, 0), @@ -559,7 +559,7 @@ func TestGetLeastRecentlyUsedImagesLessThanFive(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -593,7 +593,7 @@ func TestRemoveAlreadyExistingImageNameWithDifferentID(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -644,7 +644,7 @@ func TestImageCleanupHappyPath(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: 1 * time.Millisecond, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -696,7 +696,7 @@ func TestImageCleanupCannotRemoveImage(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -746,7 +746,7 @@ func TestImageCleanupRemoveImageById(t *testing.T) { imageManager := &dockerImageManager{ client: client, - state: dockerstate.NewDockerTaskEngineState(), + state: dockerstate.NewTaskEngineState(), minimumAgeBeforeDeletion: config.DefaultImageDeletionAge, numImagesToDelete: config.DefaultNumImagesToDeletePerCycle, imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval, @@ -791,7 +791,7 @@ func TestDeleteImage(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ Name: "testContainer", @@ -820,7 +820,7 @@ func TestDeleteImageNotFoundError(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ Name: "testContainer", @@ -849,7 +849,7 @@ func TestDeleteImageOtherRemoveImageErrors(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ Name: "testContainer", @@ -878,7 +878,7 @@ func TestDeleteImageIDNull(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageManager.SetSaver(statemanager.NewNoopStateManager()) imageManager.deleteImage("", nil) } @@ -887,7 +887,7 @@ func TestRemoveLeastRecentlyUsedImageNoImage(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageManager.SetSaver(statemanager.NewNoopStateManager()) err := imageManager.removeLeastRecentlyUsedImage() if err == nil { @@ -899,7 +899,7 @@ func TestRemoveUnusedImagesNoImages(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageManager.SetSaver(statemanager.NewNoopStateManager()) imageManager.removeUnusedImages() } @@ -908,7 +908,7 @@ func TestGetImageStateFromImageName(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ Name: "testContainer", @@ -932,7 +932,7 @@ func TestGetImageStateFromImageNameNoImageState(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() client := NewMockDockerClient(ctrl) - imageManager := &dockerImageManager{client: client, state: dockerstate.NewDockerTaskEngineState()} + imageManager := &dockerImageManager{client: client, state: dockerstate.NewTaskEngineState()} imageManager.SetSaver(statemanager.NewNoopStateManager()) container := &api.Container{ Name: "testContainer", diff --git a/agent/engine/docker_task_engine.go b/agent/engine/docker_task_engine.go index d1f8254f6ef..9bee45eb740 100644 --- a/agent/engine/docker_task_engine.go +++ b/agent/engine/docker_task_engine.go @@ -62,7 +62,7 @@ type DockerTaskEngine struct { // current state and mappings to/from dockerId and name. // This is used to checkpoint state to disk so tasks may survive agent // failures or updates - state *dockerstate.DockerTaskEngineState + state dockerstate.TaskEngineState managedTasks map[string]*managedTask taskStopGroup *utilsync.SequentialWaitGroup @@ -96,7 +96,7 @@ type DockerTaskEngine struct { // The distinction between created and initialized is that when created it may // be serialized/deserialized, but it will not communicate with docker until it // is also initialized. -func NewDockerTaskEngine(cfg *config.Config, client DockerClient, credentialsManager credentials.Manager, containerChangeEventStream *eventstream.EventStream, imageManager ImageManager, state *dockerstate.DockerTaskEngineState) *DockerTaskEngine { +func NewDockerTaskEngine(cfg *config.Config, client DockerClient, credentialsManager credentials.Manager, containerChangeEventStream *eventstream.EventStream, imageManager ImageManager, state dockerstate.TaskEngineState) *DockerTaskEngine { dockerTaskEngine := &DockerTaskEngine{ cfg: cfg, client: client, @@ -410,8 +410,8 @@ func (engine *DockerTaskEngine) handleDockerEvents(ctx context.Context) { func (engine *DockerTaskEngine) handleDockerEvent(event DockerContainerChangeEvent) bool { log.Debug("Handling a docker event", "event", event) - task, taskFound := engine.state.TaskById(event.DockerID) - cont, containerFound := engine.state.ContainerById(event.DockerID) + task, taskFound := engine.state.TaskByID(event.DockerID) + cont, containerFound := engine.state.ContainerByID(event.DockerID) if !taskFound || !containerFound { log.Debug("Event for container not managed", "dockerId", event.DockerID) return false @@ -717,7 +717,7 @@ func (engine *DockerTaskEngine) transitionContainer(task *api.Task, container *a // State is a function primarily meant for testing usage; it is explicitly not // part of the TaskEngine interface and should not be relied upon. // It returns an internal representation of the state of this DockerTaskEngine. -func (engine *DockerTaskEngine) State() *dockerstate.DockerTaskEngineState { +func (engine *DockerTaskEngine) State() dockerstate.TaskEngineState { return engine.state } diff --git a/agent/engine/docker_task_engine_test.go b/agent/engine/docker_task_engine_test.go index 3ce775a0165..b403e7de9e0 100644 --- a/agent/engine/docker_task_engine_test.go +++ b/agent/engine/docker_task_engine_test.go @@ -53,7 +53,7 @@ func mocks(t *testing.T, cfg *config.Config) (*gomock.Controller, *MockDockerCli containerChangeEventStream := eventstream.NewEventStream("TESTTASKENGINE", context.Background()) containerChangeEventStream.StartListening() imageManager := NewMockImageManager(ctrl) - taskEngine := NewTaskEngine(cfg, client, credentialsManager, containerChangeEventStream, imageManager, dockerstate.NewDockerTaskEngineState()) + taskEngine := NewTaskEngine(cfg, client, credentialsManager, containerChangeEventStream, imageManager, dockerstate.NewTaskEngineState()) taskEngine.(*DockerTaskEngine)._time = mockTime return ctrl, client, mockTime, taskEngine, credentialsManager, imageManager } diff --git a/agent/engine/dockerstate/docker_task_engine_state.go b/agent/engine/dockerstate/docker_task_engine_state.go index f5cfe583fc6..cb24eb07ff0 100644 --- a/agent/engine/dockerstate/docker_task_engine_state.go +++ b/agent/engine/dockerstate/docker_task_engine_state.go @@ -14,6 +14,7 @@ package dockerstate import ( + "encoding/json" "sync" "github.com/aws/amazon-ecs-agent/agent/api" @@ -23,7 +24,36 @@ import ( var log = logger.ForModule("dockerstate") -// dockerTaskEngineState keeps track of all mappings between tasks we know about +// TaskEngineState keeps track of all mappings between tasks we know about +// and containers docker runs +type TaskEngineState interface { + // AllTasks returns all of the tasks + AllTasks() []*api.Task + // AllImageStates returns all of the image.ImageStates + AllImageStates() []*image.ImageState + // ContainerByID returns an api.DockerContainer for a given container ID + ContainerByID(id string) (*api.DockerContainer, bool) + // ContainerMapByArn returns a map of containers belonging to a particular task ARN + ContainerMapByArn(arn string) (map[string]*api.DockerContainer, bool) + // TaskByID returns an api.Task for a given container ID + TaskByID(cid string) (*api.Task, bool) + // TaskByArn returns a task for a given ARN + TaskByArn(arn string) (*api.Task, bool) + // AddTask adds a task to the state to be stored + AddTask(task *api.Task) + // AddContainer adds a container to the state to be stored for a given task + AddContainer(container *api.DockerContainer, task *api.Task) + // AddImageState adds an image.ImageState to be stored + AddImageState(imageState *image.ImageState) + // RemoveTask removes a task from the state + RemoveTask(task *api.Task) + // RemoveImageState removes an image.ImageState + RemoveImageState(imageState *image.ImageState) + json.Marshaler + json.Unmarshaler +} + +// DockerTaskEngineState keeps track of all mappings between tasks we know about // and containers docker runs // It contains a mutex that can be used to ensure out-of-date state cannot be // accessed before an update comes and to ensure multiple goroutines can safely @@ -41,22 +71,62 @@ type DockerTaskEngineState struct { tasks map[string]*api.Task // taskarn -> api.Task idToTask map[string]string // DockerId -> taskarn - taskToId map[string]map[string]*api.DockerContainer // taskarn -> (containername -> api.DockerContainer) + taskToID map[string]map[string]*api.DockerContainer // taskarn -> (containername -> api.DockerContainer) idToContainer map[string]*api.DockerContainer // DockerId -> api.DockerContainer imageStates map[string]*image.ImageState } -func NewDockerTaskEngineState() *DockerTaskEngineState { +// NewTaskEngineState returns a new TaskEngineState +func NewTaskEngineState() TaskEngineState { + return newDockerTaskEngineState() +} + +func newDockerTaskEngineState() *DockerTaskEngineState { return &DockerTaskEngineState{ tasks: make(map[string]*api.Task), idToTask: make(map[string]string), - taskToId: make(map[string]map[string]*api.DockerContainer), + taskToID: make(map[string]map[string]*api.DockerContainer), idToContainer: make(map[string]*api.DockerContainer), imageStates: make(map[string]*image.ImageState), } } -func (state *DockerTaskEngineState) ContainerById(id string) (*api.DockerContainer, bool) { +// AllTasks returns all of the tasks +func (state *DockerTaskEngineState) AllTasks() []*api.Task { + state.lock.RLock() + defer state.lock.RUnlock() + + return state.allTasks() +} + +func (state *DockerTaskEngineState) allTasks() []*api.Task { + ret := make([]*api.Task, len(state.tasks)) + ndx := 0 + for _, task := range state.tasks { + ret[ndx] = task + ndx++ + } + return ret +} + +// AllImageStates returns all of the image.ImageStates +func (state *DockerTaskEngineState) AllImageStates() []*image.ImageState { + state.lock.RLock() + defer state.lock.RUnlock() + + return state.allImageStates() +} + +func (state *DockerTaskEngineState) allImageStates() []*image.ImageState { + var allImageStates []*image.ImageState + for _, imageState := range state.imageStates { + allImageStates = append(allImageStates, imageState) + } + return allImageStates +} + +// ContainerByID returns an api.DockerContainer for a given container ID +func (state *DockerTaskEngineState) ContainerByID(id string) (*api.DockerContainer, bool) { state.lock.RLock() defer state.lock.RUnlock() @@ -64,16 +134,17 @@ func (state *DockerTaskEngineState) ContainerById(id string) (*api.DockerContain return c, ok } +// ContainerMapByArn returns a map of containers belonging to a particular task ARN func (state *DockerTaskEngineState) ContainerMapByArn(arn string) (map[string]*api.DockerContainer, bool) { state.lock.RLock() defer state.lock.RUnlock() - ret, ok := state.taskToId[arn] + ret, ok := state.taskToID[arn] return ret, ok } -// TaskById retrieves the task of a given docker container id -func (state *DockerTaskEngineState) TaskById(cid string) (*api.Task, bool) { +// TaskByID retrieves the task of a given docker container id +func (state *DockerTaskEngineState) TaskByID(cid string) (*api.Task, bool) { state.lock.RLock() defer state.lock.RUnlock() @@ -84,6 +155,19 @@ func (state *DockerTaskEngineState) TaskById(cid string) (*api.Task, bool) { return state.taskByArn(arn) } +// TaskByArn returns a task for a given ARN +func (state *DockerTaskEngineState) TaskByArn(arn string) (*api.Task, bool) { + state.lock.RLock() + defer state.lock.RUnlock() + + return state.taskByArn(arn) +} + +func (state *DockerTaskEngineState) taskByArn(arn string) (*api.Task, bool) { + t, ok := state.tasks[arn] + return t, ok +} + // AddTask adds a new task to the state func (state *DockerTaskEngineState) AddTask(task *api.Task) { state.lock.Lock() @@ -92,6 +176,39 @@ func (state *DockerTaskEngineState) AddTask(task *api.Task) { state.tasks[task.Arn] = task } +// AddContainer adds a container to the state. +// If the container has been added with only a name and no docker-id, this +// updates the state to include the docker id +func (state *DockerTaskEngineState) AddContainer(container *api.DockerContainer, task *api.Task) { + state.lock.Lock() + defer state.lock.Unlock() + if task == nil || container == nil { + log.Crit("Addcontainer called with nil task/container") + return + } + + _, exists := state.tasks[task.Arn] + if !exists { + log.Debug("AddContainer called with unknown task; adding", "arn", task.Arn) + state.tasks[task.Arn] = task + } + + if container.DockerID != "" { + state.idToTask[container.DockerID] = task.Arn + } + existingMap, exists := state.taskToID[task.Arn] + if !exists { + existingMap = make(map[string]*api.DockerContainer, len(task.Containers)) + state.taskToID[task.Arn] = existingMap + } + existingMap[container.Container.Name] = container + + if container.DockerID != "" { + state.idToContainer[container.DockerID] = container + } +} + +// AddImageState adds an image.ImageState to be stored func (state *DockerTaskEngineState) AddImageState(imageState *image.ImageState) { if imageState == nil { log.Debug("Cannot add empty image state") @@ -118,11 +235,11 @@ func (state *DockerTaskEngineState) RemoveTask(task *api.Task) { return } delete(state.tasks, task.Arn) - containerMap, ok := state.taskToId[task.Arn] + containerMap, ok := state.taskToID[task.Arn] if !ok { return } - delete(state.taskToId, task.Arn) + delete(state.taskToID, task.Arn) for _, dockerContainer := range containerMap { delete(state.idToTask, dockerContainer.DockerID) @@ -130,6 +247,7 @@ func (state *DockerTaskEngineState) RemoveTask(task *api.Task) { } } +// RemoveImageState removes an image.ImageState func (state *DockerTaskEngineState) RemoveImageState(imageState *image.ImageState) { if imageState == nil { log.Debug("Cannot remove empty image state") @@ -145,79 +263,3 @@ func (state *DockerTaskEngineState) RemoveImageState(imageState *image.ImageStat } delete(state.imageStates, imageState.Image.ImageID) } - -// AddContainer adds a container to the state. -// If the container has been added with only a name and no docker-id, this -// updates the state to include the docker id -func (state *DockerTaskEngineState) AddContainer(container *api.DockerContainer, task *api.Task) { - state.lock.Lock() - defer state.lock.Unlock() - if task == nil || container == nil { - log.Crit("Addcontainer called with nil task/container") - return - } - - _, exists := state.tasks[task.Arn] - if !exists { - log.Debug("AddContainer called with unknown task; adding", "arn", task.Arn) - state.tasks[task.Arn] = task - } - - if container.DockerID != "" { - state.idToTask[container.DockerID] = task.Arn - } - existingMap, exists := state.taskToId[task.Arn] - if !exists { - existingMap = make(map[string]*api.DockerContainer, len(task.Containers)) - state.taskToId[task.Arn] = existingMap - } - existingMap[container.Container.Name] = container - - if container.DockerID != "" { - state.idToContainer[container.DockerID] = container - } -} - -func (state *DockerTaskEngineState) TaskByArn(arn string) (*api.Task, bool) { - state.lock.RLock() - defer state.lock.RUnlock() - - return state.taskByArn(arn) -} - -func (state *DockerTaskEngineState) taskByArn(arn string) (*api.Task, bool) { - t, ok := state.tasks[arn] - return t, ok -} - -func (state *DockerTaskEngineState) AllTasks() []*api.Task { - state.lock.RLock() - defer state.lock.RUnlock() - - return state.allTasks() -} - -func (state *DockerTaskEngineState) allTasks() []*api.Task { - ret := make([]*api.Task, len(state.tasks)) - ndx := 0 - for _, task := range state.tasks { - ret[ndx] = task - ndx += 1 - } - return ret -} - -func (state *DockerTaskEngineState) AllImageStates() []*image.ImageState { - state.lock.RLock() - defer state.lock.RUnlock() - - return state.allImageStates() -} - -func (state *DockerTaskEngineState) allImageStates() []*image.ImageState { - var allImageStates []*image.ImageState - for _, imageState := range state.imageStates { - allImageStates = append(allImageStates, imageState) - } - return allImageStates -} diff --git a/agent/engine/dockerstate/dockerstate_test.go b/agent/engine/dockerstate/dockerstate_test.go index d1c719cd2d2..581c598bfb2 100644 --- a/agent/engine/dockerstate/dockerstate_test.go +++ b/agent/engine/dockerstate/dockerstate_test.go @@ -22,9 +22,9 @@ import ( ) func TestCreateDockerTaskEngineState(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() - if _, ok := state.ContainerById("test"); ok { + if _, ok := state.ContainerByID("test"); ok { t.Error("Empty state should not have a test container") } @@ -32,7 +32,7 @@ func TestCreateDockerTaskEngineState(t *testing.T) { t.Error("Empty state should not have a test task") } - if _, ok := state.TaskById("test"); ok { + if _, ok := state.TaskByID("test"); ok { t.Error("Empty state should not have a test taskid") } @@ -46,7 +46,7 @@ func TestCreateDockerTaskEngineState(t *testing.T) { } func TestAddTask(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() testTask := &api.Task{Arn: "test"} state.AddTask(testTask) @@ -65,7 +65,7 @@ func TestAddTask(t *testing.T) { } func TestTwophaseAddContainer(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() testTask := &api.Task{Arn: "test", Containers: []*api.Container{&api.Container{ Name: "testContainer", }}} @@ -119,7 +119,7 @@ func TestTwophaseAddContainer(t *testing.T) { t.Fatal("DockerID should have been updated") } - container, ok = state.ContainerById("did") + container, ok = state.ContainerByID("did") if !ok { t.Fatal("Could not get container by id") } @@ -129,7 +129,7 @@ func TestTwophaseAddContainer(t *testing.T) { } func TestRemoveTask(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() testContainer := &api.Container{ Name: "c1", } @@ -159,7 +159,7 @@ func TestRemoveTask(t *testing.T) { } func TestAddImageState(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() testImage := &image.Image{ImageID: "sha256:imagedigest"} testImageState := &image.ImageState{Image: testImage} @@ -177,7 +177,7 @@ func TestAddImageState(t *testing.T) { } func TestAddEmptyImageState(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() state.AddImageState(nil) if len(state.AllImageStates()) != 0 { @@ -186,7 +186,7 @@ func TestAddEmptyImageState(t *testing.T) { } func TestAddEmptyIdImageState(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() testImage := &image.Image{ImageID: ""} testImageState := &image.ImageState{Image: testImage} @@ -198,7 +198,7 @@ func TestAddEmptyIdImageState(t *testing.T) { } func TestRemoveImageState(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() testImage := &image.Image{ImageID: "sha256:imagedigest"} testImageState := &image.ImageState{Image: testImage} @@ -214,7 +214,7 @@ func TestRemoveImageState(t *testing.T) { } func TestRemoveEmptyImageState(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() testImage := &image.Image{ImageID: "sha256:imagedigest"} testImageState := &image.ImageState{Image: testImage} @@ -230,7 +230,7 @@ func TestRemoveEmptyImageState(t *testing.T) { } func TestRemoveNonExistingImageState(t *testing.T) { - state := NewDockerTaskEngineState() + state := NewTaskEngineState() testImage := &image.Image{ImageID: "sha256:imagedigest"} testImageState := &image.ImageState{Image: testImage} diff --git a/agent/engine/dockerstate/generate_mocks.go b/agent/engine/dockerstate/generate_mocks.go new file mode 100644 index 00000000000..c5f0ded6742 --- /dev/null +++ b/agent/engine/dockerstate/generate_mocks.go @@ -0,0 +1,16 @@ +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package dockerstate + +//go:generate go run ../../../scripts/generate/mockgen.go github.com/aws/amazon-ecs-agent/agent/engine/dockerstate TaskEngineState mocks/dockerstate_mocks.go diff --git a/agent/engine/dockerstate/json.go b/agent/engine/dockerstate/json.go index cdb54a93f85..9a618f2f926 100644 --- a/agent/engine/dockerstate/json.go +++ b/agent/engine/dockerstate/json.go @@ -25,8 +25,8 @@ import ( // DockerTaskEngine state type savedState struct { Tasks []*api.Task - IdToContainer map[string]*api.DockerContainer // DockerId -> api.DockerContainer - IdToTask map[string]string // DockerId -> taskarn + IdToContainer map[string]*api.DockerContainer `json:"IdToContainer"` // DockerId -> api.DockerContainer + IdToTask map[string]string `json:"IdToTask"` // DockerId -> taskarn ImageStates []*image.ImageState } @@ -53,7 +53,7 @@ func (state *DockerTaskEngineState) UnmarshalJSON(data []byte) error { // reset it by just creating a new one and swapping shortly. // This also means we don't have to lock for the remainder of this function // because we are the only ones with a reference to clean - clean := NewDockerTaskEngineState() + clean := newDockerTaskEngineState() for _, task := range saved.Tasks { clean.AddTask(task) diff --git a/agent/engine/dockerstate/mocks/dockerstate_mocks.go b/agent/engine/dockerstate/mocks/dockerstate_mocks.go new file mode 100644 index 00000000000..2cde6ebabf4 --- /dev/null +++ b/agent/engine/dockerstate/mocks/dockerstate_mocks.go @@ -0,0 +1,169 @@ +// Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +// Automatically generated by MockGen. DO NOT EDIT! +// Source: github.com/aws/amazon-ecs-agent/agent/engine/dockerstate (interfaces: TaskEngineState) + +package mock_dockerstate + +import ( + api "github.com/aws/amazon-ecs-agent/agent/api" + image "github.com/aws/amazon-ecs-agent/agent/engine/image" + gomock "github.com/golang/mock/gomock" +) + +// Mock of TaskEngineState interface +type MockTaskEngineState struct { + ctrl *gomock.Controller + recorder *_MockTaskEngineStateRecorder +} + +// Recorder for MockTaskEngineState (not exported) +type _MockTaskEngineStateRecorder struct { + mock *MockTaskEngineState +} + +func NewMockTaskEngineState(ctrl *gomock.Controller) *MockTaskEngineState { + mock := &MockTaskEngineState{ctrl: ctrl} + mock.recorder = &_MockTaskEngineStateRecorder{mock} + return mock +} + +func (_m *MockTaskEngineState) EXPECT() *_MockTaskEngineStateRecorder { + return _m.recorder +} + +func (_m *MockTaskEngineState) AddContainer(_param0 *api.DockerContainer, _param1 *api.Task) { + _m.ctrl.Call(_m, "AddContainer", _param0, _param1) +} + +func (_mr *_MockTaskEngineStateRecorder) AddContainer(arg0, arg1 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "AddContainer", arg0, arg1) +} + +func (_m *MockTaskEngineState) AddImageState(_param0 *image.ImageState) { + _m.ctrl.Call(_m, "AddImageState", _param0) +} + +func (_mr *_MockTaskEngineStateRecorder) AddImageState(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "AddImageState", arg0) +} + +func (_m *MockTaskEngineState) AddTask(_param0 *api.Task) { + _m.ctrl.Call(_m, "AddTask", _param0) +} + +func (_mr *_MockTaskEngineStateRecorder) AddTask(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "AddTask", arg0) +} + +func (_m *MockTaskEngineState) AllImageStates() []*image.ImageState { + ret := _m.ctrl.Call(_m, "AllImageStates") + ret0, _ := ret[0].([]*image.ImageState) + return ret0 +} + +func (_mr *_MockTaskEngineStateRecorder) AllImageStates() *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "AllImageStates") +} + +func (_m *MockTaskEngineState) AllTasks() []*api.Task { + ret := _m.ctrl.Call(_m, "AllTasks") + ret0, _ := ret[0].([]*api.Task) + return ret0 +} + +func (_mr *_MockTaskEngineStateRecorder) AllTasks() *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "AllTasks") +} + +func (_m *MockTaskEngineState) ContainerByID(_param0 string) (*api.DockerContainer, bool) { + ret := _m.ctrl.Call(_m, "ContainerByID", _param0) + ret0, _ := ret[0].(*api.DockerContainer) + ret1, _ := ret[1].(bool) + return ret0, ret1 +} + +func (_mr *_MockTaskEngineStateRecorder) ContainerByID(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "ContainerByID", arg0) +} + +func (_m *MockTaskEngineState) ContainerMapByArn(_param0 string) (map[string]*api.DockerContainer, bool) { + ret := _m.ctrl.Call(_m, "ContainerMapByArn", _param0) + ret0, _ := ret[0].(map[string]*api.DockerContainer) + ret1, _ := ret[1].(bool) + return ret0, ret1 +} + +func (_mr *_MockTaskEngineStateRecorder) ContainerMapByArn(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "ContainerMapByArn", arg0) +} + +func (_m *MockTaskEngineState) MarshalJSON() ([]byte, error) { + ret := _m.ctrl.Call(_m, "MarshalJSON") + ret0, _ := ret[0].([]byte) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +func (_mr *_MockTaskEngineStateRecorder) MarshalJSON() *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "MarshalJSON") +} + +func (_m *MockTaskEngineState) RemoveImageState(_param0 *image.ImageState) { + _m.ctrl.Call(_m, "RemoveImageState", _param0) +} + +func (_mr *_MockTaskEngineStateRecorder) RemoveImageState(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "RemoveImageState", arg0) +} + +func (_m *MockTaskEngineState) RemoveTask(_param0 *api.Task) { + _m.ctrl.Call(_m, "RemoveTask", _param0) +} + +func (_mr *_MockTaskEngineStateRecorder) RemoveTask(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "RemoveTask", arg0) +} + +func (_m *MockTaskEngineState) TaskByArn(_param0 string) (*api.Task, bool) { + ret := _m.ctrl.Call(_m, "TaskByArn", _param0) + ret0, _ := ret[0].(*api.Task) + ret1, _ := ret[1].(bool) + return ret0, ret1 +} + +func (_mr *_MockTaskEngineStateRecorder) TaskByArn(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "TaskByArn", arg0) +} + +func (_m *MockTaskEngineState) TaskByID(_param0 string) (*api.Task, bool) { + ret := _m.ctrl.Call(_m, "TaskByID", _param0) + ret0, _ := ret[0].(*api.Task) + ret1, _ := ret[1].(bool) + return ret0, ret1 +} + +func (_mr *_MockTaskEngineStateRecorder) TaskByID(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "TaskByID", arg0) +} + +func (_m *MockTaskEngineState) UnmarshalJSON(_param0 []byte) error { + ret := _m.ctrl.Call(_m, "UnmarshalJSON", _param0) + ret0, _ := ret[0].(error) + return ret0 +} + +func (_mr *_MockTaskEngineStateRecorder) UnmarshalJSON(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "UnmarshalJSON", arg0) +} diff --git a/agent/engine/dockerstate/testutils/docker_state_equal.go b/agent/engine/dockerstate/testutils/docker_state_equal.go index 33916352bd8..063376c356c 100644 --- a/agent/engine/dockerstate/testutils/docker_state_equal.go +++ b/agent/engine/dockerstate/testutils/docker_state_equal.go @@ -21,7 +21,7 @@ import api_testutils "github.com/aws/amazon-ecs-agent/agent/api/testutils" // DockerStatesEqual determines if the two given dockerstates are equal, for // equal meaning they have the same tasks and their tasks are equal -func DockerStatesEqual(lhs, rhs *dockerstate.DockerTaskEngineState) bool { +func DockerStatesEqual(lhs, rhs dockerstate.TaskEngineState) bool { // Simple equality check; just verify that all tasks are equal lhsTasks := lhs.AllTasks() rhsTasks := rhs.AllTasks() diff --git a/agent/engine/dockerstate/testutils/json_test.go b/agent/engine/dockerstate/testutils/json_test.go index 40d3dad9329..811dd6e4876 100644 --- a/agent/engine/dockerstate/testutils/json_test.go +++ b/agent/engine/dockerstate/testutils/json_test.go @@ -48,12 +48,12 @@ func createTestTask(arn string, numContainers int) *api.Task { return task } -func decodeEqual(t *testing.T, state *dockerstate.DockerTaskEngineState) *dockerstate.DockerTaskEngineState { +func decodeEqual(t *testing.T, state dockerstate.TaskEngineState) dockerstate.TaskEngineState { data, err := json.Marshal(&state) if err != nil { t.Error(err) } - otherState := dockerstate.NewDockerTaskEngineState() + otherState := dockerstate.NewTaskEngineState() err = json.Unmarshal(data, &otherState) if err != nil { t.Error(err) @@ -66,10 +66,10 @@ func decodeEqual(t *testing.T, state *dockerstate.DockerTaskEngineState) *docker } func TestJsonEncoding(t *testing.T) { - state := dockerstate.NewDockerTaskEngineState() + state := dockerstate.NewTaskEngineState() decodeEqual(t, state) - testState := dockerstate.NewDockerTaskEngineState() + testState := dockerstate.NewTaskEngineState() testTask := createTestTask("test1", 1) testState.AddTask(testTask) for i, cont := range testTask.Containers { diff --git a/agent/engine/engine_integ_test.go b/agent/engine/engine_integ_test.go index 1e73d4582cf..bdf095f34ea 100644 --- a/agent/engine/engine_integ_test.go +++ b/agent/engine/engine_integ_test.go @@ -78,7 +78,7 @@ func setup(cfg *config.Config, t *testing.T) (TaskEngine, func(), credentials.Ma t.Fatalf("Error creating Docker client: %v", err) } credentialsManager := credentials.NewManager() - state := dockerstate.NewDockerTaskEngineState() + state := dockerstate.NewTaskEngineState() imageManager := NewImageManager(cfg, dockerClient, state) imageManager.SetSaver(statemanager.NewNoopStateManager()) taskEngine := NewDockerTaskEngine(cfg, dockerClient, credentialsManager, diff --git a/agent/engine/engine_mocks.go b/agent/engine/engine_mocks.go index fdfa2368c14..7e262ae405d 100644 --- a/agent/engine/engine_mocks.go +++ b/agent/engine/engine_mocks.go @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/handlers/mocks/handlers_mocks.go b/agent/handlers/mocks/handlers_mocks.go index 76a59507377..b63b06a375e 100644 --- a/agent/handlers/mocks/handlers_mocks.go +++ b/agent/handlers/mocks/handlers_mocks.go @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the @@ -42,9 +42,9 @@ func (_m *MockDockerStateResolver) EXPECT() *_MockDockerStateResolverRecorder { return _m.recorder } -func (_m *MockDockerStateResolver) State() *dockerstate.DockerTaskEngineState { +func (_m *MockDockerStateResolver) State() dockerstate.TaskEngineState { ret := _m.ctrl.Call(_m, "State") - ret0, _ := ret[0].(*dockerstate.DockerTaskEngineState) + ret0, _ := ret[0].(dockerstate.TaskEngineState) return ret0 } diff --git a/agent/handlers/mocks/http/handlers_mocks.go b/agent/handlers/mocks/http/handlers_mocks.go index ee13d96ca86..9881a07cacc 100644 --- a/agent/handlers/mocks/http/handlers_mocks.go +++ b/agent/handlers/mocks/http/handlers_mocks.go @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/handlers/types.go b/agent/handlers/types.go index 7f9b93e61a7..b8509008d87 100644 --- a/agent/handlers/types.go +++ b/agent/handlers/types.go @@ -41,5 +41,5 @@ type ContainerResponse struct { } type DockerStateResolver interface { - State() *dockerstate.DockerTaskEngineState + State() dockerstate.TaskEngineState } diff --git a/agent/handlers/v1_handlers.go b/agent/handlers/v1_handlers.go index a74763054b7..d6cfcb96bdd 100644 --- a/agent/handlers/v1_handlers.go +++ b/agent/handlers/v1_handlers.go @@ -90,7 +90,7 @@ func newTaskResponse(task *api.Task, containerMap map[string]*api.DockerContaine } } -func newTasksResponse(state *dockerstate.DockerTaskEngineState) *TasksResponse { +func newTasksResponse(state dockerstate.TaskEngineState) *TasksResponse { allTasks := state.AllTasks() taskResponses := make([]*TaskResponse, len(allTasks)) for ndx, task := range allTasks { @@ -102,7 +102,7 @@ func newTasksResponse(state *dockerstate.DockerTaskEngineState) *TasksResponse { } // Creates JSON response and sets the http status code for the task queried. -func createTaskJSONResponse(task *api.Task, found bool, resourceId string, state *dockerstate.DockerTaskEngineState) ([]byte, int) { +func createTaskJSONResponse(task *api.Task, found bool, resourceId string, state dockerstate.TaskEngineState) ([]byte, int) { var responseJSON []byte status := http.StatusOK if found { @@ -134,7 +134,7 @@ func tasksV1RequestHandlerMaker(taskEngine DockerStateResolver) func(http.Respon } if dockerIdExists { // Create TaskResponse for the docker id in the query. - task, found := dockerTaskEngineState.TaskById(dockerId) + task, found := dockerTaskEngineState.TaskByID(dockerId) responseJSON, status = createTaskJSONResponse(task, found, dockerId, dockerTaskEngineState) w.WriteHeader(status) } else if taskArnExists { @@ -161,7 +161,7 @@ func licenseHandler(w http.ResponseWriter, h *http.Request) { } } -func setupServer(containerInstanceArn *string, taskEngine DockerStateResolver, cfg *config.Config) http.Server { +func setupServer(containerInstanceArn *string, taskEngine DockerStateResolver, cfg *config.Config) *http.Server { serverFunctions := map[string]func(w http.ResponseWriter, r *http.Request){ "/v1/metadata": metadataV1RequestHandlerMaker(containerInstanceArn, cfg), "/v1/tasks": tasksV1RequestHandlerMaker(taskEngine), @@ -190,7 +190,7 @@ func setupServer(containerInstanceArn *string, taskEngine DockerStateResolver, c loggingServeMux := http.NewServeMux() loggingServeMux.Handle("/", LoggingHandler{serverMux}) - server := http.Server{ + server := &http.Server{ Addr: ":" + strconv.Itoa(config.AgentIntrospectionPort), Handler: loggingServeMux, ReadTimeout: 5 * time.Second, diff --git a/agent/handlers/v1_handlers_test.go b/agent/handlers/v1_handlers_test.go index d13962e32ad..2180796139a 100644 --- a/agent/handlers/v1_handlers_test.go +++ b/agent/handlers/v1_handlers_test.go @@ -135,7 +135,7 @@ func TestBackendMismatchMapping(t *testing.T) { Containers: containers, } - state := dockerstate.NewDockerTaskEngineState() + state := dockerstate.NewTaskEngineState() stateSetupHelper(state, []*api.Task{testTask}) mockStateResolver.EXPECT().State().Return(state) @@ -266,7 +266,7 @@ var testTasks = []*api.Task{ }, } -func stateSetupHelper(state *dockerstate.DockerTaskEngineState, tasks []*api.Task) { +func stateSetupHelper(state dockerstate.TaskEngineState, tasks []*api.Task) { for _, task := range tasks { state.AddTask(task) for _, container := range task.Containers { @@ -285,7 +285,7 @@ func performMockRequest(t *testing.T, path string) *httptest.ResponseRecorder { mockStateResolver := mock_handlers.NewMockDockerStateResolver(ctrl) - state := dockerstate.NewDockerTaskEngineState() + state := dockerstate.NewTaskEngineState() stateSetupHelper(state, testTasks) mockStateResolver.EXPECT().State().Return(state) diff --git a/agent/httpclient/mock/httpclient.go b/agent/httpclient/mock/httpclient.go index 405c63ea6e1..40af7d40cad 100644 --- a/agent/httpclient/mock/httpclient.go +++ b/agent/httpclient/mock/httpclient.go @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/logger/audit/mocks/audit_log_mocks.go b/agent/logger/audit/mocks/audit_log_mocks.go index 41478fe5ece..76faa0aeb01 100644 --- a/agent/logger/audit/mocks/audit_log_mocks.go +++ b/agent/logger/audit/mocks/audit_log_mocks.go @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/statemanager/mocks/statemanager_mocks.go b/agent/statemanager/mocks/statemanager_mocks.go index 391a01f93e2..f96d573d787 100644 --- a/agent/statemanager/mocks/statemanager_mocks.go +++ b/agent/statemanager/mocks/statemanager_mocks.go @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2015-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/statemanager/state_manager_test.go b/agent/statemanager/state_manager_test.go index 3f55650cb9f..b34a7a789e8 100644 --- a/agent/statemanager/state_manager_test.go +++ b/agent/statemanager/state_manager_test.go @@ -40,7 +40,7 @@ func TestLoadsV1DataCorrectly(t *testing.T) { defer cleanup() cfg := &config.Config{DataDir: filepath.Join(".", "testdata", "v1", "1")} - taskEngine := engine.NewTaskEngine(&config.Config{}, nil, nil, nil, nil, dockerstate.NewDockerTaskEngineState()) + taskEngine := engine.NewTaskEngine(&config.Config{}, nil, nil, nil, nil, dockerstate.NewTaskEngineState()) var containerInstanceArn, cluster, savedInstanceID string var sequenceNumber int64 diff --git a/agent/statemanager/state_manager_unix_test.go b/agent/statemanager/state_manager_unix_test.go index 75eaa0f1ca7..5d72bed5c5f 100644 --- a/agent/statemanager/state_manager_unix_test.go +++ b/agent/statemanager/state_manager_unix_test.go @@ -43,7 +43,7 @@ func TestStateManager(t *testing.T) { // Now let's make some state to save containerInstanceArn := "" - taskEngine := engine.NewTaskEngine(&config.Config{}, nil, nil, nil, nil, dockerstate.NewDockerTaskEngineState()) + taskEngine := engine.NewTaskEngine(&config.Config{}, nil, nil, nil, nil, dockerstate.NewTaskEngineState()) manager, err = statemanager.NewStateManager(cfg, statemanager.AddSaveable("TaskEngine", taskEngine), statemanager.AddSaveable("ContainerInstanceArn", &containerInstanceArn)) require.Nil(t, err) @@ -59,7 +59,7 @@ func TestStateManager(t *testing.T) { assertFileMode(t, filepath.Join(tmpDir, "ecs_agent_data.json")) // Now make sure we can load that state sanely - loadedTaskEngine := engine.NewTaskEngine(&config.Config{}, nil, nil, nil, nil, dockerstate.NewDockerTaskEngineState()) + loadedTaskEngine := engine.NewTaskEngine(&config.Config{}, nil, nil, nil, nil, dockerstate.NewTaskEngineState()) var loadedContainerInstanceArn string manager, err = statemanager.NewStateManager(cfg, statemanager.AddSaveable("TaskEngine", &loadedTaskEngine), statemanager.AddSaveable("ContainerInstanceArn", &loadedContainerInstanceArn)) diff --git a/agent/stats/engine.go b/agent/stats/engine.go index d2f73840c5f..443a314e3f5 100644 --- a/agent/stats/engine.go +++ b/agent/stats/engine.go @@ -76,7 +76,7 @@ func (resolver *DockerContainerMetadataResolver) ResolveTask(dockerID string) (* if resolver.dockerTaskEngine == nil { return nil, fmt.Errorf("Docker task engine uninitialized") } - task, found := resolver.dockerTaskEngine.State().TaskById(dockerID) + task, found := resolver.dockerTaskEngine.State().TaskByID(dockerID) if !found { return nil, fmt.Errorf("Could not map docker id to task: %s", dockerID) } @@ -89,7 +89,7 @@ func (resolver *DockerContainerMetadataResolver) ResolveContainer(dockerID strin if resolver.dockerTaskEngine == nil { return nil, fmt.Errorf("Docker task engine uninitialized") } - container, found := resolver.dockerTaskEngine.State().ContainerById(dockerID) + container, found := resolver.dockerTaskEngine.State().ContainerByID(dockerID) if !found { return nil, fmt.Errorf("Could not map docker id to container: %s", dockerID) } diff --git a/scripts/generate/mockgen.go b/scripts/generate/mockgen.go index 34f63694e45..d6bc4b4b491 100644 --- a/scripts/generate/mockgen.go +++ b/scripts/generate/mockgen.go @@ -1,4 +1,4 @@ -// Copyright 2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2016-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the @@ -55,7 +55,7 @@ func main() { copyrightHeader := fmt.Sprintf(copyrightHeaderFormat, time.Now().Year()) path, _ := filepath.Split(outputPath) - err := os.MkdirAll(path, os.ModeDir) + err := os.MkdirAll(path, os.ModeDir|0755) if err != nil { fmt.Println(err) os.Exit(1) From c9a12956c68fe0b32cfda7852cb33a515536e796 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Thu, 9 Feb 2017 18:03:23 -0800 Subject: [PATCH 5/9] engine: Unit tests for managedTask.cleanupTask --- .../engine/docker_image_manager_integ_test.go | 6 +- agent/engine/task_manager.go | 9 +- agent/engine/task_manager_test.go | 179 ++++++++++++++++++ agent/stats/engine_integ_test.go | 4 +- 4 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 agent/engine/task_manager_test.go diff --git a/agent/engine/docker_image_manager_integ_test.go b/agent/engine/docker_image_manager_integ_test.go index 30bbb936f23..3e2df01aa0a 100644 --- a/agent/engine/docker_image_manager_integ_test.go +++ b/agent/engine/docker_image_manager_integ_test.go @@ -234,7 +234,7 @@ func TestIntegImageCleanupThreshold(t *testing.T) { testTask.SetSentStatus(api.TaskStopped) // Allow Task cleanup to occur - time.Sleep(2 * time.Second) + time.Sleep(5 * time.Second) // Verify Task is cleaned up err = verifyTaskIsCleanedUp(taskName, taskEngine) @@ -385,7 +385,7 @@ func TestImageWithSameNameAndDifferentID(t *testing.T) { task3.SetSentStatus(api.TaskStopped) // Allow Task cleanup to occur - time.Sleep(2 * time.Second) + time.Sleep(5 * time.Second) err = verifyTaskIsCleanedUp("task1", taskEngine) assert.NoError(t, err, "task1") @@ -511,7 +511,7 @@ func TestImageWithSameIDAndDifferentNames(t *testing.T) { task3.SetSentStatus(api.TaskStopped) // Allow Task cleanup to occur - time.Sleep(2 * time.Second) + time.Sleep(5 * time.Second) err = verifyTaskIsCleanedUp("task1", taskEngine) assert.NoError(t, err, "task1") diff --git a/agent/engine/task_manager.go b/agent/engine/task_manager.go index c9af9050941..b75d76ca1f5 100644 --- a/agent/engine/task_manager.go +++ b/agent/engine/task_manager.go @@ -498,22 +498,29 @@ func (mtask *managedTask) cleanupTask(taskStoppedDuration time.Duration) { for !mtask.waitEvent(cleanupTimeBool) { } stoppedSentBool := make(chan bool) + taskStopped := false go func() { for i := 0; i < _maxStoppedWaitTimes; i++ { // ensure that we block until api.TaskStopped is actually sent sentStatus := mtask.GetSentStatus() if sentStatus >= api.TaskStopped { stoppedSentBool <- true + taskStopped = true close(stoppedSentBool) return } - seelog.Warnf("Blocking cleanup for task %v until the task has been reported stopped. SentStatus: %v (%d/%d)", mtask, sentStatus, i, _maxStoppedWaitTimes) + seelog.Warnf("Blocking cleanup for task %v until the task has been reported stopped. SentStatus: %v (%d/%d)", mtask, sentStatus, i+1, _maxStoppedWaitTimes) mtask._time.Sleep(_stoppedSentWaitInterval) } + stoppedSentBool <- true }() // wait for api.TaskStopped to be sent for !mtask.waitEvent(stoppedSentBool) { } + if !taskStopped { + seelog.Errorf("Aborting cleanup for task %v as it is not reported stopped. SentStatus: %v", mtask, mtask.GetSentStatus()) + return + } log.Info("Cleaning up task's containers and data", "task", mtask.Task) diff --git a/agent/engine/task_manager_test.go b/agent/engine/task_manager_test.go new file mode 100644 index 00000000000..15b0c999660 --- /dev/null +++ b/agent/engine/task_manager_test.go @@ -0,0 +1,179 @@ +// +build !integration +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + +package engine + +import ( + "testing" + "time" + + "github.com/aws/amazon-ecs-agent/agent/api" + "github.com/aws/amazon-ecs-agent/agent/engine/dockerstate/mocks" + "github.com/aws/amazon-ecs-agent/agent/engine/testdata" + "github.com/aws/amazon-ecs-agent/agent/statemanager" + "github.com/aws/amazon-ecs-agent/agent/utils/ttime/mocks" + "github.com/stretchr/testify/assert" + + "github.com/golang/mock/gomock" +) + +func TestCleanupTask(t *testing.T) { + ctrl := gomock.NewController(t) + mockTime := mock_ttime.NewMockTime(ctrl) + mockState := mock_dockerstate.NewMockTaskEngineState(ctrl) + mockClient := NewMockDockerClient(ctrl) + mockImageManager := NewMockImageManager(ctrl) + defer ctrl.Finish() + + taskEngine := &DockerTaskEngine{ + saver: statemanager.NewNoopStateManager(), + state: mockState, + client: mockClient, + imageManager: mockImageManager, + } + mTask := &managedTask{ + Task: testdata.LoadTask("sleep5"), + _time: mockTime, + engine: taskEngine, + acsMessages: make(chan acsTransition), + dockerMessages: make(chan dockerContainerChange), + } + mTask.SetKnownStatus(api.TaskStopped) + mTask.SetSentStatus(api.TaskStopped) + container := mTask.Containers[0] + dockerContainer := &api.DockerContainer{ + DockerName: "dockerContainer", + } + + // Expectations for triggering cleanup + now := mTask.GetKnownStatusTime() + taskStoppedDuration := 1 * time.Minute + mockTime.EXPECT().Now().Return(now).AnyTimes() + cleanupTimeTrigger := make(chan time.Time) + mockTime.EXPECT().After(gomock.Any()).Return(cleanupTimeTrigger) + go func() { + cleanupTimeTrigger <- now + }() + + // Expectations to verify that the task gets removed + mockState.EXPECT().ContainerMapByArn(mTask.Arn).Return(map[string]*api.DockerContainer{container.Name: dockerContainer}, true) + mockClient.EXPECT().RemoveContainer(dockerContainer.DockerName, gomock.Any()).Return(nil) + mockImageManager.EXPECT().RemoveContainerReferenceFromImageState(container).Return(nil) + mockState.EXPECT().RemoveTask(mTask.Task) + mTask.cleanupTask(taskStoppedDuration) +} + +func TestCleanupTaskWaitsForStoppedSent(t *testing.T) { + ctrl := gomock.NewController(t) + mockTime := mock_ttime.NewMockTime(ctrl) + mockState := mock_dockerstate.NewMockTaskEngineState(ctrl) + mockClient := NewMockDockerClient(ctrl) + mockImageManager := NewMockImageManager(ctrl) + defer ctrl.Finish() + + taskEngine := &DockerTaskEngine{ + saver: statemanager.NewNoopStateManager(), + state: mockState, + client: mockClient, + imageManager: mockImageManager, + } + mTask := &managedTask{ + Task: testdata.LoadTask("sleep5"), + _time: mockTime, + engine: taskEngine, + acsMessages: make(chan acsTransition), + dockerMessages: make(chan dockerContainerChange), + } + mTask.SetKnownStatus(api.TaskStopped) + mTask.SetSentStatus(api.TaskRunning) + container := mTask.Containers[0] + dockerContainer := &api.DockerContainer{ + DockerName: "dockerContainer", + } + + // Expectations for triggering cleanup + now := mTask.GetKnownStatusTime() + taskStoppedDuration := 1 * time.Minute + mockTime.EXPECT().Now().Return(now).AnyTimes() + cleanupTimeTrigger := make(chan time.Time) + mockTime.EXPECT().After(gomock.Any()).Return(cleanupTimeTrigger) + go func() { + cleanupTimeTrigger <- now + }() + timesCalled := 0 + callsExpected := 3 + mockTime.EXPECT().Sleep(gomock.Any()).AnyTimes().Do(func(_ interface{}) { + timesCalled++ + if timesCalled == callsExpected { + mTask.SetSentStatus(api.TaskStopped) + } else if timesCalled > callsExpected { + t.Errorf("Sleep called too many times, called %d but expected %d", timesCalled, callsExpected) + } + }) + assert.Equal(t, api.TaskRunning, mTask.GetSentStatus()) + + // Expectations to verify that the task gets removed + mockState.EXPECT().ContainerMapByArn(mTask.Arn).Return(map[string]*api.DockerContainer{container.Name: dockerContainer}, true) + mockClient.EXPECT().RemoveContainer(dockerContainer.DockerName, gomock.Any()).Return(nil) + mockImageManager.EXPECT().RemoveContainerReferenceFromImageState(container).Return(nil) + mockState.EXPECT().RemoveTask(mTask.Task) + mTask.cleanupTask(taskStoppedDuration) + assert.Equal(t, api.TaskStopped, mTask.GetSentStatus()) +} + +func TestCleanupTaskGivesUpIfWaitingTooLong(t *testing.T) { + ctrl := gomock.NewController(t) + mockTime := mock_ttime.NewMockTime(ctrl) + mockState := mock_dockerstate.NewMockTaskEngineState(ctrl) + mockClient := NewMockDockerClient(ctrl) + mockImageManager := NewMockImageManager(ctrl) + defer ctrl.Finish() + + taskEngine := &DockerTaskEngine{ + saver: statemanager.NewNoopStateManager(), + state: mockState, + client: mockClient, + imageManager: mockImageManager, + } + mTask := &managedTask{ + Task: testdata.LoadTask("sleep5"), + _time: mockTime, + engine: taskEngine, + acsMessages: make(chan acsTransition), + dockerMessages: make(chan dockerContainerChange), + } + mTask.SetKnownStatus(api.TaskStopped) + mTask.SetSentStatus(api.TaskRunning) + + // Expectations for triggering cleanup + now := mTask.GetKnownStatusTime() + taskStoppedDuration := 1 * time.Minute + mockTime.EXPECT().Now().Return(now).AnyTimes() + cleanupTimeTrigger := make(chan time.Time) + mockTime.EXPECT().After(gomock.Any()).Return(cleanupTimeTrigger) + go func() { + cleanupTimeTrigger <- now + }() + _maxStoppedWaitTimes = 10 + defer func() { + // reset + _maxStoppedWaitTimes = int(maxStoppedWaitTimes) + }() + mockTime.EXPECT().Sleep(gomock.Any()).Times(_maxStoppedWaitTimes) + assert.Equal(t, api.TaskRunning, mTask.GetSentStatus()) + + // No cleanup expected + mTask.cleanupTask(taskStoppedDuration) + assert.Equal(t, api.TaskRunning, mTask.GetSentStatus()) +} diff --git a/agent/stats/engine_integ_test.go b/agent/stats/engine_integ_test.go index e891d8e10df..02c7929bfb6 100644 --- a/agent/stats/engine_integ_test.go +++ b/agent/stats/engine_integ_test.go @@ -246,7 +246,7 @@ func TestStatsEngineWithNewContainers(t *testing.T) { func TestStatsEngineWithDockerTaskEngine(t *testing.T) { containerChangeEventStream := eventStream("TestStatsEngineWithDockerTaskEngine") - taskEngine := ecsengine.NewTaskEngine(&config.Config{}, nil, nil, containerChangeEventStream, nil, dockerstate.NewDockerTaskEngineState()) + taskEngine := ecsengine.NewTaskEngine(&config.Config{}, nil, nil, containerChangeEventStream, nil, dockerstate.NewTaskEngineState()) container, err := createGremlin(client) if err != nil { t.Fatalf("Error creating container: %v", err) @@ -376,7 +376,7 @@ func TestStatsEngineWithDockerTaskEngine(t *testing.T) { func TestStatsEngineWithDockerTaskEngineMissingRemoveEvent(t *testing.T) { containerChangeEventStream := eventStream("TestStatsEngineWithDockerTaskEngine") - taskEngine := ecsengine.NewTaskEngine(&config.Config{}, nil, nil, containerChangeEventStream, nil, dockerstate.NewDockerTaskEngineState()) + taskEngine := ecsengine.NewTaskEngine(&config.Config{}, nil, nil, containerChangeEventStream, nil, dockerstate.NewTaskEngineState()) container, err := createGremlin(client) if err != nil { From 3d154daca403f7a0566164a197b670ca6bbcd12e Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Thu, 9 Feb 2017 18:11:31 -0800 Subject: [PATCH 6/9] engine: Abort tasks with corrupted internal state --- agent/engine/docker_task_engine.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/agent/engine/docker_task_engine.go b/agent/engine/docker_task_engine.go index 9bee45eb740..c050a708390 100644 --- a/agent/engine/docker_task_engine.go +++ b/agent/engine/docker_task_engine.go @@ -648,11 +648,7 @@ func (engine *DockerTaskEngine) removeContainer(task *api.Task, container *api.C func (engine *DockerTaskEngine) updateTask(task *api.Task, update *api.Task) { managedTask, ok := engine.managedTasks[task.Arn] if !ok { - log.Crit("ACS message for a task we thought we managed, but don't!", "arn", task.Arn) - // Is this the right thing to do? - // Calling startTask should overwrite our bad 'state' data with the new - // task which we do manage.. but this is still scary and shouldn't have happened - engine.startTask(update) + log.Crit("ACS message for a task we thought we managed, but don't! Aborting.", "arn", task.Arn) return } // Keep the lock because sequence numbers cannot be correct unless they are From 4477e305d224709f3718b7960adb93f028d43dc9 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Fri, 10 Feb 2017 12:10:40 -0800 Subject: [PATCH 7/9] engine: Extract wait logic for stop reporting --- agent/engine/task_manager.go | 54 +++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/agent/engine/task_manager.go b/agent/engine/task_manager.go index b75d76ca1f5..7167801e0e0 100644 --- a/agent/engine/task_manager.go +++ b/agent/engine/task_manager.go @@ -476,9 +476,6 @@ func (mtask *managedTask) time() ttime.Time { return mtask._time } -var _stoppedSentWaitInterval = stoppedSentWaitInterval -var _maxStoppedWaitTimes = int(maxStoppedWaitTimes) - func (mtask *managedTask) cleanupTask(taskStoppedDuration time.Duration) { cleanupTimeDuration := mtask.GetKnownStatusTime().Add(taskStoppedDuration).Sub(ttime.Now()) // There is a potential deadlock here if cleanupTime is negative. Ignore the computed @@ -497,27 +494,10 @@ func (mtask *managedTask) cleanupTask(taskStoppedDuration time.Duration) { // wait for the cleanup time to elapse, signalled by cleanupTimeBool for !mtask.waitEvent(cleanupTimeBool) { } - stoppedSentBool := make(chan bool) - taskStopped := false - go func() { - for i := 0; i < _maxStoppedWaitTimes; i++ { - // ensure that we block until api.TaskStopped is actually sent - sentStatus := mtask.GetSentStatus() - if sentStatus >= api.TaskStopped { - stoppedSentBool <- true - taskStopped = true - close(stoppedSentBool) - return - } - seelog.Warnf("Blocking cleanup for task %v until the task has been reported stopped. SentStatus: %v (%d/%d)", mtask, sentStatus, i+1, _maxStoppedWaitTimes) - mtask._time.Sleep(_stoppedSentWaitInterval) - } - stoppedSentBool <- true - }() + // wait for api.TaskStopped to be sent - for !mtask.waitEvent(stoppedSentBool) { - } - if !taskStopped { + ok := mtask.waitForStopReported() + if !ok{ seelog.Errorf("Aborting cleanup for task %v as it is not reported stopped. SentStatus: %v", mtask, mtask.GetSentStatus()) return } @@ -573,3 +553,31 @@ func (mtask *managedTask) discardPendingMessages() { } } } + +var _stoppedSentWaitInterval = stoppedSentWaitInterval +var _maxStoppedWaitTimes = int(maxStoppedWaitTimes) + +// waitForStopReported will wait for the task to be reported stopped and return true, or will time-out and return false. +// Messages on the mtask.dockerMessages and mtask.acsMessages channels will be handled while this function is waiting. +func (mtask *managedTask) waitForStopReported() bool { + stoppedSentBool := make(chan bool) + taskStopped := false + go func() { + for i := 0; i < _maxStoppedWaitTimes; i++ { + // ensure that we block until api.TaskStopped is actually sent + sentStatus := mtask.GetSentStatus() + if sentStatus >= api.TaskStopped { + taskStopped = true + break + } + seelog.Warnf("Blocking cleanup for task %v until the task has been reported stopped. SentStatus: %v (%d/%d)", mtask, sentStatus, i+1, _maxStoppedWaitTimes) + mtask._time.Sleep(_stoppedSentWaitInterval) + } + stoppedSentBool <- true + close(stoppedSentBool) + }() + // wait for api.TaskStopped to be sent + for !mtask.waitEvent(stoppedSentBool) { + } + return taskStopped +} \ No newline at end of file From d113ab416fb3b1ae937e5b8f7da02c480e0f8504 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Fri, 10 Feb 2017 12:28:27 -0800 Subject: [PATCH 8/9] Update copyright on modified files --- agent/api/container.go | 2 +- agent/api/container_test.go | 2 +- agent/api/port_binding.go | 2 +- agent/api/port_binding_test.go | 2 +- agent/api/task.go | 2 +- agent/api/task_test.go | 2 +- agent/api/testutils/container_equal.go | 2 +- agent/api/testutils/container_equal_test.go | 2 +- agent/api/types.go | 2 +- agent/engine/default.go | 2 +- agent/engine/docker_image_manager.go | 2 +- agent/engine/docker_image_manager_integ_test.go | 2 +- agent/engine/docker_image_manager_test.go | 2 +- agent/engine/dockerauth/ecr.go | 2 +- agent/engine/dockerauth/ecr_test.go | 2 +- agent/engine/dockerstate/docker_task_engine_state.go | 2 +- agent/engine/dockerstate/dockerstate_test.go | 2 +- agent/engine/dockerstate/json.go | 2 +- agent/engine/dockerstate/testutils/docker_state_equal.go | 2 +- agent/engine/dockerstate/testutils/json_test.go | 2 +- agent/engine/engine_unix_integ_test.go | 2 +- agent/engine/engine_windows_integ_test.go | 2 +- agent/eventhandler/handler_test.go | 2 +- agent/eventhandler/task_handler.go | 2 +- agent/eventhandler/task_handler_types.go | 2 +- agent/handlers/types.go | 2 +- agent/handlers/v1_handlers.go | 2 +- agent/handlers/v1_handlers_test.go | 2 +- agent/statemanager/state_manager_test.go | 2 +- agent/statemanager/state_manager_unix_test.go | 2 +- agent/stats/container_test.go | 2 +- agent/stats/engine_integ_test.go | 2 +- 32 files changed, 32 insertions(+), 32 deletions(-) diff --git a/agent/api/container.go b/agent/api/container.go index 6907cdcc149..47102e78d29 100644 --- a/agent/api/container.go +++ b/agent/api/container.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/api/container_test.go b/agent/api/container_test.go index 68387fe6c84..f453a937a09 100644 --- a/agent/api/container_test.go +++ b/agent/api/container_test.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/api/port_binding.go b/agent/api/port_binding.go index 29da63e115e..66a0e3da192 100644 --- a/agent/api/port_binding.go +++ b/agent/api/port_binding.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/api/port_binding_test.go b/agent/api/port_binding_test.go index 6da97b252b8..305477892b0 100644 --- a/agent/api/port_binding_test.go +++ b/agent/api/port_binding_test.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/api/task.go b/agent/api/task.go index b5d93bd0091..d420cf530c0 100644 --- a/agent/api/task.go +++ b/agent/api/task.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/api/task_test.go b/agent/api/task_test.go index 32c3007a26f..875eccc9810 100644 --- a/agent/api/task_test.go +++ b/agent/api/task_test.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/api/testutils/container_equal.go b/agent/api/testutils/container_equal.go index aeea7e3344e..b11ef9f8a13 100644 --- a/agent/api/testutils/container_equal.go +++ b/agent/api/testutils/container_equal.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/api/testutils/container_equal_test.go b/agent/api/testutils/container_equal_test.go index dd2cfa690c3..08f065674fe 100644 --- a/agent/api/testutils/container_equal_test.go +++ b/agent/api/testutils/container_equal_test.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/api/types.go b/agent/api/types.go index 0a29d8f2924..897ccc77163 100644 --- a/agent/api/types.go +++ b/agent/api/types.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/default.go b/agent/engine/default.go index fa3f00f87c6..6dbf4a4b00f 100644 --- a/agent/engine/default.go +++ b/agent/engine/default.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/docker_image_manager.go b/agent/engine/docker_image_manager.go index e9dad1e9969..1e572fe25fd 100644 --- a/agent/engine/docker_image_manager.go +++ b/agent/engine/docker_image_manager.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/docker_image_manager_integ_test.go b/agent/engine/docker_image_manager_integ_test.go index 3e2df01aa0a..46c7ffc2c2e 100644 --- a/agent/engine/docker_image_manager_integ_test.go +++ b/agent/engine/docker_image_manager_integ_test.go @@ -1,5 +1,5 @@ // +build integration -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/docker_image_manager_test.go b/agent/engine/docker_image_manager_test.go index faa5a8f81a6..f05789b7b1c 100644 --- a/agent/engine/docker_image_manager_test.go +++ b/agent/engine/docker_image_manager_test.go @@ -1,5 +1,5 @@ // +build !integration -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/dockerauth/ecr.go b/agent/engine/dockerauth/ecr.go index a144e99798c..005a65469a3 100644 --- a/agent/engine/dockerauth/ecr.go +++ b/agent/engine/dockerauth/ecr.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/dockerauth/ecr_test.go b/agent/engine/dockerauth/ecr_test.go index f47c2e7430f..9a1d3c0092c 100644 --- a/agent/engine/dockerauth/ecr_test.go +++ b/agent/engine/dockerauth/ecr_test.go @@ -1,5 +1,5 @@ // +build !integration -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/dockerstate/docker_task_engine_state.go b/agent/engine/dockerstate/docker_task_engine_state.go index cb24eb07ff0..4fe7a42cc7b 100644 --- a/agent/engine/dockerstate/docker_task_engine_state.go +++ b/agent/engine/dockerstate/docker_task_engine_state.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/dockerstate/dockerstate_test.go b/agent/engine/dockerstate/dockerstate_test.go index 581c598bfb2..84f8cbc3830 100644 --- a/agent/engine/dockerstate/dockerstate_test.go +++ b/agent/engine/dockerstate/dockerstate_test.go @@ -1,5 +1,5 @@ // +build !integration -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/dockerstate/json.go b/agent/engine/dockerstate/json.go index 9a618f2f926..926e6d87416 100644 --- a/agent/engine/dockerstate/json.go +++ b/agent/engine/dockerstate/json.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/dockerstate/testutils/docker_state_equal.go b/agent/engine/dockerstate/testutils/docker_state_equal.go index 063376c356c..e0227dd8105 100644 --- a/agent/engine/dockerstate/testutils/docker_state_equal.go +++ b/agent/engine/dockerstate/testutils/docker_state_equal.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/dockerstate/testutils/json_test.go b/agent/engine/dockerstate/testutils/json_test.go index 811dd6e4876..1b3d3838aad 100644 --- a/agent/engine/dockerstate/testutils/json_test.go +++ b/agent/engine/dockerstate/testutils/json_test.go @@ -1,5 +1,5 @@ // +build !integration -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/engine_unix_integ_test.go b/agent/engine/engine_unix_integ_test.go index 636bdf55558..eb2ef9f7a85 100644 --- a/agent/engine/engine_unix_integ_test.go +++ b/agent/engine/engine_unix_integ_test.go @@ -1,6 +1,6 @@ // +build !windows,integration -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/engine/engine_windows_integ_test.go b/agent/engine/engine_windows_integ_test.go index 193565790d9..382813cab72 100644 --- a/agent/engine/engine_windows_integ_test.go +++ b/agent/engine/engine_windows_integ_test.go @@ -1,6 +1,6 @@ // +build windows,integration -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/eventhandler/handler_test.go b/agent/eventhandler/handler_test.go index 3a27e0cb625..818550629b2 100644 --- a/agent/eventhandler/handler_test.go +++ b/agent/eventhandler/handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/eventhandler/task_handler.go b/agent/eventhandler/task_handler.go index 9ef3fac05bb..3b620868c1b 100644 --- a/agent/eventhandler/task_handler.go +++ b/agent/eventhandler/task_handler.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/eventhandler/task_handler_types.go b/agent/eventhandler/task_handler_types.go index f6119c7a688..0b915f5c150 100644 --- a/agent/eventhandler/task_handler_types.go +++ b/agent/eventhandler/task_handler_types.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/handlers/types.go b/agent/handlers/types.go index b8509008d87..7a8a07f15c8 100644 --- a/agent/handlers/types.go +++ b/agent/handlers/types.go @@ -1,4 +1,4 @@ -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/handlers/v1_handlers.go b/agent/handlers/v1_handlers.go index d6cfcb96bdd..0712bc39f34 100644 --- a/agent/handlers/v1_handlers.go +++ b/agent/handlers/v1_handlers.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/handlers/v1_handlers_test.go b/agent/handlers/v1_handlers_test.go index 2180796139a..535df1da549 100644 --- a/agent/handlers/v1_handlers_test.go +++ b/agent/handlers/v1_handlers_test.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/statemanager/state_manager_test.go b/agent/statemanager/state_manager_test.go index b34a7a789e8..28e27006f9d 100644 --- a/agent/statemanager/state_manager_test.go +++ b/agent/statemanager/state_manager_test.go @@ -1,4 +1,4 @@ -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/statemanager/state_manager_unix_test.go b/agent/statemanager/state_manager_unix_test.go index 5d72bed5c5f..13f1be2eac1 100644 --- a/agent/statemanager/state_manager_unix_test.go +++ b/agent/statemanager/state_manager_unix_test.go @@ -1,6 +1,6 @@ // +build !windows -// Copyright 2014-2015 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/stats/container_test.go b/agent/stats/container_test.go index 28271537547..5b092ff3830 100644 --- a/agent/stats/container_test.go +++ b/agent/stats/container_test.go @@ -1,5 +1,5 @@ //+build !integration -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the diff --git a/agent/stats/engine_integ_test.go b/agent/stats/engine_integ_test.go index 02c7929bfb6..ca4fc0c8dc6 100644 --- a/agent/stats/engine_integ_test.go +++ b/agent/stats/engine_integ_test.go @@ -1,6 +1,6 @@ //+build !windows,integration // Disabled on Windows until Stats are actually supported -// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the From 61371b469c0bb5eb7bda07684e942dbfbc9d2d2b Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Fri, 10 Feb 2017 15:25:49 -0800 Subject: [PATCH 9/9] engine: Simplify code in mtask.cleanupTask --- agent/engine/dockerstate/docker_task_engine_state.go | 2 +- agent/engine/task_manager.go | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/agent/engine/dockerstate/docker_task_engine_state.go b/agent/engine/dockerstate/docker_task_engine_state.go index 4fe7a42cc7b..199289aed75 100644 --- a/agent/engine/dockerstate/docker_task_engine_state.go +++ b/agent/engine/dockerstate/docker_task_engine_state.go @@ -225,7 +225,7 @@ func (state *DockerTaskEngineState) AddImageState(imageState *image.ImageState) } // RemoveTask removes a task from this state. It removes all containers and -// other associated metadata. It does aquire the write lock. +// other associated metadata. It does acquire the write lock. func (state *DockerTaskEngineState) RemoveTask(task *api.Task) { state.lock.Lock() defer state.lock.Unlock() diff --git a/agent/engine/task_manager.go b/agent/engine/task_manager.go index 7167801e0e0..f3e5287c136 100644 --- a/agent/engine/task_manager.go +++ b/agent/engine/task_manager.go @@ -507,16 +507,12 @@ func (mtask *managedTask) cleanupTask(taskStoppedDuration time.Duration) { // For the duration of this, simply discard any task events; this ensures the // speedy processing of other events for other tasks handleCleanupDone := make(chan struct{}) - go func() { - mtask.engine.sweepTask(mtask.Task) - mtask.engine.state.RemoveTask(mtask.Task) - handleCleanupDone <- struct{}{} - }() // discard events while the task is being removed from engine state - mtask.discardEventsUntil(handleCleanupDone) + go mtask.discardEventsUntil(handleCleanupDone) + mtask.engine.sweepTask(mtask.Task) + mtask.engine.state.RemoveTask(mtask.Task) log.Debug("Finished removing task data; removing from state no longer managing", "task", mtask.Task) // Now remove ourselves from the global state and cleanup channels - go mtask.discardEventsUntil(handleCleanupDone) // keep discarding events until the task is fully gone mtask.engine.processTasks.Lock() delete(mtask.engine.managedTasks, mtask.Arn) handleCleanupDone <- struct{}{}