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 support for mounted instance-store ephemeral storage #4735

Merged

Conversation

alec-rabold
Copy link
Contributor

@alec-rabold alec-rabold commented Oct 2, 2023

  • Add option to RAID0 instance-store disks for node ephemeral-storage

Relevant issue: #2723

Something like this would allow instance-store disks to be used for ephemeral-storage and have Karpenter aware of the capacity.

Please let me know your thoughts!

@alec-rabold alec-rabold requested a review from a team as a code owner October 2, 2023 19:47
@netlify
Copy link

netlify bot commented Oct 2, 2023

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 4cab2f6
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/657a0ddaf82efd0008201cbb

@github-actions
Copy link
Contributor

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

@njtran njtran added needs-review PRs that are still going through the review process and removed lifecycle/stale labels Oct 31, 2023
@runningman84
Copy link

it would be great to have this for bottlerocket too...

@alec-rabold alec-rabold force-pushed the feature/instance-store-ephemeral-storage branch 2 times, most recently from 4872753 to 69cf3ae Compare November 22, 2023 22:25
@alec-rabold
Copy link
Contributor Author

it would be great to have this for bottlerocket too...

Bottlerocket currently doesn't have native support for this (see: bottlerocket-os/bottlerocket#3060).

With this change, you could still enable MountNodeEphemeralStorage to read in epehermal-storage capacity from the instance -- you'd just be responsible for configuring the disks in your user data script yourself.

@jonathan-innis jonathan-innis self-assigned this Nov 27, 2023
pkg/apis/crds/karpenter.k8s.aws_awsnodetemplates.yaml Outdated Show resolved Hide resolved
pkg/apis/v1beta1/ec2nodeclass.go Outdated Show resolved Hide resolved
pkg/providers/amifamily/bootstrap/eksbootstrap.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/types.go Outdated Show resolved Hide resolved
pkg/utils/nodeclass/nodeclass.go Outdated Show resolved Hide resolved
@jonathan-innis
Copy link
Contributor

Nice work 🎉 Really cool to see progress on this!

@jonathan-innis jonathan-innis removed the needs-review PRs that are still going through the review process label Dec 5, 2023
@alec-rabold alec-rabold force-pushed the feature/instance-store-ephemeral-storage branch from 69cf3ae to 5cc1de8 Compare December 5, 2023 22:07
@ilkinmammadzada
Copy link

We are using custom AMI, our own bootstrapping script (+custom script to mount local NVMe as a ephemeral storage). We will just set "instanceStorePolicy: RAID0" and karpenter will consider local storage to calculate host ephemeral-storage. Did I understand right?

@alec-rabold
Copy link
Contributor Author

We are using custom AMI, our own bootstrapping script (+custom script to mount local NVMe as a ephemeral storage). We will just set "instanceStorePolicy: RAID0" and karpenter will consider local storage to calculate host ephemeral-storage. Did I understand right?

Yep that's correct, it'll use the total size of the instance store volume(s) for ephemeral-storage capacity; only AL2 would RAID them. I wonder if it makes sense to have another policy option for using the instance store size and explicitly not trying to RAID.

@jonathan-innis
Copy link
Contributor

I wonder if it makes sense to have another policy option for using the instance store size and explicitly not trying to RAID

@jonathan-innis
Copy link
Contributor

I wonder if it makes sense to have another policy option for using the instance store size and explicitly not trying to RAID

If you're using a custom AMI and don't need the RAID0 option explicitly passed through to the userData, then this shouldn't matter for you, since the custom AMI doesn't have any userData that's templated out by default. Seems reasonable to me to just start with RAID0 or Ignore and then see if there is a need to expand this out later.

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

Awesome work here 🎉

pkg/apis/v1beta1/ec2nodeclass.go Show resolved Hide resolved
test/suites/integration/storage_test.go Show resolved Hide resolved
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

@coveralls
Copy link

coveralls commented Dec 10, 2023

