Merge pull request #397 from Danil-Grigorev/sync-once-wrong-instance

Bug 1918910: Only log error on wrong instance type for scale from zero
openshift · Mar 25, 2021 · 2d4e76f · 2d4e76f
2 parents 7176b88 + 177bff7
commit 2d4e76f
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 8 deletions.
diff --git a/pkg/actuators/machineset/controller.go b/pkg/actuators/machineset/controller.go
@@ -12,6 +12,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/klog/v2"
 	awsproviderv1 "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsprovider/v1beta1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -75,7 +76,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	}
 	originalMachineSetToPatch := client.MergeFrom(machineSet.DeepCopy())
 
-	result, err := reconcile(machineSet)
+	result, err := r.reconcile(machineSet)
 	if err != nil {
 		logger.Error(err, "Failed to reconcile MachineSet")
 		r.recorder.Eventf(machineSet, corev1.EventTypeWarning, "ReconcileError", "%v", err)
@@ -104,14 +105,19 @@ func isInvalidConfigurationError(err error) bool {
 	return false
 }
 
-func reconcile(machineSet *machinev1.MachineSet) (ctrl.Result, error) {
+func (r *Reconciler) reconcile(machineSet *machinev1.MachineSet) (ctrl.Result, error) {
 	providerConfig, err := awsproviderv1.ProviderSpecFromRawExtension(machineSet.Spec.Template.Spec.ProviderSpec.Value)
 	if err != nil {
 		return ctrl.Result{}, mapierrors.InvalidMachineConfiguration("failed to get providerConfig: %v", err)
 	}
 	instanceType, ok := InstanceTypes[providerConfig.InstanceType]
 	if !ok {
-		return ctrl.Result{}, mapierrors.InvalidMachineConfiguration("unknown instance type: %s", providerConfig.InstanceType)
+		klog.Error("Unable to set scale from zero annotations: unknown instance type: %s", providerConfig.InstanceType)
+		klog.Error("Autoscaling from zero will not work. To fix this, manually populate machine annotations for your instance type: %v", []string{cpuKey, memoryKey, gpuKey})
+
+		// Returning no error to prevent further reconciliation, as user intervention is now required but emit an informational event
+		r.recorder.Eventf(machineSet, corev1.EventTypeWarning, "FailedUpdate", "Failed to set autoscaling from zero annotations, instance type unknown")
+		return ctrl.Result{}, nil
 	}
 
 	if machineSet.Annotations == nil {

diff --git a/pkg/actuators/machineset/controller_test.go b/pkg/actuators/machineset/controller_test.go
@@ -108,7 +108,7 @@ var _ = Describe("MachineSetReconciler", func() {
 			instanceType:        "",
 			existingAnnotations: make(map[string]string),
 			expectedAnnotations: make(map[string]string),
-			expectedEvents:      []string{"ReconcileError"},
+			expectedEvents:      []string{"FailedUpdate"},
 		}),
 		Entry("with a a1.2xlarge", reconcileTestCase{
 			instanceType:        "a1.2xlarge",
@@ -155,7 +155,7 @@ var _ = Describe("MachineSetReconciler", func() {
 				"existing": "annotation",
 				"annother": "existingAnnotation",
 			},
-			expectedEvents: []string{"ReconcileError"},
+			expectedEvents: []string{"FailedUpdate"},
 		}),
 	)
 })
@@ -202,7 +202,8 @@ func TestReconcile(t *testing.T) {
 			instanceType:        "",
 			existingAnnotations: make(map[string]string),
 			expectedAnnotations: make(map[string]string),
-			expectErr:           true,
+			// Expect no error and only log entry in such case as we don't update instance types dynamically
+			expectErr: false,
 		},
 		{
 			name:                "with a a1.2xlarge",
@@ -253,7 +254,8 @@ func TestReconcile(t *testing.T) {
 				"existing": "annotation",
 				"annother": "existingAnnotation",
 			},
-			expectErr: true,
+			// Expect no error and only log entry in such case as we don't update instance types dynamically
+			expectErr: false,
 		},
 	}
 
@@ -264,7 +266,11 @@ func TestReconcile(t *testing.T) {
 			machineSet, err := newTestMachineSet("default", tc.instanceType, tc.existingAnnotations)
 			g.Expect(err).ToNot(HaveOccurred())
 
-			_, err = reconcile(machineSet)
+			r := Reconciler{
+				recorder: record.NewFakeRecorder(1),
+			}
+
+			_, err = r.reconcile(machineSet)
 			g.Expect(err != nil).To(Equal(tc.expectErr))
 			g.Expect(machineSet.Annotations).To(Equal(tc.expectedAnnotations))
 		})