From 902a764ec033d647e799953ca61863936f3c1f77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kuba=20Tu=C5=BCnik?= Date: Tue, 21 Jun 2022 14:07:18 +0200 Subject: [PATCH] CA: GCE: Return UnknownArch from ToSystemArchitecture for invalid architectures PR #4973 changed ToSystemArchitecture behavior to return DefaultArch instead of UnknownArch for invalid architectures. This kind of defaulting makes sense while parsing KUBE_ENV, but prevents using the function in contexts where an invalid architecture should result in an error. This commit reverts ToSystemArchitecture to previous behavior, and moves defaulting to the callsite. --- .../cloudprovider/gce/templates.go | 28 +++-- .../cloudprovider/gce/templates_test.go | 116 ++++++++++++++++-- 2 files changed, 123 insertions(+), 21 deletions(-) diff --git a/cluster-autoscaler/cloudprovider/gce/templates.go b/cluster-autoscaler/cloudprovider/gce/templates.go index 2e40d39ee00a..46be651db469 100644 --- a/cluster-autoscaler/cloudprovider/gce/templates.go +++ b/cluster-autoscaler/cloudprovider/gce/templates.go @@ -187,9 +187,10 @@ func (t *GceTemplateBuilder) BuildNodeFromTemplate(mig Mig, template *gce.Instan if osDistribution == OperatingSystemDistributionUnknown { return nil, fmt.Errorf("could not obtain os-distribution from kube-env from template metadata") } - arch := extractSystemArchitectureFromKubeEnv(kubeEnvValue) - if arch == UnknownArch { - return nil, fmt.Errorf("could not obtain arch from kube-env from template metadata") + arch, err := extractSystemArchitectureFromKubeEnv(kubeEnvValue) + if err != nil { + arch = DefaultArch + klog.Errorf("Couldn't extract architecture from kube-env for MIG %q, falling back to %q. Error: %v", mig.Id(), arch, err) } var ephemeralStorage int64 = -1 @@ -578,20 +579,23 @@ func (s SystemArchitecture) Name() string { return string(s) } -func extractSystemArchitectureFromKubeEnv(kubeEnv string) SystemArchitecture { - arch, found, err := extractAutoscalerVarFromKubeEnv(kubeEnv, "arch") +func extractSystemArchitectureFromKubeEnv(kubeEnv string) (SystemArchitecture, error) { + archName, found, err := extractAutoscalerVarFromKubeEnv(kubeEnv, "arch") if err != nil { - klog.Errorf("error while obtaining arch from AUTOSCALER_ENV_VARS; using default %v", err) - return UnknownArch + return UnknownArch, fmt.Errorf("error while obtaining arch from AUTOSCALER_ENV_VARS: %v", err) } if !found { - klog.V(4).Infof("no arch defined in AUTOSCALER_ENV_VARS; using default %v", err) - return DefaultArch + return UnknownArch, fmt.Errorf("no arch defined in AUTOSCALER_ENV_VARS") } - return ToSystemArchitecture(arch) + arch := ToSystemArchitecture(archName) + if arch == UnknownArch { + return UnknownArch, fmt.Errorf("unknown arch %q defined in AUTOSCALER_ENV_VARS", archName) + } + return arch, nil } -// ToSystemArchitecture parses a string to SystemArchitecture +// ToSystemArchitecture parses a string to SystemArchitecture. Returns UnknownArch if the string doesn't represent a +// valid architecture. func ToSystemArchitecture(arch string) SystemArchitecture { switch arch { case string(Arm64): @@ -599,7 +603,7 @@ func ToSystemArchitecture(arch string) SystemArchitecture { case string(Amd64): return Amd64 default: - return DefaultArch + return UnknownArch } } diff --git a/cluster-autoscaler/cloudprovider/gce/templates_test.go b/cluster-autoscaler/cloudprovider/gce/templates_test.go index fe1c6fb187a7..470c0ba6011f 100644 --- a/cluster-autoscaler/cloudprovider/gce/templates_test.go +++ b/cluster-autoscaler/cloudprovider/gce/templates_test.go @@ -27,6 +27,8 @@ import ( gpuUtils "k8s.io/autoscaler/cluster-autoscaler/utils/gpu" "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" gce "google.golang.org/api/compute/v1" apiv1 "k8s.io/api/core/v1" @@ -170,15 +172,6 @@ func TestBuildNodeFromTemplateSetsResources(t *testing.T) { attachedLocalSSDCount: 4, expectedErr: false, }, - { - scenario: "handle empty arch gracefully", - kubeEnv: "AUTOSCALER_ENV_VARS: os_distribution=cos;arch=;os=linux;ephemeral_storage_local_ssd_count=2\n", - physicalCpu: 8, - physicalMemory: 200 * units.MiB, - ephemeralStorageLocalSSDCount: 2, - attachedLocalSSDCount: 4, - expectedErr: false, - }, { scenario: "ephemeral storage on local SSDs with kube-reserved", kubeEnv: "AUTOSCALER_ENV_VARS: kube_reserved=cpu=0,memory=0,ephemeral-storage=10Gi;os_distribution=cos;os=linux;ephemeral_storage_local_ssd_count=2\n", @@ -1172,6 +1165,111 @@ func TestParseKubeReserved(t *testing.T) { } } +func TestToSystemArchitecture(t *testing.T) { + for tn, tc := range map[string]struct { + archName string + wantArch SystemArchitecture + }{ + "valid architecture is converted": { + archName: "amd64", + wantArch: Amd64, + }, + "invalid architecture results in UnknownArchitecture": { + archName: "some-arch", + wantArch: UnknownArch, + }, + } { + t.Run(tn, func(t *testing.T) { + gotArch := ToSystemArchitecture(tc.archName) + if diff := cmp.Diff(tc.wantArch, gotArch); diff != "" { + t.Errorf("ToSystemArchitecture diff (-want +got):\n%s", diff) + } + }) + } +} + +func TestExtractSystemArchitectureFromKubeEnv(t *testing.T) { + for tn, tc := range map[string]struct { + kubeEnv string + wantArch SystemArchitecture + wantErr error + }{ + "valid arch defined in AUTOSCALER_ENV_VARS": { + kubeEnv: "AUTOSCALER_ENV_VARS: os_distribution=cos;arch=arm64;os=linux\n", + wantArch: Arm64, + }, + "invalid arch defined in AUTOSCALER_ENV_VARS": { + kubeEnv: "AUTOSCALER_ENV_VARS: os_distribution=cos;arch=blah;os=linux\n", + wantArch: UnknownArch, + wantErr: cmpopts.AnyError, + }, + "empty arch defined in AUTOSCALER_ENV_VARS": { + kubeEnv: "AUTOSCALER_ENV_VARS: os_distribution=cos;arch=;os=linux\n", + wantArch: UnknownArch, + wantErr: cmpopts.AnyError, + }, + + "no arch defined in AUTOSCALER_ENV_VARS": { + kubeEnv: "AUTOSCALER_ENV_VARS: os_distribution=cos;os=linux\n", + wantArch: UnknownArch, + wantErr: cmpopts.AnyError, + }, + "KUBE_ENV parsing error": { + kubeEnv: "some-invalid-string", + wantArch: UnknownArch, + wantErr: cmpopts.AnyError, + }, + } { + t.Run(tn, func(t *testing.T) { + gotArch, gotErr := extractSystemArchitectureFromKubeEnv(tc.kubeEnv) + if diff := cmp.Diff(tc.wantArch, gotArch); diff != "" { + t.Errorf("extractSystemArchitectureFromKubeEnv diff (-want +got):\n%s", diff) + } + if diff := cmp.Diff(tc.wantErr, gotErr, cmpopts.EquateErrors()); diff != "" { + t.Errorf("extractSystemArchitectureFromKubeEnv error diff (-want +got):\n%s", diff) + } + }) + } +} + +func TestBuildNodeFromTemplateArch(t *testing.T) { + for tn, tc := range map[string]struct { + kubeEnv string + wantArch SystemArchitecture + }{ + "valid arch defined in KUBE_ENV is passed through": { + kubeEnv: "AUTOSCALER_ENV_VARS: os_distribution=cos;arch=arm64;os=linux\n", + wantArch: Arm64, + }, + "invalid arch defined in KUBE_ENV is defaulted to the default arch": { + kubeEnv: "AUTOSCALER_ENV_VARS: os_distribution=cos;arch=;os=linux\n", + wantArch: DefaultArch, + }, + } { + t.Run(tn, func(t *testing.T) { + mig := &gceMig{gceRef: GceRef{Name: "some-name", Project: "some-proj", Zone: "us-central1-b"}} + template := &gce.InstanceTemplate{ + Name: "node-name", + Properties: &gce.InstanceProperties{ + Metadata: &gce.Metadata{ + Items: []*gce.MetadataItems{{Key: "kube-env", Value: &tc.kubeEnv}}, + }, + Disks: []*gce.AttachedDisk{}, + }, + } + tb := &GceTemplateBuilder{} + gotNode, gotErr := tb.BuildNodeFromTemplate(mig, template, 16, 128, nil, &GceReserved{}) + if gotErr != nil { + t.Fatalf("BuildNodeFromTemplate unexpected error: %v", gotErr) + } + gotArch := gotNode.Labels[apiv1.LabelArchStable] + if diff := cmp.Diff(tc.wantArch.Name(), gotArch); diff != "" { + t.Errorf("BuildNodeFromTemplate arch label diff (-want +got):\n%s", diff) + } + }) + } +} + func makeTaintSet(taints []apiv1.Taint) map[apiv1.Taint]bool { set := make(map[apiv1.Taint]bool) for _, taint := range taints {