From 49170e336fc553a7fc51852ece97d58ad000e23a Mon Sep 17 00:00:00 2001 From: Julien Perrochet Date: Mon, 29 Jul 2024 14:50:11 +0200 Subject: [PATCH] [scd] inline cleanup of implicit subscription into CRDB call when removing OIR --- Makefile | 3 ++ build/dev/docker-compose_dss.yaml | 13 +++--- pkg/scd/operational_intents_handler.go | 14 ------- .../store/cockroach/operational_intents.go | 42 +++++++++++++++++-- 4 files changed, 47 insertions(+), 25 deletions(-) diff --git a/Makefile b/Makefile index 0e1c3fb48..15078b582 100644 --- 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 index b4a221953..e84e4d078 100644 --- 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 index a09fcb74c..f1972b966 100644 --- 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 index e98d3d280..25c3a2422 100644 --- a/pkg/scd/store/cockroach/operational_intents.go +++ b/pkg/scd/store/cockroach/operational_intents.go @@ -146,10 +146,32 @@ 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 ` ) @@ -157,13 +179,25 @@ func (s *repo) DeleteOperationalIntent(ctx context.Context, id dssmodels.ID) err 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