From 2059f0f3fd4353a61e48e5bb9fd804995b6552de Mon Sep 17 00:00:00 2001 From: safwank97 Date: Fri, 15 Sep 2023 10:57:20 -0400 Subject: [PATCH 1/7] Propagate errors of ARO MachineHealthCheckController to ARO Operator --- cmd/aro/operator.go | 6 +- .../machinehealthcheck_controller.go | 64 ++++++++++++------- .../machinehealthcheck_controller_test.go | 36 ++++++----- 3 files changed, 64 insertions(+), 42 deletions(-) diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go index 6ed38bca917..ba3de7fcb1e 100644 --- a/cmd/aro/operator.go +++ b/cmd/aro/operator.go @@ -191,10 +191,10 @@ func operator(ctx context.Context, log *logrus.Entry) error { client)).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create controller %s: %v", autosizednodes.ControllerName, err) } - if err = (machinehealthcheck.NewReconciler( - log.WithField("controller", machinehealthcheck.ControllerName), + if err = (machinehealthcheck.NewMachineHealthCheckReconciler( + log.WithField("controller", machinehealthcheck.MHCControllerName), client, dh)).SetupWithManager(mgr); err != nil { - return fmt.Errorf("unable to create controller %s: %v", machinehealthcheck.ControllerName, err) + return fmt.Errorf("unable to create controller %s: %v", machinehealthcheck.MHCControllerName, err) } if err = (ingress.NewReconciler( log.WithField("controller", ingress.ControllerName), diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go index 809aeb0b9a7..825684dafcd 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go @@ -13,7 +13,6 @@ import ( monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" "github.com/sirupsen/logrus" kruntime "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -22,6 +21,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" + "github.com/Azure/ARO-RP/pkg/operator/controllers/base" "github.com/Azure/ARO-RP/pkg/util/dynamichelper" ) @@ -32,50 +32,63 @@ var machinehealthcheckYaml []byte var mhcremediationalertYaml []byte const ( - ControllerName string = "MachineHealthCheck" - managed string = "aro.machinehealthcheck.managed" - enabled string = "aro.machinehealthcheck.enabled" + MHCControllerName string = "MachineHealthCheck" + MHCManaged string = "aro.machinehealthcheck.managed" + MHCEnabled string = "aro.machinehealthcheck.enabled" ) -type Reconciler struct { - log *logrus.Entry +type MachineHealthCheckReconciler struct { + base.AROController dh dynamichelper.Interface - - client client.Client } -func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *Reconciler { - return &Reconciler{ - log: log, - dh: dh, - client: client, +func NewMachineHealthCheckReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *MachineHealthCheckReconciler { + return &MachineHealthCheckReconciler{ + AROController: base.AROController{ + Log: log, + Client: client, + Name: MHCControllerName, + }, + dh: dh, } } -func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { - instance := &arov1alpha1.Cluster{} - err := r.client.Get(ctx, types.NamespacedName{Name: arov1alpha1.SingletonClusterName}, instance) +// Reconcile watches MachineHealthCheck objects, and if any changes, +// reconciles the associated ARO MachineHealthCheck object + +func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { + instance, err := r.GetCluster(ctx) + if err != nil { return reconcile.Result{}, err } - if !instance.Spec.OperatorFlags.GetSimpleBoolean(enabled) { - r.log.Debug("controller is disabled") + if !instance.Spec.OperatorFlags.GetSimpleBoolean(MHCEnabled) { + r.Log.Debug("controller is disabled") return reconcile.Result{}, nil } - r.log.Debug("running") - if !instance.Spec.OperatorFlags.GetSimpleBoolean(managed) { + r.Log.Debug("running") + if !instance.Spec.OperatorFlags.GetSimpleBoolean(MHCManaged) { + r.SetProgressing(ctx, "We are disabling the MHCHealthCheck because this cluster at current moment is NOT MHC Managed.") + err := r.dh.EnsureDeleted(ctx, "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck") if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + return reconcile.Result{RequeueAfter: time.Hour}, err } err = r.dh.EnsureDeleted(ctx, "PrometheusRule", "openshift-machine-api", "mhc-remediation-alert") if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + return reconcile.Result{RequeueAfter: time.Hour}, err } + return reconcile.Result{}, nil } @@ -84,6 +97,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. for _, asset := range [][]byte{machinehealthcheckYaml, mhcremediationalertYaml} { resource, _, err := scheme.Codecs.UniversalDeserializer().Decode(asset, nil, nil) if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + return reconcile.Result{}, err } @@ -105,21 +121,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // create/update the MHC CR err = r.dh.Ensure(ctx, resources...) if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + return reconcile.Result{}, err } + r.ClearConditions(ctx) return reconcile.Result{}, nil } // SetupWithManager will manage only our MHC resource with our specific controller name -func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { +func (r *MachineHealthCheckReconciler) SetupWithManager(mgr ctrl.Manager) error { aroClusterPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { return strings.EqualFold(arov1alpha1.SingletonClusterName, o.GetName()) }) return ctrl.NewControllerManagedBy(mgr). For(&arov1alpha1.Cluster{}, builder.WithPredicates(aroClusterPredicate)). - Named(ControllerName). + Named(MHCControllerName). Owns(&machinev1beta1.MachineHealthCheck{}). Owns(&monitoringv1.PrometheusRule{}). Complete(r) diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go index d25953cd03a..0c21580b36a 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go @@ -23,7 +23,7 @@ import ( ) // Test reconcile function -func TestReconciler(t *testing.T) { +func TestMachineHealthCheckReconciler(t *testing.T) { type test struct { name string instance *arov1alpha1.Cluster @@ -46,7 +46,7 @@ func TestReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - enabled: strconv.FormatBool(false), + MHCEnabled: strconv.FormatBool(false), }, }, }, @@ -64,8 +64,8 @@ func TestReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - enabled: strconv.FormatBool(true), - managed: strconv.FormatBool(false), + MHCEnabled: strconv.FormatBool(true), + MHCManaged: strconv.FormatBool(false), }, }, }, @@ -83,8 +83,8 @@ func TestReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - enabled: strconv.FormatBool(true), - managed: strconv.FormatBool(false), + MHCEnabled: strconv.FormatBool(true), + MHCManaged: strconv.FormatBool(false), }, }, }, @@ -102,8 +102,8 @@ func TestReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - enabled: strconv.FormatBool(true), - managed: strconv.FormatBool(false), + MHCEnabled: strconv.FormatBool(true), + MHCManaged: strconv.FormatBool(false), }, }, }, @@ -122,8 +122,8 @@ func TestReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - enabled: strconv.FormatBool(true), - managed: strconv.FormatBool(true), + MHCEnabled: strconv.FormatBool(true), + MHCManaged: strconv.FormatBool(true), }, }, }, @@ -140,8 +140,8 @@ func TestReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - enabled: strconv.FormatBool(true), - managed: strconv.FormatBool(true), + MHCEnabled: strconv.FormatBool(true), + MHCManaged: strconv.FormatBool(true), }, }, }, @@ -165,11 +165,13 @@ func TestReconciler(t *testing.T) { } ctx := context.Background() - r := &Reconciler{ - log: logrus.NewEntry(logrus.StandardLogger()), - dh: mdh, - client: clientBuilder.Build(), - } + + r := NewMachineHealthCheckReconciler( + logrus.NewEntry(logrus.StandardLogger()), + clientBuilder.Build(), + mdh, + ) + request := ctrl.Request{} request.Name = "cluster" From 8af014d4e6bed5767802fd281d0328d69d96ceef Mon Sep 17 00:00:00 2001 From: safwank97 Date: Tue, 19 Sep 2023 09:14:39 -0400 Subject: [PATCH 2/7] adding a clearCondition to clear the progressing field and removing a blank line --- .../machinehealthcheck/machinehealthcheck_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go index 825684dafcd..3f5be5c3c9c 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go @@ -56,7 +56,6 @@ func NewMachineHealthCheckReconciler(log *logrus.Entry, client client.Client, dh // Reconcile watches MachineHealthCheck objects, and if any changes, // reconciles the associated ARO MachineHealthCheck object - func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { instance, err := r.GetCluster(ctx) @@ -89,6 +88,7 @@ func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ct return reconcile.Result{RequeueAfter: time.Hour}, err } + r.ClearConditions(ctx) return reconcile.Result{}, nil } From 8858a8a40f23af511df2429b330c060d97ce7d42 Mon Sep 17 00:00:00 2001 From: safwank97 Date: Wed, 4 Oct 2023 17:39:31 -0400 Subject: [PATCH 3/7] Modifying test file for validating against the changes made on MHC Controller --- .../machinehealthcheck_controller.go | 4 +- .../machinehealthcheck_controller_test.go | 82 +++++++++++++++++-- 2 files changed, 76 insertions(+), 10 deletions(-) diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go index 3f5be5c3c9c..68e3e5974a0 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go @@ -70,12 +70,13 @@ func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ct r.Log.Debug("running") if !instance.Spec.OperatorFlags.GetSimpleBoolean(MHCManaged) { - r.SetProgressing(ctx, "We are disabling the MHCHealthCheck because this cluster at current moment is NOT MHC Managed.") + r.SetProgressing(ctx, "MHC and it's alerts are disabled because this cluster is not MHC managed.") err := r.dh.EnsureDeleted(ctx, "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck") if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) + r.ClearProgressing(ctx) return reconcile.Result{RequeueAfter: time.Hour}, err } @@ -84,6 +85,7 @@ func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ct if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) + r.ClearProgressing(ctx) return reconcile.Result{RequeueAfter: time.Hour}, err } diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go index 0c21580b36a..9395425b35a 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go @@ -19,24 +19,36 @@ import ( arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper" _ "github.com/Azure/ARO-RP/pkg/util/scheme" + utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" + operatorv1 "github.com/openshift/api/operator/v1" ) // Test reconcile function func TestMachineHealthCheckReconciler(t *testing.T) { + + transitionTime := metav1.Time{Time: time.Now()} + defaultAvailable := utilconditions.ControllerDefaultAvailable(MHCControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(MHCControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(MHCControllerName) + + defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} + type test struct { name string instance *arov1alpha1.Cluster mocks func(mdh *mock_dynamichelper.MockInterface) + wantConditions []operatorv1.OperatorCondition wantErr string wantRequeueAfter time.Duration } for _, tt := range []*test{ { - name: "Failure to get instance", - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, - wantErr: `clusters.aro.openshift.io "cluster" not found`, + name: "Failure to get instance", + mocks: func(mdh *mock_dynamichelper.MockInterface) {}, + wantConditions: defaultConditions, + wantErr: `clusters.aro.openshift.io "cluster" not found`, }, { name: "Enabled Feature Flag is false", @@ -49,12 +61,16 @@ func TestMachineHealthCheckReconciler(t *testing.T) { MHCEnabled: strconv.FormatBool(false), }, }, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, }, mocks: func(mdh *mock_dynamichelper.MockInterface) { mdh.EXPECT().EnsureDeleted(gomock.Any(), "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck").Times(0) mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(nil).Times(0) }, - wantErr: "", + wantConditions: defaultConditions, + wantErr: "", }, { name: "Managed Feature Flag is false: ensure mhc and its alert are deleted", @@ -68,12 +84,16 @@ func TestMachineHealthCheckReconciler(t *testing.T) { MHCManaged: strconv.FormatBool(false), }, }, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, }, mocks: func(mdh *mock_dynamichelper.MockInterface) { mdh.EXPECT().EnsureDeleted(gomock.Any(), "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck").Times(1) mdh.EXPECT().EnsureDeleted(gomock.Any(), "PrometheusRule", "openshift-machine-api", "mhc-remediation-alert").Times(1) }, - wantErr: "", + wantConditions: defaultConditions, + wantErr: "", }, { name: "Managed Feature Flag is false: mhc fails to delete, an error is returned", @@ -87,11 +107,24 @@ func TestMachineHealthCheckReconciler(t *testing.T) { MHCManaged: strconv.FormatBool(false), }, }, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, }, mocks: func(mdh *mock_dynamichelper.MockInterface) { mdh.EXPECT().EnsureDeleted(gomock.Any(), "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck").Return(errors.New("Could not delete mhc")) }, - wantErr: "Could not delete mhc", + wantErr: "Could not delete mhc", + wantConditions: []operatorv1.OperatorCondition{ + defaultAvailable, + defaultProgressing, + { + Type: MHCControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionTrue, + LastTransitionTime: transitionTime, + Message: "Could not delete mhc", + }, + }, wantRequeueAfter: time.Hour, }, { @@ -106,12 +139,25 @@ func TestMachineHealthCheckReconciler(t *testing.T) { MHCManaged: strconv.FormatBool(false), }, }, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, }, mocks: func(mdh *mock_dynamichelper.MockInterface) { mdh.EXPECT().EnsureDeleted(gomock.Any(), "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck").Times(1) mdh.EXPECT().EnsureDeleted(gomock.Any(), "PrometheusRule", "openshift-machine-api", "mhc-remediation-alert").Return(errors.New("Could not delete mhc alert")) }, - wantErr: "Could not delete mhc alert", + wantErr: "Could not delete mhc alert", + wantConditions: []operatorv1.OperatorCondition{ + defaultAvailable, + defaultProgressing, + { + Type: MHCControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionTrue, + LastTransitionTime: transitionTime, + Message: "Could not delete mhc alert", + }, + }, wantRequeueAfter: time.Hour, }, { @@ -130,7 +176,8 @@ func TestMachineHealthCheckReconciler(t *testing.T) { mocks: func(mdh *mock_dynamichelper.MockInterface) { mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(nil).Times(1) }, - wantErr: "", + wantConditions: defaultConditions, + wantErr: "", }, { name: "When ensuring resources fails, an error is returned", @@ -144,11 +191,24 @@ func TestMachineHealthCheckReconciler(t *testing.T) { MHCManaged: strconv.FormatBool(true), }, }, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, }, mocks: func(mdh *mock_dynamichelper.MockInterface) { mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(errors.New("failed to ensure")) }, wantErr: "failed to ensure", + wantConditions: []operatorv1.OperatorCondition{ + defaultAvailable, + defaultProgressing, + { + Type: MHCControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionTrue, + LastTransitionTime: transitionTime, + Message: "failed to ensure", + }, + }, }, } { t.Run(tt.name, func(t *testing.T) { @@ -178,7 +238,11 @@ func TestMachineHealthCheckReconciler(t *testing.T) { result, err := r.Reconcile(ctx, request) if tt.wantRequeueAfter != result.RequeueAfter { - t.Errorf("Wanted to requeue after %v but was set to %v", tt.wantRequeueAfter, result.RequeueAfter) + t.Errorf("wanted to requeue after %v but was set to %v", tt.wantRequeueAfter, result.RequeueAfter) + } + + if tt.instance != nil { + utilconditions.AssertControllerConditions(t, ctx, r.AROController.Client, tt.wantConditions) } utilerror.AssertErrorMessage(t, err, tt.wantErr) From 31f4058edaf3d5774eec44b6921bb0896d2664e8 Mon Sep 17 00:00:00 2001 From: safwank97 Date: Thu, 5 Oct 2023 04:54:01 -0400 Subject: [PATCH 4/7] Resolving linting issues --- .../machinehealthcheck/machinehealthcheck_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go index 9395425b35a..c88e05a28b6 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go @@ -26,7 +26,6 @@ import ( // Test reconcile function func TestMachineHealthCheckReconciler(t *testing.T) { - transitionTime := metav1.Time{Time: time.Now()} defaultAvailable := utilconditions.ControllerDefaultAvailable(MHCControllerName) defaultProgressing := utilconditions.ControllerDefaultProgressing(MHCControllerName) From abe539ef327306b17f8397e8b2b7757d6b5dd920 Mon Sep 17 00:00:00 2001 From: safwank97 Date: Thu, 5 Oct 2023 13:51:37 -0400 Subject: [PATCH 5/7] Resolving linting issues focused around mixed import type --- cmd/aro/operator.go | 4 +-- .../machinehealthcheck_controller.go | 14 ++++---- .../machinehealthcheck_controller_test.go | 36 +++++++++---------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go index ba3de7fcb1e..507c761669b 100644 --- a/cmd/aro/operator.go +++ b/cmd/aro/operator.go @@ -192,9 +192,9 @@ func operator(ctx context.Context, log *logrus.Entry) error { return fmt.Errorf("unable to create controller %s: %v", autosizednodes.ControllerName, err) } if err = (machinehealthcheck.NewMachineHealthCheckReconciler( - log.WithField("controller", machinehealthcheck.MHCControllerName), + log.WithField("controller", machinehealthcheck.ControllerName), client, dh)).SetupWithManager(mgr); err != nil { - return fmt.Errorf("unable to create controller %s: %v", machinehealthcheck.MHCControllerName, err) + return fmt.Errorf("unable to create controller %s: %v", machinehealthcheck.ControllerName, err) } if err = (ingress.NewReconciler( log.WithField("controller", ingress.ControllerName), diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go index 68e3e5974a0..d518d981efb 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go @@ -32,9 +32,9 @@ var machinehealthcheckYaml []byte var mhcremediationalertYaml []byte const ( - MHCControllerName string = "MachineHealthCheck" - MHCManaged string = "aro.machinehealthcheck.managed" - MHCEnabled string = "aro.machinehealthcheck.enabled" + ControllerName string = "MachineHealthCheck" + managed string = "aro.machinehealthcheck.managed" + enabled string = "aro.machinehealthcheck.enabled" ) type MachineHealthCheckReconciler struct { @@ -48,7 +48,7 @@ func NewMachineHealthCheckReconciler(log *logrus.Entry, client client.Client, dh AROController: base.AROController{ Log: log, Client: client, - Name: MHCControllerName, + Name: ControllerName, }, dh: dh, } @@ -63,13 +63,13 @@ func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ct return reconcile.Result{}, err } - if !instance.Spec.OperatorFlags.GetSimpleBoolean(MHCEnabled) { + if !instance.Spec.OperatorFlags.GetSimpleBoolean(enabled) { r.Log.Debug("controller is disabled") return reconcile.Result{}, nil } r.Log.Debug("running") - if !instance.Spec.OperatorFlags.GetSimpleBoolean(MHCManaged) { + if !instance.Spec.OperatorFlags.GetSimpleBoolean(managed) { r.SetProgressing(ctx, "MHC and it's alerts are disabled because this cluster is not MHC managed.") err := r.dh.EnsureDeleted(ctx, "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck") @@ -141,7 +141,7 @@ func (r *MachineHealthCheckReconciler) SetupWithManager(mgr ctrl.Manager) error return ctrl.NewControllerManagedBy(mgr). For(&arov1alpha1.Cluster{}, builder.WithPredicates(aroClusterPredicate)). - Named(MHCControllerName). + Named(ControllerName). Owns(&machinev1beta1.MachineHealthCheck{}). Owns(&monitoringv1.PrometheusRule{}). Complete(r) diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go index c88e05a28b6..2db7d61bbb6 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/golang/mock/gomock" + operatorv1 "github.com/openshift/api/operator/v1" "github.com/sirupsen/logrus" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" @@ -21,15 +22,14 @@ import ( _ "github.com/Azure/ARO-RP/pkg/util/scheme" utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" - operatorv1 "github.com/openshift/api/operator/v1" ) // Test reconcile function func TestMachineHealthCheckReconciler(t *testing.T) { transitionTime := metav1.Time{Time: time.Now()} - defaultAvailable := utilconditions.ControllerDefaultAvailable(MHCControllerName) - defaultProgressing := utilconditions.ControllerDefaultProgressing(MHCControllerName) - defaultDegraded := utilconditions.ControllerDefaultDegraded(MHCControllerName) + defaultAvailable := utilconditions.ControllerDefaultAvailable(ControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(ControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(ControllerName) defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} @@ -57,7 +57,7 @@ func TestMachineHealthCheckReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - MHCEnabled: strconv.FormatBool(false), + enabled: strconv.FormatBool(false), }, }, Status: arov1alpha1.ClusterStatus{ @@ -79,8 +79,8 @@ func TestMachineHealthCheckReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - MHCEnabled: strconv.FormatBool(true), - MHCManaged: strconv.FormatBool(false), + enabled: strconv.FormatBool(true), + managed: strconv.FormatBool(false), }, }, Status: arov1alpha1.ClusterStatus{ @@ -102,8 +102,8 @@ func TestMachineHealthCheckReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - MHCEnabled: strconv.FormatBool(true), - MHCManaged: strconv.FormatBool(false), + enabled: strconv.FormatBool(true), + managed: strconv.FormatBool(false), }, }, Status: arov1alpha1.ClusterStatus{ @@ -118,7 +118,7 @@ func TestMachineHealthCheckReconciler(t *testing.T) { defaultAvailable, defaultProgressing, { - Type: MHCControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Type: ControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, Status: operatorv1.ConditionTrue, LastTransitionTime: transitionTime, Message: "Could not delete mhc", @@ -134,8 +134,8 @@ func TestMachineHealthCheckReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - MHCEnabled: strconv.FormatBool(true), - MHCManaged: strconv.FormatBool(false), + enabled: strconv.FormatBool(true), + managed: strconv.FormatBool(false), }, }, Status: arov1alpha1.ClusterStatus{ @@ -151,7 +151,7 @@ func TestMachineHealthCheckReconciler(t *testing.T) { defaultAvailable, defaultProgressing, { - Type: MHCControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Type: ControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, Status: operatorv1.ConditionTrue, LastTransitionTime: transitionTime, Message: "Could not delete mhc alert", @@ -167,8 +167,8 @@ func TestMachineHealthCheckReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - MHCEnabled: strconv.FormatBool(true), - MHCManaged: strconv.FormatBool(true), + enabled: strconv.FormatBool(true), + managed: strconv.FormatBool(true), }, }, }, @@ -186,8 +186,8 @@ func TestMachineHealthCheckReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - MHCEnabled: strconv.FormatBool(true), - MHCManaged: strconv.FormatBool(true), + enabled: strconv.FormatBool(true), + managed: strconv.FormatBool(true), }, }, Status: arov1alpha1.ClusterStatus{ @@ -202,7 +202,7 @@ func TestMachineHealthCheckReconciler(t *testing.T) { defaultAvailable, defaultProgressing, { - Type: MHCControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Type: ControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, Status: operatorv1.ConditionTrue, LastTransitionTime: transitionTime, Message: "failed to ensure", From bf8b8d987ad31b6b83d1b7ff87f76d35c3ab80d2 Mon Sep 17 00:00:00 2001 From: safwank97 Date: Thu, 12 Oct 2023 22:57:23 -0400 Subject: [PATCH 6/7] Refactoring the code to follow our controller patterns and adding proper message for setProgressing use case --- cmd/aro/operator.go | 2 +- .../machinehealthcheck_controller.go | 12 ++++++------ .../machinehealthcheck_controller_test.go | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go index 507c761669b..6ed38bca917 100644 --- a/cmd/aro/operator.go +++ b/cmd/aro/operator.go @@ -191,7 +191,7 @@ func operator(ctx context.Context, log *logrus.Entry) error { client)).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create controller %s: %v", autosizednodes.ControllerName, err) } - if err = (machinehealthcheck.NewMachineHealthCheckReconciler( + if err = (machinehealthcheck.NewReconciler( log.WithField("controller", machinehealthcheck.ControllerName), client, dh)).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create controller %s: %v", machinehealthcheck.ControllerName, err) diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go index d518d981efb..7a9a137b2f3 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go @@ -37,14 +37,14 @@ const ( enabled string = "aro.machinehealthcheck.enabled" ) -type MachineHealthCheckReconciler struct { +type Reconciler struct { base.AROController dh dynamichelper.Interface } -func NewMachineHealthCheckReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *MachineHealthCheckReconciler { - return &MachineHealthCheckReconciler{ +func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *Reconciler { + return &Reconciler{ AROController: base.AROController{ Log: log, Client: client, @@ -56,7 +56,7 @@ func NewMachineHealthCheckReconciler(log *logrus.Entry, client client.Client, dh // Reconcile watches MachineHealthCheck objects, and if any changes, // reconciles the associated ARO MachineHealthCheck object -func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { +func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { instance, err := r.GetCluster(ctx) if err != nil { @@ -70,7 +70,7 @@ func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ct r.Log.Debug("running") if !instance.Spec.OperatorFlags.GetSimpleBoolean(managed) { - r.SetProgressing(ctx, "MHC and it's alerts are disabled because this cluster is not MHC managed.") + r.SetProgressing(ctx, "Not MHC Managed for cluster maintenance purpose.") err := r.dh.EnsureDeleted(ctx, "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck") if err != nil { @@ -134,7 +134,7 @@ func (r *MachineHealthCheckReconciler) Reconcile(ctx context.Context, request ct } // SetupWithManager will manage only our MHC resource with our specific controller name -func (r *MachineHealthCheckReconciler) SetupWithManager(mgr ctrl.Manager) error { +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { aroClusterPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { return strings.EqualFold(arov1alpha1.SingletonClusterName, o.GetName()) }) diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go index 2db7d61bbb6..a9ef0dd5a46 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go @@ -225,7 +225,7 @@ func TestMachineHealthCheckReconciler(t *testing.T) { ctx := context.Background() - r := NewMachineHealthCheckReconciler( + r := NewReconciler( logrus.NewEntry(logrus.StandardLogger()), clientBuilder.Build(), mdh, From c78161cac89202bcd0f19491c05cd305e6ca4134 Mon Sep 17 00:00:00 2001 From: safwank97 Date: Tue, 17 Oct 2023 13:01:08 -0400 Subject: [PATCH 7/7] removing the setProgressing and clearProgressing calls --- .../machinehealthcheck/machinehealthcheck_controller.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go index 7a9a137b2f3..7db456217f0 100644 --- a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go +++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go @@ -70,13 +70,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. r.Log.Debug("running") if !instance.Spec.OperatorFlags.GetSimpleBoolean(managed) { - r.SetProgressing(ctx, "Not MHC Managed for cluster maintenance purpose.") - err := r.dh.EnsureDeleted(ctx, "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck") if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) - r.ClearProgressing(ctx) return reconcile.Result{RequeueAfter: time.Hour}, err } @@ -85,7 +82,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) - r.ClearProgressing(ctx) return reconcile.Result{RequeueAfter: time.Hour}, err }