10000 CFn v2: Implement stack deletion by simonrw · Pull Request #12576 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

CFn v2: Implement stack deletion #12576

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

CFn v2: Implement stack deletion #12576

wants to merge 10 commits into from

Conversation

simonrw
Copy link
Contributor
@simonrw simonrw commented May 2, 2025

Motivation

Quite a few resource tests fail because the deletion of a stack is not correctly implemented. The tests assert that resources are deleted, but they aren't.

Changes

  • Implement deletion by creating a "hidden" change set, and performing the execution by passing None in for the after template and parameters
  • Update visit_node_depends_on to not visit the depends on target twice
  • Handle DELETE_IN_PROGRESS events in the _execute_resource_action
  • Hack around the payloads for our resource providers as some providers used desired_state and some used previous_state in the delete method
  • Unskip 22 test assertions to do with deleting the stack.

Limitations

While developing this change I noticed that we do not delete resources in the correct order. Namely:

Resources:
  A:
    Type: ...
    DependsOn:
      - B
  B:
    Type: ...

The resources are correctly deployed in the right order (B then A) but they are also deleted in that order (B then A) rather than the order in CloudFormation: A then B. I will tackle this in a follow up PR.

@simonrw simonrw added aws:cloudformation AWS CloudFormation semver: patch Non-breaking changes which can be included in patch releases labels May 2, 2025
Copy link
github-actions bot commented May 2, 2025

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 4m 56s ⏱️ - 37m 1s
2 118 tests  - 2 752  1 494 ✅  - 2 600  624 💤  - 152  0 ❌ ±0 
2 120 runs   - 2 752  1 494 ✅  - 2 600  626 💤  - 152  0 ❌ ±0 

Results for commit 00c0cb1. ± Comparison against base commit 8b3dcdd.

This pull request removes 2752 tests.
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…
This pull request skips 1 test.
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_sqs ‑ test_report_batch_item_failures

♻️ This comment has been updated with latest results.

@simonrw simonrw added this to the Playground milestone May 6, 2025
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from a459112 to 36039d4 Compare May 6, 2025 13:01
@simonrw simonrw requested a review from MEPalma May 6, 2025 16:12
@simonrw simonrw added the review: merge when ready Signals to the reviewer that a PR can be merged if accepted label May 6, 2025
@simonrw simonrw marked this pull request as ready for review May 6, 2025 16:13
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from 60db6f4 to ac56fa8 Compare May 6, 2025 20:42
@simonrw simonrw marked this pull request as draft May 7, 2025 10:10
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch 2 times, most recently from dc764a7 to a3bcf96 Compare May 8, 2025 13:02
@simonrw simonrw changed the base branch from master to MEP-CFN-parity_b0 May 8, 2025 13:03
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from a3bcf96 to d7e8b94 Compare May 8, 2025 13:03
Base automatically changed from MEP-CFN-parity_b0 to master May 8, 2025 13:04
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch 2 times, most recently from a46547d to 4ad07df Compare May 8, 2025 15:17
@simonrw simonrw marked this pull request as ready for review May 8, 2025 15:28
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from 006a1e2 to d71e8ab Compare May 8, 2025 16:31
@simonrw simonrw changed the base branch from master to cfn/v2/skip-binary-media-types May 8, 2025 16:32
Base automatically changed from cfn/v2/skip-binary-media-types to master May 8, 2025 17:15
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch 3 times, most recently from 577117b to 0e6c490 Compare June 2, 2025 10:19
Copy link
github-actions bot commented Jun 2, 2025

Test Results - Preflight, Unit

21 595 tests  ±0   19 940 ✅ ±0   6m 12s ⏱️ -1s
     1 suites ±0    1 655 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 00c0cb1. ± Comparison against base commit 8b3dcdd.

♻️ This comment has been updated with latest results.

Copy link
github-actions bot commented Jun 2, 2025

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 16s ⏱️ -7s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 00c0cb1. ± Comparison against base commit 8b3dcdd.

♻️ This comment has been updated with latest results.

