[scd] inline cleanup of implicit subscription into CRDB call when rem…

…oving OIR
interuss · Aug 5, 2024 · 49170e3 · 49170e3
1 parent e11b815
commit 49170e3
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 25 deletions.
diff --git a/Makefile b/Makefile
@@ -133,6 +133,9 @@ test-e2e: down-locally build-dss start-locally probe-locally collect-local-logs
 tag:
 	scripts/tag.sh $(UPSTREAM_OWNER)/dss/v$(VERSION)
 
+.PHONY: restart-all
+restart-all: build-dss down-locally start-locally
+
 .PHONY: start-locally
 start-locally:
 	build/dev/run_locally.sh up -d

diff --git a/build/dev/docker-compose_dss.yaml b/build/dev/docker-compose_dss.yaml
@@ -10,13 +10,12 @@ services:
   local-dss-crdb:
     image: cockroachdb/cockroach:v21.2.7
     command: start-single-node --insecure
-    expose:
-      - 26257
     ports:
+      - "26257:26257"
       - "8080:8080"
     restart: always
     networks:
-      - dss_sandbox_default_network 
+      - dss_sandbox_default_network
 
   local-dss-rid-bootstrapper:
     build:
@@ -30,7 +29,7 @@ services:
     depends_on:
       - local-dss-crdb
     networks:
-      - dss_sandbox_default_network 
+      - dss_sandbox_default_network
 
   local-dss-scd-bootstrapper:
     build:
@@ -44,7 +43,7 @@ services:
     depends_on:
       - local-dss-crdb
     networks:
-      - dss_sandbox_default_network 
+      - dss_sandbox_default_network
 
   local-dss-core-service:
     build:
@@ -63,7 +62,7 @@ services:
       - local-dss-rid-bootstrapper
       - local-dss-scd-bootstrapper
     networks:
-      - dss_sandbox_default_network 
+      - dss_sandbox_default_network
 
   local-dss-dummy-oauth:
     build:
@@ -74,7 +73,7 @@ services:
     ports:
       - "8085:8085"
     networks:
-      - dss_sandbox_default_network 
+      - dss_sandbox_default_network
 
 networks:
   dss_sandbox_default_network:

diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go
@@ -92,11 +92,6 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest
 				"OperationalIntent owned by %s, but %s attempted to delete", old.Manager, *req.Auth.ClientID)
 		}
 
-		removeImplicitSubscription, err := subscriptionCanBeRemoved(ctx, r, old.SubscriptionID)
-		if err != nil {
-			return stacktrace.Propagate(err, "Could not determine if Subscription can be removed")
-		}
-
 		// Find Subscriptions that may overlap the OperationalIntent's Volume4D
 		allsubs, err := r.SearchSubscriptions(ctx, &dssmodels.Volume4D{
 			StartTime: old.StartTime,
@@ -130,15 +125,6 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest
 			return stacktrace.Propagate(err, "Unable to delete OperationalIntent from repo")
 		}
 
-		// removeImplicitSubscription is only true if the OIR had a subscription defined
-		if removeImplicitSubscription {
-			// Automatically remove a now-unused implicit Subscription
-			err = r.DeleteSubscription(ctx, *old.SubscriptionID)
-			if err != nil {
-				return stacktrace.Propagate(err, "Unable to delete associated implicit Subscription")
-			}
-		}
-
 		// Return response to client
 		response = &restapi.ChangeOperationalIntentReferenceResponse{
 			OperationalIntentReference: *old.ToRest(),

diff --git a/pkg/scd/store/cockroach/operational_intents.go b/pkg/scd/store/cockroach/operational_intents.go
@@ -146,24 +146,58 @@ func (s *repo) GetOperationalIntent(ctx context.Context, id dssmodels.ID) (*scdm
 func (s *repo) DeleteOperationalIntent(ctx context.Context, id dssmodels.ID) error {
 	var (
 		deleteOperationQuery = `
-			DELETE FROM
+			WITH deleted_oir AS (
+            DELETE FROM
 				scd_operations
 			WHERE
 				id = $1
+            RETURNING
+                id, subscription_id
+            ),
+            exists_and_is_implicit AS (
+                SELECT subscription_id
+                FROM deleted_oir
+                JOIN scd_subscriptions ON scd_subscriptions.id = deleted_oir.subscription_id
+                WHERE scd_subscriptions.implicit = true
+            ),
+            dependent_oirs AS ( -- NOTE: this sub-query will still return the OIR being deleted (!)
+                SELECT id
+                FROM scd_operations
+                WHERE subscription_id = (SELECT subscription_id FROM deleted_oir)
+            ),
+            deleted_subscription_id AS (
+                DELETE FROM scd_subscriptions
+                WHERE id = (SELECT subscription_id FROM exists_and_is_implicit)
+                AND (SELECT COUNT(*) FROM dependent_oirs) = 1 -- NOTE: see above, the OIR being removed is still counted here, hence a value of 1
+                RETURNING id
+            )
+            SELECT id FROM deleted_oir
 		`
 	)
 
 	uid, err := id.PgUUID()
 	if err != nil {
 		return stacktrace.Propagate(err, "Failed to convert id to PgUUID")
 	}
-	res, err := s.q.Exec(ctx, deleteOperationQuery, uid)
+	res, err := s.q.Query(ctx, deleteOperationQuery, uid)
 	if err != nil {
 		return stacktrace.Propagate(err, "Error in query: %s", deleteOperationQuery)
 	}
+	defer res.Close()
 
-	if res.RowsAffected() == 0 {
-		return stacktrace.NewError("Could not delete Operation that does not exist")
+	// Check that the deleted OIR ID was returned:
+	var opID dssmodels.ID
+	if res.Next() {
+		err = res.Scan(&opID)
+		if err != nil {
+			return stacktrace.Propagate(err, "Error scanning deleted Operation ID")
+		}
+	} else if resErr := res.Err(); resErr != nil {
+		return stacktrace.Propagate(resErr, "Error in query: %s", deleteOperationQuery)
+	}
+
+	if opID == "" || opID != id {
+		return stacktrace.NewError("Could not delete Operation that does not exist. Delete: %s, returned opID: %s", id, opID)
 	}
 
 	return nil