Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait for stack deletion in test cleanup #9019

Merged
merged 1 commit into from Aug 31, 2023

Conversation

dominikschubert
Copy link
Member

@dominikschubert dominikschubert commented Aug 30, 2023

Motivation

The usage of the deploy_cfn_template fixture is a major cause of temporary resource leaks.
Not waiting for the deletion to be completed causes the delete operations to bleed into the next test exexution(s).

Changes

After the yield, deploy_cfn_template now properly waits for the stack deletion before moving on to the next test case.

Warnings

This will increase test execution time. Part of the intention of this PR is to find out by how much.

Update: Seems this didn't introduce as much delay as initially thought:

image

@dominikschubert dominikschubert self-assigned this Aug 30, 2023
@dominikschubert dominikschubert added the semver: patch Non-breaking changes which can be included in patch releases label Aug 30, 2023
@coveralls
Copy link

Coverage Status

coverage: 80.369% (-0.001%) from 80.37% when pulling fe37e1e on cfn-wait-for-deletion into 5b16d3b on master.

@github-actions
Copy link

LocalStack Community integration with Pro

       2 files         2 suites   1h 25m 29s ⏱️
2 148 tests 1 674 ✔️ 474 💤 0
2 149 runs  1 674 ✔️ 475 💤 0

Results for commit fe37e1e.

Copy link
Member

@pinzon pinzon left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

Small question, but generally this looks like it will do the right thing.

LOG.debug(
f"Failed cleaning up change set {entry_change_set_id=} and stack {entry_stack_id=}: {e}"
)
LOG.debug(f"Failed cleaning up stack {entry_stack_id=}: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

How often does this line show up in the logs? It would be interesting to see how often this cleanup fails, since that may leave resources around

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I think we are a bit too lenient with the delete success status when deleting stacks, I haven't found a single case where this is logged in CI 🤔

There are a few cases where deletion seems to fail though, e.g.

2023-08-30T07:42:43.410 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id FilePublishingRoleDefaultPolicy in iteration cycle 2. Retrying in next cycle.
2023-08-30T07:42:43.419 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id ImagePublishingRoleDefaultPolicy in iteration cycle 2. Retrying in next cycle.
2023-08-30T07:42:43.428 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id FilePublishingRoleDefaultPolicy in iteration cycle 3. Retrying in next cycle.
2023-08-30T07:42:43.438 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id ImagePublishingRoleDefaultPolicy in iteration cycle 3. Retrying in next cycle.
2023-08-30T07:42:43.447 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id FilePublishingRoleDefaultPolicy in iteration cycle 4. Retrying in next cycle.
2023-08-30T07:42:43.457 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id ImagePublishingRoleDefaultPolicy in iteration cycle 4. Retrying in next cycle.
2023-08-30T07:42:43.467 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id FilePublishingRoleDefaultPolicy in iteration cycle 5. Retrying in next cycle.
2023-08-30T07:42:43.476 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id ImagePublishingRoleDefaultPolicy in iteration cycle 5. Retrying in next cycle.
2023-08-30T07:42:43.487 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id FilePublishingRoleDefaultPolicy in iteration cycle 6. Retrying in next cycle.
2023-08-30T07:42:43.497 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id ImagePublishingRoleDefaultPolicy in iteration cycle 6. Retrying in next cycle.
2023-08-30T07:42:43.506 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id FilePublishingRoleDefaultPolicy in iteration cycle 7. Retrying in next cycle.
2023-08-30T07:42:43.516 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id ImagePublishingRoleDefaultPolicy in iteration cycle 7. Retrying in next cycle.
2023-08-30T07:42:43.526 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id FilePublishingRoleDefaultPolicy in iteration cycle 8. Retrying in next cycle.
2023-08-30T07:42:43.536 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id ImagePublishingRoleDefaultPolicy in iteration cycle 8. Retrying in next cycle.
2023-08-30T07:42:43.546 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id FilePublishingRoleDefaultPolicy in iteration cycle 9. Retrying in next cycle.
2023-08-30T07:42:43.555 WARNING --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Failed delete of resource with id ImagePublishingRoleDefaultPolicy in iteration cycle 9. Retrying in next cycle.
2023-08-30T07:42:43.565 ERROR --- [  asgi_gw_13] localstack.services.cloudformation.engine.template_deployer : Last cycle failed to delete resource with id FilePublishingRoleDefaultPolicy. Final exception: An error occurred (NoSuchEntity) when calling the DeletePolicy operation: Policy stack-cdk--54UC7WVIBG855 was not found.

We should investigate this a bit further. I think the whole delete stack process is currently a bit broken

@dominikschubert dominikschubert merged commit b3715e6 into master Aug 31, 2023
30 of 31 checks passed
@dominikschubert dominikschubert deleted the cfn-wait-for-deletion branch August 31, 2023 06:36
ashish1500616 pushed a commit to ashish1500616/localstack that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants