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.

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

fix(eks): Helm release name length & --wait option. #6276

Merged
merged 4 commits into from Feb 18, 2020
Merged

fix(eks): Helm release name length & --wait option. #6276

merged 4 commits into from Feb 18, 2020

Conversation

michaelmoussa
Copy link
Contributor

This PR addresses a couple of issues I encountered earlier today involving the HelmChart construct.

  1. The Helm release name length limit is 53 characters, not 63 (source). Adding a chart will always fail if CDK is allowed to name it automatically, since it uses the last 63 characters of the node's unique ID.

  2. Since Helm does not wait by default for all resources created by the chart to be in a ready/running state, a race condition can exist that will cause a CDK deploy to fail if CDK moves on to the next Helm chart before the previous one is finished, even if an explicit dependency has been added. For example, adding the metrics-server chart, followed by external-dns, even if the external-dns chart has metrics-server as a dependency, can produce a Error: could not get apiVersions from Kubernetes: unable to retrieve the complete list of server APIs: metrics.k8s.io/v1beta1: the server is currently unable to handle the request. Setting the new wait property to true will add the --wait flag to the Helm tool execution, which will make Helm wait up to 300s for everything to finish before marking that CDK construct complete and allowing any dependent constructs to proceed.

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 1f6dc81
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 50fc191
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: da34454
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@michaelmoussa michaelmoussa mentioned this pull request Feb 14, 2020
Merged
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 766777b
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@michaelmoussa michaelmoussa mentioned this pull request Feb 16, 2020
Closed
eladb
eladb previously requested changes Feb 17, 2020
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Still needs some test snapshot updates

packages/@aws-cdk/aws-eks/lib/helm-chart.ts Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/helm-chart.ts Show resolved Hide resolved
packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py Show resolved Hide resolved
@mergify mergify bot dismissed eladb’s stale review February 17, 2020 18:52

Pull request has been modified.

@michaelmoussa
Copy link
Contributor Author

I rebased again and pushed the integration test expectations. Hopefully it will finish before the branch goes out of date again haha

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 44112d3
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 93486f8
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 40b66cd
  • 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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 9a60f3a
  • 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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 46aa551
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@michaelmoussa
Copy link
Contributor Author

Rebasing again...

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 749c366
  • 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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8d1053b
  • 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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: c91277a
  • 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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8b730b2
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@michaelmoussa
Fix Helm release name limit (53, not 63)
cdc6253 
See: https://github.com/helm/helm/blob/b63822ed12de1badbb61736d2f7ea4e756ef757d/pkg/action/install.go#L52-L55
@michaelmoussa
Support asking Helm to wait for install to finish
59d489b 
Currently, Helm will not wait until all applicable resources are in a ready
state before deciding a release is successful and moving on. This can be
problematic for subsequent chart installations that will necessarily fail
if the previous has not completed, even if explicitly added as a dependency.
@michaelmoussa
Update integration test expectations
db3a8bc
@michaelmoussa
Add test for --wait default
301e522
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 301e522
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Feb 18, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 1811e00 into aws:master Feb 18, 2020
@michaelmoussa michaelmoussa deleted the eks-on-fargate-helm-fixes branch February 19, 2020 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladb eladb eladb approved these changes

Assignees

@eladb eladb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@michaelmoussa @aws-cdk-automation @eladb