fix(ecs): apply volumesFrom to task containers - #3811
Conversation
31c0dde to
3f72b31
Compare
KenkoGeek
left a comment
There was a problem hiding this comment.
Reviewed the wire round-trip, launch ordering, Docker HostConfig mapping and regression coverage against the reported volumesFrom workload. The dependency walk creates each source before its consumer while the task response is restored to task-definition order, and Docker receives the source container id with the requested ro/rw mode. Unknown/cyclic sources fail before any container is created, and the Docker-backed test exercises the actual entrypoint-from-shared-volume failure shape. The full required matrix is green on this head. I do not see a remaining correctness blocker.
pgermosen
left a comment
There was a problem hiding this comment.
Traced the topological sort and cycle detection by hand, self-reference and diamond-dependency cases both hold up, and confirmed the rejection actually happens before any container gets created rather than just trusting the commit message. Checked VolumeFrom's field names and the readOnly default against the live docs too, exact match. Nice touch preserving the original task-definition order in the response despite launching in dependency order internally. Approving.
|
The branch conflicted after FireLens support (#3768) landed. I merged main at eb78767 without rewriting the existing commit; the new signed head is 4a205dd. The resolution keeps both features: volume sources and FireLens routers participate in the same dependency ordering, so a router can inherit a volume before its logging applications start. Mixed volume/log-router cycles fail before any container is created. Docker still receives VolumesFrom while extra hosts remain omitted for container-network mode. Added three combined regressions. Focused verification completed with 71 tests passed, no failures/errors, and 2 Docker-backed tests skipped because no daemon was available. The package build and documentation checks also pass. The PR description now reflects the combined behavior and current validation; CI for this new head needs to finish before the earlier approvals can be treated as covering it. |
pgermosen
left a comment
There was a problem hiding this comment.
The unified ordering fix looks right, traced the combined source-router-app chain and the mixed-cycle case by hand and both come out correct, well tested too. What's actually blocking this is the CI job that's failing, native arm64 / compat-opentofu hung to the full 12-minute timeout on both retries, at the identical point in tofu destroy, while five other PRs merged in just the last few hours all passed that same job in 3-4 minutes. That rules out plain flakiness. I couldn't pin an exact cause from the log though, the compat suite only spins up a bare ECS cluster and never actually calls RunTask or RegisterTaskDefinition, so it doesn't obviously exercise the volumesFrom or FireLens code this PR touches either. Still reproducible specifically on this branch though, worth understanding before merging rather than retrying a third time.
|
I investigated both teardown failures before retrying again. The evidence points to the S3 null-version deletion issue tracked in #3864, which #3867 has since fixed. Both attempts used the same native artifact (10550936908), built from c1fe74e (4a205dd merged into fdec637). The failing logs show the same ordering:
The OpenTofu fixture only makes CUR depend on the bucket, so CUR emission and versioning can run concurrently. On the old branch I reproduced the underlying API behavior directly: writing before enabling versioning creates a null version; deleting that version returns 204 but leaves it present, and DeleteBucket returns BucketNotEmpty. Reversing the order deletes cleanly. This matches the ordering-sensitive failure documented in #3864; the CI logs do not include the actual S3 list/delete requests, so I am not treating log timestamps alone as a request trace. I merged main at 004d562 in signed commit 53030a5, incorporating #3867 and reconciling the newer FireLens changes from #3790. The same local reproduction now leaves zero versions and DeleteBucket returns 204. The volume dependency ordering now preserves main's configuration-copy-before-start and Unix-socket logging behavior. Validation: 164 focused tests and the package build pass. Five Docker tests were unavailable locally; one unchanged POSIX-path fixture was excluded on Windows after the wider run exposed its platform assumption. Documentation checks pass. This update triggers a fresh native build containing the S3 fix, rather than reusing the old artifact. I will treat the native arm64/OpenTofu result on this head as the remaining validation, not claim the CI hang resolved from the JVM reproduction alone. |
pgermosen
left a comment
There was a problem hiding this comment.
The compat-opentofu hang is gone, it's passing in under 4 minutes now instead of dying at the 12-minute mark, and the rest of the suite is clean too. I checked the ordering logic directly given how much churned underneath it from the FireLens merge, and the combined FireLens/volumesFrom dependency graph is structurally the same, same tests still pass the same assertions, just adapted to the new unix-socket transport. Approving.
Summary
ECS discarded container
volumesFromentries and did not configure Docker volume inheritance, so an application whose entrypoint lived in a source container volume could not start.Store and return the entries, launch source containers before consumers, and pass the source Docker IDs and requested access modes to Docker. Startup ordering also includes FireLens router dependencies, allowing
source -> router -> appwhile rejecting unknown sources and mixed dependency cycles before containers are created. ECS responses retain task-definition container order.Closes #3806
Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
Task-definition
volumesFromvalues round-trip through RegisterTaskDefinition and DescribeTaskDefinition. The runtime preserves read-only/read-write inheritance and uses the source container's Docker ID.The integration retains main's generated Fluent Bit/Fluentd configuration, S3 configuration prefetch, Unix-socket logging, and protected network handling. Volume sources and log routers participate in one dependency ordering pass. Router configuration is copied before startup, and a missing S3 configuration fails before any volume-source container is created.
The Docker-backed regression builds the reported sidecar shape (
VOLUME /sharedcontaining/shared/wrapper) and exercises an entrypoint inherited from that volume. Additional coverage checks combined FireLens/volume ordering, mixed cycles, and Docker HostConfig.Verification
On JDK 25, after incorporating main at
004d562(including #3867 and #3790):/host/abs/app/config.ymlis not an absolute Windows path). The successful focused run excludes those Docker tests and that one fixture method; Linux CI retains their normal coverage.git diff --check: passed.VersionId=nullobject and then deletes its bucket successfully. The previous head left the object present and returned BucketNotEmpty.The full repository suite and native ARM/OpenTofu job were not run locally. The latter must pass on the newly built image before the earlier teardown failure is considered resolved.
Checklist
./mvnw testpasses locally