GCE ephemeral storage on local SSDs #4318

adrienjt · 2021-09-08T22:03:15Z

If EPH_STORAGE_LOCAL_SSD_COUNT=N is specified in kube-env (in AUTOSCALER_ENV_VARS), physical ephemeral storage is N*375GiB instead of the boot disk size, and only the variable part of OS reserved (filesystem overhead vs. fixed OS partition) is subtracted to compute capacity.

The existing BLOCK_EPH_STORAGE_BOOT_DISK is ignored if EPH_STORAGE_LOCAL_SSD_COUNT>0.

fixes #4281 and #1869

k8s-ci-robot · 2021-09-08T22:03:22Z

Welcome @adrienjt!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

MaciekPytel · 2021-09-09T11:20:03Z

cc: @jayantjain93 I think you're the one with the most context to review

jayantjain93 · 2021-09-13T12:28:26Z

cluster-autoscaler/cloudprovider/gce/templates.go

+			if disk.Type == "SCRATCH" && disk.InitializeParams.DiskType == "local-ssd" {
+				// we could just multiply ssdCount by a constant 375GiB, because all GCE local SSDs are 375GiB
+				// but this loop verifies that the instance template has at least the requested number of local SSDs
+				// and is forward-compatible with a potentially different size


This doesn't make the change forward-compatible imo.

This only works for the case when a new disk size is introduced and all the disks for the node are of the same size. In all other cases, this will create an unsuspecting bug.

I would recommend that we use a constant 375GiB.

This only works for the case when a new disk size is introduced and all the disks for the node are of the same size.

That's what I had in mind, e.g., imagine that a new machine family is introduced with 500GiB local SSDs. It wouldn't make much sense to have local SSDs of different sizes on the same node IMO, because that's not RAID-friendly, but to avoid unsuspecting bugs, I could continue the loop to check that all disks are the same size. Wdyt?

imagine that a new machine family is introduced with 500GiB local SSDs. It wouldn't make much sense to have local SSDs of different sizes on the same node IMO

I am trying to avoid these speculations of what GCE might end up supporting.

I would just do away with all the checking of size also. If we're not supporting any other disk size than 375GiB, there isn't really a point to run the loop. Because running the loop makes the assumption of all disk are of x size. But we don't know how x will evolve or if all disk will be of same size.

We should at least make sure that enough local SSDs are attached. A loop is needed to count them. So we might as well also check that all disks are the same size, and use that size.

adrienjt · 2021-09-13T16:24:46Z

cluster-autoscaler/cloudprovider/gce/templates.go

+				// we could just multiply ssdCount by a constant 375GiB, because all GCE local SSDs are 375GiB
+				// but this loop verifies that the instance template has at least the requested number of local SSDs
+				// and is forward-compatible with a potentially different size
+				count++


That is correct.

jayantjain93 · 2021-09-13T13:16:34Z

cluster-autoscaler/cloudprovider/gce/templates.go

@@ -228,18 +231,61 @@ func (t *GceTemplateBuilder) BuildNodeFromTemplate(mig Mig, template *gce.Instan
 	return &node, nil
 }

-// isEphemeralStorageWithInstanceTemplateDisabled will allow bypassing Disk Size of Boot Disk from being
+func ephemeralStorageLocalSSDCount(kubeEnvValue string) int64 {
+	v, found, err := extractAutoscalerVarFromKubeEnv(kubeEnvValue, "EPH_STORAGE_LOCAL_SSD_COUNT")


I would change the variable name to ephemeral-storage-local-ssd-count

Works for me. With underscores though, right? Most variables in AUTOSCALER_ENV_VARS are lowercase with underscores. For this one, I was just trying to be consistent with BLOCK_EPH_STORAGE_BOOT_DISK... which isn't consistent with other variables, but is the "closest relative".

I thought you were trying to be consistent with BLOCK_EPH_STORAGE_BOOT_DISK, but that is a special case flag which is used for blocking, where as this is a capacity.

Underscores sounds fine, I might have missed the hierarchy for AUTOSCALER_ENV_VARS.

jayantjain93 · 2021-09-13T13:28:36Z

cluster-autoscaler/cloudprovider/gce/reserved.go

-		storage := int64(math.Ceil(0.015635*float64(diskSize))) + int64(math.Ceil(4.148*GiB)) // os partition estimation
-		storage += int64(math.Min(100*MiB, math.Ceil(0.001*float64(diskSize))))               // over-provisioning buffer
+		if !ephemeralStorageOnLocalSSDs {
+			storage += int64(math.Ceil(4.148 * GiB)) // os partition estimation


Are these values/changes verified and what process did you follow to do test this.

These current values are obtained experimentally and there wasn't any testing done to segregate the os partition and filesystem overhead, hence I'm not sure if these values correctly estimate the ephemeral storage on the local storage.

I've observed that the value given by df (the 1K-blocks column), which is what the kubelet uses as capacity if system reserved is 0, in our system (not GKE, custom Ubuntu node images), is 386913664 KiB (~= 369 GiB) for a single 375 GiB local SSD, so the overhead is 1.60%, close to your value for COS, and lower than your value for Ubuntu, but that's okay, I'd rather reserve more than not enough.

Note: when I increase the number of disks to 4 or 8, RAID increases the overhead value to 1.61%.

Do you think it is possible to test this on standard node image for cos and ubuntu (used in GKE)?

We want make sure that it works well for most number of disks, and we would be okay to put additional offset for disks 4 to 8, if we can map this data more accurately.

We had spent an extensive amount of time to estimate the values used here, and deviations in this had caused major supportability issues for us. Hence, my concerns about checking these values.

If you're okay to share some data files, I wouldn't mind having a look.

https://docs.google.com/spreadsheets/d/1j0dWPXlr8J24H5af_piYGCb0zVg51P9HCLbz6IvOofo/edit?usp=sharing

See new code for how I use the results from the GKE experiment.

jayantjain93 · 2021-09-13T13:29:13Z

cluster-autoscaler/cloudprovider/gce/reserved.go

-		storage := int64(math.Ceil(0.03083*float64(diskSize))) + int64(math.Ceil(0.171*GiB)) // os partition estimation
-		storage += int64(math.Min(100*MiB, math.Ceil(0.001*float64(diskSize))))              // over-provisioning buffer
+		if !ephemeralStorageOnLocalSSDs {
+			storage += int64(math.Ceil(0.171 * GiB)) // os partition estimation


Same issue as above about testing.

jayantjain93 · 2021-09-13T13:48:01Z

cluster-autoscaler/cloudprovider/gce/templates_test.go

 			if tc.kubeEnv != "" {
 				template.Properties.Metadata.Items = []*gce.MetadataItems{{Key: "kube-env", Value: &tc.kubeEnv}}
 			}
-			node, err := tb.BuildNodeFromTemplate(mig, template, tc.physicalCpu, tc.physicalMemory, tc.pods)
+			node, err := tb.BuildNodeFromTemplate(mig, template, physicalCpu, physicalMemory, tc.pods)


removing tc., uses static constant and forgoes testing of the cases.

We need to use tc. so testing can be done on an individual level. This closes door to every test a different value for physicalCpu and physicalMemory other than those defined as a static value

The same values are used for all tests.

These values aren't variables of the system under test. As explained in the comment that I added, the test itself computes expected CPU/memory capacity/allocatable, so it doesn't make sense to use different values for different test cases.

As I mentioned all the values are same (since we end up copy pasting the scenarios with additional fields). But those values are still being individually tested.
With static values we will not be able to unit test different values for BuildNodeFromTemplate, which is compared in line:216

I'm not seeing any gains from making this static. Maybe I'm missing context ?

I was trying to wrap my head around what this test actually tests, since it's calling submethods BuildCapacity and CalculateAllocatable in the test itself. It turns out physicalCpu and physicalMemory are test invariants: changing their values doesn't affect the test results, because they are used to compute the expected results as well.

That being said, someone could add logic in BuildNodeFromTemplate, outside of BuildCapacity and CalculateAllocatable, that would use physicalCpu and physicalMemory as variables, in which case they wouldn't be invariants anymore, but that's not currently the case.

Anyway, I've dropped my changes in this regard because I don't want this PR to be blocked because of my attempt to clarify test boundaries. I'm happy to let someone else fix that later, if anyone cares.

jayantjain93 · 2021-09-13T13:49:44Z

cluster-autoscaler/cloudprovider/gce/templates_test.go

+					Type: "SCRATCH",
+					InitializeParams: &gce.AttachedDiskInitializeParams{
+						DiskType:   "local-ssd",
+						DiskSizeGb: localSSDSizeGiB,


I would recommend not using a static value for DiskSizeGb instead define a variable in the test struct making the testing scenarios extendable.

Correct. I'll adjust if we decide to keep it non-static.

jayantjain93 · 2021-09-13T13:50:37Z

cluster-autoscaler/cloudprovider/gce/templates_test.go

 							},
 						},
 					},
 				},
 			}
+			for i := int64(0); i < tc.attachedLocalSSDCount; i++ {
+				template.Properties.Disks = append(template.Properties.Disks, &gce.AttachedDisk{


I would think defining the template may become obsolete if we use a static value for Disk size.

Correct. I'll adjust if we do.

not using a static value

jayantjain93 · 2021-09-13T13:52:36Z

cluster-autoscaler/cloudprovider/gce/templates_test.go

+	var physicalCpu int64 = 8
+	var physicalMemory int64 = 200 * units.MiB
+	var bootDiskSizeGiB int64 = 300


recommend not introducing these variables.
The values these variables are referring to are test cases, and each should be considered individually (even if they may share a common value).

see reply to other comment, esp. (2): #4318 (comment)

reverted, cf. #4318 (comment)

jayantjain93 · 2021-09-13T13:59:08Z

cluster-autoscaler/cloudprovider/gce/templates_test.go

-					physicalEphemeralStorage = 0
-				}
-				capacity, err := tb.BuildCapacity(tc.physicalCpu, tc.physicalMemory, tc.accelerators, OperatingSystemLinux, OperatingSystemDistributionCOS, physicalEphemeralStorage*units.GiB, tc.pods)
+				capacity, err := tb.BuildCapacity(physicalCpu, physicalMemory, tc.accelerators, OperatingSystemLinux, OperatingSystemDistributionCOS, tc.physicalEphemeralStorageGiB*units.GiB, tc.ephemeralStorageOnLocalSSD, tc.pods)


This statement is by design. To test reading a value in the template with the Block flag vs the correct value determined by tc.isEphemeralStorageBlocked.
The test case loses it's validity by changing the value in line:125.

The value in line 125 is the expected physicalEphemeralStorageGiB (before reservation). It includes the expectation that isEphemeralStorageBlocked. For tests where isEphemeralStorageBlocked was true, I expect physicalEphemeralStorageGiB to be zero.

That isn't correct. This flag is used for local ssd cases, where we have a boot disk present but we don't elect to acknowledge the size as boot disk. This is testing the flag control on the expected value. The non-zero value is set in the instance template.

If any functional change happens we expect to catch it here.

reverted, cf. #4318 (comment)

jayantjain93 · 2021-09-13T14:06:08Z

@MaciekPytel Thanks for the heads up.

@adrienjt I have reviewed the changes and left some comments.
p.s. Thanks for the taking the time out to support this feature.

adrienjt · 2021-09-13T17:40:34Z

@jayantjain93 thank you for your review. I have replied to your comments, please let me know what you think and I'll update the code once we agree.

jayantjain93 · 2021-09-14T09:04:39Z

cluster-autoscaler/cloudprovider/gce/reserved.go

 		return storage
 	case OperatingSystemDistributionWindowsLTSC, OperatingSystemDistributionWindowsSAC:
-		storage := int64(math.Ceil(0.1133 * GiB)) // os partition estimation
-		storage += int64(math.Ceil(0.010 * GiB))  // over-provisioning buffer
+		storage += int64(math.Ceil(0.1133 * GiB)) // os partition estimation


These are experimental observations across multiple machine types and zones. So the formulas are the closest representation of what we observed.

I would confirm the values for windows also on SSDs.

jayantjain93 · 2021-09-14T09:07:24Z

cluster-autoscaler/cloudprovider/gce/templates.go

@@ -228,18 +231,61 @@ func (t *GceTemplateBuilder) BuildNodeFromTemplate(mig Mig, template *gce.Instan
 	return &node, nil
 }

-// isEphemeralStorageWithInstanceTemplateDisabled will allow bypassing Disk Size of Boot Disk from being
+func ephemeralStorageLocalSSDCount(kubeEnvValue string) int64 {
+	v, found, err := extractAutoscalerVarFromKubeEnv(kubeEnvValue, "EPH_STORAGE_LOCAL_SSD_COUNT")


I thought you were trying to be consistent with BLOCK_EPH_STORAGE_BOOT_DISK, but that is a special case flag which is used for blocking, where as this is a capacity.

Underscores sounds fine, I might have missed the hierarchy for AUTOSCALER_ENV_VARS.

jayantjain93 · 2021-09-14T09:11:28Z

cluster-autoscaler/cloudprovider/gce/templates.go

+			if disk.Type == "SCRATCH" && disk.InitializeParams.DiskType == "local-ssd" {
+				// we could just multiply ssdCount by a constant 375GiB, because all GCE local SSDs are 375GiB
+				// but this loop verifies that the instance template has at least the requested number of local SSDs
+				// and is forward-compatible with a potentially different size


imagine that a new machine family is introduced with 500GiB local SSDs. It wouldn't make much sense to have local SSDs of different sizes on the same node IMO

I am trying to avoid these speculations of what GCE might end up supporting.

I would just do away with all the checking of size also. If we're not supporting any other disk size than 375GiB, there isn't really a point to run the loop. Because running the loop makes the assumption of all disk are of x size. But we don't know how x will evolve or if all disk will be of same size.

jayantjain93 · 2021-09-14T09:43:54Z

cluster-autoscaler/cloudprovider/gce/templates_test.go

 			if tc.kubeEnv != "" {
 				template.Properties.Metadata.Items = []*gce.MetadataItems{{Key: "kube-env", Value: &tc.kubeEnv}}
 			}
-			node, err := tb.BuildNodeFromTemplate(mig, template, tc.physicalCpu, tc.physicalMemory, tc.pods)
+			node, err := tb.BuildNodeFromTemplate(mig, template, physicalCpu, physicalMemory, tc.pods)


As I mentioned all the values are same (since we end up copy pasting the scenarios with additional fields). But those values are still being individually tested.
With static values we will not be able to unit test different values for BuildNodeFromTemplate, which is compared in line:216

I'm not seeing any gains from making this static. Maybe I'm missing context ?

jayantjain93 · 2021-09-14T09:52:54Z

cluster-autoscaler/cloudprovider/gce/templates_test.go

-					physicalEphemeralStorage = 0
-				}
-				capacity, err := tb.BuildCapacity(tc.physicalCpu, tc.physicalMemory, tc.accelerators, OperatingSystemLinux, OperatingSystemDistributionCOS, physicalEphemeralStorage*units.GiB, tc.pods)
+				capacity, err := tb.BuildCapacity(physicalCpu, physicalMemory, tc.accelerators, OperatingSystemLinux, OperatingSystemDistributionCOS, tc.physicalEphemeralStorageGiB*units.GiB, tc.ephemeralStorageOnLocalSSD, tc.pods)


That isn't correct. This flag is used for local ssd cases, where we have a boot disk present but we don't elect to acknowledge the size as boot disk. This is testing the flag control on the expected value. The non-zero value is set in the instance template.

If any functional change happens we expect to catch it here.

jayantjain93 · 2021-09-14T13:14:39Z

cluster-autoscaler/cloudprovider/gce/reserved.go

-		storage := int64(math.Ceil(0.015635*float64(diskSize))) + int64(math.Ceil(4.148*GiB)) // os partition estimation
-		storage += int64(math.Min(100*MiB, math.Ceil(0.001*float64(diskSize))))               // over-provisioning buffer
+		if !ephemeralStorageOnLocalSSDs {
+			storage += int64(math.Ceil(4.148 * GiB)) // os partition estimation


Do you think it is possible to test this on standard node image for cos and ubuntu (used in GKE)?

We want make sure that it works well for most number of disks, and we would be okay to put additional offset for disks 4 to 8, if we can map this data more accurately.

We had spent an extensive amount of time to estimate the values used here, and deviations in this had caused major supportability issues for us. Hence, my concerns about checking these values.

If you're okay to share some data files, I wouldn't mind having a look.

adrienjt · 2021-09-20T09:08:02Z

@jayantjain93 I have addressed your comments and updated the PR. Please let me know what you think.

jayantjain93 · 2021-09-22T04:44:40Z

@jayantjain93 I have addressed your comments and updated the PR. Please let me know what you think.

Thanks for the follow up @adrienjt. I did start having a look, but it may take me a couple days to get back to you.

jayantjain93

Just a couple of comments.
Also, the PR has run the workflow which is passing the unit tests.
Rest is LGTM.

jayantjain93 · 2021-09-22T09:12:53Z

cluster-autoscaler/cloudprovider/gce/reserved.go

-		storage += int64(math.Min(100*MiB, math.Ceil(0.001*float64(diskSize))))               // over-provisioning buffer
-		return storage
+		if ephemeralStorageLocalSSDCount > 0 {
+			storage = 1024 * ephemeralStorageOnLocalSSDFilesystemOverheadInKiBByOSAndDiskCount[OperatingSystemDistributionCOS][mapActualToMeasuredEphemeralStorageLocalSSDCount(ephemeralStorageLocalSSDCount)]


It seems cleaner to just return the actual overhead from the function rather than to return a disk count that again uses some array. Maybe refactoring this to return the reserved for count/os would be more readable.
e.g.

func reservedEphemeralStorageForLocalSSD(diskCount, OSDistribution) (reservedStorage) { }

And let all the complexity be handled there including kiB to B conversion.

Also, it's not really OS-reserved, but really a filesystem overhead, so it would make sense to move this out of CalculateOSReservedEphemeralStorage.

jayantjain93 · 2021-09-22T11:27:06Z

cluster-autoscaler/cloudprovider/gce/templates.go

+					diskSizeGiB = disk.InitializeParams.DiskSizeGb
+				} else if diskSizeGiB != disk.InitializeParams.DiskSizeGb {
+					return 0, fmt.Errorf("local SSDs of different sizes are not supported")
+				}


since #4318 (comment) is outdated, starting a new thread.

I like the idea of verifying the count and disk size, but I think the disk size check could be implemented differently.

I would think of something like this:

for _, disk := range instanceProperties.Disks { if disk != nil && disk.InitializeParams != nil { if disk.Type == "SCRATCH" && disk.InitializeParams.DiskType == "local-ssd" && disk.InitializeParams.DiskSizeGb == LOCAL_SSD_DISK_SIZE { count++ } } } if count < ssdCount { return 0, fmt.Errorf("actual local SSD count is lower than ephemeral_storage_local_ssd_count") }

and avoid fetching the size of the disk, and be relevant to checking.
Thoughts?

In your proposal, it's okay if the template has ssdCount disks of size LOCAL_SSD_DISK_SIZE, and N additional disks of any sizes. I guess that's all we need. The check I had, "local SSDs of different sizes are not supported", is indeed too strict, and would allow any (unique) disk size for which experimental filesystem overheads have not been measured. Anyway, in practice, all disks are 375GB at the moment, so the checks are equivalent, but I'll switch to yours.

jayantjain93 · 2021-09-27T10:16:28Z

cluster-autoscaler/cloudprovider/gce/reserved.go

+func mapActualToMeasuredEphemeralStorageLocalSSDCount(count int64) int64 {
+	if count <= 8 {
+		return count
+	}
+	if count <= 16 {
+		return 16
+	}
+	return 24 // max attachable
+}
+
+// EphemeralStorageOnLocalSSDFilesystemOverheadInBytes estimates the difference
+// between the total physical capacity of the local SSDs and the ephemeral
+// storage filesystem capacity. It uses experimental values measured in GKE,
+// which are good-enough approximations for custom Kubernetes on GCE.
+func EphemeralStorageOnLocalSSDFilesystemOverheadInBytes(diskCount int64, osDistribution OperatingSystemDistribution) int64 {
+	c := mapActualToMeasuredEphemeralStorageLocalSSDCount(diskCount)
+	o, ok := ephemeralStorageOnLocalSSDFilesystemOverheadInKiBByOSAndDiskCount[osDistribution]
+	if !ok {
+		klog.Errorf("Ephemeral storage backed by local SSDs is not supported for image family %v", osDistribution)
+		return 0
+	}
+	return 1024 * o[c]
+}


I don't see any reason to keep these two functions separate.

Having a local variable measurableCount etc. would work in EphemeralStorageOnLocalSSDFilesystemOverheadInBytes.

jayantjain93 · 2021-09-27T10:17:46Z

cluster-autoscaler/cloudprovider/gce/reserved.go

+// which are good-enough approximations for custom Kubernetes on GCE.
+func EphemeralStorageOnLocalSSDFilesystemOverheadInBytes(diskCount int64, osDistribution OperatingSystemDistribution) int64 {
+	c := mapActualToMeasuredEphemeralStorageLocalSSDCount(diskCount)
+	o, ok := ephemeralStorageOnLocalSSDFilesystemOverheadInKiBByOSAndDiskCount[osDistribution]


With this new refactor, OperatingSystemDistributionCOSContainerd and OperatingSystemDistributionUbuntuContainerd os distributions now become unsupported.
I don't think this was intentional since the overhead are the same for containerd and docker runtimes.

Good catch, thank you!

Added a test to capture that.

jayantjain93 · 2021-09-27T10:22:15Z

cluster-autoscaler/cloudprovider/gce/templates.go

+	return int64(n)
+}
+
+const localSSDDiskSizeInGiB = 375


Please move this to the top of the file with other consts line:45
and start the same of the variable with cap. LocalSSDDiskSizeInGiB

jayantjain93 · 2021-09-27T11:51:39Z

cluster-autoscaler/cloudprovider/gce/templates_test.go

+			physicalMemory:                200 * units.MiB,
+			ephemeralStorageLocalSSDCount: 2,
+			attachedLocalSSDCount:         4,
+			expectedErr:                   false,
 		},


I think we might need a test case with ephemeral storage in kube_reserved and localSSD.

Can you confirm if kube_reserved is using LocalSSDs as ephemeral storage and not boot disk in the current case

I confirm. Adding a test...

jayantjain93 · 2021-09-27T11:54:19Z

/ok-to-test

If ephemeral_storage_local_ssd_count=N is specified in kube-env (in AUTOSCALER_ENV_VARS), physical ephemeral storage is N*375GiB instead of the boot disk size, and capacity is measured from GKE experiments. The existing BLOCK_EPH_STORAGE_BOOT_DISK is ignored if ephemeral_storage_local_ssd_count>0.

jayantjain93 · 2021-09-28T04:16:54Z

/lgtm

jayantjain93 · 2021-09-28T04:20:23Z

@MaciekPytel , this needs to be approved for testing. And needs a final approval.

jayantjain93 · 2021-09-28T04:23:14Z

/assign @MaciekPytel

x13n · 2021-10-14T15:01:44Z

cluster-autoscaler/cloudprovider/gce/templates.go

+
+	if count < ssdCount {
+		return 0, fmt.Errorf("actual local SSD count is lower than ephemeral_storage_local_ssd_count")
+	}


Apologies for a dumb question, but why do we need both kube-env var and instance property? Wouldn't it be simpler to just rely on init params? Is there any use case for having an instance with more local SSDs than configured in kube-env?

@adrienjt @jayantjain93

No worries. We use a subset of local SSDs as a persistent volume and the rest for ephemeral storage.

x13n · 2021-10-15T15:01:21Z

/lgtm

MaciekPytel · 2021-10-18T13:58:25Z

/approve

k8s-ci-robot · 2021-10-18T13:58:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrienjt, jayantjain93, MaciekPytel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cluster-autoscaler/OWNERS~~ [MaciekPytel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

brokenjacobs · 2022-08-02T17:37:06Z

Dunno if anyone will see this, but is it possible that this PR works for SAS attached local-ssd but not NVMe attached? Seeing this behavior on gke v1.22.8-gke.202 and not sure if it's a regression of just no support for nvme disks.

x13n · 2022-08-08T12:04:45Z

This PR helps for self-managed clusters in GCE, but doesn't work out of the box for GKE. We need some changes on GKE side to set up env vars correctly first.

brokenjacobs · 2022-08-08T14:30:10Z

This PR helps for self-managed clusters in GCE, but doesn't work out of the box for GKE. We need some changes on GKE side to set up env vars correctly first.

That certainly 'sounds' like a simple fix. Why is it taking a year to implement?

x13n · 2022-08-09T08:17:51Z

Yeah, it should be simple, just didn't get prioritized (this PR was sent by an external contributor). Let me check if that's indeed the only change needed and get back to you later this week.

x13n · 2022-08-11T13:00:22Z

So I did a quick prototype and it passed an otherwise failing scale-from-0 scenario. I'll work on getting this change in.

x13n · 2022-08-24T14:06:07Z

This should work as soon as cluster versions higher than 1.24.3-gke.2200 become available.

brokenjacobs · 2022-08-26T18:42:55Z

Might want to update the limitations page with a note:
https://cloud.google.com/kubernetes-engine/docs/concepts/cluster-autoscaler#limitations

When Pods request ephemeral storage, cluster autoscaler does not support scaling up a node pool with zero nodes that uses [Local SSDs as ephemeral storage](https://cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/local-ssd#creating_a_node_pool_using_ephemeral_storage_on_local_ssds).

x13n · 2022-10-14T07:45:00Z

Yup, the docs were updated. The feature is available from 1.24.5-gke.600 onwards.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 8, 2021

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 8, 2021

k8s-ci-robot requested review from aleksandra-malinowska and feiskyer September 8, 2021 22:03

jbartosik added the area/cluster-autoscaler label Sep 10, 2021

jayantjain93 suggested changes Sep 13, 2021

View reviewed changes

jayantjain93 suggested changes Sep 14, 2021

View reviewed changes

adrienjt force-pushed the gce-ephemeral-storage-local-ssd branch from 3455568 to 3e19bac Compare September 20, 2021 09:00

adrienjt requested a review from jayantjain93 September 21, 2021 15:19

jayantjain93 reviewed Sep 22, 2021

View reviewed changes

adrienjt force-pushed the gce-ephemeral-storage-local-ssd branch 2 times, most recently from d4b92c2 to 5efd026 Compare September 24, 2021 23:45

jayantjain93 reviewed Sep 27, 2021

View reviewed changes

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 27, 2021

adrienjt force-pushed the gce-ephemeral-storage-local-ssd branch from 41111b0 to d3dd856 Compare September 27, 2021 18:22

k8s-ci-robot assigned jayantjain93 Sep 28, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2021

jayantjain93 approved these changes Sep 28, 2021

View reviewed changes

k8s-ci-robot assigned MaciekPytel Sep 28, 2021

x13n reviewed Oct 14, 2021

View reviewed changes

k8s-ci-robot assigned x13n Oct 15, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2021

k8s-ci-robot merged commit 47bc0f2 into kubernetes:master Oct 18, 2021

adrienjt mentioned this pull request Oct 18, 2021

exported const LocalSSDDiskSizeInGiB should have comment #4405

Merged

palmerabollo mentioned this pull request Feb 21, 2022

CA will not scale up from 0 if unschedulable pods require ephemeral storage #1869

Closed

GCE ephemeral storage on local SSDs #4318

GCE ephemeral storage on local SSDs #4318

Conversation

adrienjt commented Sep 8, 2021 • edited Loading

k8s-ci-robot commented Sep 8, 2021

MaciekPytel commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienjt Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienjt Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayantjain93 commented Sep 13, 2021 • edited Loading

adrienjt commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienjt commented Sep 20, 2021

jayantjain93 commented Sep 22, 2021 • edited Loading

jayantjain93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienjt Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayantjain93 commented Sep 27, 2021

jayantjain93 commented Sep 28, 2021

jayantjain93 commented Sep 28, 2021

jayantjain93 commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x13n commented Oct 15, 2021

MaciekPytel commented Oct 18, 2021

k8s-ci-robot commented Oct 18, 2021

brokenjacobs commented Aug 2, 2022

x13n commented Aug 8, 2022

brokenjacobs commented Aug 8, 2022

x13n commented Aug 9, 2022

x13n commented Aug 11, 2022

x13n commented Aug 24, 2022

adrienjt commented Sep 8, 2021 •

edited

Loading

adrienjt Sep 13, 2021 •

edited

Loading

adrienjt Sep 13, 2021 •

edited

Loading

jayantjain93 commented Sep 13, 2021 •

edited

Loading

jayantjain93 commented Sep 22, 2021 •

edited

Loading

adrienjt Sep 22, 2021 •

edited

Loading