From 0f5338e22795f265cc8992e795892b5bf1d6db06 Mon Sep 17 00:00:00 2001 From: Julien Perrochet Date: Thu, 18 Jul 2024 22:35:18 +0200 Subject: [PATCH] [scd] properly cleanup implicit sub on oir update --- pkg/scd/operational_intents_handler.go | 79 ++++++++++++++++++-------- 1 file changed, 54 insertions(+), 25 deletions(-) diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go index 4679d59cb..d2ba4056c 100644 --- a/pkg/scd/operational_intents_handler.go +++ b/pkg/scd/operational_intents_handler.go @@ -16,6 +16,33 @@ import ( "github.com/interuss/stacktrace" ) +func subscriptionCanBeRemoved(ctx context.Context, r repos.Repository, subscriptionID *dssmodels.ID) (bool, error) { + // Get the Subscription supporting the OperationalIntent, if one is defined + if subscriptionID != nil { + sub, err := r.GetSubscription(ctx, *subscriptionID) + if err != nil { + return false, stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo") + } + if sub == nil { + return false, stacktrace.NewError("OperationalIntent's Subscription missing from repo") + } + + if sub.ImplicitSubscription { + // Get the Subscription's dependent OperationalIntents + dependentOps, err := r.GetDependentOperationalIntents(ctx, sub.ID) + if err != nil { + return false, stacktrace.Propagate(err, "Could not find dependent OperationalIntents") + } + if len(dependentOps) == 0 { + return false, stacktrace.NewError("An implicit Subscription had no dependent OperationalIntents") + } else if len(dependentOps) == 1 { + return true, nil + } + } + } + return false, nil +} + // DeleteOperationalIntentReference deletes a single operational intent ref for a given ID at // the specified version. func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *restapi.DeleteOperationalIntentReferenceRequest, @@ -56,30 +83,9 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest "OperationalIntent owned by %s, but %s attempted to delete", old.Manager, *req.Auth.ClientID) } - // Get the Subscription supporting the OperationalIntent, if one is defined - var sub *scdmodels.Subscription - removeImplicitSubscription := false - if old.SubscriptionID != nil { - sub, err = r.GetSubscription(ctx, *old.SubscriptionID) - if err != nil { - return stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo") - } - if sub == nil { - return stacktrace.NewError("OperationalIntent's Subscription missing from repo") - } - - if sub.ImplicitSubscription { - // Get the Subscription's dependent OperationalIntents - dependentOps, err := r.GetDependentOperationalIntents(ctx, sub.ID) - if err != nil { - return stacktrace.Propagate(err, "Could not find dependent OperationalIntents") - } - if len(dependentOps) == 0 { - return stacktrace.NewError("An implicit Subscription had no dependent OperationalIntents") - } else if len(dependentOps) == 1 { - removeImplicitSubscription = true - } - } + 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 @@ -118,7 +124,7 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest // removeImplicitSubscription is only true if the OIR had a subscription defined if removeImplicitSubscription { // Automatically remove a now-unused implicit Subscription - err = r.DeleteSubscription(ctx, sub.ID) + err = r.DeleteSubscription(ctx, *old.SubscriptionID) if err != nil { return stacktrace.Propagate(err, "Unable to delete associated implicit Subscription") } @@ -457,6 +463,8 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize if err != nil { return stacktrace.Propagate(err, "Could not get OperationalIntent from repo") } + + var previousSubscriptionID *dssmodels.ID if old != nil { if old.Manager != manager { return stacktrace.NewErrorWithCode(dsserr.PermissionDenied, @@ -468,6 +476,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } version = int32(old.Version) + previousSubscriptionID = old.SubscriptionID } else { if ovn != "" { return stacktrace.NewErrorWithCode(dsserr.NotFound, "OperationalIntent does not exist and therefore is not version %s", ovn) @@ -477,6 +486,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } var sub *scdmodels.Subscription + removePreviousImplicitSubscription := false if subscriptionID.Empty() { // Create an implicit subscription if the implicit subscription params are set: // for situations where these params are required but have not been set, @@ -490,6 +500,17 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize } } + // If an implicit subscription creation is requested, we need to check if: + // - a previous subscription was attached, + // - if so, if it was an implicit subscription + // - if so, if we can remove it after creating the new implicit subscription + removePreviousImplicitSubscription, err = subscriptionCanBeRemoved(ctx, r, previousSubscriptionID) + if err != nil { + return stacktrace.Propagate(err, "Could not determine if previous Subscription can be removed") + } + + // We _could_ reuse the previous subscription ID if it was an implicit subscription, + // but for now we chose to always create a new subscription. subToUpsert := scdmodels.Subscription{ ID: dssmodels.ID(uuid.New().String()), Manager: manager, @@ -692,6 +713,14 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize return stacktrace.Propagate(err, "Failed to upsert OperationalIntent in repo") } + // Check if the previously attached subscription should be removedre + if removePreviousImplicitSubscription { + err = r.DeleteSubscription(ctx, *previousSubscriptionID) + if err != nil { + return stacktrace.Propagate(err, "Unable to delete previous implicit Subscription") + } + } + // Find Subscriptions that may need to be notified allsubs, err := r.SearchSubscriptions(ctx, notifyVol4) if err != nil {