@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from 0e6c490 to 72c5fd5 Compare June 2, 2025 11:03
@simonrw simonrw changed the base branch from master to MEP-CFN-fn_select June 2, 2025 11:03
@simonrw simonrw marked this pull request as draft June 2, 2025 11:06
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from 72c5fd5 to 96722da Compare June 2, 2025 11:07
Copy link
github-actions bot commented Jun 2, 2025

Test Results - Alternative Providers

986 tests  ±0   569 ✅ +1   28m 27s ⏱️ + 5m 38s
  4 suites ±0   417 💤  - 1 
  4 files   ±0     0 ❌ ±0 

Results for commit 00c0cb1. ± Comparison against base commit 8b3dcdd.

♻️ This comment has been updated with latest results.

Copy link
github-actions bot commented Jun 2, 2025

Test Results (amd64) - Integration, Bootstrap

    5 files  ±0      5 suites  ±0   2h 22m 37s ⏱️ +37s
5 227 tests +2  4 298 ✅ +1  929 💤 +1  0 ❌ ±0 
5 233 runs  +2  4 298 ✅ +1  935 💤 +1  0 ❌ ±0 

Results for commit 00c0cb1. ± Comparison against base commit 8b3dcdd.

This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both.
tests.bootstrap.test_strict_service_loading ‑ test_strict_service_loading
tests.bootstrap.test_service_loading ‑ test_eager_and_strict_service_loading
tests.bootstrap.test_service_loading ‑ test_eager_service_loading
tests.bootstrap.test_service_loading ‑ test_strict_service_loading
This pull request skips 1 test.
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_sqs ‑ test_report_batch_item_failures

♻️ This comment has been updated with latest results.

@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from 96722da to 820bb88 Compare June 2, 2025 13:43
Base automatically changed from MEP-CFN-fn_select to master June 2, 2025 16:22
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from 820bb88 to ecbe2a5 Compare June 3, 2025 13:46
simonrw added 8 commits June 3, 2025 22:52
We create a dummy change set which contains no resources. This is then
executed, therefore deleting all resources.

This commit also unskips (and re-validates) an ec2 test which exercises
this behaviour.
We should return the resource state if the resource is still being deleted, because the next time through the loop an item is fetched from the model.
Because we were not updating the stack template, when we tried to destroy the stack we would try to delete resources that no longer existed. Now we don't.
@simonrw simonrw force-pushed the cfn/v2/delete-stack branch from a99cf4d to 7572bc8 Compare June 3, 2025 21:52
@simonrw simonrw marked this pull request as ready for review June 5, 2025 15:13
simonrw added a commit that referenced this pull request Jun 9, 2025
## Motivation

Currently C
8000
DK bootstrap fails for a variety of reasons. Without this,
users who use CDK cannot test their applications with our new engine.

## Changes

* Handle dynamic parameter lookup of SSM parameters
* Set resource status on success/failure of a deployment
* Handle the case where an Fn::Join is called without a list as a second argument, in the special case of an empty string, e.g. `Fn::Join: ["", ""] as the CDK does this
* Add helper `find_stack_v2` function for finding v2 stacks
* Implement `describe_stack_resources` to support our CDK bootstrap tests
* Update the cdk bootstrap test skip reason as our lack of deletion
  causes test failures

## Testing

I have not added any integration tests for this since its functionality
is covered by our existing `TestCdkInit.test_cdk_bootstrap`
(parametrized) tests.
Unfortunately since we don't support deletions yet (see
#12576), the tests fail
when run together. Each test individually passes, so I've updated the
skip reason to reflect the updated nature of the failure.

To test manually, unskip the tests, and run

```
pytest tests/aws/services/cloudformation/v2/ported_from_v1/resources/test_cdk.py::TestCdkInit::test_cdk_bootstrap[10]
```
Copy link
Contributor
@MEPalma MEPalma left a comment

Choose a reason for hiding this comment

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

Thanks! This is definitely one of the key pieces for the v2 engine; great to see so many test cases unblocked now.

resource_name=depends_on_resource_logical_id, node_template=self._node_template
)
self.visit_node_resource(node_resource)
for node_resource in self._node_template.resources.resources:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we avoiding _get_node_resource_for? It looks like the user can define depends_on values that don't match a resource in the template

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:cloudformation AWS CloudFormation review: merge when ready Signals to the reviewer that a PR can be merged if accepted 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.

2 participants
0