Skip to content

fix(aws-ecs): close the real-cloud parity gaps in #1449 - #1528

Open
thzgajendra wants to merge 3 commits into
stackshy:developmentfrom
thzgajendra:fix/aws-ecs-parity-1449
Open

thzgajendra wants to merge 3 commits into
stackshy:developmentfrom
thzgajendra:fix/aws-ecs-parity-1449

Conversation

@thzgajendra

@thzgajendra thzgajendra commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • AWS ECS in cloudemu still differed from real ECS in the ways listed in AWS ECS: open real-cloud parity gaps #1449: some operations were missing, some accepted calls AWS rejects, and ECS published too few events and no metrics.
  • This PR fixes every item of AWS ECS: open real-cloud parity gaps #1449 that was still reproducible on development: five items were already fixed there and only get regression tests (see below).
  • The work is one PR in separate areas (validation fixes, exec and network details, task protection, task sets, deployments and namespaces, events, metrics) so each can be reviewed on its own.

Changes

The problem (in plain words)

Someone running ECS workflows against cloudemu (Terraform, CI scripts, EventBridge-driven automation) saw behaviour that real ECS does not have:

  • aws ecs execute-command worked on any task, even one started without enableExecuteCommand, or already stopped.
  • create-cluster on an existing name failed, run-task --launch-type BOGUS returned an empty result instead of an error, assignPublicIp came back empty, and a second stop-task replaced the stop reason.
  • Task sets, task protection, service deployments and list-services-by-namespace did not exist (unknown ECS operation).
  • EventBridge only got "RUNNING" and "STOPPED" task events and one service event; there were no deployment or container-instance events, and CloudWatch had no ECS metrics.

How this fixes it

Rules live in the provider (providers/aws/ecs), so the Go library and cloudemu serve behave the same; the wire handler only translates. New operations are small optional interfaces (TaskSets, TaskProtection, ServiceDeployments, ServiceNamespaces) next to driver.ECS, found by type assertion and also exposed through the portable services/ecs wrapper.

  • Validation: idempotent CreateCluster; assignPublicIp defaults to DISABLED and is validated; launchType must be one of the SDK enum values; a repeated StopTask keeps the first reason; duplicate CreateService answers "Creation of service was not idempotent."; ExecuteCommand needs enableExecuteCommand, a RUNNING task, an interactive call and an existing container. Tasks carry enableExecuteCommand, containers report managedAgents and networkInterfaces.
  • Task protection: GetTaskProtection / UpdateTaskProtection (1-2880 minutes, default 120, service tasks only, MISSING / TASK_NOT_VALID / DEPLOYMENT_BLOCKED). Protected tasks survive a scale-in or redeploy.
  • Task sets: create / update / delete / describe / update-primary for EXTERNAL services. A set runs its own tasks, computedDesiredCount is the service desired count x scale rounded up, and one set is PRIMARY. Deleting a set drains it and stops its tasks (force is accepted and changes nothing, since scale-down is immediate). UpdateService on an EXTERNAL service rejects a different task definition, platform version, network configuration, load balancers, capacity providers or service registries (create a new task set instead), and the service's runningCount and pendingCount come from its task sets.
  • Deployments: every create, force-new-deployment or deploying update records a service deployment and revision; list / describe / describe-revisions / stop. ListServicesByNamespace works through serviceConnectConfiguration.namespace.
  • Events: one EventBridge event per task lifecycle state (PROVISIONING for awsvpc, PENDING, ACTIVATING, RUNNING, DEACTIVATING, STOPPING, DEPROVISIONING for awsvpc, STOPPED) with detail.version +1 each; ECS Deployment State Change; ECS Container Instance State Change; more Service Action events. Events are published after all emulator locks are released (a test re-enters ECS from the event consumer at every emit site).
  • Metrics: ECS/ContainerInsights counts (clusters with containerInsights enabled or enhanced, or enabled on the account) and AWS/ECS CPUReservation, MemoryReservation, LiveTaskCount. No usage-driven series are fabricated.
  • State: the new stores are in the snapshot, so --persist, snapshot / rewind / fork carry them; state written by the previous binary loads (a service without deployment records gets its current one).

Already fixed on development (regression tests only, no code change): capacity provider CRUD, ListTagsForResource for a cluster named *cluster*, TagResource on an unknown ARN, containerArn on task containers, include=TAGS on Describe*.

Diagram

flowchart LR
  CLI[aws CLI / SDK / Terraform] --> W[server/aws/ecs wire handler]
  W -->|decode, map errors| P[providers/aws/ecs mock]
  P --> S[(memstore: tasks, services,<br/>task sets, deployments, revisions)]
  P -->|task / deployment / instance events| EB[EventBridge default bus]
  P -->|counts, reservation| CW[CloudWatch]
  P -->|snapshot| PS[--persist / snapshots]
