From 217e8debbe4492b9612198de58169f9c28eb3551 Mon Sep 17 00:00:00 2001
From: Aditya Thebe <contact@adityathebe.com>
Date: Wed, 9 Oct 2024 09:56:54 +0545
Subject: [PATCH] fix: canary suspension

---
 pkg/controllers/canary_controller.go | 28 +++++++++++++++++++++-------
 pkg/db/canary.go                     | 12 ++++++++----
 pkg/jobs/canary/canary_jobs.go       |  3 ++-
 pkg/jobs/canary/sync.go              |  5 +++--
 pkg/runner/runner.go                 |  4 ++++
 pkg/utils/utils.go                   | 14 ++++++++++++++
 6 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/pkg/controllers/canary_controller.go b/pkg/controllers/canary_controller.go
index 1e24cc88c..a159beb27 100644
--- a/pkg/controllers/canary_controller.go
+++ b/pkg/controllers/canary_controller.go
@@ -79,6 +79,7 @@ func (r *CanaryReconciler) Reconcile(parentCtx gocontext.Context, req ctrl.Reque
 	if runner.IsCanaryIgnored(&canary.ObjectMeta) {
 		return ctrl.Result{}, nil
 	}
+
 	ctx := r.Context.WithObject(canary.ObjectMeta).WithName(req.NamespacedName.String())
 
 	canary.SetRunnerName(r.RunnerName)
@@ -203,14 +204,9 @@ func (r *CanaryReconciler) updateCanaryInDB(ctx dutyContext.Context, canary *v1.
 		dbCanary = cacheObj.(*pkg.Canary)
 	}
 
-	// Compare canary spec and spec in database
-	// If they do not match, persist the canary in database
-	canarySpecJSON, err := json.Marshal(canary.Spec)
-	if err != nil {
+	if changed, err := hasCanaryChanged(canary, dbCanary); err != nil {
 		return nil, err
-	}
-	opts := jsondiff.DefaultJSONOptions()
-	if diff, _ := jsondiff.Compare(canarySpecJSON, dbCanary.Spec, &opts); diff != jsondiff.FullMatch {
+	} else if changed {
 		dbCanary, err = r.persistAndCacheCanary(ctx, canary)
 		if err != nil {
 			return nil, err
@@ -220,6 +216,24 @@ func (r *CanaryReconciler) updateCanaryInDB(ctx dutyContext.Context, canary *v1.
 	return dbCanary, nil
 }
 
+func hasCanaryChanged(canary *v1.Canary, dbCanary *pkg.Canary) (bool, error) {
+	if !utils.IsMapIdentical(canary.Annotations, dbCanary.Annotations) {
+		return true, nil
+	}
+
+	// Compare canary spec and spec in database
+	// If they do not match, persist the canary in database
+	canarySpecJSON, err := json.Marshal(canary.Spec)
+	if err != nil {
+		return false, err
+	}
+
+	opts := jsondiff.DefaultJSONOptions()
+	diff, _ := jsondiff.Compare(canarySpecJSON, dbCanary.Spec, &opts)
+	specChanged := diff != jsondiff.FullMatch
+	return specChanged, nil
+}
+
 func (r *CanaryReconciler) Report() {
 	for payload := range canaryJobs.CanaryStatusChannel {
 		var canary v1.Canary
diff --git a/pkg/db/canary.go b/pkg/db/canary.go
index 407e14586..4a61f5055 100644
--- a/pkg/db/canary.go
+++ b/pkg/db/canary.go
@@ -449,11 +449,15 @@ func PersistCanaryModel(ctx context.Context, model pkg.Canary) (*pkg.Canary, boo
 
 	var changed bool
 	if existing.ID != uuid.Nil {
-		jsonDiff, err := diff.JSONCompare(string(model.Spec), string(existing.Spec))
-		if err != nil {
-			return nil, false, fmt.Errorf("failed to compare old and existing model")
+		if !utils.IsMapIdentical(model.Annotations, existing.Annotations) {
+			changed = true
+		} else {
+			jsonDiff, err := diff.JSONCompare(string(model.Spec), string(existing.Spec))
+			if err != nil {
+				return nil, false, fmt.Errorf("failed to compare old and existing model")
+			}
+			changed = jsonDiff != ""
 		}
-		changed = jsonDiff != ""
 	}
 
 	var oldCheckIDs []string
diff --git a/pkg/jobs/canary/canary_jobs.go b/pkg/jobs/canary/canary_jobs.go
index 891c708a7..feacbb19c 100644
--- a/pkg/jobs/canary/canary_jobs.go
+++ b/pkg/jobs/canary/canary_jobs.go
@@ -63,9 +63,10 @@ func (j CanaryJob) GetNamespacedName() types.NamespacedName {
 }
 
 func (j CanaryJob) Run(ctx dutyjob.JobRuntime) error {
-	if runner.IsCanaryIgnored(&j.Canary.ObjectMeta) {
+	if runner.IsCanarySuspended(&j.Canary.ObjectMeta) || runner.IsCanaryIgnored(&j.Canary.ObjectMeta) {
 		return nil
 	}
+
 	canaryID := j.DBCanary.ID.String()
 	ctx.History.ResourceID = canaryID
 	ctx.History.ResourceType = "canary"
diff --git a/pkg/jobs/canary/sync.go b/pkg/jobs/canary/sync.go
index d8e264ed6..af2b757fb 100644
--- a/pkg/jobs/canary/sync.go
+++ b/pkg/jobs/canary/sync.go
@@ -94,7 +94,7 @@ func SyncCanaryJob(ctx context.Context, dbCanary pkg.Canary) error {
 		return nil
 	}
 
-	if runner.IsCanaryIgnored(&canary.ObjectMeta) {
+	if runner.IsCanarySuspended(&canary.ObjectMeta) || runner.IsCanaryIgnored(&canary.ObjectMeta) {
 		Unschedule(id)
 		return nil
 	}
@@ -208,9 +208,10 @@ func ScanCanaryConfigs(ctx context.Context) {
 		}
 
 		for _, canary := range configs {
-			if runner.IsCanaryIgnored(&canary.ObjectMeta) {
+			if runner.IsCanarySuspended(&canary.ObjectMeta) || runner.IsCanaryIgnored(&canary.ObjectMeta) {
 				continue
 			}
+
 			_, _, err := db.PersistCanary(ctx, canary, path.Base(configfile))
 			if err != nil {
 				logger.Errorf("could not persist %s: %v", canary.Name, err)
diff --git a/pkg/runner/runner.go b/pkg/runner/runner.go
index 19f4e3c12..0d4316bd3 100644
--- a/pkg/runner/runner.go
+++ b/pkg/runner/runner.go
@@ -46,5 +46,9 @@ func IsCanaryIgnored(canary *metav1.ObjectMeta) bool {
 		}
 	}
 
+	return false
+}
+
+func IsCanarySuspended(canary *metav1.ObjectMeta) bool {
 	return canary.Annotations != nil && canary.Annotations["suspend"] == "true"
 }
diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
index a737cab91..7202456a9 100644
--- a/pkg/utils/utils.go
+++ b/pkg/utils/utils.go
@@ -172,3 +172,17 @@ func ParseTime(t string) *time.Time {
 
 	return nil
 }
+
+func IsMapIdentical[K comparable](map1, map2 map[string]K) bool {
+	if len(map1) != len(map2) {
+		return false
+	}
+
+	for k, v1 := range map1 {
+		if v2, exists := map2[k]; !exists || v1 != v2 {
+			return false
+		}
+	}
+
+	return true
+}