From 700ffaf04dbd5e6ddd0538960978f525a7ecbc93 Mon Sep 17 00:00:00 2001
From: Yash Mehrotra <yashmehrotra95@gmail.com>
Date: Wed, 26 Jul 2023 14:46:14 +0530
Subject: [PATCH] refactor: canary deletion handling

---
 pkg/controllers/canary_controller.go |  4 +--
 pkg/db/canary.go                     | 45 ++++++++++++++--------------
 pkg/db/topology.go                   |  2 +-
 pkg/jobs/canary/canary_jobs.go       |  4 +--
 4 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/pkg/controllers/canary_controller.go b/pkg/controllers/canary_controller.go
index 8ea3c6610..3e7b1d4b6 100644
--- a/pkg/controllers/canary_controller.go
+++ b/pkg/controllers/canary_controller.go
@@ -87,7 +87,7 @@ func (r *CanaryReconciler) Reconcile(ctx gocontext.Context, req ctrl.Request) (c
 	}
 
 	if !canary.DeletionTimestamp.IsZero() {
-		if err := db.DeleteCanary(*canary); err != nil {
+		if err := db.DeleteCanary(canary.GetPersistedID(), canary.DeletionTimestamp.Time); err != nil {
 			logger.Error(err, "failed to delete canary")
 		}
 		canaryJobs.DeleteCanaryJob(canary.GetPersistedID())
@@ -95,7 +95,7 @@ func (r *CanaryReconciler) Reconcile(ctx gocontext.Context, req ctrl.Request) (c
 		return ctrl.Result{}, r.Update(ctx, canary)
 	}
 
-	dbCanary, checks, changed, err := db.PersistCanary(*canary, "kubernetes/"+string(canary.ObjectMeta.UID))
+	dbCanary, checks, changed, err := db.PersistCanary(*canary, "kubernetes/"+canary.GetPersistedID())
 	if err != nil {
 		return ctrl.Result{Requeue: true}, err
 	}
diff --git a/pkg/db/canary.go b/pkg/db/canary.go
index 792f22a5a..dfd73c394 100644
--- a/pkg/db/canary.go
+++ b/pkg/db/canary.go
@@ -182,37 +182,38 @@ func RemoveTransformedChecks(ids []string) error {
 		Error
 }
 
-func DeleteCanary(canary v1.Canary) error {
-	logger.Infof("deleting canary %s/%s", canary.Namespace, canary.Name)
-	model, err := pkg.CanaryFromV1(canary)
-	if err != nil {
-		return err
-	}
-	deleteTime := time.Now()
-	persistedID := canary.GetPersistedID()
-	var checkIDs []string
-	for _, checkID := range canary.Status.Checks {
-		checkIDs = append(checkIDs, checkID)
-	}
-	metrics.UnregisterGauge(checkIDs)
-	if persistedID == "" {
-		logger.Errorf("Canary %s/%s has not been persisted", canary.Namespace, canary.Name)
-		return nil
-	}
-	if err := Gorm.Where("id = ?", persistedID).Find(&model).UpdateColumn("deleted_at", deleteTime).Error; err != nil {
+func DeleteCanary(id string, deleteTime time.Time) error {
+	logger.Infof("Deleting canary[%s]", id)
+
+	if err := Gorm.Table("canaries").Where("id = ?", id).UpdateColumn("deleted_at", deleteTime).Error; err != nil {
 		return err
 	}
-	if err := DeleteChecksForCanary(persistedID, deleteTime); err != nil {
+	checkIDs, err := DeleteChecksForCanary(id, deleteTime)
+	if err != nil {
 		return err
 	}
-	if err := DeleteCheckComponentRelationshipsForCanary(persistedID, deleteTime); err != nil {
+	metrics.UnregisterGauge(checkIDs)
+
+	if err := DeleteCheckComponentRelationshipsForCanary(id, deleteTime); err != nil {
 		return err
 	}
 	return nil
 }
 
-func DeleteChecksForCanary(id string, deleteTime time.Time) error {
-	return Gorm.Table("checks").Where("canary_id = ? and deleted_at is null", id).UpdateColumn("deleted_at", deleteTime).Error
+func DeleteChecksForCanary(id string, deleteTime time.Time) ([]string, error) {
+	var checkIDs []string
+	var checks []pkg.Check
+	err := Gorm.Model(&checks).
+		Table("checks").
+		Clauses(clause.Returning{Columns: []clause.Column{{Name: "id"}}}).
+		Where("canary_id = ? and deleted_at IS NULL", id).
+		UpdateColumn("deleted_at", deleteTime).
+		Error
+
+	for _, c := range checks {
+		checkIDs = append(checkIDs, c.ID.String())
+	}
+	return checkIDs, err
 }
 
 func DeleteCheckComponentRelationshipsForCanary(id string, deleteTime time.Time) error {
diff --git a/pkg/db/topology.go b/pkg/db/topology.go
index 23189ae9e..b48ca3732 100644
--- a/pkg/db/topology.go
+++ b/pkg/db/topology.go
@@ -377,7 +377,7 @@ func DeleteInlineCanariesForComponent(componentID string, deleteTime time.Time)
 		return err
 	}
 	for _, c := range canaries {
-		if err := DeleteChecksForCanary(c.ID.String(), deleteTime); err != nil {
+		if _, err := DeleteChecksForCanary(c.ID.String(), deleteTime); err != nil {
 			logger.Debugf("Error deleting checks for canary %v", c.ID)
 			continue
 		}
diff --git a/pkg/jobs/canary/canary_jobs.go b/pkg/jobs/canary/canary_jobs.go
index 20785f819..f4231de12 100644
--- a/pkg/jobs/canary/canary_jobs.go
+++ b/pkg/jobs/canary/canary_jobs.go
@@ -415,7 +415,7 @@ func ReconcileDeletedCanaryChecks() {
 		ID        string
 		DeletedAt time.Time
 	}
-	// Select all components whose topology ID is deleted but their deleted at is not marked
+	// Select all components whose canary ID is deleted but their deleted at is not marked
 	err := db.Gorm.Raw(`
         SELECT DISTINCT(canaries.id), canaries.deleted_at
         FROM canaries
@@ -432,7 +432,7 @@ func ReconcileDeletedCanaryChecks() {
 	}
 
 	for _, r := range rows {
-		if err := db.DeleteChecksForCanary(r.ID, r.DeletedAt); err != nil {
+		if err := db.DeleteCanary(r.ID, r.DeletedAt); err != nil {
 			logger.Errorf("Error deleting checks for canary[%s]: %v", r.ID, err)
 			jobHistory.AddError(err.Error())
 		}