(cli): hotswap should not disable rollback

[tmokmss]

What is the problem?

Hi team!
Currently when we enable hotswap, it also disables rollback implicitly, which is not desirable at least for me. Here is why:

• Hotswap works without disabling rollback, e.g. by the command: cdk deploy --hotswap --rollback true. Then why does the hotswap flag currently have unnecessary and confusing side effect? If we need hotswap and no-rollback, we can just type cdk deploy --hotswap -R, which is shorter and more explicit.
• Hotswap can be enabled at any time whereas rollback must not be disabled in some cases. If we disable rollback when there's a replacement change, the deploy fails in the middle of deployment, requiring some manual operation to recover from the state. Due to this behavior, we need additional consideration when we disable rollback. That's why I think rollback shouldn't be disabled implicitly.

When I develop CDK apps, I always deploy with hotswap. Hotswap is great in that it automatically switch to a normal CFn deploy when there are changes that cannot be hotswapped, thus happy to use it almost brainlessly. However, I must also add --rollback true because otherwise deploy fails if there's any replacement change. Unlike hotswap, it won't switch when it cannot be used. It'd be great if we can just use --hotswap flag simply without worring about rollback. Actually I sometimes forget to add rollback flag, resulting in a failed deployment.

I'd like to hear your opinion about this, thanks!

Reproduction Steps

run deploy with hotswap enabled

npx cdk deploy --hotswap

What did you expect to happen?

Rollback is enabled as per default since I don't specify any flags related to it.

What actually happened?

Rollback is disabled. Actually if we need hotswap with rollback, we should have used the command below instead:

npx cdk deploy --hotswap --rollback true

CDK CLI Version

1.129.0

Framework Version

1.129.0

Node.js Version

v14.15.0

OS

macOS

Language

Typescript

Language Version

3.9.7

Other information

No response

[skinny85]

Hey @tmokmss,

thanks so much for opening the issue! A few comments.

First of all, I don't think you need to add the true part - I think just passing cdk deploy --hotswap --rollback will do what you want. So that's a little bit shorter 🙂.

Second, I assume the only problem with disabling rollback is resource replacement, right? In that case, --hotswap can't really be used - it will always fall back to doing a full deployment. Which means passing --hotswap doesn't really make sense for these changes, as it's the same as running cdk deploy. But I guess the problem here is that you just want to invoke cdk deploy --hotswap always - you don't want to think about what command to run, am I understanding you correctly?

Finally, we actually have another feature in the works, called "cdk watch" (here's the PR: #17240). With it, the idea is that you would invoke cdk watch --rollback, and cdk deploy will happen automatically, without the need for invoking another command, when a change to your projects - which means you won't forget to put --rollback anymore, you will only have to put it once, at the beginning! Would that change your opinion on the default?

@eladb do you want to chime in here?

Thanks,
Adam

[tmokmss]

Hi @skinny85

But I guess the problem here is that you just want to invoke cdk deploy --hotswap always - you don't want to think about what command to run, am I understanding you correctly?

Yes, that's exactly what I thought! I'm too lazy to decide which option to use on each deploy.
And cdk watch looks great. I'm really looking forward to using it:)

What I don't understand yet is that is there any reason to disable rollback by default when we use hotswap. Isn't it simpler to enable them by separate flags?

Thanks.

[rix0rrr]

Also --rollback is the default. You don't have to pass it, it exists so that you can write --no-rollback.

[skinny85]

Also --rollback is the default. You don't have to pass it, it exists so that you can write --no-rollback.

--no-rollback is the default if --hotswap is passed.

[skinny85]

And it is also the default for the new watch command.

[skinny85]

@tmokmss one clarifying question. Does a CloudFormation deployment with rollback disabled always fail when there's a replacement? Or does it fail only if the replacement failed (for example, that could happen if you used a physical name of the resource, so creating the replacement one fails)?

[tmokmss]

@skinny85
I think it always fails, because CFn returns the following error (I couldn't find any CFn document referring to this behavior though.)
Replacement type updates not supported on stack with disable-rollback.

I confirmed to reproduce by the following steps (I know hotswap does nothing on this kind of stack, but just for an example):

1. deploy the stack below: npx cdk deploy

import * as cdk from "@aws-cdk/core";
import * as dynamodb from "@aws-cdk/aws-dynamodb";

export class TestStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    new dynamodb.Table(this, "Table", {
      partitionKey: { name: "id", type: dynamodb.AttributeType.STRING },
    });
  }
}

2. change partitonKey name "id" to some string to trigger a replacement change
3. deploy it with hotswap (npx cdk deploy --hotswap) and you'll get the below error

1:09:29 PM | UPDATE_FAILED        | AWS::DynamoDB::Table | TableCD117FA1
Replacement type updates not supported on stack with disable-rollback.

4. Now you cannot deploy even without hotswap flag (npx cdk deploy).

TestStack failed: Error [ValidationError]: This stack is currently in a non-terminal [UPDATE_FAILED] state. To update the stack from this state, please use the disable-rollback parameter with update-stack API. To rollback to the last known good state, use the rollback-stack API

I guess CFn doesn't allow this because otherwise a deployment failure without rollback may result in a complex situation such that two physical resources (old and new) are associated with the same logical id.

[eladb]

I guess CFN doesn't allow this because otherwise a deployment failure without rollback may result in a complex situation such that two physical resources (old and new) are associated with the same logical id.

Yes, I think you are correct that this is the motivation. I also tend to agree that this is a strong case for not disabling rollbacks for hotswaps by default and requiring an explicit opt-in switch to disable them. I think it will also be valuable to add some documentation about the fact if the deployment includes resource replacement, rollbacks cannot be disabled.

[skinny85]

@eladb agreed. I've opened a PR changing this behavior for --hotswap: #17317, would appreciate a review.

I'll also make sure the "watch" PR (#17240) is updated to no longer default to rollback being disabled.

[github-actions]

⚠️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.