Skip to content

CFNV2: correct state modelling of failed stacks #13040

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.

Sign up for GitHub

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

Jump to bottom
Merged
simonrw merged 16 commits into from
Aug 21, 2025
Merged

CFNV2: correct state modelling of failed stacks #13040

simonrw merged 16 commits into from
Aug 21, 2025

Conversation

@simonrw
Copy link
Contributor

@simonrw simonrw commented Aug 20, 2025 •
edited
Loading

Motivation

While implementing the fix for test_deletion_of_failed_nested_stack I found that our modelling of stack and change set state needed some work, particularly around deletion.

Note

This PR is part of a stack building in #13034

Important

This is the final PR to unskip all CloudFormation tests for the new engine in this repo 🎉

Changes

Service changes

  • Allow the execution to contain a failure_message. If this value is present it means the deployment was a failure. Otherwise it was successful
  • Create a specific exception for triggering rollback
    • this is not specifically handled at the moment, but it may come in handy in the future
    • It is used to terminate the deployment process so no further resources are deployed after failure
  • If the stack fails then model the rollback process
  • Only assign the physical resource id from resources that succeed to deploy
  • Signal that an operation may be "part of a replacement"
    • when performing replacements, two actual operations occur:
      • creation of new resource
      • deletion of old resource
    • before this change, after an update the deleted state (which is only partial, with no physical resource id in particular) was being stored on the stack causing further bugs
    • now with this change we only update the stack state if the deletion is not part of a replacement, so we don't clobber the new resources' state
  • Previously we were not setting failed resources' state on the stack which caused failures to look up the resource properties in future operations. This is now fixed
  • Model the resource status in the preproc (and associated types)
  • In the transformer, override visit_node_resource to skip failures to visit and just move on to the properties to coninue traversing the graph
  • Better handling of state propagation from Executor -> ChangeSet/Stack after execute_change_set
  • Correctly set stack status after create_stack on failure
  • Increase API parity in minor ways

Test changes

  • 🎉Remove the skip_if_v2_provider decorator as we don't need it any more 🎉
  • Validate test_create_and_then_update_refreshes_template_metadata test
    • the previous version was broken
    • the v1 engine does not support this new version of the test 😂
  • Support test_deletion_of_failed_nested_stack
  • Improve the test_sns_topic_fifo_without_suffix_fails test to move the classification from needs_fixing to validated

Follow ups

  • Differentiate between failures to update and failure to create
  • Unify the post deploy actions from execute_change_set with those of create_stack and update_stack

@simonrw simonrw added this to the 4.8 milestone Aug 20, 2025
@simonrw simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Aug 20, 2025
@simonrw simonrw force-pushed the cfn/v2/deletion-state-modelling branch 2 times, most recently from 7b2b868 to 00e725c Compare August 20, 2025 21:28
@github-actions
Copy link

github-actions bot commented Aug 20, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 144 tests  ±0   20 407 ✅ ±0   6m 29s ⏱️ +6s
     1 suites ±0    1 737 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 5326eb2. ± Comparison against base commit 93a1ec5.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Aug 20, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 19s ⏱️ +9s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5326eb2. ± Comparison against base commit 93a1ec5.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Aug 20, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 42m 58s ⏱️ + 1h 19m 15s
4 631 tests +4 076  4 189 ✅ +3 859  442 💤 +217  0 ❌ ±0 
4 633 runs  +4 076  4 189 ✅ +3 859  444 💤 +217  0 ❌ ±0 

Results for commit 5326eb2. ± Comparison against base commit 93a1ec5.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Aug 20, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files  ±    0      5 suites  ±0   2h 21m 9s ⏱️ + 1h 47m 37s
4 990 tests +4 411  4 396 ✅ +4 041  594 💤 +370  0 ❌ ±0 
4 996 runs  +4 411  4 396 ✅ +4 041  600 💤 +370  0 ❌ ±0 

Results for commit 5326eb2. ± Comparison against base commit 93a1ec5.

♻️ This comment has been updated with latest results.

@simonrw simonrw force-pushed the cfn/v2/deletion-state-modelling branch from d838141 to 38d98ef Compare August 21, 2025 12:17
@simonrw simonrw marked this pull request as ready for review August 21, 2025 15:51
Base automatically changed from cfn/v2/language-extensions/foreach to main August 21, 2025 16:48
@simonrw simonrw added the docs: skip Pull request does not require documentation changes label Aug 21, 2025
pinzon
pinzon approved these changes Aug 21, 2025
View reviewed changes
Copy link
Member

@pinzon pinzon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

More parity!! 🎉

Some ideas for the future:

  • make the visitor group the fnTransforms and forEachs together so we don't have to add new class attributes every time.
  • we will need to start adding tests for rollbacks 🥳
  • Implement the staticPreproc

Also added a comment that we should address sooner rather than later.

localstack-core/localstack/services/cloudformation/engine/v2/change_set_model_executor.py
Comment on lines +106 to +113
if failure_message:
# TODO: differentiate between update and create
self._change_set.stack.set_stack_status(StackStatus.ROLLBACK_IN_PROGRESS)
else:
# TODO: correct status
self._change_set.stack.set_stack_status(
StackStatus.UPDATE_COMPLETE_CLEANUP_IN_PROGRESS
)
Copy link
Member

@pinzon pinzon Aug 21, 2025

Choose a reason for hiding this comment

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

Finally rollback support 🎉

tests/aws/services/cloudformation/conftest.py
SKIP_TYPE_RE = re.compile(r"^CFNV2\((?P<reason>[^\)]+)\)")


def skip_if_v2_provider(*types: str, reason: str = ""):
Copy link
Member

@pinzon pinzon Aug 21, 2025

Choose a reason for hiding this comment

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

🎉

localstack-core/localstack/services/cloudformation/v2/provider.py
new_stack_status = StackStatus.CREATE_FAILED

change_set.stack.set_stack_status(new_stack_status)
# stack status is taken care of in the executor
Copy link
Member

@pinzon pinzon Aug 21, 2025

Choose a reason for hiding this comment

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

Important: We should be more constant on which entity is in charge of updating the stack status. As it is now, sometimes it's the provider, other times it's the executor. This could cause confusion when working in the codebase.

Copy link
Contributor Author

@simonrw simonrw Aug 21, 2025

Choose a reason for hiding this comment

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

You are absolutely right I'm glad you picked up on that. A good cleanup opportunity

@simonrw @pinzon
Update test_changesets.py
5181ffb 
Co-authored-by: Cristopher Pinzón <[email protected]>
@simonrw simonrw merged commit 1679947 into main Aug 21, 2025
11 of 13 checks passed
@simonrw simonrw deleted the cfn/v2/deletion-state-modelling branch August 21, 2025 17:25
@simonrw simonrw mentioned this pull request Sep 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pinzon pinzon pinzon approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

Assignees

@pinzon pinzon

Labels

docs: skip Pull request does not require documentation changes semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Milestone

4.8

Development

Successfully merging this pull request may close these issues.

3 participants

@simonrw @pinzon