CFn: DAG based deploy order #10849

simonrw · 2024-05-20T10:08:54Z

Motivation

When deploying stacks, our CloudFormation engine currently performs a number of retries to deploy a stack. This is required because the implementation does not take dependency order into account when deploying resources. Resources are strictly taken from the template in the order they are defined. We retry up to 30 times for deployments and 10 times for deletions.

This means that a stack with 31 resources defined in the reverse order of their dependencies will fail to deploy.

This is a particularly bad example, yes, but it is a fragile and brittle implementation that (especially on deletes) leads to potential breakages.

If we had a way of ordering dependencies so that we could iterate through them in order, then we would not need a retry process, and the behaviour would conform to the AWS behaviour. This would also be a first step towards concurrent deployment in our CFn engine.

Changes

Introduce order_resources and order_changes which order resources and changes based on the dependencies of each resource (or associated change).
Extract V2 template deployer which
- specialises two methods, apply_changes_in_loop and delete_stack
- new methods use the dependency sorting, and remove the outer loop retrying the deployment process a number of times
Add CFN_LEGACY_TEMPLATE_DEPLOYER flag to revert back to old behaviour
Perform initial validation in create_changeset where references to non-existent resources leads to a validation error during the call to create_changeset as per AWS, where before it would fail the stack deploy.
Fix resolve_dependencies for Fn::Join use case
Fix resources which return None in a progress event from the delete handlers:
- CDK::Metadata
- AWS::IAM::User
- AWS::Lambda::Alias
Add test that permutes a 3-chain of resources to cover all deployment orders (this test fails with the old implementation). This test also checks the order of resource deletions as well.

github-actions · 2024-05-20T11:08:50Z

LocalStack Community integration with Pro

2 files 2 suites 1h 39m 46s ⏱️
3 019 tests 2 696 ✅ 323 💤 0 ❌
3 021 runs 2 696 ✅ 325 💤 0 ❌

Results for commit 1aa3144.

♻️ This comment has been updated with latest results.

simonrw · 2024-05-21T11:13:30Z

localstack/services/cloudformation/engine/template_utils.py

