Merge pull request #6275 from azylinski/rm-flag-ephemeral-storage-sup…

…port Remove gce-expander-ephemeral-storage-support flag
kubernetes · Nov 21, 2023 · de44d78 · de44d78
2 parents dcaba44 + e836e47
commit de44d78
Show file tree

Hide file tree

Showing 5 changed files with 44 additions and 58 deletions.
diff --git a/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go b/cluster-autoscaler/cloudprovider/gce/gce_cloud_provider.go
@@ -374,7 +374,7 @@ func BuildGCE(opts config.AutoscalingOptions, do cloudprovider.NodeGroupDiscover
 		klog.Fatalf("Failed to create GCE Manager: %v", err)
 	}
 
-	pricingModel := NewGcePriceModel(NewGcePriceInfo(), opts.GCEOptions.ExpanderEphemeralStorageSupport)
+	pricingModel := NewGcePriceModel(NewGcePriceInfo())
 	provider, err := BuildGceCloudProvider(manager, rl, pricingModel)
 	if err != nil {
 		klog.Fatalf("Failed to create GCE cloud provider: %v", err)

diff --git a/cluster-autoscaler/cloudprovider/gce/gce_price_model.go b/cluster-autoscaler/cloudprovider/gce/gce_price_model.go
@@ -30,15 +30,13 @@ import (
 
 // GcePriceModel implements PriceModel interface for GCE.
 type GcePriceModel struct {
-	PriceInfo               PriceInfo
-	EphemeralStorageSupport bool
+	PriceInfo PriceInfo
 }
 
 // NewGcePriceModel gets a new instance of GcePriceModel
-func NewGcePriceModel(info PriceInfo, ephemeralStorageSupport bool) *GcePriceModel {
+func NewGcePriceModel(info PriceInfo) *GcePriceModel {
 	return &GcePriceModel{
-		PriceInfo:               info,
-		EphemeralStorageSupport: ephemeralStorageSupport,
+		PriceInfo: info,
 	}
 }
 
@@ -81,35 +79,33 @@ func (model *GcePriceModel) NodePrice(node *apiv1.Node, startTime time.Time, end
 	}
 
 	// Ephemeral Storage
-	if model.EphemeralStorageSupport {
-		// Local SSD price
-		if node.Labels[ephemeralStorageLocalSsdLabel] == "true" || node.Annotations[EphemeralStorageLocalSsdAnnotation] == "true" {
-			localSsdCount, _ := strconv.ParseFloat(node.Annotations[LocalSsdCountAnnotation], 64)
-			localSsdPrice := model.PriceInfo.LocalSsdPricePerHour()
-			if hasPreemptiblePricing(node) {
-				localSsdPrice = model.PriceInfo.SpotLocalSsdPricePerHour()
-			}
-			price += localSsdCount * float64(LocalSSDDiskSizeInGiB) * localSsdPrice * getHours(startTime, endTime)
-		}
-
-		// Boot disk price
-		bootDiskSize, _ := strconv.ParseInt(node.Annotations[BootDiskSizeAnnotation], 10, 64)
-		if bootDiskSize == 0 {
-			klog.Errorf("Boot disk size is not found for node %s, using default size %v", node.Name, DefaultBootDiskSizeGB)
-			bootDiskSize = DefaultBootDiskSizeGB
-		}
-		bootDiskType := node.Annotations[BootDiskTypeAnnotation]
-		if val, ok := node.Labels[bootDiskTypeLabel]; ok {
-			bootDiskType = val
-		}
-		if bootDiskType == "" {
-			klog.Errorf("Boot disk type is not found for node %s, using default type %s", node.Name, DefaultBootDiskType)
-			bootDiskType = DefaultBootDiskType
+	// Local SSD price
+	if node.Labels[ephemeralStorageLocalSsdLabel] == "true" || node.Annotations[EphemeralStorageLocalSsdAnnotation] == "true" {
+		localSsdCount, _ := strconv.ParseFloat(node.Annotations[LocalSsdCountAnnotation], 64)
+		localSsdPrice := model.PriceInfo.LocalSsdPricePerHour()
+		if hasPreemptiblePricing(node) {
+			localSsdPrice = model.PriceInfo.SpotLocalSsdPricePerHour()
 		}
-		bootDiskPrice := model.PriceInfo.BootDiskPricePerHour()[bootDiskType]
+		price += localSsdCount * float64(LocalSSDDiskSizeInGiB) * localSsdPrice * getHours(startTime, endTime)
+	}
 
-		price += bootDiskPrice * float64(bootDiskSize) * getHours(startTime, endTime)
+	// Boot disk price
+	bootDiskSize, _ := strconv.ParseInt(node.Annotations[BootDiskSizeAnnotation], 10, 64)
+	if bootDiskSize == 0 {
+		klog.Errorf("Boot disk size is not found for node %s, using default size %v", node.Name, DefaultBootDiskSizeGB)
+		bootDiskSize = DefaultBootDiskSizeGB
+	}
+	bootDiskType := node.Annotations[BootDiskTypeAnnotation]
+	if val, ok := node.Labels[bootDiskTypeLabel]; ok {
+		bootDiskType = val
+	}
+	if bootDiskType == "" {
+		klog.Errorf("Boot disk type is not found for node %s, using default type %s", node.Name, DefaultBootDiskType)
+		bootDiskType = DefaultBootDiskType
 	}
+	bootDiskPrice := model.PriceInfo.BootDiskPricePerHour()[bootDiskType]
+
+	price += bootDiskPrice * float64(bootDiskSize) * getHours(startTime, endTime)
 
 	// GPUs
 	if gpuRequest, found := node.Status.Capacity[gpu.ResourceNvidiaGPU]; found {
@@ -196,12 +192,10 @@ func (model *GcePriceModel) getBasePrice(resources apiv1.ResourceList, instanceT
 	}
 	price += float64(mem.Value()) / float64(units.GiB) * memPrice * hours
 
-	if model.EphemeralStorageSupport {
-		ephemeralStorage := resources[apiv1.ResourceEphemeralStorage]
-		// For simplification using a fixed price for default boot disk.
-		ephemeralStoragePrice := model.PriceInfo.BootDiskPricePerHour()[DefaultBootDiskType]
-		price += float64(ephemeralStorage.Value()) / float64(units.GiB) * ephemeralStoragePrice * hours
-	}
+	ephemeralStorage := resources[apiv1.ResourceEphemeralStorage]
+	// For simplification using a fixed price for default boot disk.
+	ephemeralStoragePrice := model.PriceInfo.BootDiskPricePerHour()[DefaultBootDiskType]
+	price += float64(ephemeralStorage.Value()) / float64(units.GiB) * ephemeralStoragePrice * hours
 
 	return price
 }

diff --git a/cluster-autoscaler/cloudprovider/gce/gce_price_model_test.go b/cluster-autoscaler/cloudprovider/gce/gce_price_model_test.go
@@ -86,7 +86,6 @@ func TestGetNodePrice(t *testing.T) {
 		cheaperNode                *apiv1.Node
 		expensiveNode              *apiv1.Node
 		priceComparisonCoefficient float64
-		expanderSupport            bool
 	}{
 		// instance types
 		"e2 is cheaper than n1": {
@@ -107,7 +106,7 @@ func TestGetNodePrice(t *testing.T) {
 		"custom node price scales linearly": {
 			cheaperNode:                testNode(t, "small_custom", "custom-1", 1000, 3.75*units.GiB, "", 0, false, false),
 			expensiveNode:              testNode(t, "large_custom", "custom-8", 8000, 30*units.GiB, "", 0, false, false),
-			priceComparisonCoefficient: 1.0 / 7.9,
+			priceComparisonCoefficient: 0.14,
 		},
 		"custom node price scales linearly 2": {
 			cheaperNode:                testNode(t, "large_custom", "custom-8", 8000, 30*units.GiB, "", 0, false, false),
@@ -194,36 +193,32 @@ func TestGetNodePrice(t *testing.T) {
 			expensiveNode:              testNode(t, "known", "n1-custom", 8000, 30*units.GiB, "", 0, false, false),
 			priceComparisonCoefficient: 1.001,
 		},
-		// Ephemeral storage support
-		"ephemeral storage support: less local SSD count is cheaper": {
+		// Ephemeral storage
+		"ephemeral storage: less local SSD count is cheaper": {
 			cheaperNode:                testNodeEphemeralStorage(t, "cheapNode", true, 2, "pd-standard", 100, false),
 			expensiveNode:              testNodeEphemeralStorage(t, "expensiveNode", true, 4, "pd-standard", 100, false),
 			priceComparisonCoefficient: 1,
-			expanderSupport:            true,
 		},
-		"ephemeral storage support: local SSD cheaper than boot disk": {
+		"ephemeral storage: local SSD cheaper than boot disk": {
 			cheaperNode:                testNodeEphemeralStorage(t, "cheapNode", true, 1, "pd-standard", 100, true),
 			expensiveNode:              testNodeEphemeralStorage(t, "expensiveNode", false, 0, "pd-ssd", 100, false),
 			priceComparisonCoefficient: 1,
-			expanderSupport:            true,
 		},
-		"ephemeral storage support: node with cheaper boot disk option is cheaper": {
+		"ephemeral storage: node with cheaper boot disk option is cheaper": {
 			cheaperNode:                testNodeEphemeralStorage(t, "cheapNode", false, 0, "pd-standard", 100, false),
 			expensiveNode:              testNodeEphemeralStorage(t, "expensiveNode", false, 0, "pd-ssd", 100, false),
 			priceComparisonCoefficient: 1,
-			expanderSupport:            true,
 		},
 		"node with default boot disk is cheaper that node with more expensive boot disk type": {
 			cheaperNode:                testNode(t, "cheapNode", "", 8000, 30*units.GiB, "", 0, false, false),
 			expensiveNode:              testNodeEphemeralStorage(t, "expensiveNode", false, 0, "pd-ssd", 100, false),
 			priceComparisonCoefficient: 1,
-			expanderSupport:            true,
 		},
 	}
 
 	for tn, tc := range cases {
 		t.Run(tn, func(t *testing.T) {
-			model := NewGcePriceModel(NewGcePriceInfo(), tc.expanderSupport)
+			model := NewGcePriceModel(NewGcePriceInfo())
 			now := time.Now()
 
 			price1, err := model.NodePrice(tc.cheaperNode, now, now.Add(time.Hour))
@@ -242,7 +237,7 @@ func TestGetPodPrice(t *testing.T) {
 	pod2 := BuildTestPodWithEphemeralStorage("a2", 2*100, 2*500*units.MiB, 2*100*units.GiB)
 	pod3 := BuildTestPodWithEphemeralStorage("a2", 2*100, 2*500*units.MiB, 100*units.GiB)
 
-	model := NewGcePriceModel(NewGcePriceInfo(), true)
+	model := NewGcePriceModel(NewGcePriceInfo())
 	now := time.Now()
 
 	price1, err := model.PodPrice(pod1, now, now.Add(time.Hour))

diff --git a/cluster-autoscaler/config/autoscaling_options.go b/cluster-autoscaler/config/autoscaling_options.go
@@ -60,8 +60,6 @@ type GCEOptions struct {
 	ConcurrentRefreshes int
 	// MigInstancesMinRefreshWaitTime is the minimum time which needs to pass before GCE MIG instances from a given MIG can be refreshed.
 	MigInstancesMinRefreshWaitTime time.Duration
-	// ExpanderEphemeralStorageSupport is whether scale-up takes ephemeral storage resources into account.
-	ExpanderEphemeralStorageSupport bool
 }
 
 const (

diff --git a/cluster-autoscaler/main.go b/cluster-autoscaler/main.go
@@ -206,9 +206,9 @@ var (
 	awsUseStaticInstanceList  = flag.Bool("aws-use-static-instance-list", false, "Should CA fetch instance types in runtime or use a static list. AWS only")
 
 	// GCE specific flags
-	concurrentGceRefreshes             = flag.Int("gce-concurrent-refreshes", 1, "Maximum number of concurrent refreshes per cloud object type.")
-	gceMigInstancesMinRefreshWaitTime  = flag.Duration("gce-mig-instances-min-refresh-wait-time", 5*time.Second, "The minimum time which needs to pass before GCE MIG instances from a given MIG can be refreshed.")
-	gceExpanderEphemeralStorageSupport = flag.Bool("gce-expander-ephemeral-storage-support", false, "Whether scale-up takes ephemeral storage resources into account for GCE cloud provider")
+	concurrentGceRefreshes            = flag.Int("gce-concurrent-refreshes", 1, "Maximum number of concurrent refreshes per cloud object type.")
+	gceMigInstancesMinRefreshWaitTime = flag.Duration("gce-mig-instances-min-refresh-wait-time", 5*time.Second, "The minimum time which needs to pass before GCE MIG instances from a given MIG can be refreshed.")
+	_                                 = flag.Bool("gce-expander-ephemeral-storage-support", true, "Whether scale-up takes ephemeral storage resources into account for GCE cloud provider (Deprecated, to be removed in 1.30+)")
 
 	enableProfiling                    = flag.Bool("profiling", false, "Is debug/pprof endpoint enabled")
 	clusterAPICloudConfigAuthoritative = flag.Bool("clusterapi-cloud-config-authoritative", false, "Treat the cloud-config flag authoritatively (do not fallback to using kubeconfig flag). ClusterAPI only")
@@ -357,9 +357,8 @@ func createAutoscalingOptions() config.AutoscalingOptions {
 		NodeDeletionDelayTimeout:         *nodeDeletionDelayTimeout,
 		AWSUseStaticInstanceList:         *awsUseStaticInstanceList,
 		GCEOptions: config.GCEOptions{
-			ConcurrentRefreshes:             *concurrentGceRefreshes,
-			MigInstancesMinRefreshWaitTime:  *gceMigInstancesMinRefreshWaitTime,
-			ExpanderEphemeralStorageSupport: *gceExpanderEphemeralStorageSupport,
+			ConcurrentRefreshes:            *concurrentGceRefreshes,
+			MigInstancesMinRefreshWaitTime: *gceMigInstancesMinRefreshWaitTime,
 		},
 		ClusterAPICloudConfigAuthoritative: *clusterAPICloudConfigAuthoritative,
 		CordonNodeBeforeTerminate:          *cordonNodeBeforeTerminate,