Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -361,12 +361,17 @@ func (ng *nodegroup) TemplateNodeInfo() (*framework.NodeInfo, error) {
},
}

nsi := ng.scalableResource.InstanceSystemInfo()
if nsi != nil {
node.Status.NodeInfo = *nsi
}

node.Status.Capacity = capacity
node.Status.Allocatable = capacity
node.Status.Conditions = cloudprovider.BuildReadyConditions()
node.Spec.Taints = ng.scalableResource.Taints()

node.Labels, err = ng.buildTemplateLabels(nodeName)
node.Labels, err = ng.buildTemplateLabels(nodeName, nsi)
if err != nil {
return nil, err
}
Expand All @@ -380,8 +385,19 @@ func (ng *nodegroup) TemplateNodeInfo() (*framework.NodeInfo, error) {
return nodeInfo, nil
}

func (ng *nodegroup) buildTemplateLabels(nodeName string) (map[string]string, error) {
labels := cloudprovider.JoinStringMaps(buildGenericLabels(nodeName), ng.scalableResource.Labels())
func (ng *nodegroup) buildTemplateLabels(nodeName string, nsi *corev1.NodeSystemInfo) (map[string]string, error) {
nsiLabels := make(map[string]string)
if nsi != nil {
nsiLabels[corev1.LabelArchStable] = nsi.Architecture
nsiLabels[corev1.LabelOSStable] = nsi.OperatingSystem
}

// The order of priority is:
// - Labels set in existing nodes for not-autoscale-from-zero cases
// - Labels set in the labels capacity annotation of machine template, machine set, and machine deployment.
// - Values in the status.nodeSystemInfo of MachineTemplates
// - Generic/default labels set in the environment of the cluster autoscaler
labels := cloudprovider.JoinStringMaps(buildGenericLabels(nodeName), nsiLabels, ng.scalableResource.Labels())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit academic, but now that there's a chance we'll be passing in a nil map[string]string to cloudprovider.JoinStringMaps, let's update the existing TestJoinStringMaps UT to validate that use case.

Something like

$ git diff
diff --git a/cluster-autoscaler/cloudprovider/util_test.go b/cluster-autoscaler/cloudprovider/util_test.go
index ba23d6180..ee99f2bcb 100644
--- a/cluster-autoscaler/cloudprovider/util_test.go
+++ b/cluster-autoscaler/cloudprovider/util_test.go
@@ -44,9 +44,11 @@ func TestBuildKubeProxy(t *testing.T) {
 }
 
 func TestJoinStringMaps(t *testing.T) {
+       emptyMapBeginning := make(map[string]string)
        map1 := map[string]string{"1": "a", "2": "b"}
        map2 := map[string]string{"3": "c", "2": "d"}
        map3 := map[string]string{"5": "e"}
-       result := JoinStringMaps(map1, map2, map3)
+       emptyMapEnd := make(map[string]string)
+       result := JoinStringMaps(emptyMapBeginning, map1, map2, map3, emptyMapEnd)
        assert.Equal(t, map[string]string{"1": "a", "2": "d", "3": "c", "5": "e"}, result)
 }


nodes, err := ng.Nodes()
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type testConfigBuilder struct {
nodeCount int
annotations map[string]string
capacity map[string]string
nodeInfo map[string]string
}

// NewTestConfigBuilder returns a builder for dynamically constructing mock ClusterAPI resources for testing.
Expand Down Expand Up @@ -91,6 +92,7 @@ func (b *testConfigBuilder) Build() *TestConfig {
isMachineDeployment,
b.annotations,
b.capacity,
b.nodeInfo,
)[0],
)[0]
}
Expand All @@ -111,6 +113,7 @@ func (b *testConfigBuilder) BuildMultiple(configCount int) []*TestConfig {
isMachineDeployment,
b.annotations,
b.capacity,
b.nodeInfo,
)...,
)
}
Expand Down Expand Up @@ -171,6 +174,18 @@ func (b *testConfigBuilder) WithCapacity(c map[string]string) *testConfigBuilder
return b
}

func (b *testConfigBuilder) WithNodeInfo(n map[string]string) *testConfigBuilder {
if n == nil {
b.nodeInfo = nil
} else {
if b.nodeInfo == nil {
b.nodeInfo = map[string]string{}
}
maps.Insert(b.nodeInfo, maps.All(n))
}
return b
}

// TestConfig contains clusterspecific information about a single test configuration.
type TestConfig struct {
spec *TestSpec
Expand Down Expand Up @@ -290,8 +305,8 @@ func createTestConfigs(specs ...TestSpec) []*TestConfig {
UID: config.machineSet.GetUID(),
}

if spec.capacity != nil {
klog.V(4).Infof("adding capacity to machine template")
if spec.capacity != nil || spec.nodeInfo != nil {
klog.V(4).Infof("creating machine template")
config.machineTemplate = &unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1",
Expand All @@ -303,13 +318,25 @@ func createTestConfigs(specs ...TestSpec) []*TestConfig {
},
},
}
}
if spec.capacity != nil {
klog.V(4).Infof("adding capacity to machine template")
if err := unstructured.SetNestedStringMap(config.machineTemplate.Object, spec.capacity, "status", "capacity"); err != nil {
panic(err)
}
} else {
klog.V(4).Infof("not adding capacity")
}

