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

fix(cli): fix ecs hotswap deployment waiting conditions #27943

Closed
wants to merge 10 commits into from

Conversation

tomwwright
Copy link
Contributor

@tomwwright tomwwright commented Nov 11, 2023

Closes #27882. See linked issue for further details.

Changes 📝

  • Add integration tests for hotswap deployments for ECS
    • Test that ECS hotswap works
    • Test that ECS hotswap waits for the deployment to complete successfully
    • Test that ECS hotswap detects deployment failure
  • Fix custom waiter expression to match on rolloutState instead of task counts
    • Provide backup waiter that doesn't rely on rolloutState to support other deployment types
  • Fix custom waiter expression to detect failed deployments

Availability of rolloutState property ⚠️

Notably the documentation describes that rolloutState is only provided for services using the rolling update.

Screenshot 2023-11-11 at 1 55 19 pm

Relying on rolloutState would effectively exclude ECS Service hotswap deployment support for other deployment types. To avoid this, I've opted to match for success on the presence of a single deployment -- my observation of the deployment state (see linked issue) makes me feel this will suffice.

Note there is no test added for other deployment types

Support for failed deployments using rolloutState

The rolloutState property is used to detect failure if it is present -- this means that non-rolling update ecs services will be unable to detect failure (parity with where we were before anyway)

I've added tests and another waiter condition so that hotswap deployments detect that the deployment has failed. At this stage the error message for this is a bit obtuse -- I figure someone can improve the message in a follow-up PR if users find it confusing. Example:

✨ hotswapping resources:
  ✨ ECS Task Definition 'cdktest0l5zgdtshw5ecshotswaptaskdefinition6E807351'
  ✨ ECS Service 'cdktest-0l5zgdtshw5-ecs-hotswap-serviceService7DDC3B7C-TJs6rF3ur3Og'
      
❌  cdktest-0l5zgdtshw5-ecs-hotswap failed: ResourceNotReady: Resource is not in the state deploymentCompleted

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Nov 11, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team November 11, 2023 03:44
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Nov 11, 2023
@tomwwright tomwwright force-pushed the fix-ecs-service-hotswap-wait branch 2 times, most recently from f4a94bd to 39531ba Compare November 11, 2023 06:08
@tomwwright
Copy link
Contributor Author

Exemption Request Fix is for CLI so snapshots are not relevant

When possible may I please have integration tests run on this PR? Thank you 🙇

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Nov 11, 2023
@tomwwright tomwwright changed the title fix(cli): improve ecs hotswap deployment waiting condition fix(cli): fix ecs hotswap deployment waiting conditions Nov 11, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 11, 2023
@tomwwright tomwwright force-pushed the fix-ecs-service-hotswap-wait branch from 25d8e0c to 9e6ae94 Compare November 11, 2023 07:58
@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@tomwwright tomwwright force-pushed the fix-ecs-service-hotswap-wait branch from aa6968f to ad6d8c9 Compare December 9, 2023 02:06
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: ad6d8c9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Dec 11, 2023
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/27943/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@tomwwright
Copy link
Contributor Author

Can I please get this re-opened? It is not an abandoned PR as evidenced by the recent updates. If possible, I would prefer to avoid creating a new PR, to avoid the noise/effort as well as the fact it appears PRs are being handled roughly in creation order

mergify bot pushed a commit that referenced this pull request Mar 26, 2024
…re (#28448)

Reraising #27943 as it was incorrectly closed as stale. See original PR for details.

Closes #27882. See linked issue for further details.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort p1 pr/needs-cli-test-run This PR needs CLI tests run against it. pr/needs-maintainer-review This PR needs a review from a Core Team Member pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli: ecs service hotswap deployment does not wait for deployment to complete
2 participants