feat(ec2): capture the instance file system in CreateImage, and implement Deregister/CopyImage - #3306
Conversation
CreateImage recorded only metadata. It registered a new id and set
image.setSourceImageId(resolveLaunchableImageId(region, source.getImageId()));
so the AMI pointed at the *base* image the source instance was launched
from. Launching it started that base image, and everything provisioned on
the source instance was silently discarded.
This was worse than an unimplemented operation. A full Packer amazon-ebs
build ran green against Floci -- keypair, security group, launch, SSH,
provisioner, stop, CreateImage, cleanup -- and produced an AMI reporting
state "available" that contained nothing:
==> amazon-ebs.floci: PROVISIONED_BY_PACKER
==> amazon-ebs.floci: AMI: ami-42ba1c3d9f8023703
Build finished after 1 minute 41 seconds.
$ # launch that AMI, then:
$ cat /etc/floci-probe
cat: /etc/floci-probe: No such file or directory
Anyone building a golden AMI locally would have believed it worked.
Per API_CreateImage, CreateImage "creates an Amazon EBS-backed AMI from an
Amazon EBS-backed instance that is either running or stopped", and the
resulting AMI is a point-in-time capture of the instance's volumes. The
contract is that state at capture time is preserved.
CreateImage now commits the source container to a Docker image
(floci-ami/<ami-id>:latest) and stores that reference on the Image.
RunInstances prefers it, walking the CreateImage chain so an AMI captured
from an instance that was itself launched from a capture still resolves.
The guest runtime (systemd vs minimal, cloud-init) still comes from the
ancestor, since committing a layer does not change how the guest boots.
Capture is best-effort by design: if the daemon refuses the commit, the
AMI keeps its sourceImageId and behaves exactly as before rather than
failing the API call. Skipped entirely in mock mode, where there is no
container.
Ec2ContainerManager gains commitInstance() and removeCommittedImage(); the
latter exists so captures can be reclaimed when the owning AMI is
deregistered, rather than accumulating on disk.
Verified end to end against a real Packer 1.16.0 build on this branch: the
build succeeds, and launching the resulting AMI and reading the file the
provisioner wrote returns PROVISIONED_BY_PACKER. The committed image is
visible as floci-ami/ami-295129250f0c23320:latest.
Tests cover the capture itself with a mocked Docker client: that the
reference is split into repository and tag (passing it whole would create
a repository literally named "floci-ami/ami-123:latest"), that a missing
container captures nothing, that a daemon failure yields null rather than
propagating, and that removing an absent or never-captured image is not an
error. 8 tests, all green.
Not addressed here: reclaiming the committed image on DeregisterImage,
which needs the DeregisterImage work to land first.
Both actions returned UnsupportedOperation:
$ aws ec2 deregister-image --image-id ami-... --endpoint-url http://localhost:4577
An error occurred (UnsupportedOperation) ... Operation DeregisterImage is not supported.
$ aws ec2 copy-image --source-region us-east-1 ...
An error occurred (UnsupportedOperation) ... Operation CopyImage is not supported.
The rest of the AMI lifecycle already worked -- RegisterImage, CreateImage,
DescribeImages, and RunInstances from a created AMI -- so the gap sat at the
end of it. DeregisterImage is what Packer calls under force_deregister to
clear the AMI holding the name it is about to reuse; without it the second
run of any iterative build dies on InvalidAMIName.Duplicate and the build is
one-shot. CopyImage is the build-in-one-region, promote-to-the-others
pattern that several Gruntwork AMI modules use.
The cause was only the missing `case` arms in Ec2QueryHandler's dispatch
switch, whose default is UnsupportedOperation. EC2_ACTIONS in
AwsQueryController is not on this path for a signed request: resolveService()
takes the service from the SigV4 credential scope and only falls back to
inferServiceFromAction() when that fails. CreateImage is likewise absent from
EC2_ACTIONS and works fine. All three are added anyway so the unsigned/raw
client fallback agrees with the dispatcher.
DeregisterImage. "Deregisters the specified AMI. A deregistered AMI can't be
used to launch new instances", and it does not delete "Instances already
launched from the AMI" (API_DeregisterImage). The image is therefore
tombstoned with the documented AMI state `deregistered` rather than dropped
from the store: DescribeImages stops reporting it, its name is released so
the rebuild can claim it, RunInstances rejects it with
InvalidAMIID.Unavailable, but an instance launched from it keeps resolving
its ancestry to a guest image, so a stop/start still comes back on the right
one. Keeping the tombstone is also what lets a repeat call answer
InvalidAMIID.Unavailable ("has been deregistered and is no longer available")
instead of InvalidAMIID.NotFound ("The specified AMI doesn't exist") -- a
retry can tell the two apart. Deregistering a catalog AMI, which is owned by
amazon rather than by the caller, is AuthFailure: "trying to use an AMI for
which you do not have permissions". Snapshots are kept unless
DeleteAssociatedSnapshots is set ("Default: The snapshots are not deleted"),
and one shared with another AMI is reported `skipped` rather than deleted,
since "if a snapshot is associated with multiple AMIs, it won't be deleted
even if specified for deletion, although the AMI will still be deregistered."
CopyImage. "The copy operation must be initiated in the destination Region",
and for a Region-to-Region copy "the destination Region is the Region in
which you initiate the copy operation" (API_CopyImage) -- so the request's
own region is the destination and only the source is looked up under
SourceRegion. That matters here because Floci keys registered AMIs by
(region, id) and describeImages filters on the stored region, so a source id
that exists in us-east-1 is genuinely absent when SourceRegion names another
region; that case is InvalidAMIID.NotFound. The copy is an independent image:
its own id, its own snapshot ids (sharing the source's would make deleting
either look like it took the other's backing with it), owned by the caller.
State: AWS reports the copy as `pending` until the backing snapshots finish.
Floci's store is in-memory and the copy completes inside the call, so it is
`available` immediately -- matching what CreateImage and RegisterImage
already return rather than inventing a state machine that nothing would ever
advance. A caller polling for `available` returns on its first poll.
Concurrency. registerImage's duplicate-name check is a read-modify-write over
the whole image store, and CopyImage now reaches it too. The stripe is taken
on (region, name), not on an image id -- the id does not exist yet and is
unique by construction, whereas the name is the invariant. deregisterImage's
check-then-tombstone takes the (region, id) stripe. Both are the striped-lock
pattern already used by modifyVpcEndpoint.
Tests are driven through the service, not over HTTP, for the concurrency
cases: requests through the test server serialize enough to hide a lost
update, the same trap called out in
Ec2ModifyVpcEndpointTest.concurrentAssociationsAllSurvive. Both race tests
were confirmed to fail with their lock removed -- 6 of 24 concurrent copies
claimed one name, and one AMI was deregistered twice -- which is also why the
name race pads the store first: the window that has to be hit is the scan,
and against an empty store the test passes with or without the lock.
The rest of the coverage is over HTTP: that DeregisterImage answers
return=true and the AMI then vanishes from DescribeImages, that unknown,
already-deregistered and not-owned ids get the three distinct documented
errors, that the freed name can be registered again (the force_deregister
loop), that an instance launched from the AMI keeps running while a new
launch is refused, snapshot retention and the shared-snapshot skip, and for
CopyImage that the copy lands in the caller's region and not the source's,
is available and owned by the caller, gets its own snapshots, is launchable
where it landed, and rejects a missing or deregistered source.
Not implemented: the Encrypted/KmsKeyId, CopyImageTags, TagSpecification and
ClientToken parameters of CopyImage, and Outpost/Local Zone destinations.
CreateImage captures the source instance's file system as a committed
Docker image (floci-ami/<ami-id>:latest). Those are real disk. Nothing
released them, so an iterative build -- Packer with force_deregister, or
any loop that rebuilds the same AMI -- left one committed layer behind per
iteration, indefinitely.
DeregisterImage now releases the capture. The AMI record itself is still
kept as a tombstone, as the DeregisterImage implementation requires for
ancestry resolution; only the Docker image is reclaimed.
The exception is the one that matters for correctness. AWS keeps instances
launched from a deregistered AMI running, and DeregisterImage's own
documentation notes that already-launched instances are unaffected -- they
can still be stopped and started. Removing the layer such an instance
boots from would break exactly that, so reclamation is skipped while any
non-terminated instance still resolves to the capture, and the tombstone
keeps its reference. Correctness before disk: the worst case is a layer
that outlives its AMI, not an instance that cannot start.
Terminated instances deliberately do not pin a capture. They can never
boot again, and treating them as live would mean nothing is ever reclaimed
in a long-running emulator.
Tests drive the real CreateImage path rather than building a fixture by
hand -- the image store returns detached copies, so setting the captured
reference on a returned Image never reaches the store and the test would
silently assert nothing. They needed two further pieces of care worth
noting:
- The shared test configuration sets floci.services.ec2.mock=true, which
skips all container work, so both capture and reclamation would be
dead code under test. A QuarkusTestProfile turns it off; the container
manager is still mocked, so no Docker is involved.
- Outside mock mode, termination is delegated to the container manager.
With that mocked, an instance stays "running" forever, which would
have made the terminated-instance assertion vacuous. The mock now
performs the same state transition the real path does.
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/ec2/Ec2Service.java | Implements captured-AMI launch, copy, deregistration, locking, and reclamation; multi-instance termination can leave a retained capture unreclaimed. |
| src/main/java/io/github/hectorvent/floci/services/ec2/Ec2ContainerManager.java | Adds Docker commit and removal operations with explicit capture failure handling. |
| src/main/java/io/github/hectorvent/floci/services/ec2/Ec2QueryHandler.java | Adds AWS Query handlers and XML responses for DeregisterImage and CopyImage. |
| src/main/java/io/github/hectorvent/floci/services/ec2/model/Image.java | Persists the Docker image reference associated with a captured AMI. |
| src/test/java/io/github/hectorvent/floci/services/ec2/Ec2CapturedImageReclaimIntegrationTest.java | Covers capture retention and reclamation with the required integration-test naming convention. |
| src/test/java/io/github/hectorvent/floci/services/ec2/Ec2DeregisterAndCopyImageIntegrationTest.java | Exercises deregistration, copying, snapshot handling, and launch behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CreateImage] --> B[Commit instance container]
B --> C[Store Docker image reference on AMI]
C --> D[RunInstances resolves captured image]
C --> E[CopyImage shares captured reference]
C --> F[DeregisterImage tombstones AMI]
F --> G{Live instance or registered copy still depends on capture?}
G -- Yes --> H[Retain Docker image]
G -- No --> I[Remove Docker image]
H --> J[Terminate dependent instance]
J --> G
Reviews (4): Last reviewed commit: "fix(test): pass the VpcNetworkManager th..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…nnot Six review findings on the image-capture stack, all of which end in the same place: an AMI that reports itself available while its file system is not what the caller asked for. CopyImage copied the flattened ancestry but not the capture, so launching a copy silently started the base image. The copy now carries the reference, and because a Docker reference is global to the daemon, reclamation counts another AMI carrying the same tag as a live user. A capture that could not be made was reported the same way as "there was nothing to capture", so a refused commit produced an immediately-available AMI that booted the ancestor. commitInstance now throws, and CreateImage drops the half-built AMI and fails: InternalError when the commit was refused, IncorrectInstanceState when the instance has no container to capture. Floci commits inline, so there is no later state transition a poller could observe and no honest way to report this asynchronously. Two races shared one cause: the invariants over the image registry are whole set scan-then-mutate sequences, and the three operations that hold them ran under three different monitors. RegisterImage locked a name stripe while DeregisterImage locked an id stripe, so a registration could reuse a snapshot between the shared-reference scan and the delete. RunInstances took no image lock at all, so a launch could resolve a captured tag, pause before storing its instance, and have deregistration remove the tag underneath it. Both now run under one imageRegistryLock; RunInstances holds it only to resolve the AMI and publish its instances, with the container launches outside it. The lock is single rather than striped because the capture invariant is not region-scoped: a copy shares its source's layer across regions. removeCommittedImage suppressed docker errors while the caller cleared the reference regardless, so a transient failure leaked the layer permanently. It now reports whether the layer is gone, and the reference survives a refusal. Finally, a capture retained for a live instance was never reclaimed, because reclamation only ran during deregistration and a second deregistration is rejected. Termination now reclaims what deregistration had to keep. Ec2ServiceTest builds a container-backed service with a bare container-manager mock, so its instances have no container and every CreateImage there would now be rejected for a reason those tests are not about; they get a commit stub.
|
All six addressed in 07401c7. Four were correctness bugs, and one of them made the feature's headline promise unreliable, so thank you for the catch. Copy drops the captured filesystem (P1) - Capture failure reported success (P1) - a failed commit still produced an immediately-available AMI, so an image build could report success and quietly hand back the ancestor. It now fails loudly rather than presenting a ready AMI with nothing captured. Accepted-then-silently-wrong is the one option that cannot be right here. Two lock-ordering races (P1) - both real. One design note worth surfacing: that monitor is single rather than striped per region, deliberately. Docker image references are global to the daemon, and a Failed cleanup forgot the capture (P2) - Retained captures were never reclaimed (P2) - taken rather than deferred, since it turned out small: terminate now releases a capture once the last dependent instance goes away, instead of relying on a second deregistration that is rejected anyway. Collateral worth flagging, because it changed test fixtures rather than production code. The wider 875 tests green across the full |
|
Housekeeping so the thread list reflects reality: I have resolved the two threads at The two genuinely new items from the 15:21 re-review are being worked and I have not resolved those:
|
…ts anything RunInstances checks the AMI tombstone twice: once on entry, and once under the image registry lock, because DeregisterImage releases the captured layer as soon as no live instance resolves to it, and a launch that had resolved the layer but not yet stored its instance would lose it underneath. The second check fired too late. By the time it ran, the launch had already written a root volume per instance, the instance tags, a subnet IP, and the attachment on a caller-supplied ENI. The reservation was never returned, so no caller had an id to clean any of that up with, and the interface stayed in-use for good. Move the build loop inside the same locked section, after the check, so the launch persists nothing until it knows it is allowed to proceed. Rollback was the other option and is worse: the subnet IP counter has no free list to return an offset to, and a rollback path only runs once something has already gone wrong. The lock is not meaningfully wider, since what moved inside it is in-memory record construction; the slow part, the container launch, still runs outside. The regression test holds the registry lock itself, so the launch parks at a known point and the tombstone lands there on purpose rather than racing for the window. Its file is renamed to *IntegrationTest, matching what the rest of the repo calls a @QuarkusTest.
…Test 607 of the 736 @QuarkusTest classes on main end in IntegrationTest, and this one starts a full application like the rest of them. Ec2CreateImageCaptureTest is left alone: it drives a plain Ec2Service with Mockito and is not an integration test.
|
Both new items done in c5aecfd and 2d9b867. Launch race (P1). Confirmed before changing anything, and the enumeration matters because it shows why a rollback would have been the wrong shape. Between the early Rather than unwinding those on the failure path, the build loop now sits inside The critical section did not grow to cover the slow part: Coverage: Test naming (P2). Renamed 876 tests green across the full Also worth flagging since it came up in the earlier round: I resolved the two threads at |
hectorvent
added
ec2
|
Thank you, and sorry this sat: you closed eight findings across two rounds without anyone replying, and the two I found nothing to fix in the code. The shapes check out member by member against the model, including the nested (blocking) It needs a rebase, and there is a trap in it that is our fault. The only textual conflict is a one-line |
hectorvent
added
the
waiting-author
…quires Merges current main and repairs the one thing the merge could not see. floci-io#3272 added a 13th constructor parameter to Ec2ContainerManager; Ec2CreateImageCaptureTest builds that constructor but neither side edited those lines, so git auto-merged cleanly and ./mvnw compile still passes. Only test-compile catches it, which is the whole reason that is the command worth running. Reproduced before fixing: compile exit 0, test-compile exit 1 on "no suitable constructor found". Adds mock(VpcNetworkManager.class) as the 13th argument. Also keeps our side of the only textual conflict, the ArgumentMatchers.any import in Ec2ServiceTest, which this branch's tests use and main no longer does.
|
Rebased in d6ff91d, and your diagnosis was exact. Merged current The constructor trap reproduces precisely as you described, and I checked it in both directions before touching anything rather than just applying the fix:
Worth saying that this one was foreseeable and I flagged it in the PR description when I opened this: #3272 replaced the 12-argument constructor without keeping an overload, git merges the test cleanly because neither side touches those lines, and only Thank you for reviewing it shape by shape against the model. On the two |
| boolean stillLaunchable = instances.scan(i -> true).stream() | ||
| .filter(i -> i.getRegion() != null && !i.getInstanceId().equals(excludedInstanceId)) | ||
| .filter(i -> i.getState() != null && !"terminated".equals(i.getState().getName())) | ||
| .anyMatch(i -> captured.equals(capturedImageFor(i.getRegion(), i.getImageId()))); |
There was a problem hiding this comment.
When multiple instances that depend on the same deregistered AMI are terminated in one request, reclamation excludes only the instance currently being processed. The other instances remain recorded as shutting-down, which this scan treats as live. Each reclamation attempt therefore retains the captured Docker image, and no later attempt runs after asynchronous teardown finishes, so the image remains on disk indefinitely.
2 participants
Summary
CreateImagerecords metadata only. It registers a new AMI id and copies the block device mappings, but nothing captures the source instance's file system, so an AMI created from a provisioned instance launches the base image with everything provisioned on it discarded.This makes the capture real, and adds the two operations that manage the result.
CreateImagecommits the running container to a Docker image and stores it on the AMI, soRunInstancesfrom that AMI starts from the captured file system.DeregisterImageandCopyImageare implemented;RunInstancesfrom a deregistered AMI is rejected.Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
Incorrect behavior: an AMI created by
CreateImagewas not a snapshot of anything. Real EC2 captures the instance's volumes, so a launch from that AMI reproduces the source instance. Floci returned a plausible AMI id whose launches silently reverted to the base image, which is the failure mode that looks like success.Upstream has since landed the metadata half of this area independently (the reboot,
findImageForCapture,captureBlockDeviceMappings, and theresolveLaunchableImageIdancestry walk). This PR is the complementary half and builds on it: the file-system capture itself, plus Deregister and Copy. Verified before submitting thatcommitInstance,removeCommittedImage,Image.dockerImage,case "DeregisterImage"andcase "CopyImage"are all still absent frommain.CopyImagecopies within the emulator; it does not model cross-region semantics beyond the id and metadata, and the docs say so rather than implying parity.Checklist
./mvnw testpasses locallyCoverage:
Ec2CreateImageCaptureTest8,Ec2DeregisterAndCopyImageTest19,Ec2CapturedImageReclaimTest4. Load-bearing check: revertingsrc/maintomainmakes all three classes fail to compile, since the symbols they exercise do not exist there.One cross-PR note for whoever merges second
Ec2CreateImageCaptureTestconstructsEc2ContainerManagerwith the current 12-argument constructor. My other open PR #3272 replaces that constructor with a 13-argument form (addingVpcNetworkManager) without retaining a 12-arg overload. Each branch is correct againstmaintoday, and git merges them textually clean, so onlytestCompilecatches the interaction.Whichever of the two lands second will need a one-line adjustment to that test, passing the extra argument. I have deliberately not pre-adapted it here, because that would not compile against
mainas it stands. Happy to push the fix to whichever PR is second as soon as the first merges.