Loading

Provider Coverage

  • AWS

Azure, GCP and OCI are left out on purpose: ECS exists only on AWS, so there is nothing to mirror.

Checklist

  • All tests pass (go test ./...)
  • Linter passes (golangci-lint run --timeout=9m ./...) - 0 issues on the changed lines
  • Every provider the change applies to implements the same behavior (AWS only)

Integration tests are not in cloudemu_test.go: as in recent service PRs they sit beside the service (provider, SDK round-trip and EventBridge/CloudWatch tests); happy to move any if asked.

  • Unit tests added to provider test files
  • Regenerated docs (go generate ./... for coverage; go run ./internal/compatgen for the compat matrix) and committed the result

Test Plan

How I tested it

  • Real E2E, the way a user runs it (fake credentials only): the AWS CLI v2.30 (48 checks) and boto3 1.43 (13 checks) against a -race cloudemu serve; the image from docker build + docker run with default flags; --async-settle; --persist --state-file where a state file written by the development binary is loaded by this branch; --enforce-auth with a restricted IAM user (new operations are authorized per operation, denied ones return AccessDenied); 50 parallel callers (same-name create-cluster, same-token create-task-set, protection and stop-task races); Terraform 1.9 with the aws provider (aws_ecs_cluster, task definition, Fargate service, EXTERNAL service, aws_ecs_task_set): apply, an in-place desired_count change on the EXTERNAL service, plan shows "No changes", destroy (the task set is destroyed without force_delete). No DATA RACE in any server log.
  • Found by that E2E and fixed here: CreateService stored its in-progress service to claim the name and kept changing it, while a parallel request read it for the new metrics. It now claims the name with a copy; TestMetrics_ConcurrentCreateServices reports 2 races without the fix and none with it.
  • Tests: 95-row matrix, each row names its test (provider tests, real aws-sdk-go-v2 round trips asserting typed exceptions, the services/ecs wrapper, an EventBridge -> SQS test, a CloudWatch round trip, snapshot round trip). The new behaviour tests fail on development (for example the idempotent CreateCluster, assignPublicIp default, launchType and stop-reason tests) and pass here.
  • Gate (local): gofmt, go build -mod=readonly, vet, go test ./..., -race on the touched packages, go mod tidy -diff, golangci-lint on the changed lines, go generate / compatgen with no drift, Structure, contrib builds, CodeQL go-security-extended.

Coverage

package before after
providers/aws/ecs 87.3% 91.5%
server/aws/ecs 80.3% 82.0%
services/ecs 58.9% 67.7%
whole module 73.4% 73.5%

Deliberate gaps and things I could not check

AWS does not document the exact text or semantics for these, and I had no real AWS account, so they are my best reading of the docs and SDK (listed in docs/coverage/nongoals/ecs.md): the task-set error on a non-EXTERNAL service, an omitted stopType meaning ROLLBACK, TASK_NOT_VALID for protecting a stopped task, the wording of the rejected EXTERNAL UpdateService fields, the invalid launchType message, the ExecuteCommand error for a task that is not RUNNING, StopTask on an already-stopped task keeping its reason, and CreateCluster being idempotent. Also not emulated: the SERVICE_DESIRED_COUNT_UPDATED event (it is not published), CPU/memory utilization metrics (no workload runs), the DELETED task state, versionInfo in container-instance events, the circuit breaker (SERVICE_DEPLOYMENT_FAILED), and Cloud Map (an unknown namespace lists nothing). Deployments complete immediately, so StopServiceDeployment only succeeds inside the --async-settle window.

docs/services.md is updated (ECS row, section, totals).

Related Issues

Closes #1449. Every item in it is either fixed here or was already fixed on development (regression tests added), so the boxes can all be ticked.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for taking on the whole of #1449 in one go. Most of it holds up well when driven from the outside: I ran the AWS CLI against cloudemu serve for every new and changed op, captured the emitted events through an EventBridge rule into SQS, read the new metrics back through CloudWatch, did a --persist restart, and ran Terraform (hashicorp/aws 6.68) over cluster, capacity providers, task definition, a Fargate service with Service Connect, an EXTERNAL service and a task set. Exec guards, idempotent CreateCluster, launchType and assignPublicIp validation, the StopTask reason, task protection, deployments, events and metrics all behave as described. A few things need fixing before this can go in, one of which breaks terraform destroy for aws_ecs_task_set.

#1449 coverage

#1449 item Status Evidence
Capacity provider CRUD Already fixed on development, regression test added CLI create/describe/update/delete-capacity-provider: ACTIVE, UPDATE_COMPLETE, INACTIVE
ListTagsForResource misroutes ARNs in clusters named *cluster* Already fixed, regression test added Cluster mycluster-x: service and cluster ARNs each return their own tags
ExecuteCommand without enableExecuteCommand / not RUNNING Fixed Raw wire: not enabled, STOPPED task, non-interactive, unknown container, multi-container without container, unknown cluster all return the expected exception; enabled RUNNING task returns a session with the real containerArn
Task sets (Create/Update/Delete/Describe, UpdateServicePrimaryTaskSet) Partly fixed All five ops work over CLI and Terraform apply/update/plan is clean, but terraform destroy fails (High 1) and the EXTERNAL service reports runningCount: 0 (Medium 3)
TagResource on nonexistent ARN accepted Already fixed, regression test added tag-resource on service/mycluster-x/nope returns InvalidParameterException
CreateCluster duplicate ACTIVE name not idempotent Fixed Second create-cluster c1 returns the existing cluster, original settings kept
containerArn missing on task containers Already fixed, regression test added describe-tasks shows containerArn on every container
assignPublicIp not defaulted to DISABLED Fixed Service, task set and revision report DISABLED; MAYBE is rejected
Intermediate task state events, Deployment State Change, container-instance deregistration events, emits under lock Fixed SQS capture: PROVISIONING through STOPPED with versions 1..8, Deployment State Change IN_PROGRESS/COMPLETED, Container Instance State Change ACTIVE v1..4, DRAINING v5, INACTIVE v6
GetTaskProtection / UpdateTaskProtection Fixed 1..2880 range, default 120 min, TASK_NOT_VALID, MISSING, DEPLOYMENT_BLOCKED, protected tasks survive scale-in
Service deployment ops (List/Describe, DescribeServiceRevisions, Stop/Rollback) Fixed, one gap List/describe/revisions work; ROLLBACK under --async-settle goes ROLLBACK_IN_PROGRESS then ROLLBACK_SUCCESSFUL and reverts the task definition; omitted stopType is rejected (Medium 2)
ListServicesByNamespace Fixed Name and ARN namespaces list the right services, Terraform service_connect_configuration included
RunTask invalid launchType Fixed BOGUS and fargate return InvalidParameterException
Per-container networkInterfaces for awsvpc Fixed attachmentId matches attachments[0].id, privateIpv4Address matches the ENI detail
include=["TAGS"] ignored on Describe* Already fixed, regression test added describe-services omits tags without --include TAGS, returns them with it
StopTask on stopped task overwrites stoppedReason Fixed Second stop with reason "second" still reports "first"
CreateService duplicate message wording Fixed "Creation of service was not idempotent." as InvalidParameterException
No AWS/ECS / ContainerInsights metrics Fixed list-metrics shows CPUReservation, MemoryReservation, LiveTaskCount and the ContainerInsights counts only for the insights-enabled cluster; get-metric-statistics returns values
No ECS EventBridge events Fixed, one divergence All four detail types delivered; manual desired-count changes emit SERVICE_DESIRED_COUNT_UPDATED (Low 1)

Nothing in #1449 is skipped. The only claim that does not hold end to end is the task-set lifecycle under Terraform.

Findings

High

1. terraform destroy of aws_ecs_task_set fails (providers/aws/ecs/task_sets.go:418)

DeleteTaskSet rejects any set whose scale is above 0 unless force is set. The Terraform resource sends Force: false by default (force_delete defaults to false) and expects the call to succeed and the set to drain. Against this branch, destroying a default task set fails:

Error: deleting ECS Task Set (ecs-svc/2163052904951,...): operation error ECS: DeleteTaskSet,
https response error StatusCode: 400, InvalidParameterException:

The provider's own acceptance configs (testAccTaskSetConfig_basic, run against real AWS) create a task set at the default 100% scale and destroy it without force_delete, so real ECS accepts a non-forced delete and drains the set. force only skips waiting for the scale-down. Apply, plan and update were clean; only destroy breaks. Anyone with an EXTERNAL-controller stack in Terraform hits this on every teardown.

Fix: accept the non-forced delete, put the set in DRAINING and stop its tasks the same way the forced path does (the emulator has no gradual drain to wait for). Update TestTaskSet* in task_sets_test.go:248 and task_sets_sdk_test.go:73, which currently assert the rejection, and drop the matching line from docs/coverage/nongoals/ecs.md.

Medium

2. StopServiceDeployment rejects a request without stopType (providers/aws/ecs/service_deployments.go:398, server/aws/ecs/service_deployments.go:257)

StopServiceDeployment marks stopType as optional, and the op's description is the ROLLBACK behavior. aws ecs stop-service-deployment --service-deployment-arn <arn> with no --stop-type gets InvalidParameterException: stopType must be ABORT or ROLLBACK. Because the stopType check runs first, an unknown ARN with no stopType also gets that error instead of ServiceDeploymentNotFoundException. Fix: treat an empty stopType as ROLLBACK and validate it after the ARN lookup.

3. An EXTERNAL-controller service always reports runningCount: 0 (providers/aws/ecs/services.go:973)

With a task set running tasks, describe-services returns the task set (PRIMARY, running 1) but the service itself shows runningCount: 0, pendingCount: 0. Meanwhile LiveTaskCount for the same service reports the tasks, so the emulator contradicts itself. Real ECS counts task-set tasks in the service's running and pending counts. Anything that waits on runningCount == desiredCount (the SDK ServicesStable waiter, CI scripts, an EXTERNAL deployer checking progress) never sees the service converge. Fix: in DescribeServices, fill running/pending for an EXTERNAL service from the sum of its task-set views (you already compute them in describedTaskSets).

4. Generated coverage now says ECS has 5 operations (docs/coverage/aws/ecs.md:4, cause in services/ecs/capabilities.go:77)

After go generate, the ECS page reads "portable interface driver.TaskSets", "Operations (5)", and moves the real 41-op driver.ECS under "Optional capabilities". The top-level tables in docs/coverage/README.md and docs/coverage/aws/README.md, and coverage.json, now list ECS at 5 ops. internal/coveragegen picks the interface the portable package references most in type positions, and the func(c driver.TaskSets) closure parameters in capabilities.go (5 of them) outnumber driver.ECS (struct field plus NewECS). The manifest is what the docs site and llms.txt read, so this under-reports ECS on day one. Fix either side: have referencedInterface count only struct-field types (or prefer the field type of the wrapper struct), or avoid naming the capability interface in closure parameter types in capabilities.go. Then regenerate and check the ECS page says driver.ECS with 41 operations plus the four optional capabilities.

Low

  1. SERVICE_DESIRED_COUNT_UPDATED is emitted for manual updates (providers/aws/ecs/services.go:739, :798). The service action events doc says this event "is not sent when the desired count is manually updated by a user"; it is for scheduler/auto scaling changes. Every update-service --desired-count here emits one, so a rule meant to watch auto scaling fires on user changes.
  2. Deployment completion disagrees with rolloutState when protected tasks block a deployment (providers/aws/ecs/events.go:372, providers/aws/ecs/service_deployments.go:107). With 2 protected tasks and desired 1, force-new-deployment leaves the deployment rolloutState: IN_PROGRESS, yet the emulator publishes SERVICE_DEPLOYMENT_COMPLETED and ListServiceDeployments says SUCCESSFUL, because both use running >= desired while rolloutState now uses ==. Use the same condition in all three.
  3. UpdateService on an EXTERNAL service silently ignores a task definition change (providers/aws/ecs/services.go:773). update-service --task-definition web returns 200 with the old task definition. Real ECS only lets you change desired count, placement, grace period and tag options there and points you at task sets for the rest. Reject taskDefinition, launchType, platformVersion, networkConfiguration and loadBalancers with InvalidParameterException instead of dropping them (network and load balancers are currently applied to the service record).
  4. UpdateTaskProtection accepts STOPPED tasks (providers/aws/ecs/task_protection.go:176). Protecting a task the scheduler already stopped returns protectionEnabled: true. Skip non-running tasks the way protectionBlocked already does.
  5. Container instance version is not on the wire. The driver now keeps ContainerInstance.Version and the events carry it, but describe-container-instances still returns no version, so a consumer cannot correlate events with describes as the ContainerInstance shape intends. The event's attributes is also null rather than [] when there are none.
  6. Launch-path race is pre-existing but this PR widens it (providers/aws/ecs/tasks.go:505). placeFargate stores the task pointer and stampLaunch then writes to it (container ARNs, startedAt, and now EventVersion and ManagedAgents) while ListTasks/DescribeTasks can clone it. A concurrent force-new-deployment plus ListTasks probe reports races under -race on this branch and on development. The new task-set, deployment and protection paths showed no races. Worth moving the Set in placeFargate after stampLaunch while you are in here.
  7. golangci-lint goes from 21 to 30 issues on the touched packages (v2.14.0): +8 goconst (CPU, MEMORY, ClusterName, taskSet, null, failures vs keyFailures) and a new gocyclo on UpdateService (12 > 10). CI does not run golangci-lint; this is the local gate.
  8. Naming. ListServicesByNamespace lives in providers/aws/ecs/namespace.go but in server/aws/ecs/service_deployments.go, so the feature does not share a filename across layers (docs/STRUCTURE.md). parity_helpers_test.go and validation_defaults_test.go are generic names; feature names read better.

