From 6ba8d27abfae738d1ba1183f474fc058639ec79b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kuba=20Tu=C5=BCnik?= Date: Wed, 13 Jul 2022 10:57:28 +0200 Subject: [PATCH] CA: GCE: implement GetMachineFamily, fix IsCustomMachine IsCustomMachine didn't take machine types with family prefix (e.g. n2-custom-2-2816) into account. --- .../cloudprovider/gce/gce_price_model.go | 17 ++---- .../cloudprovider/gce/machine_types.go | 58 +++++++++++++++---- .../cloudprovider/gce/machine_types_test.go | 47 +++++++++++++++ 3 files changed, 98 insertions(+), 24 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/gce_price_model.go b/cluster-autoscaler/cloudprovider/gce/gce_price_model.go index 9ee9f438e341..80eaf63c92c4 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_price_model.go +++ b/cluster-autoscaler/cloudprovider/gce/gce_price_model.go @@ -19,7 +19,6 @@ package gce import ( "math" "strconv" - "strings" "time" apiv1 "k8s.io/api/core/v1" @@ -142,10 +141,10 @@ func (model *GcePriceModel) getPreemptibleDiscount(node *apiv1.Node) float64 { if !found { return 1.0 } - instanceFamily := getInstanceFamily(instanceType) + instanceFamily, _ := GetMachineFamily(instanceType) discountMap := model.PriceInfo.PredefinedPreemptibleDiscount() - if isInstanceCustom(instanceType) { + if IsCustomMachine(instanceType) { discountMap = model.PriceInfo.CustomPreemptibleDiscount() } @@ -171,8 +170,8 @@ func (model *GcePriceModel) getBasePrice(resources apiv1.ResourceList, instanceT return 0 } hours := getHours(startTime, endTime) - instanceFamily := getInstanceFamily(instanceType) - isCustom := isInstanceCustom(instanceType) + instanceFamily, _ := GetMachineFamily(instanceType) + isCustom := IsCustomMachine(instanceType) price := 0.0 cpu := resources[apiv1.ResourceCPU] @@ -224,14 +223,6 @@ func getHours(startTime time.Time, endTime time.Time) float64 { return hours } -func getInstanceFamily(instanceType string) string { - return strings.Split(instanceType, "-")[0] -} - -func isInstanceCustom(instanceType string) bool { - return strings.Contains(instanceType, "custom") -} - // hasPreemptiblePricing returns whether we should use preemptible pricing for a node, based on labels. Spot VMs have // dynamic pricing, which is different than the static pricing for Preemptible VMs we use here. However it should be close // enough in practice and we really only look at prices in comparison with each other. Spot VMs will always be cheaper diff --git a/cluster-autoscaler/cloudprovider/gce/machine_types.go b/cluster-autoscaler/cloudprovider/gce/machine_types.go index de0c5c0eef9d..21e765fb8ea6 100644 --- a/cluster-autoscaler/cloudprovider/gce/machine_types.go +++ b/cluster-autoscaler/cloudprovider/gce/machine_types.go @@ -18,6 +18,7 @@ package gce import ( "fmt" + "strconv" "strings" "k8s.io/autoscaler/cluster-autoscaler/utils/units" @@ -37,8 +38,29 @@ type MachineType struct { } // IsCustomMachine checks if a machine type is custom or predefined. -func IsCustomMachine(name string) bool { - return strings.HasPrefix(name, "custom-") +func IsCustomMachine(machineType string) bool { + // Custom types are in the form "-custom--", with the "-" prefix being optional for the N1 + // family. Examples: n2-custom-48-184320, custom-48-184320 (equivalent to n1-custom-48-184320). + // Docs: https://cloud.google.com/compute/docs/instances/creating-instance-with-custom-machine-type#gcloud. + parts := strings.Split(machineType, "-") + if len(parts) < 2 { + return false + } + return parts[0] == "custom" || parts[1] == "custom" +} + +// GetMachineFamily gets the machine family from the machine type. Machine family is determined as everything before the first +// dash character, unless this first part is "custom", which actually means "n1": +// https://cloud.google.com/compute/docs/instances/creating-instance-with-custom-machine-type#gcloud. +func GetMachineFamily(machineType string) (string, error) { + parts := strings.Split(machineType, "-") + if len(parts) < 2 { + return "", fmt.Errorf("unable to parse machine type %q", machineType) + } + if parts[0] == "custom" { + return "n1", nil + } + return parts[0], nil } // NewMachineTypeFromAPI creates a MachineType object based on machine type representation @@ -57,20 +79,34 @@ func NewMachineTypeFromAPI(name string, mt *gce_api.MachineType) (MachineType, e // NewCustomMachineType creates a MachineType object describing a custom GCE machine. // CPU and Memory are based on parsing custom machine name. func NewCustomMachineType(name string) (MachineType, error) { - // example custom-2-2816 - var cpu, mem int64 - var count int - count, err := fmt.Sscanf(name, "custom-%d-%d", &cpu, &mem) + if !IsCustomMachine(name) { + return MachineType{}, fmt.Errorf("%q is not a valid custom machine type", name) + } + + parts := strings.Split(name, "-") + var cpuPart, memPart string + if len(parts) == 3 { + cpuPart = parts[1] + memPart = parts[2] + } else if len(parts) == 4 { + cpuPart = parts[2] + memPart = parts[3] + } else { + return MachineType{}, fmt.Errorf("unable to parse custom machine type %q", name) + } + + cpu, err := strconv.ParseInt(cpuPart, 10, 64) if err != nil { - return MachineType{}, err + return MachineType{}, fmt.Errorf("unable to parse CPU %q from machine type %q: %v", cpuPart, name, err) } - if count != 2 { - return MachineType{}, fmt.Errorf("failed to parse all params in %s", name) + memBytes, err := strconv.ParseInt(memPart, 10, 64) + if err != nil { + return MachineType{}, fmt.Errorf("unable to parse memory %q from machine type %q: %v", memPart, name, err) } - mem = mem * units.MiB + return MachineType{ Name: name, CPU: cpu, - Memory: mem, + Memory: memBytes * units.MiB, }, nil } diff --git a/cluster-autoscaler/cloudprovider/gce/machine_types_test.go b/cluster-autoscaler/cloudprovider/gce/machine_types_test.go index 3226d23a093e..b2feab377115 100644 --- a/cluster-autoscaler/cloudprovider/gce/machine_types_test.go +++ b/cluster-autoscaler/cloudprovider/gce/machine_types_test.go @@ -21,6 +21,8 @@ import ( "k8s.io/autoscaler/cluster-autoscaler/utils/units" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" ) @@ -37,6 +39,12 @@ func TestNewCustomMachineType(t *testing.T) { expectCPU: 2, expectMemory: 2816 * units.MiB, }, + { + name: "n2-custom-2-2816", + expectCustom: true, + expectCPU: 2, + expectMemory: 2816 * units.MiB, + }, { name: "other-a2-2816", }, @@ -62,3 +70,42 @@ func TestNewCustomMachineType(t *testing.T) { }) } } + +func TestGetMachineFamily(t *testing.T) { + for tn, tc := range map[string]struct { + machineType string + wantFamily string + wantErr error + }{ + "predefined machine type": { + machineType: "n1-standard-8", + wantFamily: "n1", + }, + "predefined short machine type": { + machineType: "e2-small", + wantFamily: "e2", + }, + "custom machine type with family prefix": { + machineType: "n2-custom-2-2816", + wantFamily: "n2", + }, + "custom machine type without family prefix": { + machineType: "custom-2-2816", + wantFamily: "n1", + }, + "invalid machine type": { + machineType: "nodashes", + wantErr: cmpopts.AnyError, + }, + } { + t.Run(tn, func(t *testing.T) { + gotFamily, gotErr := GetMachineFamily(tc.machineType) + if diff := cmp.Diff(tc.wantFamily, gotFamily); diff != "" { + t.Errorf("GetMachineFamily(%q): diff (-want +got):\n%s", tc.machineType, diff) + } + if diff := cmp.Diff(tc.wantErr, gotErr, cmpopts.EquateErrors()); diff != "" { + t.Errorf("GetMachineFamily(%q): err diff (-want +got):\n%s", tc.machineType, diff) + } + }) + } +}