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

Update Provisioner Docs Example #1231

Merged
merged 3 commits into from
Feb 1, 2022
Merged

Update Provisioner Docs Example #1231

merged 3 commits into from
Feb 1, 2022

Conversation

DWSR
Copy link
Contributor

@DWSR DWSR commented Jan 27, 2022

The spec.limits.resources.cpu field should be a string, not an int.

1. Issue, if available:

An example in the docs is incorrect

2. Description of changes:

Updated example to be correct

3. How was this change tested?

N/A

4. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

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

The `spec.limits.resources.cpu` field should be a string, not an int.
@netlify
Copy link

netlify bot commented Jan 27, 2022

✔️ Deploy Preview for karpenter-docs-prod ready!

🔨 Explore the source changes: c7f79a8

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61f9b39112b1fc00087ef9b7

😎 Browse the preview: https://deploy-preview-1231--karpenter-docs-prod.netlify.app

@bwagner5
Copy link
Contributor

Did you experience an error with that yaml? I just applied it to my cluster and it worked for me:

$ cat <<EOF | kubectl apply -f -
apiVersion: karpenter.sh/v1alpha5
kind: Provisioner
metadata:
  name: test-1
spec:
  limits:
    resources:
      cpu: 1000
      memory: 1000Gi
  requirements:
....
$ k get provisioner test-1 -o yaml
apiVersion: karpenter.sh/v1alpha5
kind: Provisioner
metadata:
  ...
  name: test-1
spec:
  kubeletConfiguration: {}
  limits:
    resources:
      cpu: 1k
      memory: 1000Gi```

@bwagner5 bwagner5 self-requested a review January 27, 2022 21:15
@@ -52,7 +52,7 @@ spec:
# Limits prevent Karpenter from creating new instances once the limit is exceeded.
limits:
resources:
cpu: 1000
cpu: "1000"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? IIUC, the integer works just fine.

@bwagner5
Copy link
Contributor

I believe quotes are only required if the field is a string and the input is a number. In that case, you'd need to use quotes to tell yaml that you're actually passing a string rather than a number.

https://github.com/aws/karpenter/blob/main/charts/karpenter/crds/karpenter.sh_provisioners.yaml#L65-L67

@DWSR
Copy link
Contributor Author

DWSR commented Jan 30, 2022

@bwagner5 @ellistarn The kube API seems to return a string regardless of the type submitted to the API server, based on my Very Scientific Testing on my 1.20.11 EKS cluster.

Applying a provisioner that uses integer limits via ArgoCD will cause ArgoCD to always mark the resource as Out of Sync.

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.

Nice catch!

Can you include this in /preview docs? We copy preview docs into the next release version when we release. Optionally, you could backport this to older versions of docs, but I'm happy with it just being included in the next release.

@ellistarn ellistarn merged commit cffadbe into aws:main Feb 1, 2022
@ellistarn
Copy link
Contributor

Thank you!

@DWSR DWSR deleted the patch-1 branch February 2, 2022 02:02
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.

3 participants