Repository navigation
fix(aws-ecs): close the real-cloud parity gaps in #1449 - #1528
thzgajendra wants to merge 3 commits into
Conversation
NitinKumar004
left a comment
There was a problem hiding this comment.
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
- 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. Everyupdate-service --desired-counthere emits one, so a rule meant to watch auto scaling fires on user changes. - 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-deploymentleaves the deploymentrolloutState: IN_PROGRESS, yet the emulator publishes SERVICE_DEPLOYMENT_COMPLETED and ListServiceDeployments says SUCCESSFUL, because both userunning >= desiredwhilerolloutStatenow uses==. Use the same condition in all three. - UpdateService on an EXTERNAL service silently ignores a task definition change (
providers/aws/ecs/services.go:773).update-service --task-definition webreturns 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. RejecttaskDefinition,launchType,platformVersion,networkConfigurationandloadBalancerswith InvalidParameterException instead of dropping them (network and load balancers are currently applied to the service record). - UpdateTaskProtection accepts STOPPED tasks (
providers/aws/ecs/task_protection.go:176). Protecting a task the scheduler already stopped returnsprotectionEnabled: true. Skip non-running tasks the wayprotectionBlockedalready does. - Container instance
versionis not on the wire. The driver now keepsContainerInstance.Versionand the events carry it, butdescribe-container-instancesstill returns noversion, so a consumer cannot correlate events with describes as the ContainerInstance shape intends. The event'sattributesis alsonullrather than[]when there are none. - Launch-path race is pre-existing but this PR widens it (
providers/aws/ecs/tasks.go:505).placeFargatestores the task pointer andstampLaunchthen writes to it (container ARNs, startedAt, and nowEventVersionandManagedAgents) while ListTasks/DescribeTasks can clone it. A concurrentforce-new-deploymentplusListTasksprobe reports races under-raceon this branch and on development. The new task-set, deployment and protection paths showed no races. Worth moving theSetinplaceFargateafterstampLaunchwhile you are in here. - golangci-lint goes from 21 to 30 issues on the touched packages (v2.14.0): +8 goconst (
CPU,MEMORY,ClusterName,taskSet,null,failuresvskeyFailures) and a new gocyclo onUpdateService(12 > 10). CI does not run golangci-lint; this is the local gate. - Naming. ListServicesByNamespace lives in
providers/aws/ecs/namespace.gobut inserver/aws/ecs/service_deployments.go, so the feature does not share a filename across layers (docs/STRUCTURE.md).parity_helpers_test.goandvalidation_defaults_test.goare generic names; feature names read better.
Docs-parity divergences
- DeleteTaskSet without
forceon a non-zero set: rejected here, accepted and drained by real ECS (API; Terraformaws_ecs_task_setacceptance configs). - StopServiceDeployment
stopTypeis 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 testonproviders/aws/...,server/aws/ecs,server/aws/eventbridge,server/aws/cloudwatch,services/ecs,internal/settle,persist,cmd/...andserver/adminall pass;-raceon the touched packages passes;go mod tidy -diffclean; 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/ecs93.8%,server/aws/ecs81.6%,services/ecs68.7%. - Throwaway
-raceprobe 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 onaws.ecsinto SQS (111 events plus the container-instance run); CloudWatchlist-metricsandget-metric-statisticsfor AWS/ECS and ECS/ContainerInsights. --async-settle: deployment IN_PROGRESS window, StopServiceDeployment ROLLBACK reverting the task definition and settling to ROLLBACK_SUCCESSFUL.--persist --state-filerestart: task set id and tags and deployment ARNs identical before and after.- Terraform: apply with no waiter hang,
plan -detailed-exitcode0, update of desired count and task-set scale, plan 0 again, destroy fails on the task set (High 1); withforce_delete = truedestroy completes and cluster, services and task definition read back INACTIVE and DescribeTaskSets returns ServiceNotActiveException. - Wiring:
SetMonitoringadded inproviders/aws/aws.gonext 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 throughserve.
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.
| return nil, apiErrf(errors.NotFound, excTaskSetNotFound, "The specified task set %q was not found.", in.TaskSet) | ||
| } | ||
|
|
||
| if !in.Force && stored.Scale.Value > 0 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // | ||
| //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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| # 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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, "") |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
|
||
| m.emitDeploymentChange(ctx, svc, id, serviceDeploymentInProgress, "ECS deployment "+id+" in progress.") | ||
|
|
||
| if svc.RunningCount >= svc.DesiredCount { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Thanks for the review. Summary of this round (fix commit 2d4eddf, plus a merge of the latest Fixed
Not changed
Verification
|
Summary
development: five items were already fixed there and only get regression tests (see below).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-commandworked on any task, even one started withoutenableExecuteCommand, or already stopped.create-clusteron an existing name failed,run-task --launch-type BOGUSreturned an empty result instead of an error,assignPublicIpcame back empty, and a secondstop-taskreplaced the stop reason.list-services-by-namespacedid not exist (unknown ECS operation).How this fixes it
Rules live in the provider (
providers/aws/ecs), so the Go library andcloudemu servebehave the same; the wire handler only translates. New operations are small optional interfaces (TaskSets,TaskProtection,ServiceDeployments,ServiceNamespaces) next todriver.ECS, found by type assertion and also exposed through the portableservices/ecswrapper.CreateCluster;assignPublicIpdefaults toDISABLEDand is validated;launchTypemust be one of the SDK enum values; a repeatedStopTaskkeeps the first reason; duplicateCreateServiceanswers "Creation of service was not idempotent.";ExecuteCommandneedsenableExecuteCommand, a RUNNING task, an interactive call and an existing container. Tasks carryenableExecuteCommand, containers reportmanagedAgentsandnetworkInterfaces.GetTaskProtection/UpdateTaskProtection(1-2880 minutes, default 120, service tasks only,MISSING/TASK_NOT_VALID/DEPLOYMENT_BLOCKED). Protected tasks survive a scale-in or redeploy.EXTERNALservices. A set runs its own tasks,computedDesiredCountis the service desired count x scale rounded up, and one set isPRIMARY. Deleting a set drains it and stops its tasks (forceis accepted and changes nothing, since scale-down is immediate).UpdateServiceon anEXTERNALservice 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'srunningCountandpendingCountcome from its task sets.ListServicesByNamespaceworks throughserviceConnectConfiguration.namespace.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).ECS/ContainerInsightscounts (clusters withcontainerInsightsenabled or enhanced, or enabled on the account) andAWS/ECSCPUReservation,MemoryReservation,LiveTaskCount. No usage-driven series are fabricated.--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,ListTagsForResourcefor a cluster named*cluster*,TagResourceon an unknown ARN,containerArnon task containers,include=TAGSon Describe*.Diagram
Provider Coverage
Azure, GCP and OCI are left out on purpose: ECS exists only on AWS, so there is nothing to mirror.
Checklist
go test ./...)golangci-lint run --timeout=9m ./...) - 0 issues on the changed linesIntegration 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.go generate ./...for coverage;go run ./internal/compatgenfor the compat matrix) and committed the resultTest Plan
How I tested it
-racecloudemu serve; the image fromdocker build+docker runwith default flags;--async-settle;--persist --state-filewhere a state file written by thedevelopmentbinary is loaded by this branch;--enforce-authwith a restricted IAM user (new operations are authorized per operation, denied ones returnAccessDenied); 50 parallel callers (same-namecreate-cluster, same-tokencreate-task-set, protection andstop-taskraces); Terraform 1.9 with the aws provider (aws_ecs_cluster, task definition, Fargate service,EXTERNALservice,aws_ecs_task_set): apply, an in-placedesired_countchange on theEXTERNALservice,planshows "No changes", destroy (the task set is destroyed withoutforce_delete). NoDATA RACEin any server log.CreateServicestored 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_ConcurrentCreateServicesreports 2 races without the fix and none with it.aws-sdk-go-v2round trips asserting typed exceptions, theservices/ecswrapper, an EventBridge -> SQS test, a CloudWatch round trip, snapshot round trip). The new behaviour tests fail ondevelopment(for example the idempotentCreateCluster,assignPublicIpdefault,launchTypeand stop-reason tests) and pass here.go build -mod=readonly, vet,go test ./...,-raceon the touched packages,go mod tidy -diff,golangci-linton the changed lines,go generate/ compatgen with no drift, Structure, contrib builds, CodeQL go-security-extended.Coverage
providers/aws/ecsserver/aws/ecsservices/ecsDeliberate 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-EXTERNALservice, an omittedstopTypemeaning ROLLBACK,TASK_NOT_VALIDfor protecting a stopped task, the wording of the rejectedEXTERNALUpdateServicefields, the invalidlaunchTypemessage, theExecuteCommanderror for a task that is not RUNNING,StopTaskon an already-stopped task keeping its reason, andCreateClusterbeing idempotent. Also not emulated: theSERVICE_DESIRED_COUNT_UPDATEDevent (it is not published), CPU/memory utilization metrics (no workload runs), theDELETEDtask state,versionInfoin container-instance events, the circuit breaker (SERVICE_DEPLOYMENT_FAILED), and Cloud Map (an unknown namespace lists nothing). Deployments complete immediately, soStopServiceDeploymentonly succeeds inside the--async-settlewindow.docs/services.mdis 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.