DBAAS-906 [RHODA][R4] Operator Metrics - Instance with Pending status on Thanos loaded on Grafana with status True (#62)

* DBAAS-906 Operator Metrics - Instance with Pending status on Thanos loaded on Grafana with status True

* DBAAS-906 Operator Metrics - Instance with Pending status on Thanos loaded on Grafana with status True: address review comments

* DBAAS-906 Operator Metrics - Instance with Pending status on Thanos loaded on Grafana with status True: address review comments

Author: jianrongzhang89

cmd/manager/main.go

@@ -135,6 +135,11 @@ func main() {
 		watch.SelectNamespacesPredicate(config.WatchedNamespaces), // select only desired namespaces
 	}
 
+	globalPredicatesWithAnnotations := []predicate.Predicate{
+		watch.CommonPredicatesWithAnnotations(),                   // ignore spurious changes. status changes etc but allow annotation changes
+		watch.SelectNamespacesPredicate(config.WatchedNamespaces), // select only desired namespaces
+	}
+
 	cfg := mgr.GetConfig()
 	clientset, err := kubernetes.NewForConfig(cfg)
 	if err != nil {
@@ -200,7 +205,7 @@ func main() {
 		AtlasDomain:      config.AtlasDomain,
 		GlobalAPISecret:  config.GlobalAPISecret,
 		ResourceWatcher:  watch.NewResourceWatcher(),
-		GlobalPredicates: globalPredicates,
+		GlobalPredicates: globalPredicatesWithAnnotations,
 		EventRecorder:    mgr.GetEventRecorderFor("AtlasDeployment"),
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "AtlasDeployment")

config/manager/manager.yaml

@@ -60,5 +60,5 @@ spec:
                 fieldRef:
                   fieldPath: metadata.namespace
             - name: SYNC_PERIOD_MIN
-              value: 180
+              value: "180"
       terminationGracePeriodSeconds: 10

pkg/controller/atlasinstance/atlasinstance_controller.go

@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"strings"
+	"time"
 
 	"go.uber.org/zap"
 
@@ -58,6 +59,12 @@ import (
 	"github.com/mongodb/mongodb-atlas-kubernetes/pkg/util/kube"
 )
 
+const (
+	instancePhaseChangedInAtlas    = "InstancePhaseChangedInAtlas"
+	instancePhaseChangedInAtlasMsg = "db instance phase has changed in Atlas"
+	updateAnnotationKey            = "atlas.mongodb.com/updated-at"
+)
+
 // MongoDBAtlasInstanceReconciler reconciles a MongoDBAtlasInstance object
 type MongoDBAtlasInstanceReconciler struct {
 	Client      client.Client
@@ -227,11 +234,17 @@ func (r *MongoDBAtlasInstanceReconciler) reconcileAtlasDeployment(cx context.Con
 		return ctrl.Result{}, err
 	}
 
-	result := setInstanceStatusWithDeploymentInfo(atlasClient, inst, atlasDeployment, instData.ProjectName)
+	stateChangedInAtlas, result := setInstanceStatusWithDeploymentInfo(atlasClient, inst, atlasDeployment, instData.ProjectName)
 	if !result.IsOk() {
+		if stateChangedInAtlas {
+			// Update an annotation in the atlas deployment resource to trigger its reconciliation
+			log.Infof("Trigger AtlasDeployment reconciliation. Reason: %v", result.Message())
+			_ = r.annotateAtlasDeployment(cx, atlasDeployment)
+		}
 		log.Infof("Error setting instance status: %v", result.Message())
 		return ctrl.Result{}, errors.New(result.Message())
 	}
+
 	return ctrl.Result{}, nil
 }
 
@@ -358,6 +371,16 @@ func (r *MongoDBAtlasInstanceReconciler) getAtlasProjectForCreation(instance *db
 	}, nil
 }
 
+func (r *MongoDBAtlasInstanceReconciler) annotateAtlasDeployment(cx context.Context, atlasDeployment *v1.AtlasDeployment) error {
+	annotations := atlasDeployment.GetAnnotations()
+	if annotations == nil {
+		annotations = make(map[string]string)
+	}
+	annotations[updateAnnotationKey] = time.Now().Format(time.RFC3339)
+	atlasDeployment.SetAnnotations(annotations)
+	return r.Client.Update(cx, atlasDeployment, &client.UpdateOptions{})
+}
+
 // getAtlasDeploymentSpec returns the spec for the desired cluster
 func getAtlasDeploymentSpec(atlasProject *v1.AtlasProject, data *InstanceData) *v1.AtlasDeploymentSpec {
 	var providerSettingsSpec *v1.ProviderSettingsSpec
@@ -461,7 +484,7 @@ func instanceMutateFn(atlasProject *v1.AtlasProject, atlasDeployment *v1.AtlasDe
 	}
 }
 
-func setInstanceStatusWithDeploymentInfo(atlasClient *mongodbatlas.Client, inst *dbaas.MongoDBAtlasInstance, atlasDeployment *v1.AtlasDeployment, project string) workflow.Result {
+func setInstanceStatusWithDeploymentInfo(atlasClient *mongodbatlas.Client, inst *dbaas.MongoDBAtlasInstance, atlasDeployment *v1.AtlasDeployment, project string) (bool, workflow.Result) {
 	instInfo, result := atlasinventory.GetClusterInfo(atlasClient, project, inst.Spec.Name)
 	if result.IsOk() {
 		// Stores the phase info in inst.Status.Phase and remove from instInfo.InstanceInf map
@@ -479,7 +502,12 @@ func setInstanceStatusWithDeploymentInfo(atlasClient *mongodbatlas.Client, inst
 		if cond.Type == status.DeploymentReadyType {
 			statusFound = true
 			if cond.Status == corev1.ConditionTrue {
-				dbaas.SetInstanceCondition(inst, dbaasv1alpha1.DBaaSInstanceProviderSyncType, metav1.ConditionStatus(cond.Status), "Ready", cond.Message)
+				if inst.Status.Phase == dbaasv1alpha1.InstancePhaseReady {
+					dbaas.SetInstanceCondition(inst, dbaasv1alpha1.DBaaSInstanceProviderSyncType, metav1.ConditionStatus(cond.Status), "Ready", cond.Message)
+					return false, result
+				}
+				dbaas.SetInstanceCondition(inst, dbaasv1alpha1.DBaaSInstanceProviderSyncType, metav1.ConditionFalse, instancePhaseChangedInAtlas, instancePhaseChangedInAtlasMsg)
+				return true, result
 			} else {
 				if strings.Contains(cond.Message, FreeClusterFailed) {
 					inst.Status.Phase = dbaasv1alpha1.InstancePhaseFailed
@@ -491,6 +519,5 @@ func setInstanceStatusWithDeploymentInfo(atlasClient *mongodbatlas.Client, inst
 	if !statusFound {
 		dbaas.SetInstanceCondition(inst, dbaasv1alpha1.DBaaSInstanceProviderSyncType, metav1.ConditionFalse, string(dbaasv1alpha1.InstancePhasePending), "Waiting for cluster creation to start")
 	}
-
-	return result
+	return false, result
 }

pkg/controller/atlasinstance/atlasinstance_test.go

@@ -250,25 +250,40 @@ func TestSetInstanceStatusWithDeploymentInfo(t *testing.T) {
 
 	namespace := "default"
 	testCase := map[string]struct {
-		deploymentName string
-		projectName    string
-		expErrMsg      string
-		expPhase       dbaasv1alpha1.DBaasInstancePhase
-		expStatus      string
+		deploymentName         string
+		projectName            string
+		deploymentCRStatus     corev1.ConditionStatus
+		expErrMsg              string
+		expPhase               dbaasv1alpha1.DBaasInstancePhase
+		expStatus              string
+		expStateChangedInAtlas bool
 	}{
-		"DeploymentCreating": {
-			deploymentName: "mydeploymentcreating",
-			projectName:    "myproject",
-			expErrMsg:      "",
-			expPhase:       dbaasv1alpha1.InstancePhaseCreating,
-			expStatus:      "True",
+		"DeploymentCreatingNorminal": {
+			deploymentName:         "mydeploymentcreating",
+			projectName:            "myproject",
+			deploymentCRStatus:     corev1.ConditionFalse,
+			expErrMsg:              "",
+			expPhase:               dbaasv1alpha1.InstancePhaseCreating,
+			expStatus:              "False",
+			expStateChangedInAtlas: false,
+		},
+		"DeploymentCreatingChangedInAtlas": {
+			deploymentName:         "mydeploymentcreating",
+			projectName:            "myproject",
+			deploymentCRStatus:     corev1.ConditionTrue,
+			expErrMsg:              "",
+			expPhase:               dbaasv1alpha1.InstancePhaseCreating,
+			expStatus:              "False",
+			expStateChangedInAtlas: true,
 		},
 		"DeploymentReady": {
-			deploymentName: "mydeploymentready",
-			projectName:    "myproject",
-			expErrMsg:      "",
-			expPhase:       dbaasv1alpha1.InstancePhaseReady,
-			expStatus:      "True",
+			deploymentName:         "mydeploymentready",
+			projectName:            "myproject",
+			deploymentCRStatus:     corev1.ConditionTrue,
+			expErrMsg:              "",
+			expPhase:               dbaasv1alpha1.InstancePhaseReady,
+			expStatus:              "True",
+			expStateChangedInAtlas: false,
 		},
 		"InvalidProject": {
 			deploymentName: "mydeploymentready",
@@ -303,12 +318,12 @@ func TestSetInstanceStatusWithDeploymentInfo(t *testing.T) {
 						Conditions: []status.Condition{
 							{
 								Type:               status.ConditionType("Ready"),
-								Status:             corev1.ConditionTrue,
+								Status:             tc.deploymentCRStatus,
 								LastTransitionTime: metav1.Now(),
 							},
 							{
 								Type:               status.ConditionType("DeploymentReady"),
-								Status:             corev1.ConditionTrue,
+								Status:             tc.deploymentCRStatus,
 								LastTransitionTime: metav1.Now(),
 							},
 						},
@@ -331,13 +346,14 @@ func TestSetInstanceStatusWithDeploymentInfo(t *testing.T) {
 					},
 				},
 			}
-			result := setInstanceStatusWithDeploymentInfo(atlasClient, inst, atlasDeployment, tc.projectName)
+			stateChangedInAtlas, result := setInstanceStatusWithDeploymentInfo(atlasClient, inst, atlasDeployment, tc.projectName)
 			if len(tc.expErrMsg) == 0 {
 				cond := dbaas.GetInstanceCondition(inst, dbaasv1alpha1.DBaaSInstanceProviderSyncType)
 				assert.NotNil(t, cond)
 				assert.True(t, result.IsOk())
 				assert.Equal(t, inst.Status.Phase, tc.expPhase)
 				assert.Equal(t, string(cond.Status), tc.expStatus)
+				assert.Equal(t, stateChangedInAtlas, tc.expStateChangedInAtlas)
 			} else {
 				assert.Contains(t, result.Message(), tc.expErrMsg)
 			}

pkg/controller/watch/predicates.go

@@ -21,6 +21,20 @@ func CommonPredicates() predicate.Funcs {
 	}
 }
 
+// CommonPredicatesWithAnnotations returns the predicate which filter out the changes done to any field except for spec (e.g. status)
+// Also we should reconcile if finalizers have changed (see https://blog.openshift.com/kubernetes-operators-best-practices/)
+// and we should reconcile if the annotations have changed to allow other controllers to trigger the reconciliation using annotations.
+func CommonPredicatesWithAnnotations() predicate.Funcs {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			if e.ObjectOld.GetGeneration() == e.ObjectNew.GetGeneration() && reflect.DeepEqual(e.ObjectNew.GetFinalizers(), e.ObjectOld.GetFinalizers()) && reflect.DeepEqual(e.ObjectNew.GetAnnotations(), e.ObjectOld.GetAnnotations()) {
+				return false
+			}
+			return true
+		},
+	}
+}
+
 // DeleteOnly returns a predicate that will filter out everything except the Delete event
 func DeleteOnly() predicate.Funcs {
 	return predicate.Funcs{