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

docs: add tool to generate instance type docs #1946

Merged
merged 1 commit into from
Jun 16, 2022

Conversation

tzneal
Copy link
Contributor

@tzneal tzneal commented Jun 16, 2022

Fixes # 1329

Description

Displays both labels and the resources that we use for bin-packing after overhead calculations.
How was this change tested?

  • Preview with netlify.

Does this change impact docs?

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

Release Note


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

@tzneal tzneal requested a review from a team as a code owner June 16, 2022 13:41
@tzneal tzneal requested a review from spring1843 June 16, 2022 13:41
@netlify
Copy link

netlify bot commented Jun 16, 2022

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit a5df3d8
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/62ab517d6ccdb0000877d6ad
😎 Deploy Preview https://deploy-preview-1946--karpenter-docs-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tzneal tzneal force-pushed the add-instance-type-docs branch from 7a3d4f9 to 5a107ac Compare June 16, 2022 13:46
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

NICE! just one comment

fmt.Fprintln(f, "<!-- this document is generated from hack/docs/instancetypes_gen_docs.go -->")
fmt.Fprintln(f, `AWS instance types offer varying resources and can be selected by labels. In particular the
ephemeral-storage is the default value for an AL2 family assuming no specific block device mappings have been configured.`)

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth calling out the the resources have overhead taken out. Although it looks like ephemeral storage does not and maybe not CPU either? https://github.com/aws/karpenter/blob/205c4db5743f1f6824253e92304a5df2f875c90c/pkg/cloudprovider/aws/amifamily/al2.go#L94

Might also be worth calling out that pods are assuming the aws-eni-limited-pod-density = true

Copy link
Contributor Author

@tzneal tzneal Jun 16, 2022

Choose a reason for hiding this comment

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

SGTM. Good catch on overhead! I calculated the value with overhead subtracted and then ignored it 😆

Displays both labels and the resources that we use for
bin-packing after overhead calculations.
@tzneal tzneal force-pushed the add-instance-type-docs branch from 5a107ac to a5df3d8 Compare June 16, 2022 15:51
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

lgtm! Big improvement! to docs!

@tzneal tzneal merged commit eda8fd7 into aws:main Jun 16, 2022
@tzneal tzneal deleted the add-instance-type-docs branch June 16, 2022 16:48
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.

2 participants