Repository navigation
fix: verify core device deletion in GTF teardown to handle late fleet… - #328
limingyaoo wants to merge 1 commit into
Conversation
| } | ||
|
|
||
| try { | ||
| Thread.sleep(deleteDrainWaitMillis()); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Manual polling with Thread.sleep() is used instead of AWS SDK waiters. This approach is inefficient, error-prone, and can lead to resource wastage. Use AWS SDK waiters (e.g., AmazonEC2Waiters, AmazonS3Waiters) which provide built-in polling mechanisms with exponential backoff. Learn more https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/waiters.html.
There was a problem hiding this comment.
AWS SDK waiters doesn't fit this case. Since waiters only poll, they don't re-issue the delete
| LOGGER.warn("Could not delete core device {}", thingName(), e); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Small question, not a blocker: the drain sleep happens before every verify, including the first attempt — so even the normal case (delete succeeds, no resurrection) pays the full 5s before remove() returns. Since teardown iterates core devices sequentially, that's +5s per device on every test run, not just the unhappy path.
Was that the intended tradeoff (always wait the full drain before trusting the check, since deleteDrainWaitMillis is a fixed 5s in production with no config path to lower it), or would it be worth confirming there's no cheap way to skip the wait when a fast path is acceptable? Either answer is fine — just want to make sure the always-pay-5s behavior was a deliberate choice and not an artifact of the loop structure.
There was a problem hiding this comment.
Yes this is deliberate. I've considered a fast path to verify immediately, but didn't use it because an immediate check right after DeleteCoreDevice can see the record as gone before a delayed status report lands.
| return; | ||
| } catch (InterruptedException ie) { | ||
| Thread.currentThread().interrupt(); | ||
| return; |
There was a problem hiding this comment.
Question on the verify block's error handling. ResourceNotFoundException is the success signal, which makes sense. But the final catch (GreengrassV2Exception e) treats any other service exception on getCoreDevice — throttling, a transient 5xx, access-denied — as a reason to return, i.e. the same outcome as a confirmed delete.
If I'm reading it right, that means a ThrottlingException on the verify call ends remove() as if the record were gone, even though we never confirmed it drained — which is the exact orphaned-record state this change is trying to prevent. It also reads a little asymmetric next to the delete block just above: a failed delete falls through and we still verify/retry, but a failed verify exits.
Is that intentional — i.e. "if we can't tell, don't spin"? If so, could we at least keep the record tracked / log at a level that surfaces it, so a throttled verify isn't silently indistinguishable from success? Or would it be safer to let a transient error fall through to the next attempt (the drain wait already rate-limits us), and only bail on a genuinely non-retryable error like access-denied?
There was a problem hiding this comment.
Good point, I've updated the pr to address it. The verify-error catch now falls through (via continue) to the next attempt instead of returning
…-status re-ingest
a92a100 to
3da3914
Compare
…-status re-ingest
Description of changes:
GreengrassCoreDeviceModel.remove() now deletes the core device, waits a short drain interval, then verifies removal via GetCoreDevice, retrying up to 3 times if the record is still present. The drain interval is an injectable @Value.Default deleteDrainWaitMillis() (default 5s) so tests can run without sleeping. Adds a unit test and the JUnit 5 / Mockito test deps to the module.
Why is this change necessary:
Teardown fired a single DeleteCoreDevice and moved on. A fleet-status report that was in flight when the delete ran can be ingested a few seconds later and re-create the core device record, so the delete "succeeds" yet the device reappears (HEALTHY), leaving orphaned resources after gdk test-e2e run. Verifying + retrying closes that race in teardown.
How was this change tested:
Added GreengrassCoreDeviceModelTest (Mockito) covering: delete sticks (1 delete, verified gone), record resurrected once then gone (2 deletes), and record persists (stops after max attempts). Tests use deleteDrainWaitMillis(0) to run fast. Also relies on CI (mvn clean verify, ubuntu + windows, JDK 8).
Any additional information or context required to review the change:
The durable fix is service-side (avoid re-creating a just-deleted record on status ingest); this makes GTF teardown robust in the meantime. Tradeoff: adds up to ~5s per core-device teardown on the happy path (tunable via the constants).
By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.