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

cli: allow users to force-hotswap when they want to #21773

Closed
1 of 2 tasks
huyphan opened this issue Aug 26, 2022 · 4 comments
Closed
1 of 2 tasks

cli: allow users to force-hotswap when they want to #21773

huyphan opened this issue Aug 26, 2022 · 4 comments
Assignees
Labels
@aws-cdk/aws-lambda Related to AWS Lambda effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 package/tools Related to AWS CDK Tools or CLI

Comments

@huyphan
Copy link
Contributor

huyphan commented Aug 26, 2022

Describe the feature

This proposal can be controversial as it changes the mental model of hotswap. So please bear with me. The main idea is to allow users to force a hotswap deployment even when there are non-hotswappable changes.

Use Case

The idea today is that, hotswap only works if the stack's changeset contains only hotswappable changes, otherwise a full deployment will be performed. This makes sense. But then the question is, what should users do when they have a non-hotswappable change in their changeset?

First, let's talk about what those non-hotswappable changes are. There are three categories:

  1. Those that are introduced by some code changes that are explicitly made by users. In this case, users only have to perform the full deployment once to get rid of the changes in the next deployment.
  2. Those that are implicitly introduced by other hotswappable changes. For example: an update to a Lambda function version (which is hotswappable) always lead to an update to associated metrics and alarms (which are non-hotswappable).
  3. Those that are not actually changes/updates, but are the CFN templates that CDK can't resolve at synthesis time to determine whether there will be changes or not. For example: Fn:Import intrincsic function.

For the last two categories, users have a few options to enable hotswap back:

  1. Permanently removing the resources that cause the unavoidable non-hotswappable changes. This is obviously not an option for many users.
  2. Temporarily removing those resources every time they need to hotswap. This is too tedious and is a terrible user experience.
  3. Having a branching logic (i.e. if ... else ...) that permanently removes the resources only when deploying to development/non-production stack. This approach removes the ability to test the production changes in development stack.
  4. Waiting for CDK to support the non-hotswappable changes.

So, the fourth option is the most feasible option there. Unfortunately, that's not a sustainable option for the CDK team given the number of potential non-hotswappable changes. It's almost a never-ending chase. Users usually have no idea when they can be unblocked.

Proposal

The reason why we ended up with today's situation is because we think if there's a change in the template, they must be deployed, even when hotswapping. That's not always necessary. If I'm testing my Lambda code, I only need the version and alias to be updated. Everything else is optional.

When we see a non-hotswappable changes, users should be able to say "it is okay to skip those changes for now, I just want my code to be up and run"; then CDK will just hotswap all the things it can and leave the rest behind.

Potential implementation

Example of how it would work when user runs cdk deploy --hotswap <stack-name> and there are unsupported changes:


This deployment contains changes that are not supported by the hotswap feature.


Non-hotswappable changes
┌───┬──────────────────────────────────-┬─────────────────────────┐
│   │ Logical ID                        | Type                    │
├───┼───────────────────────────────────┼─────────────────────────┼
│ ~ │ MyLambdaVersionMetric             | AWS::CloudWatch::Metric |
└───┴───────────────────────────────────┴─────────────────────────┘

How do you want to proceed? 
1) Perform a [F]ull deployment
2) Perform [H]otswap and skip the unsupported changes
3) [A]bort the deployment

Your choice [1/2/3/F/H/A]: 

(I stole the idea of prompting users from @CaerusKaru, so the credit goes to him.)

The concerns

This feature potentially causes more drifts in user's stacks; and this is mostly a concern for a production stack. But this is an existing risk of hotswap. In the RFC 0001-cdk-update , we stated that

The interface to CDK hotswap requires the developer to specify the stacks that they will be deploying explicitly, so by default it is affecting only the developer's stacks, and when a production stack is defined it is up to the AWS account administrator to ensure that the interactive developer's roles do not have modification access to the hotswap resources.

So the goal is not about making hotswap safe, but rather controlling the destination where hotswap is allowed.

Proposed Solution

No response

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

N/A

Environment details (OS name and version, etc.)

N/A

@huyphan huyphan added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 26, 2022
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Aug 26, 2022
@huyphan huyphan changed the title (module name): (short issue description) cli: allow users to force-hotswap when they want to Aug 26, 2022
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Aug 26, 2022
@peterwoodworth peterwoodworth added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Sep 1, 2022
@kaizencc kaizencc removed their assignment Sep 7, 2022
@comcalvi
Copy link
Contributor

comcalvi commented Dec 12, 2022

@huyphan thanks for opening this feature request. I see the use case for this, and would be happy to implement it. I'm thinking this will take a new form, --hotswap-only, that will hotswap whatever it can and ignore non-hotswappable changes.

There are two levels of "ignoring" we can do. The first is to only ignore changes to non-hotswappable resources. This covers the use case you described (eg, a lambda function is updated, which in turn causes a metric to update). We could go one step further, and allow hotswap to ignore changes to properties on a hotswappable resource itself. For example, this would allow you to change a Lambda Function's name and code and allow a hotswap deployment to proceed. This deployment would not change the function name, only its code. The function name won't be updated until a full deployment is performed. Are you asking for the first level I described only, or the second?

@comcalvi comcalvi assigned comcalvi and unassigned rix0rrr Dec 12, 2022
@huyphan
Copy link
Contributor Author

huyphan commented Dec 12, 2022

Hey @comcalvi , I think the first level would work for me and would be more intuitive for general users.

mergify bot pushed a commit that referenced this issue Feb 8, 2023
…fall back if necessary (#23653)

Changes the behavior of `--hotswap` to ignore all non-hotswappable changes and hotswap what it can. This works at two levels: changes to non-hotswappable resources are ignored, as well as non-hotswappable changes to hotswappable resources (eg `Tags` on a Lambda Function).

In addition, non-hotswappable changes are now logged; the logical ID, rejected changes, resource type, and reason why the changes were rejected are all provided for each non-hotswappable change.

At some point, support for tags of lambda functions was added. This either broke or simply never worked, and so this PR removes all logic to handle Tags.

The existing behavior of `--hotswap` can be used in `--hotswap-fallback`. It is preserved and unmodified by this change.

Closes #22784, #21773, #21556, #23640.

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@comcalvi
Copy link
Contributor

fixed by #23653

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Development

No branches or pull requests

5 participants