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

feat: Add the ability to manage Provisioners within the helm chart #1956

Closed
wants to merge 8 commits into from
Closed

feat: Add the ability to manage Provisioners within the helm chart #1956

wants to merge 8 commits into from

Conversation

cest-pas-faux
Copy link
Contributor

Fixes #

Description

This PR adds the Provisioner template to the chart to render the Provisioners along the helm release and manage their lifecycle instead of having to handle plain manifests in another part of the CI.

Included a basic sample for the documentation, the spec key from the array will be passed as is.

$ cat -n tmpfiles/karpenter/templates/provisioner.yaml
     1  ---
     2  # Source: karpenter/templates/provisioner.yaml
     3  apiVersion: karpenter.sh/v1alpha5
     4  kind: Provisioner
     5  metadata:
     6    name: sample-1
     7    namespace: karpenter
     8    labels:
     9      helm.sh/chart: karpenter-0.11.1
    10      app.kubernetes.io/name: karpenter
    11      app.kubernetes.io/instance: release-name
    12      app.kubernetes.io/version: "0.11.1"
    13      app.kubernetes.io/managed-by: Helm
    14    annotations:
    15      test: toto
    16  spec:
    17    limits:
    18      resources:
    19        cpu: "1000"
    20        memory: 1000Gi
    21    requirements:
    22    - key: node.kubernetes.io/instance-type
    23      operator: In
    24      values:
    25      - m5.large
    26      - m5.2xlarge
    27    - key: topology.kubernetes.io/zone
    28      operator: In
    29      values:
    30      - us-west-2a
    31      - us-west-2b
    32    - key: kubernetes.io/arch
    33      operator: In
    34      values:
    35      - arm64
    36      - amd64
    37    - key: karpenter.sh/capacity-type
    38      operator: In
    39      values:
    40      - spot
    41      - on-demand
    42    ttlSecondsAfterEmpty: 30
    43    ttlSecondsUntilExpired: 2592000

How was this change tested?

  • Locally by rendering the templates
  • Compared templates against the ones actually used on production
  • Deployed a template in a dummy cluster

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Release Note

NONE

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

@cest-pas-faux cest-pas-faux requested a review from a team as a code owner June 19, 2022 13:44
@cest-pas-faux cest-pas-faux requested a review from dewjam June 19, 2022 13:44
@netlify
Copy link

netlify bot commented Jun 19, 2022

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 9238fa3
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/62b0bd18eea5db0008b75243

@cest-pas-faux cest-pas-faux changed the title feat: Add the ability to manage Provisioners with the helm chart feat: Add the ability to manage Provisioners within the helm chart Jun 19, 2022
Copy link
Contributor

@ellistarn ellistarn left a comment

Choose a reason for hiding this comment

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

very cool!


# -- Provisioners definition
provisioners: {}
# - metadata:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this example, can you comment the link to the provisioner API in the website so this doesn't become obsolete? https://karpenter.sh/v0.11.1/provisioner/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified the sample and added a helm-doc link, let me know if it suits you.
Also, how to make this link updated at each release ?

@bwagner5
Copy link
Contributor

Hey @cest-pas-faux ! Thanks for this PR!

There are some issues with managing resources like the Provisioner within the helm chart to install Karpenter. The Provisioner resource requires a validating and defaulting webhook to run on create/update. So if a Provisioner is applied by helm while Karpenter is applying the Karpenter controller and webhook pod, there's a race that occurs where the Provisioner resource apply will fail and cause the helm install to fail. The race is pretty easy to trigger too since there's 5-10 seconds of startup latency on the Karpenter pod. We previously had an opinionated "default" provisioner in the helm chart and ended up removing it because it was causing a lot of issues.

In #810 we moved the "default" Provisioner to the post-install hook in Helm since Helm doesn't order the resources other than the CRDs in the crds dir. post-install resources are not managed by helm though for updates so it would be a one-time install. Also, when using helm install with Provisioner in the post-install hook, a user would be required to use the --wait flag to avoid the webhook race again.

I think this would be cool if helm had the proper hooks to apply resources like this :(

@cest-pas-faux
Copy link
Contributor Author

cest-pas-faux commented Jun 22, 2022

Hey @cest-pas-faux ! Thanks for this PR!

There are some issues with managing resources like the Provisioner within the helm chart to install Karpenter. The Provisioner resource requires a validating and defaulting webhook to run on create/update. So if a Provisioner is applied by helm while Karpenter is applying the Karpenter controller and webhook pod, there's a race that occurs where the Provisioner resource apply will fail and cause the helm install to fail. The race is pretty easy to trigger too since there's 5-10 seconds of startup latency on the Karpenter pod. We previously had an opinionated "default" provisioner in the helm chart and ended up removing it because it was causing a lot of issues.

In #810 we moved the "default" Provisioner to the post-install hook in Helm since Helm doesn't order the resources other than the CRDs in the crds dir. post-install resources are not managed by helm though for updates so it would be a one-time install. Also, when using helm install with Provisioner in the post-install hook, a user would be required to use the --wait flag to avoid the webhook race again.

I think this would be cool if helm had the proper hooks to apply resources like this :(

Yep helm is very cool but there is some limitations that are painful to deal with.

I just reminded myself that on first install, the CRD does not exists, hence the manifest validation will fail if the --disable-validation flag is not set on the helm command, creating a chicken-and-egg situation.

The dirty workaround would be to retry the deployment but that's pretty anti-pattern.

EDIT: Another solution would be to check the api capabilities in the provisioner template, and run only if the crd is available.

Maybe I could make another chart named karpenter-provisioner with the same components I added to this PR.
Let me know if you want me to work on this suggestion, or close this topic.

@bwagner5
Copy link
Contributor

bwagner5 commented Jun 22, 2022

Yep helm is very cool but there is some limitations that are painful to deal with.

I just reminded myself that on first install, the CRD does not exists, hence the manifest validation will fail if the --disable-validation flag is not set on the helm command, creating a chicken-and-egg situation.

The dirty workaround would be to retry the deployment but that's pretty anti-pattern.

EDIT: Another solution would be to check the api capabilities in the provisioner template, and run only if the crd is available.

Maybe I could make another chart named karpenter-provisioner with the same components I added to this PR.
Let me know if you want me to work on this suggestion, or close this topic.

Another helm chart is probably the way to go if we want this and maybe it would work w/ the main chart as a dependency (not sure)?

I'm not sure if it adds that much value as another helm chart though rather than just applying the Provisioner manifests with kubectl. I definitely don't think we'd want to manage two helm charts right now in this repo though :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants