Add ephemeral storage pricing to GCE expander #4911

yaroslava-serdiuk · 2022-05-24T14:46:55Z

Which component this PR applies to?

cluster-autoscaler

What type of PR is this?

/kind feature

What this PR does / why we need it:

Improve scale-up decisions

x13n

I'm not convinced passing all this information through annotations is the way to go. I think this may be problematic if there are annotations provided on actual nodes, perhaps partially missing, leading to incorrect scaling decisions. An alternative would be to just allow GcePriceModel to extract information from instance templates directly. WDYT?

cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go

cluster-autoscaler/main.go

cluster-autoscaler/config/autoscaling_options.go

cluster-autoscaler/processors/nodeinfosprovider/gce_node_info_provider.go

x13n · 2022-05-30T08:20:39Z

cluster-autoscaler/cloudprovider/gce/templates.go

+
+	localSsdCount, err := getLocalSsdCount(template.Properties)
+	ephemeralStorageLocalSsdCount := ephemeralStorageLocalSSDCount(kubeEnvValue)
+	if err == nil && ephemeralStorageLocalSsdCount > 0 {


This used to be inside else branch, which guaranteed ephemeralStorage is only calculated once. Now it may be calculated twice. Why refactor it that way?

So I actually need to add annotations for boot disk and local ssd (which I added in following commits) and I add annotation after parsing info getBootDiskEphemeralStorageFromInstanceTemplateProperties() function . In the original order information about boot disk is not parsed from kube-env if ephemeral storage is backed up with local ssd.

x13n · 2022-05-30T08:20:41Z

cluster-autoscaler/cloudprovider/gce/templates.go

-		ephemeralStorage, err = getLocalSSDEphemeralStorageFromInstanceTemplateProperties(template.Properties, ssdCount)
-	} else if !isBootDiskEphemeralStorageWithInstanceTemplateDisabled(kubeEnvValue) {
+
+	if !isBootDiskEphemeralStorageWithInstanceTemplateDisabled(kubeEnvValue) {


Can we get rid of the double negation here? if isBootDiskEphemeralStorageWithInstanceTemplateEnabled(kubeEnvValue) would be a bit easier to read. Alternatively, just rename the function to isLocalSSDEphemeralStorageWithInstanceTemplateEnabled, which is what this function is actually checking.

Sure, but there is a reason why it is named this way. By default ephemeral storage is backed up by boot disk and in other cases we disable this option, so here we're checking "BLOCK_EPH_STORAGE_BOOT_DISK" variable in kube-env. I think from this perspective the name is fine.

cluster-autoscaler/cloudprovider/gce/gce_price_info.go

cluster-autoscaler/cloudprovider/gce/gce_price_model_test.go

cluster-autoscaler/cloudprovider/gce/gce_price_model.go

yaroslava-serdiuk · 2022-05-31T09:15:15Z

I'm not convinced passing all this information through annotations is the way to go. I think this may be problematic if there are annotations provided on actual nodes, perhaps partially missing, leading to incorrect scaling decisions. An alternative would be to just allow GcePriceModel to extract information from instance templates directly. WDYT?

To extract information from instance templates directly we need to extend GcePriceModel struct to include GceManager or a part of GceManager.
We can use annotation to pass information about node to node template object, probably it better to introduce annotation like "cluster-autoscaler/" or "cluster-autoscaler/gce/". We already use similar approach to pass/get certain information through labels.
So for me it seems better to parse kube-env at the beginning and use that information later in the loop, rather than extend functionality of GcePriceModel.

cluster-autoscaler/processors/nodeinfosprovider/gce_node_info_provider.go

cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go

cluster-autoscaler/cloudprovider/gce/gce_price_info.go

cluster-autoscaler/cloudprovider/gce/gce_price_model.go

x13n · 2022-06-01T12:53:43Z

cluster-autoscaler/cloudprovider/gce/gce_price_model.go

@@ -159,6 +197,12 @@ func (model *GcePriceModel) getBasePrice(resources apiv1.ResourceList, instanceT
 	}
 	price += float64(mem.Value()) / float64(units.GiB) * memPrice * hours

+	if model.EphemeralStorageSupport {
+		ephemeralStorage := resources[apiv1.ResourceEphemeralStorage]
+		ephemeralStoragePrice := model.PriceInfo.LocalSsdPricePerHour()


Maybe add a comment here that this is a simplification to use local ssd price even if ephemeral storage is not backed by local ssd?

Good point, thanks!

I actually changed the price to the price for pd-standard since it's cheaper.

cluster-autoscaler/cloudprovider/gce/templates.go

x13n · 2022-06-02T13:11:07Z

LGTM, just one minor comment.

x13n · 2022-06-02T13:48:19Z

/lgtm

towca · 2022-06-03T16:00:28Z

cluster-autoscaler/cloudprovider/gce/gce_manager.go

+	explicitlyConfigured            map[GceRef]bool
+	migAutoDiscoverySpecs           []migAutoDiscoveryConfig
+	reserved                        *GceReserved
+	expanderEphemeralStorageSupport bool


I don't think this is actually used anywhere, right?

Correct, thanks for finding this!

cluster-autoscaler/cloudprovider/gce/gce_cloud_provider_test.go

towca · 2022-06-03T16:14:19Z

/lgtm
/approve

k8s-ci-robot · 2022-06-03T16:14:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: towca, yaroslava-serdiuk

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~~ [towca]

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

…toragae Add ephemeral storage pricing to GCE expander

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 24, 2022

k8s-ci-robot requested review from aleksandra-malinowska and feiskyer May 24, 2022 14:47

yaroslava-serdiuk force-pushed the ephemeral-storagae branch 6 times, most recently from 9b0f889 to 42ad8d9 Compare May 25, 2022 09:26

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 25, 2022

yaroslava-serdiuk force-pushed the ephemeral-storagae branch from 42ad8d9 to f2408eb Compare May 25, 2022 10:20

x13n requested changes May 30, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2022

jbartosik added the area/cluster-autoscaler label May 31, 2022

yaroslava-serdiuk force-pushed the ephemeral-storagae branch from f2408eb to ccf3822 Compare May 31, 2022 15:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 31, 2022

yaroslava-serdiuk force-pushed the ephemeral-storagae branch 5 times, most recently from c55da9d to f5d6734 Compare June 1, 2022 12:30

x13n requested changes Jun 1, 2022

View reviewed changes

yaroslava-serdiuk force-pushed the ephemeral-storagae branch 2 times, most recently from 13cfdb0 to 8d6cd12 Compare June 1, 2022 16:00

yaroslava-serdiuk force-pushed the ephemeral-storagae branch from 8d6cd12 to 470760c Compare June 1, 2022 18:58

x13n reviewed Jun 2, 2022

View reviewed changes

cluster-autoscaler/cloudprovider/gce/templates.go Show resolved Hide resolved

yaroslava-serdiuk force-pushed the ephemeral-storagae branch 2 times, most recently from 9819ebb to cd611ca Compare June 2, 2022 13:25

k8s-ci-robot assigned x13n Jun 2, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2022

towca reviewed Jun 3, 2022

View reviewed changes

yaroslava-serdiuk added 8 commits June 3, 2022 16:12

GCE: Add --gce-expander-ephemeral-storage-support flag

7fe27dd

Define AnnotationNodeInfoProvider processor

d919ce3

Add nodeTemplate annotations to node annotations

466052a

Refactor getLocalSSDEphemeralStorageFromInstanceTemplateProperties

2e04e19

Add ephemeral storage annotations to the node template

3f77f1f

Add local SSD and boot disk price to PriceInfo

6acc9d1

Add ephemeral storage price to NodePrice

581f1d7

Add ephemeral storage price to PodPrice

1cbcfbc

yaroslava-serdiuk force-pushed the ephemeral-storagae branch from cd611ca to 1cbcfbc Compare June 3, 2022 16:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2022

k8s-ci-robot assigned towca Jun 3, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2022

k8s-ci-robot merged commit 4a97d16 into kubernetes:master Jun 3, 2022

navinjoy pushed a commit to navinjoy/autoscaler that referenced this pull request Oct 26, 2022

Merge pull request kubernetes#4911 from yaroslava-serdiuk/ephemeral-s…

4d87293

…toragae Add ephemeral storage pricing to GCE expander

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ephemeral storage pricing to GCE expander #4911

Add ephemeral storage pricing to GCE expander #4911

yaroslava-serdiuk commented May 24, 2022

x13n left a comment

x13n May 30, 2022

yaroslava-serdiuk May 31, 2022

x13n May 30, 2022

yaroslava-serdiuk May 31, 2022

yaroslava-serdiuk commented May 31, 2022

x13n Jun 1, 2022

yaroslava-serdiuk Jun 1, 2022

yaroslava-serdiuk Jun 1, 2022

x13n commented Jun 2, 2022

x13n commented Jun 2, 2022

towca Jun 3, 2022

yaroslava-serdiuk Jun 3, 2022

towca commented Jun 3, 2022

k8s-ci-robot commented Jun 3, 2022

Add ephemeral storage pricing to GCE expander #4911

Add ephemeral storage pricing to GCE expander #4911

Conversation

yaroslava-serdiuk commented May 24, 2022

Which component this PR applies to?

What type of PR is this?

What this PR does / why we need it:

x13n 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

Choose a reason for hiding this comment

yaroslava-serdiuk commented May 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x13n commented Jun 2, 2022

x13n commented Jun 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

towca commented Jun 3, 2022

k8s-ci-robot commented Jun 3, 2022