Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MANIFEST_PULLED internal container and task states #4137

Merged
merged 3 commits into from
Apr 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion agent/api/container/container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ func TestIsKnownSteadyState(t *testing.T) {
func TestGetNextStateProgression(t *testing.T) {
// This creates a container with `iota` ContainerStatus (NONE)
container := &Container{}
// NONE should transition to PULLED
// NONE should transition to MANIFEST_PULLED
assert.Equal(t, container.GetNextKnownStateProgression(), apicontainerstatus.ContainerManifestPulled)
container.SetKnownStatus(apicontainerstatus.ContainerManifestPulled)
// MANIFEST_PULLED should transition to PULLED
assert.Equal(t, container.GetNextKnownStateProgression(), apicontainerstatus.ContainerPulled)
container.SetKnownStatus(apicontainerstatus.ContainerPulled)
// PULLED should transition to CREATED
Expand Down
5 changes: 3 additions & 2 deletions agent/api/task/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +1052,7 @@ func (task *Task) initializeASMAuthResource(credentialsManager credentials.Manag
if container.ShouldPullWithASMAuth() {
container.BuildResourceDependency(asmAuthResource.GetName(),
resourcestatus.ResourceStatus(asmauth.ASMAuthStatusCreated),
apicontainerstatus.ContainerPulled)
apicontainerstatus.ContainerManifestPulled)
amogh09 marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -3682,7 +3682,8 @@ func (task *Task) ToHostResources() map[string]*ecs.Resource {
func (task *Task) HasActiveContainers() bool {
for _, container := range task.Containers {
containerStatus := container.GetKnownStatus()
if containerStatus >= apicontainerstatus.ContainerPulled && containerStatus <= apicontainerstatus.ContainerResourcesProvisioned {
if containerStatus >= apicontainerstatus.ContainerManifestPulled &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this if-statement seems like it would make more sense to be containerStatus > ContainerStatusNone && containerStatus < ContainerStatusStopped

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah with current states both are equivalent but I get your point. It's a bit out of scope of this PR so I'd rather not change that logic here.

containerStatus <= apicontainerstatus.ContainerResourcesProvisioned {
return true
}
}
Expand Down
10 changes: 9 additions & 1 deletion agent/api/task/task_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2843,7 +2843,15 @@ func TestPostUnmarshalTaskASMDockerAuth(t *testing.T) {
}

err := task.PostUnmarshalTask(cfg, credentialsManager, resFields, nil, nil)
assert.NoError(t, err)
require.NoError(t, err)
assert.Equal(t,
apicontainer.ResourceDependency{
Name: asmauth.ResourceName,
RequiredStatus: resourcestatus.ResourceStatus(asmauth.ASMAuthStatusCreated),
},
task.Containers[0].
TransitionDependenciesMap[apicontainerstatus.ContainerManifestPulled].
ResourceDependencies[0])
}

func TestPostUnmarshalTaskSecret(t *testing.T) {
Expand Down
6 changes: 6 additions & 0 deletions agent/engine/dependencygraph/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,12 @@ func containerOrderingDependenciesIsResolved(target *apicontainer.Container,
targetContainerKnownStatus := target.GetKnownStatus()
dependsOnContainerKnownStatus := dependsOnContainer.GetKnownStatus()

// Containers are allowed to transition to MANIFEST_PULLED irrespective of any container
// ordering dependencies.
if targetContainerKnownStatus < apicontainerstatus.ContainerManifestPulled {
return true
}

// The 'target' container desires to be moved to 'Created' or the 'steady' state.
// Allow this only if the environment variable ECS_PULL_DEPENDENT_CONTAINERS_UPFRONT is enabled and
// known status of the `target` container state has not reached to 'Pulled' state;
Expand Down
273 changes: 253 additions & 20 deletions agent/engine/dependencygraph/graph_test.go

Large diffs are not rendered by default.

19 changes: 13 additions & 6 deletions agent/engine/dependencygraph/graph_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,19 @@ func TestVerifyCgroupDependenciesResolved(t *testing.T) {
ExpectedResolved bool
}{
{
Name: "resource none,container pull depends on resource created",
TargetKnown: apicontainerstatus.ContainerStatusNone,
TargetDep: apicontainerstatus.ContainerPulled,
DependencyKnown: resourcestatus.ResourceStatus(cgroup.CgroupStatusNone),
RequiredStatus: resourcestatus.ResourceStatus(cgroup.CgroupCreated),

Name: "resource none, container pull depends on resource created",
TargetKnown: apicontainerstatus.ContainerStatusNone,
TargetDep: apicontainerstatus.ContainerPulled,
DependencyKnown: resourcestatus.ResourceStatus(cgroup.CgroupStatusNone),
RequiredStatus: resourcestatus.ResourceStatus(cgroup.CgroupCreated),
ExpectedResolved: true,
},
{
Name: "resource none, current is manifest_pulled, container pull depends on resource created",
TargetKnown: apicontainerstatus.ContainerManifestPulled,
TargetDep: apicontainerstatus.ContainerPulled,
DependencyKnown: resourcestatus.ResourceStatus(cgroup.CgroupStatusNone),
RequiredStatus: resourcestatus.ResourceStatus(cgroup.CgroupCreated),
ExpectedResolved: false,
},
{
Expand Down
14 changes: 14 additions & 0 deletions agent/engine/docker_task_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ func (engine *DockerTaskEngine) reconcileHostResources() {

func (engine *DockerTaskEngine) initializeContainerStatusToTransitionFunction() {
containerStatusToTransitionFunction := map[apicontainerstatus.ContainerStatus]transitionApplyFunc{
apicontainerstatus.ContainerManifestPulled: engine.pullContainerManifest,
apicontainerstatus.ContainerPulled: engine.pullContainer,
apicontainerstatus.ContainerCreated: engine.createContainer,
apicontainerstatus.ContainerRunning: engine.startContainer,
Expand Down Expand Up @@ -1231,6 +1232,19 @@ func (engine *DockerTaskEngine) GetDaemonManagers() map[string]dm.DaemonManager
return engine.daemonManagers
}

// Pulls the manifest of the container image.
func (engine *DockerTaskEngine) pullContainerManifest(
task *apitask.Task, container *apicontainer.Container,
) dockerapi.DockerContainerMetadata {
// Currently a no-op
logger.Debug("Manifest pull is currently a no-op", logger.Fields{
field.TaskID: task.GetID(),
field.Container: container.Name,
field.Image: container.Image,
})
return dockerapi.DockerContainerMetadata{}
}

func (engine *DockerTaskEngine) pullContainer(task *apitask.Task, container *apicontainer.Container) dockerapi.DockerContainerMetadata {
switch container.Type {
case apicontainer.ContainerCNIPause, apicontainer.ContainerNamespacePause, apicontainer.ContainerServiceConnectRelay, apicontainer.ContainerManagedDaemon:
Expand Down
17 changes: 10 additions & 7 deletions agent/engine/task_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -620,8 +620,9 @@ func (mtask *managedTask) emitTaskEvent(task *apitask.Task, reason string) {
}
if !taskKnownStatus.BackendRecognized() {
logger.Debug("Skipping event emission for task", logger.Fields{
field.TaskID: mtask.GetID(),
field.Error: "status not recognized by ECS",
field.TaskID: mtask.GetID(),
field.Error: "status not recognized by ECS",
field.KnownStatus: taskKnownStatus.String(),
})
return
}
Expand Down Expand Up @@ -862,34 +863,36 @@ func (mtask *managedTask) handleEventError(containerChange dockerContainerChange
switch event.Status {
// event.Status is the desired container transition from container's known status
// (* -> event.Status)
case apicontainerstatus.ContainerPulled:
case apicontainerstatus.ContainerManifestPulled, apicontainerstatus.ContainerPulled:
// If the agent pull behavior is always or once, we receive the error because
// the image pull fails, the task should fail. If we don't fail task here,
// the image or manifest pull fails, the task should fail. If we don't fail task here,
// then the cached image will probably be used for creating container, and we
// don't want to use cached image for both cases.
if mtask.cfg.ImagePullBehavior == config.ImagePullAlwaysBehavior ||
mtask.cfg.ImagePullBehavior == config.ImagePullOnceBehavior {
logger.Error("Error while pulling image; moving task to STOPPED", logger.Fields{
logger.Error("Error while pulling image or its manifest; moving task to STOPPED", logger.Fields{
field.TaskID: mtask.GetID(),
field.Image: container.Image,
field.Container: container.Name,
field.Error: event.Error,
field.Status: event.Status,
})
// The task should be stopped regardless of whether this container is
// essential or non-essential.
mtask.SetDesiredStatus(apitaskstatus.TaskStopped)
return false
}
// If the agent pull behavior is prefer-cached, we receive the error because
// the image pull fails and there is no cached image in local, we don't make
// the image or manifest pull fails and there is no cached image in local, we don't make
// the task fail here, will let create container handle it instead.
// If the agent pull behavior is default, use local image cache directly,
// assuming it exists.
logger.Error("Error while pulling image; will try to run anyway", logger.Fields{
logger.Error("Error while pulling image or its manifest; will try to run anyway", logger.Fields{
field.TaskID: mtask.GetID(),
field.Image: container.Image,
field.Container: container.Name,
field.Error: event.Error,
field.Status: event.Status,
})
// proceed anyway
return true
Expand Down
Loading
Loading