@@ -74,6 +74,9 @@ def resolve_dependencies(d: dict, evaluated_conditions: dict[str, bool]) -> set[
                    items = items.union(resolve_dependencies(item, evaluated_conditions))
            else:
                pass
+    elif isinstance(d, list):


Added to support the second argument of Fn::Join e.g.

Fn::Join: - "" - - https:// - Ref: LsApi42D61DD0 - .execute-api. - Ref: AWS::Region - "." - Ref: AWS::URLSuffix - / - Ref: LsApiDeploymentStageprod82D8C5E7 - /

github-actions · 2024-05-21T11:54:21Z

S3 Image Test Results (AMD64 / ARM64)

2 files 2 suites 3m 7s ⏱️
403 tests 353 ✅ 50 💤 0 ❌
806 runs 706 ✅ 100 💤 0 ❌

Results for commit 1aa3144.

♻️ This comment has been updated with latest results.

simonrw · 2024-05-29T13:24:50Z

tests/aws/services/cloudformation/api/test_reference_resolving.py

-
-
-@markers.aws.only_localstack
-def test_unexisting_resource_dependency(deploy_cfn_template, aws_client):


This is a duplicate of test_useful_error_when_invalid_ref

pinzon

LGTM👌

pinzon · 2024-05-29T21:30:00Z

localstack-core/localstack/services/iam/resource_providers/aws_iam_user.py

@@ -133,7 +133,7 @@ def delete(
        """
        iam_client = request.aws_client_factory.iam
        iam_client.delete_user(UserName=request.desired_state["Id"])
-        return ProgressEvent(status=OperationStatus.SUCCESS, resource_model=None)
+        return ProgressEvent(status=OperationStatus.SUCCESS, resource_model=request.previous_state)


So, this is the appropriate value from now on right?

pinzon · 2024-05-29T21:30:49Z

tests/aws/services/cloudformation/api/test_stacks.py

+@pytest.mark.parametrize(
+    "deploy_order", TEMPLATE_ORDER_CASES, ids=["-".join(vals) for vals in TEMPLATE_ORDER_CASES]
+)
+def test_stack_deploy_order(deploy_cfn_template, aws_client, snapshot, deploy_order: tuple[str]):


Great test 👌

pinzon · 2024-05-29T21:32:04Z

tests/aws/services/cloudformation/engine/test_references.py

@@ -57,8 +57,8 @@ def test_fn_sub_cases(self, deploy_cfn_template, aws_client, snapshot):
        snapshot.match("outputs", deployment.outputs)


-@markers.aws.only_localstack
-def test_useful_error_when_invalid_ref(deploy_cfn_template):
+@markers.aws.validated


thrau

nice! this is great, a DAG is definitely the better concept for the deployer 🙌

just curious: have you looked into using cfnlint? i played around with this over a year ago so it's been a while, but IIRC it can create a graph from a cfn template that you can then run a depth first search, which i assume then gives the correct the deployment order?

simonrw · 2024-05-30T15:56:27Z

nice! this is great, a DAG is definitely the better concept for the deployer 🙌

just curious: have you looked into using cfnlint? i played around with this over a year ago so it's been a while, but IIRC it can create a graph from a cfn template that you can then run a depth first search, which i assume then gives the correct the deployment order?

I think it would be a definite improvement to use something like networkx to handle the graph-like nature. We already implement determining resource dependencies, but there could be something good to learn from with cfnlint. They do a lot of parsing of the template that we could learn from as well.

After investigating, I found that we don't actually use cfnlint, but it's a transitive dependency from moto. I don't think we should rely on cfnlint as it's a large package, and it might be nice to remove it in the future.

I want to get a first working version in place (even if hacky) which we can then refactor in a follow up PR.

dominikschubert

Great change! Let's get it over the line and see if we encounter any issues with it.

I haven't checked every line of the copied TemplateDeployerBase subclasses and focused mostly on the other parts where I didn't find any obvious issues 🥳 We also already did a walkthrough of the changes recently, so from my side it's a go 👍

dominikschubert · 2024-06-04T21:34:02Z

localstack-core/localstack/services/cdk/resource_providers/cdk_metadata.py

@@ -70,7 +70,7 @@ def delete(

        return ProgressEvent(
            status=OperationStatus.SUCCESS,
-            resource_model=None,
+            resource_model=request.previous_state,


is there a particular reason this was sneaked in here? :)

Just curious since I saw some warnings related to that operation

I was fixing warnings with teardowns, and found some warnings to do with the resource_model of the resulting ProgressEvent from the delete method be None when trying to access the physical resource id. I think it is within the scope to reduce the stack delete warnings, as this is evidence that the deploy order is correct.

dominikschubert · 2024-06-04T21:53:36Z

localstack-core/localstack/services/cloudformation/engine/template_deployer.py

@@ -791,7 +802,102 @@ class ChangeConfig(TypedDict):
    ResourceChange: ResourceChange


-class TemplateDeployer:
+def order_resources(


Would prefer having those outside of the template deployer, just to avoid ballooning that file again

dominikschubert · 2024-06-04T21:57:31Z

localstack-core/localstack/services/cloudformation/engine/template_deployer.py

+    nodes: dict[str, list[str]] = {}
+    for logical_resource_id, properties in resources.items():
+        nodes.setdefault(logical_resource_id, [])
+        deps = get_deps_for_resource(properties, resolved_conditions)


great that you were able to reuse that 👍

dominikschubert · 2024-06-04T21:59:18Z

localstack-core/localstack/services/cloudformation/engine/template_deployer.py

+    indegrees = {k: 0 for k in nodes.keys()}
+    for dependencies in nodes.values():
+        for dependency in dependencies:
+            indegrees[dependency] += 1
+
+    # Place all elements with indegree 0 in queue
+    queue = [k for k in nodes.keys() if indegrees[k] == 0]
+
+    sorted_logical_resource_ids = []
+
+    # Continue until all nodes have been dealt with
+    while len(queue) > 0:
+        # node of current iteration is the first one from the queue
+        curr = queue.pop(0)
+        sorted_logical_resource_ids.append(curr)
+
+        # remove the current node from other dependencies
+        for dependency in nodes[curr]:
+            indegrees[dependency] -= 1
+
+            if indegrees[dependency] == 0:
+                queue.append(dependency)
+
+    # check for circular dependencies
+    if len(sorted_logical_resource_ids) != len(nodes):
+        raise Exception("Circular dependency found.")


nit: extracting the circular dependency detection would help make it more readable

dominikschubert · 2024-06-04T22:03:25Z

localstack-core/localstack/services/cloudformation/engine/template_deployer.py

+    return sorted_changes
+
+
+class TemplateDeployerBase(ABC):


While I appreciate it, I also really hope we can remove this soon*ish 😬

simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label May 20, 2024

simonrw self-assigned this May 20, 2024

simonrw force-pushed the cfn/better-resource-deploy-order branch from e483288 to 0ba64e5 Compare May 20, 2024 21:17

simonrw commented May 21, 2024

View reviewed changes

simonrw force-pushed the cfn/better-resource-deploy-order branch 4 times, most recently from f4fbf38 to 9196d9d Compare May 28, 2024 11:52

simonrw marked this pull request as ready for review May 29, 2024 13:23

simonrw requested review from joe4dev, dominikschubert, dfangl, pinzon, Morijarti and thrau as code owners May 29, 2024 13:23

simonrw commented May 29, 2024

View reviewed changes

pinzon approved these changes May 29, 2024

View reviewed changes

thrau reviewed May 30, 2024

View reviewed changes

dominikschubert approved these changes Jun 4, 2024

View reviewed changes

simonrw and others added 8 commits June 5, 2024 22:00

Add resources ordering function

2efc47b

Extract order_changes function and integrate

d26ed2b

Ignore parameters when resolving dependencies

05cce63

Fix missing resource in sort

1d0f48e

Handle !Join in resolve_dependencies

893481a

Add skipped test for shuffling deployment order

3565680

Implement correct teardown order

5bcb937

Handle invalid resource reference

5167c01

simonrw and others added 11 commits June 5, 2024 22:00

Fix unit test

9a19da0

Add legacy template deployer config variable

2f1f3c4

Introduce abstraction on template deployer implementation

f12791f

Validate missing resources in template early

04d41a1

Update the template_original for updated stacks

2fdc1c8

Remove duplicate test

311bb45

validate transformed template

0d6f9bb

Fix missing resource model for delete events

a9a9d84

Remove old test_stack_deploy_order

c7762fb

Cleanup

a730f46

Extract resource ordering to new submodule

1aa3144

simonrw force-pushed the cfn/better-resource-deploy-order branch from 8882126 to 1aa3144 Compare June 5, 2024 21:06

simonrw merged commit 43e8992 into master Jun 6, 2024
37 checks passed

simonrw deleted the cfn/better-resource-deploy-order branch June 6, 2024 12:34

dominikschubert mentioned this pull request Jun 11, 2024

Add note to CFn service page about the new default deployment engine localstack/docs#1320

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CFn: DAG based deploy order #10849

CFn: DAG based deploy order #10849

simonrw commented May 20, 2024 •

edited

github-actions bot commented May 20, 2024 •

edited

simonrw May 21, 2024 •

edited

github-actions bot commented May 21, 2024 •

edited

simonrw May 29, 2024

pinzon left a comment

pinzon May 29, 2024

pinzon May 29, 2024

pinzon May 29, 2024

thrau left a comment

simonrw commented May 30, 2024

dominikschubert left a comment

dominikschubert Jun 4, 2024

dominikschubert Jun 4, 2024

simonrw Jun 5, 2024

dominikschubert Jun 4, 2024

dominikschubert Jun 4, 2024

dominikschubert Jun 4, 2024

dominikschubert Jun 4, 2024



		@markers.aws.only_localstack
		def test_unexisting_resource_dependency(deploy_cfn_template, aws_client):

CFn: DAG based deploy order #10849

CFn: DAG based deploy order #10849

Conversation

simonrw commented May 20, 2024 • edited

Motivation

Changes

github-actions bot commented May 20, 2024 • edited

LocalStack Community integration with Pro

simonrw May 21, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented May 21, 2024 • edited

S3 Image Test Results (AMD64 / ARM64)

Choose a reason for hiding this comment

pinzon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thrau left a comment

Choose a reason for hiding this comment

simonrw commented May 30, 2024

dominikschubert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonrw commented May 20, 2024 •

edited

github-actions bot commented May 20, 2024 •

edited

simonrw May 21, 2024 •

edited

github-actions bot commented May 21, 2024 •

edited