Docs-parity divergences

  • DeleteTaskSet without force on a non-zero set: rejected here, accepted and drained by real ECS (API; Terraform aws_ecs_task_set acceptance configs).
  • StopServiceDeployment stopType is optional (API); required here.
  • SERVICE_DESIRED_COUNT_UPDATED is not sent for user updates (events); sent here.
  • Checked and matching: ExecuteCommand errors and "not enabled" message (API); UpdateTaskProtection range, default and failure reasons (API, failure reasons); CreateTaskSet errors, clientToken and launchType enum (API); Deployment State Change and Service Action detail shapes (deployment events); AWS/ECS metric names, units and dimensions including LiveTaskCount (metrics).

What I verified

  • Built and tested the pushed head dc10341 against merge base b68b0c8: build, vet, go test on providers/aws/..., server/aws/ecs, server/aws/eventbridge, server/aws/cloudwatch, services/ecs, internal/settle, persist, cmd/... and server/admin all pass; -race on the touched packages passes; go mod tidy -diff clean; gofmt clean; go generate ./... leaves no diff (the content issue is Medium 4). CI is all green, including Test 1-4, Race, Lint, Contrib (dockerengine, realengine, server, terraform, testcontainers) and Compat matrix.
  • Coverage: providers/aws/ecs 93.8%, server/aws/ecs 81.6%, services/ecs 68.7%.
  • Throwaway -race probe running DescribeServices, DescribeTaskSets, UpdateTaskSet, UpdateService (desired count and force deploy), ListServiceDeployments, UpdateTaskProtection, StopTask and DescribeTasks concurrently: the only races are the pre-existing launch path in Low 6, reproduced on development too.
  • Live cloudemu serve, AWS CLI over every new and changed op including the error cases above; EventBridge rule on aws.ecs into SQS (111 events plus the container-instance run); CloudWatch list-metrics and get-metric-statistics for AWS/ECS and ECS/ContainerInsights.
  • --async-settle: deployment IN_PROGRESS window, StopServiceDeployment ROLLBACK reverting the task definition and settling to ROLLBACK_SUCCESSFUL.
  • --persist --state-file restart: task set id and tags and deployment ARNs identical before and after.
  • Terraform: apply with no waiter hang, plan -detailed-exitcode 0, update of desired count and task-set scale, plan 0 again, destroy fails on the task set (High 1); with force_delete = true destroy completes and cluster, services and task definition read back INACTIVE and DescribeTaskSets returns ServiceNotActiveException.
  • Wiring: SetMonitoring added in providers/aws/aws.go next to the other services, events go through the existing awsevents emitter, new stores are in the ECS snapshot, ECS X-Amz-Target is already covered by the authz prefix. Dockerfile and serve wiring are unchanged, so I did not rebuild the image; the same binary path was exercised through serve.

Verdict

Request changes. High 1 and Mediums 2 to 4 should be fixed before merge; the Lows can go in this PR or a follow-up.

Comment thread providers/aws/ecs/task_sets.go Outdated
return nil, apiErrf(errors.NotFound, excTaskSetNotFound, "The specified task set %q was not found.", in.TaskSet)
}

if !in.Force && stored.Scale.Value > 0 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This breaks terraform destroy for aws_ecs_task_set: the provider sends Force: false by default and gets InvalidParameterException back. Its acceptance configs create a set at the default 100% scale and delete it without force_delete against real AWS, so real ECS accepts a non-forced delete and drains the set; force only skips waiting for the scale-down. Could this go to DRAINING and stop the tasks like the forced path instead of rejecting? The tests at task_sets_test.go:248 and task_sets_sdk_test.go:73 and the nongoals line would need to change too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good catch. DeleteTaskSet no longer needs force: a non-forced delete now drains the set (DRAINING, its tasks stopped) the same way a forced one does, and force is accepted with no visible effect, since scale-down is immediate here (providers/aws/ecs/task_sets.go:401). docs/coverage/nongoals/ecs.md and docs/services.md are updated.
Verified: TestDeleteTaskSet_DrainsWithOrWithoutForce and TestSDK_TaskSets_FullLifecycle fail without the fix. E2E against cloudemu serve with Terraform 1.9: terraform destroy of a default aws_ecs_task_set without force_delete failed with the 400 before and now destroys cleanly (apply, in-place desired_count change, plan exit 0, destroy).

