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

[aws-eks] allow specifying timeouts for HelmCharts #8215

Closed
gibbster opened this issue May 26, 2020 · 6 comments · Fixed by #8338
Closed

[aws-eks] allow specifying timeouts for HelmCharts #8215

gibbster opened this issue May 26, 2020 · 6 comments · Fixed by #8338
Assignees
Labels
@aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service effort/small Small work item – less than a day of effort feature-request A feature should be added or improved.

Comments

@gibbster
Copy link

For long running helm charts the creation times out.

Reproduction Steps

I had this issue with the following resource (based on a minimal cluster defined through the CDK Cluster component).

new eks.HelmChart(this, 'spinnaker', {
  cluster,
  chart: 'spinnaker',
  repository: 'https://kubernetes-charts.storage.googleapis.com/',
  version: '2.0.0-rc3',
  namespace: 'spinnaker'
});

Error Log

3/9 | 3:05:54 PM | CREATE_FAILED | Custom::AWSCDK-EKS-HelmChart | spinnaker/Resource/Default (spinnakerC4B2DB2C) Failed to create resource. TimeoutError: Connection timed out after 120000ms

Environment

  • **CLI Version :1.41.0 (build 9e071d2)
  • **Framework Version: 1.41.0
  • **OS : macOS 10.14.6
  • **Language : typescript

Other

The helm command has an option for timeout, and in fact the chart I was trying to run (stable/spinnaker) suggests a timeout of 600s: https://github.com/helm/charts/blob/master/stable/spinnaker/README.md. I believe that the chart should add timeout as a option:

export interface HelmChartOptions {
/**
* The name of the chart.
*/
readonly chart: string;
/**
* The name of the release.
* @default - If no release name is given, it will use the last 53 characters of the node's unique id.
*/
readonly release?: string;
/**
* The chart version to install.
* @default - If this is not specified, the latest version is installed
*/
readonly version?: string;
/**
* The repository which contains the chart. For example: https://kubernetes-charts.storage.googleapis.com/
* @default - No repository will be used, which means that the chart needs to be an absolute URL.
*/
readonly repository?: string;
/**
* The Kubernetes namespace scope of the requests.
* @default default
*/
readonly namespace?: string;
/**
* The values to be used by the chart.
* @default - No values are provided to the chart.
*/
readonly values?: {[key: string]: any};
/**
* Whether or not Helm should wait until all Pods, PVCs, Services, and minimum number of Pods of a
* Deployment, StatefulSet, or ReplicaSet are in a ready state before marking the release as successful.
* @default - Helm will not wait before marking release as successful
*/
readonly wait?: boolean;
}

1.41.0 (build 9e071d2)


This is 🐛 Bug Report

@gibbster gibbster added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 26, 2020
@SomayaB SomayaB added the @aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service label May 27, 2020
@eladb
Copy link
Contributor

eladb commented May 27, 2020

It would be possible to allow timeouts of up to 15m. @pahud is this something you'd be interested to pick up?

@eladb eladb added feature-request A feature should be added or improved. and removed bug This issue is a bug. labels May 27, 2020
@eladb eladb changed the title HelmChart times out with long running charts eks: allow specifying timeouts for HelmCharts May 27, 2020
@eladb eladb added the effort/small Small work item – less than a day of effort label May 27, 2020
@pahud
Copy link
Contributor

pahud commented May 27, 2020

@eladb Sure!

The interesting thing is the Kubelctl Provider default timeout is 15min now.

timeout: Duration.minutes(15),

And the error message provided doesn't seem to be a lambda timeout

3/9 | 3:05:54 PM | CREATE_FAILED | Custom::AWSCDK-EKS-HelmChart | spinnaker/Resource/Default (spinnakerC4B2DB2C) Failed to create resource. TimeoutError: Connection timed out after 120000ms

I'll try re-produce it in my environment and check the logs.

@eduardomourar
Copy link
Contributor

We are getting the same error while running:

cluster.addChart('Prometheus', {
  chart: 'prometheus-operator',
  repository: 'https://kubernetes-charts.storage.googleapis.com/',
  version: '8.13.8',
  namespace: 'default'
});

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Jun 2, 2020
@eduardomourar
Copy link
Contributor

I can confirm that there is a timeout from the AWS SDK (as specified here) and it can solved by adding to the provider framework code:

lambda.config.update({httpOptions: { timeout: 900000 }});

I will make a PR soon to fix this.

@pahud
Copy link
Contributor

pahud commented Jun 2, 2020

It sounds like an underlying http timeout between the provider framework and the kubectl provider which executes the helm command. Is the fix working in your environment? @eduardomourar

@eduardomourar
Copy link
Contributor

Yes, so far so good. My helm charts are taking about 3 minutes to deploy and without any issue now.

I believe the issue is that the invokeFunction used by the provider framework has this 2 minutes timeout, so it was not honoring the overall timeout (15 minutes) set for the target lambda.

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jun 5, 2020
@mergify mergify bot closed this as completed in #8338 Jun 11, 2020
mergify bot pushed a commit that referenced this issue Jun 11, 2020
This creates an additional option called `timeout` that will be passed down whenever deploying helm chart to an EKS cluster.

In order to allow the timeout parameter to work while performing helm commands, the provider framework has to honor the maximum timeout of 15 minutes from target process (lambda in this case).

closes #8215 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@iliapolo iliapolo changed the title eks: allow specifying timeouts for HelmCharts [aws-eks] allow specifying timeouts for HelmCharts Aug 16, 2020
@iliapolo iliapolo removed the in-progress This issue is being actively worked on. label Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service effort/small Small work item – less than a day of effort feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants