Skip to content

Commit

Permalink
cluster-autoscaler: fix tests and GCE NodePrice
Browse files Browse the repository at this point in the history
Recent changes configured providers to set stable nodes labels names
exclusively (ie. LabelTopologyZone and not LabelZoneFailureDomain, etc),
with older labels names backfilled at nodeInfos templates generation time
(from GetNodeInfoFromTemplate), which isn't invoked from most tests cases.
GCE NodePirce() might have been dereferencing potentially missing labels.
And run hack/update-gofmt.sh where hack/verify-all.sh fails, to pass CI.
  • Loading branch information
bpineau committed Aug 3, 2021
1 parent 21fc0c1 commit 79c63a7
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestBuildGenericLabels(t *testing.T) {
}
nodeName := "virtual-node"
labels := buildGenericLabels(template, nodeName)
assert.Equal(t, labels[apiv1.LabelInstanceType], template.InstanceType.instanceTypeID)
assert.Equal(t, labels[apiv1.LabelInstanceTypeStable], template.InstanceType.instanceTypeID)
}

func TestExtractLabelsFromAsg(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions cluster-autoscaler/cloudprovider/aws/aws_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ func TestBuildGenericLabels(t *testing.T) {
},
Region: "us-east-1",
}, "sillyname")
assert.Equal(t, "us-east-1", labels[apiv1.LabelZoneRegion])
assert.Equal(t, "us-east-1", labels[apiv1.LabelZoneRegionStable])
assert.Equal(t, "sillyname", labels[apiv1.LabelHostname])
assert.Equal(t, "c4.large", labels[apiv1.LabelInstanceType])
assert.Equal(t, "c4.large", labels[apiv1.LabelInstanceTypeStable])
assert.Equal(t, cloudprovider.DefaultArch, labels[apiv1.LabelArchStable])
assert.Equal(t, cloudprovider.DefaultOS, labels[apiv1.LabelOSStable])
}
Expand Down
21 changes: 17 additions & 4 deletions cluster-autoscaler/cloudprovider/gce/gce_price_model.go
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ func (model *GcePriceModel) NodePrice(node *apiv1.Node, startTime time.Time, end
// Base instance price
if node.Labels != nil {
isPreemptible = node.Labels[preemptibleLabel] == "true"
if machineType, found := node.Labels[apiv1.LabelInstanceType]; found {
if machineType, found := getInstanceTypeFromLabels(node.Labels); found {
priceMapToUse := instancePrices
if isPreemptible {
priceMapToUse = preemptiblePrices
Expand All @@ -362,8 +362,10 @@ func (model *GcePriceModel) NodePrice(node *apiv1.Node, startTime time.Time, end
}
}
if !basePriceFound {
price = getBasePrice(node.Status.Capacity, node.Labels[apiv1.LabelInstanceType], startTime, endTime)
price = price * getPreemptibleDiscount(node)
if machineType, found := getInstanceTypeFromLabels(node.Labels); found {
price = getBasePrice(node.Status.Capacity, machineType, startTime, endTime)
price = price * getPreemptibleDiscount(node)
}
}

// GPUs
Expand Down Expand Up @@ -407,7 +409,10 @@ func getPreemptibleDiscount(node *apiv1.Node) float64 {
if node.Labels[preemptibleLabel] != "true" {
return 1.0
}
instanceType := node.Labels[apiv1.LabelInstanceType]
instanceType, found := getInstanceTypeFromLabels(node.Labels)
if !found {
return 1.0
}
instanceFamily := getInstanceFamily(instanceType)

discountMap := predefinedPreemptibleDiscount
Expand Down Expand Up @@ -476,3 +481,11 @@ func getAdditionalPrice(resources apiv1.ResourceList, startTime time.Time, endTi
price += float64(gpu.MilliValue()) / 1000.0 * gpuPricePerHour * hours
return price
}

func getInstanceTypeFromLabels(labels map[string]string) (string, bool) {
machineType, found := labels[apiv1.LabelInstanceTypeStable]
if !found {
machineType, found = labels[apiv1.LabelInstanceType]
}
return machineType, found
}
14 changes: 7 additions & 7 deletions cluster-autoscaler/cloudprovider/gce/templates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,13 +226,13 @@ func TestBuildGenericLabels(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
expectedLabels := map[string]string{
apiv1.LabelTopologyRegion: "us-central1",
apiv1.LabelTopologyZone: "us-central1-b",
gceCSITopologyKeyZone: "us-central1-b",
apiv1.LabelHostname: "sillyname",
apiv1.LabelInstanceTypeStable: "n1-standard-8",
apiv1.LabelArchStable: cloudprovider.DefaultArch,
apiv1.LabelOSStable: tc.expectedOsLabel,
apiv1.LabelTopologyRegion: "us-central1",
apiv1.LabelTopologyZone: "us-central1-b",
gceCSITopologyKeyZone: "us-central1-b",
apiv1.LabelHostname: "sillyname",
apiv1.LabelInstanceTypeStable: "n1-standard-8",
apiv1.LabelArchStable: cloudprovider.DefaultArch,
apiv1.LabelOSStable: tc.expectedOsLabel,
}
labels, err := BuildGenericLabels(GceRef{
Name: "kubernetes-minion-group",
Expand Down

0 comments on commit 79c63a7

Please sign in to comment.