// visible first and a repeated stop continues as-is; a completed deployment is a
// ConflictException.
func (m *Mock) StopServiceDeployment(ctx context.Context, arn, stopType string) (string, error) {
if stopType != driver.StopTypeAbort && stopType != driver.StopTypeRollback {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

stopType is optional in the StopServiceDeployment API (the op describes ROLLBACK), so aws ecs stop-service-deployment --service-deployment-arn <arn> with no --stop-type fails here. Treat empty as ROLLBACK. Since this check runs before the ARN lookup, an unknown ARN without stopType also gets this error instead of ServiceDeploymentNotFoundException.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good catch. An omitted stopType now means ROLLBACK, and the enum check still runs before the lookup, so an unknown ARN without stopType gets ServiceDeploymentNotFoundException (providers/aws/ecs/service_deployments.go:398).
Verified: TestStopServiceDeployment_OmittedStopTypeIsRollback, the extended TestStopServiceDeployment_ConflictAndNotFound and TestSDK_ServiceDeployments fail without the fix. E2E (boto3, --async-settle): stop-service-deployment with no stop type is accepted and rolls back; the unknown ARN returns the not-found exception.

if s, ok := m.resolveService(want, id); ok {
out := cloneService(s)
out.Tags = m.liveTags(s.ARN, s.Tags)
out.TaskSets = m.describedTaskSets(want, s)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For an EXTERNAL service the task sets are filled in, but the service's own runningCount/pendingCount stay 0 even while a set runs tasks (LiveTaskCount for the same service reports them). Real ECS counts task-set tasks in the service counts, and the ServicesStable waiter or any runningCount == desiredCount check never converges here. Summing the task-set views from describedTaskSets would fix it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good catch. An EXTERNAL service now reports runningCount and pendingCount as the sum of its task sets, in DescribeServices and in the UpdateService response (providers/aws/ecs/services.go:1072).
Verified: TestExternalService_CountsFollowTaskSets and the SDK check fail without the fix (running was 0). E2E: a service with a 2-task set reports 2/0 and 3 after a rescale. One thing I could not confirm: the SDK ServicesStable waiter wants exactly one deployment and an EXTERNAL service has none here; I do not know what real ECS reports, so I left that alone.

Comment thread services/ecs/capabilities.go Outdated
//
//nolint:gocritic // in is passed by value to mirror the driver interface; the copy is cheap for a mock.
func (e *ECS) CreateTaskSet(ctx context.Context, in driver.CreateTaskSetInput) (*driver.TaskSet, error) {
return callCapability(ctx, e, "CreateTaskSet", in, func(c driver.TaskSets) (*driver.TaskSet, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The func(c driver.TaskSets) closure parameters (5 of them) make coveragegen's referencedInterface pick TaskSets as the primary interface over driver.ECS (field plus NewECS). The regenerated docs now say ECS has 5 operations with driver.TaskSets as its portable interface, and the 41 real ops are listed as an optional capability. Either count only struct-field types in coveragegen or avoid naming the capability type in a parameter here, then regenerate.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good catch. I replaced the generic callCapability[C, R] with doOne[R] and the call sites now do the capabilityOf[...] type assertion themselves, so no capability interface is named in a closure parameter (services/ecs/capabilities.go:58). Fixing coveragegen to count only struct fields would be a good hardening, but I kept it out of this PR.
Verified: regenerated with go generate ./...; docs/coverage/aws/ecs.md is driver.ECS again with the capabilities listed as optional, and the generated-docs drift check is clean.

Comment thread docs/coverage/aws/ecs.md Outdated
# ECS

AWS's `ecs` service · portable interface `driver.ECS` · [AWS index](./README.md)
AWS's `ecs` service · portable interface `driver.TaskSets` · [AWS index](./README.md)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should still read driver.ECS with 41 operations; see the note on services/ecs/capabilities.go. The README tables and coverage.json carry the same 5-op count.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, fixed by the same change. docs/coverage/aws/ecs.md reads driver.ECS with 41 operations again, and the README tables and coverage.json are back to 41 / "ECS". The four optional capabilities (ServiceDeployments, ServiceNamespaces, TaskProtection, TaskSets) are listed separately.
Verified: go generate ./... and go run ./internal/compatgen leave no diff.

Comment thread providers/aws/ecs/services.go Outdated
// a desired-count change, a started deployment and the steady state it reached.
func (m *Mock) emitServiceUpdateEvents(ctx context.Context, svc *driver.Service, countChanged, deployed, redeployed bool) {
if countChanged {
m.emitServiceAction(ctx, svc.ARN, svc.ClusterARN, serviceDesiredCountUpdated, serviceEventTypeInfo, "")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The ECS events doc says SERVICE_DESIRED_COUNT_UPDATED "is not sent when the desired count is manually updated by a user"; it is for the scheduler and auto scaling. This emits it on every user update-service --desired-count (same at line 798 for EXTERNAL services).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, you are right. UpdateService no longer publishes SERVICE_DESIRED_COUNT_UPDATED on either path (managed or EXTERNAL), and the constant is removed (providers/aws/ecs/services.go, providers/aws/ecs/events.go). docs/coverage/nongoals/ecs.md says it is not published.
Verified: TestServiceActionEvents and the new EXTERNAL sibling test TestExternalDesiredCountUpdate_NoDesiredCountEvent fail without the fix. E2E: an EventBridge -> SQS rule saw no such event after update-service --desired-count.

Comment thread providers/aws/ecs/events.go Outdated

m.emitDeploymentChange(ctx, svc, id, serviceDeploymentInProgress, "ECS deployment "+id+" in progress.")

if svc.RunningCount >= svc.DesiredCount {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With protected tasks keeping the service above its desired count, rolloutState is IN_PROGRESS (rolloutState now uses ==), but this publishes SERVICE_DEPLOYMENT_COMPLETED and the recorded service deployment is SUCCESSFUL (service_deployments.go:107 uses <). I reproduced it with 2 protected tasks, desired 1 and force-new-deployment. The three should use the same condition.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good catch. One deploymentComplete(running, desired) check now drives the rollout state, the completed event and the deployment record, so they cannot disagree (providers/aws/ecs/services.go:652).
Verified: TestForceDeploy_ProtectedSurplusStaysInProgress fails without the fix (2 extra completed events, record SUCCESSFUL) and TestDeploymentComplete pins the rule. E2E with 2 protected tasks, desired 1 and force-new-deployment: rolloutState and the deployment record stay IN_PROGRESS and no COMPLETED event is published. Known gap: that record never moves to SUCCESSFUL later when protection ends and the extra tasks stop; I did not add that.


updated := cloneService(current)
applyServiceScalars(&updated, in)
applyServiceRefs(&updated, in)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On an EXTERNAL service real ECS only allows desired count, placement, grace period and tag options through UpdateService and directs the rest to task sets. Here --task-definition is silently dropped (200 with the old value) while network configuration and load balancers are applied to the service. Rejecting those fields with InvalidParameterException would match.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good catch. On an EXTERNAL service UpdateService now rejects a different taskDefinition, platformVersion, networkConfiguration, loadBalancers, capacityProviderStrategy or serviceRegistries with InvalidParameterException ("Unable to update on services with an EXTERNAL deployment controller. Create a new task set instead."), and the whole call is refused so nothing is half-applied. The same value echoed back still passes, and desired count, grace period and tags still work (providers/aws/ecs/services.go:680). The exact message text is my reading of the docs; AWS does not publish it.
Verified: 4 provider tests, TestSDK_UpdateService_ExternalRejectsTaskDefinition, and a guard test that ECS-controller services are unaffected. E2E: task definition and network config rejected, scalars applied.

// a task that no service manages, since only service tasks can be protected.
func (m *Mock) protectableTask(want, id string) (*driver.Task, *driver.Failure) {
t, ok := m.resolveTask(id)
if !ok || clusterNameFromARN(t.ClusterARN) != want {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A STOPPED service task passes this check, so UpdateTaskProtection returns protectionEnabled: true for a task the scheduler already stopped. protectionBlocked already skips stopped tasks; the same filter here would avoid protecting dead tasks.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good catch. A STOPPED task now reads as unprotected, and enabling protection on one returns the failure TASK_NOT_VALID ("The task is stopped; only running service tasks can be protected.") (providers/aws/ecs/task_protection.go:31, :145). While fixing it I also closed a race: protecting during a stop could bring the task back as RUNNING, so the protect step now runs under placeMu (:134). The failure wording is my reading, since AWS does not document it.
Verified: TestUpdateTaskProtection_StoppedTask, TestGetTaskProtection_StoppedAfterProtected and TestTaskProtection_EnableRacingStopKeepsStopped fail without the fix. E2E: a stopped task returns TASK_NOT_VALID and Get returns false.

task.LastStatus = statusRunning
task.PlatformVersion = fargatePlatformVersion(platformVersion)
task.Attachments = []driver.Attachment{m.syntheticENI(netCfg)}
linkContainerInterfaces(task)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Pre-existing, but this PR adds more writes after it: placeFargate stores the task pointer here, then stampLaunch mutates it (container ARNs, startedAt, now EventVersion and ManagedAgents) while ListTasks/DescribeTasks may clone it. A concurrent force-new-deployment plus ListTasks probe shows races under -race on this branch and on development. Moving the Set after stampLaunch would close it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good catch. placeFargate no longer stores the task; launchTask saves it once after stampLaunch, so readers only ever see a finished task (providers/aws/ecs/tasks.go:506). I confirmed the race also exists on development; it is fixed here because this PR adds more writes in that window.
Verified: TestConcurrentFargateRunTaskVsReadRace and TestConcurrentFargateForceDeployVsReadRace report DATA RACE without the fix and pass with it. E2E: 40 parallel RunTask calls against readers on a -race server gave 0 race reports.

- DeleteTaskSet drains the set with or without force (Terraform destroy works)
- StopServiceDeployment treats an omitted stopType as ROLLBACK
- EXTERNAL services report running/pending counts from their task sets and
  reject task-set-level UpdateService fields (task definition, platform
  version, network configuration, load balancers, capacity providers,
  service registries)
- stop publishing SERVICE_DESIRED_COUNT_UPDATED from UpdateService
- one shared deployment-complete check for rollout state, event and record
- task protection: stopped tasks are TASK_NOT_VALID and read as unprotected;
  protect runs under placeMu so it cannot revive a stopping task
- placeFargate no longer stores a half-built task (data race with readers)
- describe-container-instances returns version; event attributes is []
- replace the generic capability helper so the generated ECS docs list
  driver.ECS (41 operations) again
- move ListServicesByNamespace handler to its own file, rename test files by
  feature, regenerate docs
@thzgajendra

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Summary of this round (fix commit 2d4eddf, plus a merge of the latest development):

Fixed

  • DeleteTaskSet without force - drains the set like a forced delete, so terraform destroy works (providers/aws/ecs/task_sets.go:401)
  • StopServiceDeployment without stopType - means ROLLBACK; an unknown ARN gets the not-found exception (providers/aws/ecs/service_deployments.go:398)
  • EXTERNAL service counts - runningCount / pendingCount come from the task sets (providers/aws/ecs/services.go:1072)
  • Generated ECS docs - services/ecs/capabilities.go no longer names a capability type in closure parameters; the docs read driver.ECS with 41 operations again (services/ecs/capabilities.go:58)
  • SERVICE_DESIRED_COUNT_UPDATED - no longer published from UpdateService
  • Deployment completion - rollout state, event and deployment record share one deploymentComplete check (providers/aws/ecs/services.go:652)
  • EXTERNAL UpdateService - a different task definition, platform version, network configuration, load balancers, capacity providers or service registries is rejected (providers/aws/ecs/services.go:680)
  • Task protection on a stopped task - TASK_NOT_VALID, and Get reads false; the protect step also runs under placeMu so it cannot revive a stopping task (providers/aws/ecs/task_protection.go:134)
  • placeFargate race - the task is stored once, after it is fully built (providers/aws/ecs/tasks.go:506)
  • Review-only items: describe-container-instances returns version and the event attributes is []; the ListServicesByNamespace handler moved to server/aws/ecs/namespace.go; test files renamed by feature; the 3 new lint findings are gone

Not changed

  • coveragegen picking the primary interface from closure parameters - I removed the trigger in the ECS wrapper and left the generator hardening for a follow-up
  • The ServicesStable waiter needs exactly one deployment and an EXTERNAL service has none here; I do not know what real ECS reports, so it is unchanged
  • A service-deployment record that is IN_PROGRESS because of protected tasks does not move to SUCCESSFUL later when the protection ends

Verification

  • go test ./..., -race on the touched packages, lint on the new lines, go generate / compatgen drift and the contrib builds all pass. Regression tests (each fails without its fix): TestDeleteTaskSet_DrainsWithOrWithoutForce, TestStopServiceDeployment_OmittedStopTypeIsRollback, TestExternalService_CountsFollowTaskSets, TestForceDeploy_ProtectedSurplusStaysInProgress, TestUpdateTaskProtection_StoppedTask, TestTaskProtection_EnableRacingStopKeepsStopped, TestConcurrentFargateRunTaskVsReadRace. Coverage: providers/aws/ecs 91.5%, server/aws/ecs 82.0%, services/ecs 67.7%, whole module 73.5% (73.4% on development).
  • Real E2E against a -race cloudemu serve with fake credentials (boto3 and Terraform 1.9, old build vs fixed build): all rows pass on the fixed build and fail on the old one; terraform destroy without force_delete works; 0 data races. The Terraform provider was 5.x, not 6.68.0, and the exact AWS wording of the new EXTERNAL-update and stopped-task messages is my reading, since AWS does not document it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AWS ECS: open real-cloud parity gaps

2 participants