From 60b2b3ca570ef6629549d2533f24b50d0647150b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Misbach?= Date: Mon, 9 Sep 2024 14:41:23 +0200 Subject: [PATCH] Revert "Squished commit (#1104)" (#1108) Reverts #1104 due to incorrect merge commit being used. --- pkg/scd/operational_intents_handler.go | 128 +++++++------------------ 1 file changed, 34 insertions(+), 94 deletions(-) diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go index d34765855..e287d1f43 100644 --- a/pkg/scd/operational_intents_handler.go +++ b/pkg/scd/operational_intents_handler.go @@ -16,37 +16,6 @@ import ( "github.com/interuss/stacktrace" ) -// subscriptionIsImplicitAndOnlyAttachedToOIR will check if: -// - the subscription is defined and is implicit -// - the subscription is attached to the specified operational intent -// - the subscription is not attached to any other operational intent -// -// This is to be used in contexts where an implicit subscription may need to be cleaned up: if true is returned, -// the subscription can be safely removed after the operational intent is deleted or attached to another subscription. -// -// NOTE: this should eventually be pushed down to CRDB as part of the queries being executed in the callers of this method. -// -// See https://github.com/interuss/dss/issues/1059 for more details -func subscriptionIsImplicitAndOnlyAttachedToOIR(ctx context.Context, r repos.Repository, oirID dssmodels.ID, subscription *scdmodels.Subscription) (bool, error) { - if subscription == nil { - return false, nil - } - if !subscription.ImplicitSubscription { - return false, nil - } - // Get the Subscription's dependent OperationalIntents - dependentOps, err := r.GetDependentOperationalIntents(ctx, subscription.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 && dependentOps[0] == oirID { - 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, @@ -100,20 +69,29 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest } // Get the Subscription supporting the OperationalIntent, if one is defined - var previousSubscription *scdmodels.Subscription + var sub *scdmodels.Subscription + removeImplicitSubscription := false if old.SubscriptionID != nil { - previousSubscription, err = r.GetSubscription(ctx, *old.SubscriptionID) + sub, err = r.GetSubscription(ctx, *old.SubscriptionID) if err != nil { return stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo") } - if previousSubscription == nil { + if sub == nil { return stacktrace.NewError("OperationalIntent's Subscription missing from repo") } - } - removeImplicitSubscription, err := subscriptionIsImplicitAndOnlyAttachedToOIR(ctx, r, id, previousSubscription) - if err != nil { - return stacktrace.Propagate(err, "Could not determine if Subscription can be removed") + 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 + } + } } // Gather the subscriptions that need to be notified @@ -141,7 +119,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, previousSubscription.ID) + err = r.DeleteSubscription(ctx, sub.ID) if err != nil { return stacktrace.Propagate(err, "Unable to delete associated implicit Subscription") } @@ -832,63 +810,33 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize // For an OIR being created, version starts at 0 version := int32(0) - var previousSub *scdmodels.Subscription if old != nil { version = int32(old.Version) - // Fetch the previous OIR's subscription if it exists - if old.SubscriptionID != nil { - previousSub, err = r.GetSubscription(ctx, *old.SubscriptionID) - if err != nil { - return stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo") - } - } } - // Determine if the previous subscription is being replaced and if it will need to be cleaned up - previousSubIsBeingReplaced := previousSub != nil && validParams.subscriptionID != previousSub.ID - removePreviousImplicitSubscription := false - if previousSubIsBeingReplaced { - removePreviousImplicitSubscription, err = subscriptionIsImplicitAndOnlyAttachedToOIR(ctx, r, validParams.id, previousSub) - if err != nil { - return stacktrace.Propagate(err, "Could not determine if previous Subscription can be removed") - } - } - - // attachedSub is the subscription that will end up being attached to the OIR - // it defaults to the previous subscription (which may be nil), and may be updated if required by the parameters - attachedSub := previousSub + var sub *scdmodels.Subscription if validParams.subscriptionID.Empty() { - // No subscription ID was provided: - // check if an implicit subscription should be created, otherwise do nothing + // Create an implicit subscription if one has been requested. + // Requesting neither an explicit nor an implicit subscription is allowed for ACCEPTED states: + // for other states, an error will have been returned earlier. + // if no implicit subscription is requested and we reached this point, we will proceed without subscription if validParams.implicitSubscription.requested { - // Parameters for a new implicit subscription have been passed: we will create - // a new implicit subscription even if another subscription was attached to this OIR before, - // regardless of whether it was an implicit subscription or not. - if attachedSub, err = createAndStoreNewImplicitSubscription(ctx, r, manager, validParams); err != nil { + if sub, err = createAndStoreNewImplicitSubscription(ctx, r, manager, validParams); err != nil { return stacktrace.Propagate(err, "Failed to create implicit subscription") } - } else { - // If no subscription ID is provided and no implicit subscription is requested, - // the OIR should have no attached subscription - attachedSub = nil } } else { // Attempt to rely on the specified subscription - // If it is different from the previous subscription, we need to fetch it from the store - // in order to ensure it correctly covers the OIR. - // We do the check below in order to avoid re-fetching the subscription if it has not changed - if attachedSub == nil || previousSubIsBeingReplaced { - attachedSub, err = r.GetSubscription(ctx, validParams.subscriptionID) - if err != nil { - return stacktrace.Propagate(err, "Unable to get requested Subscription from store") - } - if attachedSub == nil { - return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Specified Subscription %s does not exist", validParams.subscriptionID) - } + sub, err = r.GetSubscription(ctx, validParams.subscriptionID) + if err != nil { + return stacktrace.Propagate(err, "Unable to get depended-upon Subscription") + } + if sub == nil { + return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Depended-upon Subscription %s does not exist", validParams.subscriptionID) } // We need to confirm that it is owned by the calling manager - if attachedSub.Manager != manager { + if sub.Manager != manager { return stacktrace.Propagate( // We do a bit of wrapping gymnastics because the root error message will be sent in the response, // and we don't want to include the effective manager in there. @@ -897,27 +845,27 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize // The propagation message will end in the logs and help with debugging. "Subscription %s owned by %s, but %s attempted to use it for an OperationalIntent", validParams.subscriptionID, - attachedSub.Manager, + sub.Manager, manager, ) } // We need to ensure the subscription covers the OIR's geo-temporal extent - attachedSub, err = ensureSubscriptionCoversOIR(ctx, r, attachedSub, validParams) + sub, err = ensureSubscriptionCoversOIR(ctx, r, sub, validParams) if err != nil { return stacktrace.Propagate(err, "Failed to ensure subscription covers OIR") } } if validParams.state.RequiresKey() { - responseConflict, err = validateKeyAndProvideConflictResponse(ctx, r, manager, validParams, attachedSub) + responseConflict, err = validateKeyAndProvideConflictResponse(ctx, r, manager, validParams, sub) if err != nil { return stacktrace.PropagateWithCode(err, stacktrace.GetCode(err), "Failed to validate key") } } // Construct the new OperationalIntent - op := validParams.toOIR(manager, attachedSub, version+1) + op := validParams.toOIR(manager, sub, version+1) // Upsert the OperationalIntent op, err = r.UpsertOperationalIntent(ctx, op) @@ -925,14 +873,6 @@ 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 removed - if removePreviousImplicitSubscription { - err = r.DeleteSubscription(ctx, previousSub.ID) - if err != nil { - return stacktrace.Propagate(err, "Unable to delete previous implicit Subscription") - } - } - notifyVolume, err := computeNotificationVolume(old, validParams.uExtent) if err != nil { return stacktrace.Propagate(err, "Failed to compute notification volume")