From f1d9035d1436a4ea1aa5220c521875c756e1640b Mon Sep 17 00:00:00 2001 From: Srinivas Atmakuri Date: Tue, 4 Jul 2023 12:00:14 +0530 Subject: [PATCH] Propagate errors of ARO Ingress controller to ARO Operator (#2940) * Propagate errors of ARO Ingress controller to Operator * Modified the test cases to use the new conditions --- .../controllers/ingress/ingress_controller.go | 30 ++++++---- .../ingress/ingress_controller_test.go | 60 +++++++++++++++++-- 2 files changed, 72 insertions(+), 18 deletions(-) diff --git a/pkg/operator/controllers/ingress/ingress_controller.go b/pkg/operator/controllers/ingress/ingress_controller.go index 7e3eff1cf2a..1f4b618ef64 100644 --- a/pkg/operator/controllers/ingress/ingress_controller.go +++ b/pkg/operator/controllers/ingress/ingress_controller.go @@ -17,6 +17,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" ) const ( @@ -31,47 +32,50 @@ const ( // Reconciler spots openshift ingress controllers has abnormal replica counts (less than 2) // when happens, it tries to rescale the controller to 2 replicas, i.e., the minimum required replicas type Reconciler struct { - log *logrus.Entry - - client client.Client + base.AROController } func NewReconciler(log *logrus.Entry, client client.Client) *Reconciler { return &Reconciler{ - log: log, - client: client, + AROController: base.AROController{ + Log: log, + Client: client, + Name: ControllerName, + }, } } 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) + instance, err := r.GetCluster(ctx) if err != nil { return reconcile.Result{}, err } if !instance.Spec.OperatorFlags.GetSimpleBoolean(controllerEnabled) { - r.log.Debug("controller is disabled") + r.Log.Debug("controller is disabled") return reconcile.Result{}, nil } - r.log.Debug("running") + r.Log.Debug("running") ingress := &operatorv1.IngressController{} - err = r.client.Get(ctx, types.NamespacedName{Namespace: openshiftIngressControllerNamespace, Name: openshiftIngressControllerName}, ingress) + err = r.Client.Get(ctx, types.NamespacedName{Namespace: openshiftIngressControllerNamespace, Name: openshiftIngressControllerName}, ingress) if err != nil { - r.log.Error(err) + r.Log.Error(err) + r.SetDegraded(ctx, err) return reconcile.Result{}, err } if ingress.Spec.Replicas != nil && *ingress.Spec.Replicas < minimumReplicas { ingress.Spec.Replicas = to.Int32Ptr(minimumReplicas) - err := r.client.Update(ctx, ingress) + err := r.Client.Update(ctx, ingress) if err != nil { - r.log.Error(err) + r.Log.Error(err) + r.SetDegraded(ctx, err) return reconcile.Result{}, err } } + r.ClearConditions(ctx) return reconcile.Result{}, nil } diff --git a/pkg/operator/controllers/ingress/ingress_controller_test.go b/pkg/operator/controllers/ingress/ingress_controller_test.go index 83423b66706..132f600b91b 100644 --- a/pkg/operator/controllers/ingress/ingress_controller_test.go +++ b/pkg/operator/controllers/ingress/ingress_controller_test.go @@ -6,6 +6,7 @@ package ingress import ( "context" "testing" + "time" "github.com/Azure/go-autorest/autorest/to" operatorv1 "github.com/openshift/api/operator/v1" @@ -17,10 +18,17 @@ import ( arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" _ "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" ) func TestReconciler(t *testing.T) { + transitionTime := metav1.Time{Time: time.Now()} + defaultAvailable := utilconditions.ControllerDefaultAvailable(ControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(ControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(ControllerName) + defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} + fakeCluster := func(controllerEnabledFlag string) *arov1alpha1.Cluster { return &arov1alpha1.Cluster{ ObjectMeta: metav1.ObjectMeta{ @@ -40,15 +48,30 @@ func TestReconciler(t *testing.T) { ingressController *operatorv1.IngressController expectedReplica int32 expectedError string + startConditions []operatorv1.OperatorCondition + wantConditions []operatorv1.OperatorCondition }{ { name: "aro ingress controller disabled", controllerEnabledFlag: "false", + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "openshift ingress controller not found", controllerEnabledFlag: "true", expectedError: "ingresscontrollers.operator.openshift.io \"default\" not found", + startConditions: defaultConditions, + wantConditions: []operatorv1.OperatorCondition{ + defaultAvailable, + defaultProgressing, + { + Type: ControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionTrue, + LastTransitionTime: transitionTime, + Message: `ingresscontrollers.operator.openshift.io "default" not found`, + }, + }, }, { name: "openshift ingress controller has 3 replicas", @@ -63,6 +86,8 @@ func TestReconciler(t *testing.T) { }, }, expectedReplica: 3, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "openshift ingress controller has 2 replicas (minimum required replicas)", @@ -77,6 +102,8 @@ func TestReconciler(t *testing.T) { }, }, expectedReplica: minimumReplicas, + startConditions: defaultConditions, + wantConditions: defaultConditions, }, { name: "openshift ingress controller has 1 replica", @@ -91,6 +118,17 @@ func TestReconciler(t *testing.T) { }, }, expectedReplica: minimumReplicas, + startConditions: []operatorv1.OperatorCondition{ + defaultAvailable, + defaultProgressing, + { + Type: ControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionTrue, + LastTransitionTime: transitionTime, + Message: `ingresscontrollers.operator.openshift.io has 1 replica`, + }, + }, + wantConditions: defaultConditions, }, { name: "openshift ingress controller has 0 replica", @@ -105,12 +143,26 @@ func TestReconciler(t *testing.T) { }, }, expectedReplica: minimumReplicas, + startConditions: []operatorv1.OperatorCondition{ + defaultAvailable, + defaultProgressing, + { + Type: ControllerName + "Controller" + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionTrue, + LastTransitionTime: transitionTime, + Message: `ingresscontrollers.operator.openshift.io has 0 replica`, + }, + }, + wantConditions: defaultConditions, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { clusterMock := fakeCluster(tt.controllerEnabledFlag) + if len(tt.startConditions) > 0 { + clusterMock.Status.Conditions = append(clusterMock.Status.Conditions, tt.startConditions...) + } clientBuilder := ctrlfake.NewClientBuilder().WithObjects(clusterMock) if tt.ingressController != nil { @@ -118,20 +170,18 @@ func TestReconciler(t *testing.T) { } clientFake := clientBuilder.Build() - r := &Reconciler{ - log: logrus.NewEntry(logrus.StandardLogger()), - client: clientFake, - } + r := NewReconciler(logrus.NewEntry(logrus.StandardLogger()), clientFake) request := ctrl.Request{} ctx := context.Background() _, err := r.Reconcile(ctx, request) utilerror.AssertErrorMessage(t, err, tt.expectedError) + utilconditions.AssertControllerConditions(t, ctx, clientFake, tt.wantConditions) if tt.ingressController != nil { ingress := &operatorv1.IngressController{} - err = r.client.Get(ctx, types.NamespacedName{Namespace: openshiftIngressControllerNamespace, Name: openshiftIngressControllerName}, ingress) + err = r.Client.Get(ctx, types.NamespacedName{Namespace: openshiftIngressControllerNamespace, Name: openshiftIngressControllerName}, ingress) if err != nil { t.Error(err) }