Pull Request Test Coverage Report for Build 7200427255

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 52 of 52 (100.0%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 82.571%

Totals Coverage Status
Change from base Build 7199293718: 0.08%
Covered Lines: 4946
Relevant Lines: 5990

💛 - Coveralls

Copy link
Contributor

Snapshot successfully published to oci://071440425669.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:v0-c60b19c5e5274953749de88b0a524034570675bf.

pkg/apis/v1beta1/ec2nodeclass.go Show resolved Hide resolved
website/content/en/docs/concepts/nodeclasses.md Outdated Show resolved Hide resolved
pkg/providers/launchtemplate/suite_test.go Show resolved Hide resolved
pkg/providers/launchtemplate/suite_test.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
pkg/providers/launchtemplate/suite_test.go Outdated Show resolved Hide resolved
pkg/providers/instancetype/suite_test.go Outdated Show resolved Hide resolved
@alec-rabold alec-rabold force-pushed the feature/instance-store-ephemeral-storage branch from fdc8272 to deceedd Compare December 12, 2023 03:43
@jonathan-innis
Copy link
Contributor

@alec-rabold One thing I did notice as I was testing this out is that it seems like there is a discrepancy between what the the instance store volume reports (and therefore what the NodeClaim reports) and what the actual capacity of the node is for ephemeral storage when it comes up. Just doing some testing, this is what I got when I tried to launch a bunch of pods that requested ephemeral-storage: 40Gi.

NodeClaim

  allocatable:
    cpu: 940m
    ephemeral-storage: "52026258176"
    memory: 1392Mi
    pods: "8"
    vpc.amazonaws.com/pod-eni: "4"
  capacity:
    cpu: "1"
    ephemeral-storage: 59G
    memory: 1835Mi
    pods: "8"
    vpc.amazonaws.com/pod-eni: "4"

Node

  allocatable:
    cpu: 940m
    ephemeral-storage: "51969867689"
    hugepages-1Gi: "0"
    hugepages-2Mi: "0"
    hugepages-32Mi: "0"
    hugepages-64Ki: "0"
    memory: 1429916Ki
    pods: "8"
  capacity:
    cpu: "1"
    ephemeral-storage: 57556000Ki
    hugepages-1Gi: "0"
    hugepages-2Mi: "0"
    hugepages-32Mi: "0"
    hugepages-64Ki: "0"
    memory: 1883548Ki

Any idea what might be causing the discrepancy? Looks like it's off by almost 4Gi.

@alec-rabold
Copy link
Contributor Author

alec-rabold commented Dec 12, 2023

Any idea what might be causing the discrepancy? Looks like it's off by almost 4Gi.

It looks like there is a difference but I think it's closer than 4Gi:

57556000 Ki (Kibi) --> 58.937344 G (Giga)

57556000 Ki (Kibi) --> 54.88967895508 Gi (Gibi)

The ephemeral-storage: 59G in NodeClaim would be the former conversion; did you get a 4Gi difference by converting it like the latter?

For 0.062656 G difference could it be due to disk partitioning / overhead? TBH I'm not totally sure how cAdvisor calculates the root-dir size. That's also assuming that the instance-store's advertised TotalSizeInGB is 100% accurate

@jonathan-innis
Copy link
Contributor

The ephemeral-storage: 59G in NodeClaim would be the former conversion; did you get a 4Gi difference by converting it like the latter

Oh yep. You're totally right. I just blindly assumed that the ephemeral storage was coming back in Gi rather than G. Looking back at it, the data is correct. Seems completely reasonable to me 👍🏼

@jonathan-innis
Copy link
Contributor

One comment on documentation, but otherwise this LGTM to ship 🚢

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

Copy link
Contributor

Snapshot successfully published to "oci://undefined.ecr.undefined.amazonaws.com/karpenter/snapshot/karpenter:v0-b28786e3009193c4e581146eb834ae9bd53d3a95".

@alec-rabold alec-rabold force-pushed the feature/instance-store-ephemeral-storage branch from b28786e to bb62cc9 Compare December 13, 2023 02:47
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

Copy link
Contributor

Snapshot successfully published to "oci://071440425669.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:v0-4cab2f6ed8b4a9ab2768a5a7fe0662065b6cffae".

@jonathan-innis jonathan-innis merged commit 571e0fb into aws:main Dec 14, 2023
26 checks passed
@alec-rabold alec-rabold deleted the feature/instance-store-ephemeral-storage branch December 14, 2023 05:24
@armenr
Copy link

armenr commented Dec 14, 2023

YO. This is a big deal! Thank you all, and especially @alec-rabold, for getting this pushed through!!! 🚀

@ajayOO8
Copy link

ajayOO8 commented Feb 22, 2024

We were testing this feature and found a bit random behavior. For same instance-type, the allocatable capacity values of ephemeral-storage comes out different.

{"level":"INFO","time":"2024-02-22T03:43:44.467Z","logger":"controller.nodeclaim.lifecycle","message":"launched nodeclaim","commit":"17d6c05","nodeclaim":"default-uswest1cstageg-rtr24","provider-id":"aws:///us-west-1c/i-foobar","instance-type":"m5ad.4xlarge","zone":"us-west-1c","capacity-type":"spot","allocatable":{"cpu":"13890m","ephemeral-storage":"114176Mi","memory":"55169334Ki","pods":"200","vpc.amazonaws.com/pod-eni":"54"}}

the correct one

{"level":"INFO","time":"2024-02-22T02:19:00.132Z","logger":"controller.nodeclaim.lifecycle","message":"launched nodeclaim","commit":"17d6c05","nodeclaim":"default-uswest1astagef-5xpmd","provider-id":"aws:///us-west-1a/i-fooobar","instance-type":"m5ad.4xlarge","zone":"us-west-1a","capacity-type":"spot","allocatable":{"cpu":"13890m","ephemeral-storage":"537852516352","memory":"55169334Ki","pods":"200","vpc.amazonaws.com/pod-eni":"54"}}

We also have blockDeviceMappings config in nodeClass configuration if it helps:

apiVersion: karpenter.k8s.aws/v1beta1
kind: EC2NodeClass
metadata:
name: default-useast1a
spec:
amiFamily: Custom
instanceStorePolicy: RAID0
tags:
blockDeviceMappings:

  • deviceName: /dev/sda1
    ebs:
    deleteOnTermination: true
    volumeSize: 64Gi
    volumeType: gp2

what could be the possible reason for this? Although this isn't but could lead to bad decision making by karpenter if there are enough wrong ones?

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.

10 participants