if spec.nodeInfo != nil {
klog.V(4).Infof("adding node info")
if err := unstructured.SetNestedStringMap(config.machineTemplate.Object, spec.nodeInfo, "status", "nodeInfo"); err != nil {
panic(err)
}
} else {
klog.V(4).Infof("not adding node info")
}

for j := 0; j < spec.nodeCount; j++ {
config.nodes[j], config.machines[j] = makeLinkedNodeAndMachine(j, spec.namespace, spec.clusterName, machineOwner, machineSetLabels)
}
Expand All @@ -324,6 +351,7 @@ func createTestConfigs(specs ...TestSpec) []*TestConfig {
type TestSpec struct {
annotations map[string]string
capacity map[string]string
nodeInfo map[string]string
machineDeploymentName string
machineSetName string
machinePoolName string
Expand All @@ -333,17 +361,17 @@ type TestSpec struct {
rootIsMachineDeployment bool
}

func createTestSpecs(namespace, clusterName, namePrefix string, scalableResourceCount, nodeCount int, isMachineDeployment bool, annotations map[string]string, capacity map[string]string) []TestSpec {
func createTestSpecs(namespace, clusterName, namePrefix string, scalableResourceCount, nodeCount int, isMachineDeployment bool, annotations map[string]string, capacity map[string]string, nodeInfo map[string]string) []TestSpec {
var specs []TestSpec

for i := 0; i < scalableResourceCount; i++ {
specs = append(specs, createTestSpec(namespace, clusterName, fmt.Sprintf("%s-%d", namePrefix, i), nodeCount, isMachineDeployment, annotations, capacity))
specs = append(specs, createTestSpec(namespace, clusterName, fmt.Sprintf("%s-%d", namePrefix, i), nodeCount, isMachineDeployment, annotations, capacity, nodeInfo))
}

return specs
}

func createTestSpec(namespace, clusterName, name string, nodeCount int, isMachineDeployment bool, annotations map[string]string, capacity map[string]string) TestSpec {
func createTestSpec(namespace, clusterName, name string, nodeCount int, isMachineDeployment bool, annotations map[string]string, capacity map[string]string, nodeInfo map[string]string) TestSpec {
return TestSpec{
annotations: annotations,
capacity: capacity,
Expand All @@ -353,6 +381,7 @@ func createTestSpec(namespace, clusterName, name string, nodeCount int, isMachin
namespace: namespace,
nodeCount: nodeCount,
rootIsMachineDeployment: isMachineDeployment,
nodeInfo: nodeInfo,
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"path"
"strconv"
"strings"
"sync"
"time"

"github.com/pkg/errors"
Expand All @@ -42,6 +43,8 @@ import (
type unstructuredScalableResource struct {
controller *machineController
unstructured *unstructured.Unstructured
infraObj *unstructured.Unstructured
infraMutex sync.RWMutex
maxSize int
minSize int
autoscalingOptions map[string]string
Expand Down Expand Up @@ -321,6 +324,17 @@ func (r unstructuredScalableResource) InstanceCapacity() (map[corev1.ResourceNam
return capacity, nil
}

// InstanceSystemInfo sets the nodeSystemInfo from the infrastructure reference resource.
// If the infrastructure reference resource is not found, returns nil.
func (r unstructuredScalableResource) InstanceSystemInfo() *apiv1.NodeSystemInfo {
infraObj, err := r.readInfrastructureReferenceResource()
if err != nil || infraObj == nil {
return nil
}
nsiObj := systemInfoFromInfrastructureObject(infraObj)
return &nsiObj
}

func (r unstructuredScalableResource) InstanceResourceSlices(nodeName string) ([]*resourceapi.ResourceSlice, error) {
var result []*resourceapi.ResourceSlice
driver := r.InstanceDRADriver()
Expand Down Expand Up @@ -390,6 +404,17 @@ func (r unstructuredScalableResource) InstanceDRADriver() string {
}

func (r unstructuredScalableResource) readInfrastructureReferenceResource() (*unstructured.Unstructured, error) {
// Cache w/ lazy loading of the infrastructure reference resource.
r.infraMutex.RLock()
if r.infraObj != nil {
defer r.infraMutex.RUnlock()
return r.infraObj, nil
}
r.infraMutex.RUnlock()

r.infraMutex.Lock()
defer r.infraMutex.Unlock()

obKind := r.unstructured.GetKind()
obName := r.unstructured.GetName()

Expand Down Expand Up @@ -440,6 +465,8 @@ func (r unstructuredScalableResource) readInfrastructureReferenceResource() (*un
return nil, err
}

r.infraObj = infra

return infra, nil
}

Expand Down Expand Up @@ -477,6 +504,25 @@ func resourceCapacityFromInfrastructureObject(infraobj *unstructured.Unstructure
return capacity
}

func systemInfoFromInfrastructureObject(infraobj *unstructured.Unstructured) apiv1.NodeSystemInfo {
nsi := apiv1.NodeSystemInfo{}
infransi, found, err := unstructured.NestedStringMap(infraobj.Object, "status", "nodeInfo")
if !found || err != nil {
return nsi
}

for k, v := range infransi {
switch k {
case "architecture":
nsi.Architecture = v
case "operatingSystem":
nsi.OperatingSystem = v
}
}

return nsi
}

// adapted from https://github.com/kubernetes/kubernetes/blob/release-1.25/pkg/util/taints/taints.go#L39
func parseTaint(st string) (apiv1.Taint, error) {
var taint apiv1.Taint
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,14 @@ import (
)

const (
cpuStatusKey = "cpu"
memoryStatusKey = "memory"
nvidiaGpuStatusKey = "nvidia.com/gpu"
cpuStatusKey = "cpu"
memoryStatusKey = "memory"
nvidiaGpuStatusKey = "nvidia.com/gpu"
architectureStatusKey = "architecture"
operatingSystemStatusKey = "operatingSystem"

arm64 = "arm64"
linux = "linux"
)

func TestSetSize(t *testing.T) {
Expand Down Expand Up @@ -595,3 +600,110 @@ func TestCanScaleFromZero(t *testing.T) {
})
}
}

func TestInstanceSystemInfo(t *testing.T) {
// use a constant capacity as that's necessary for the business logic to consider the resource scalable
capacity := map[string]string{
cpuStatusKey: "1",
memoryStatusKey: "4G",
}
testConfigs := []struct {
name string
nodeInfo map[string]string
expectedArch string
expectedOS string
}{
{
"with no architecture or operating system in machine template's status' nodeInfo, the system info is empty",
map[string]string{},
"",
"",
},
{
"with architecture in machine template's status' nodeInfo, the system info is filled in the scalable resource",
map[string]string{
architectureStatusKey: arm64,
},
arm64,
"",
},
{
"with operating system in machine template's status' nodeInfo, the system info is filled in the scalable resource",
map[string]string{
operatingSystemStatusKey: linux,
},
"",
linux,
},
{
"with architecture and operating system in machine template's status' nodeInfo, the system info is filled in the scalable resource",
map[string]string{
architectureStatusKey: arm64,
operatingSystemStatusKey: linux,
},
arm64,
linux,
},
}

for _, tc := range testConfigs {
testname := fmt.Sprintf("MachineSet %s", tc.name)
t.Run(testname, func(t *testing.T) {
mdTestConfig := NewTestConfigBuilder().
ForMachineSet().
WithNodeCount(1).
WithCapacity(capacity).
WithNodeInfo(tc.nodeInfo).
Build()
controller := NewTestMachineController(t)
defer controller.Stop()
controller.AddTestConfigs(mdTestConfig)

testResource := mdTestConfig.machineSet

sr, err := newUnstructuredScalableResource(controller.machineController, testResource)
if err != nil {
t.Fatal(err)
}

sysInfo := sr.InstanceSystemInfo()
if sysInfo.Architecture != tc.expectedArch {
t.Errorf("expected architecture %s, got %s", tc.nodeInfo[architectureStatusKey], sysInfo.Architecture)
}
if sysInfo.OperatingSystem != tc.expectedOS {
t.Errorf("expected operating system %s, got %s", tc.nodeInfo[operatingSystemStatusKey], sysInfo.OperatingSystem)
}
})
}

for _, tc := range testConfigs {
testname := fmt.Sprintf("MachineDeployment %s", tc.name)
t.Run(testname, func(t *testing.T) {
mdTestConfig := NewTestConfigBuilder().
ForMachineDeployment().
WithNodeCount(1).
WithCapacity(capacity).
WithNodeInfo(tc.nodeInfo).
Build()
controller := NewTestMachineController(t)
defer controller.Stop()
controller.AddTestConfigs(mdTestConfig)

testResource := mdTestConfig.machineDeployment

sr, err := newUnstructuredScalableResource(controller.machineController, testResource)
if err != nil {
t.Fatal(err)
}

sysInfo := sr.InstanceSystemInfo()
if sysInfo.Architecture != tc.expectedArch {
t.Errorf("expected architecture %s, got %s", tc.nodeInfo[architectureStatusKey], sysInfo.Architecture)
}

if sysInfo.OperatingSystem != tc.expectedOS {
t.Errorf("expected operating system %s, got %s", tc.nodeInfo[operatingSystemStatusKey], sysInfo.OperatingSystem)
}
})
}
}
Loading