Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[scd] properly cleanup implicit sub on oir update #1057

Closed
wants to merge 4 commits into from
Closed

[scd] properly cleanup implicit sub on oir update #1057

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6731893
[scd] properly cleanup implicit sub on oir update
Shastick Jul 18, 2024
e2dc466
Comments
Shastick Jul 25, 2024
efbb391
rename and make subscriptionCanBeRemoved more specific
Shastick Aug 13, 2024
9ca5e54
second round of comments, handle another corner case
Shastick Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 106 additions & 50 deletions pkg/scd/operational_intents_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,37 @@ 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,
Expand Down Expand Up @@ -68,30 +99,20 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest
"Current version is %s but client specified version %s", old.OVN, ovn)
}

// Get the Subscription supporting the OperationalIntent, if one is defined
var sub *scdmodels.Subscription
removeImplicitSubscription := false
var previousSubscription *scdmodels.Subscription
if old.SubscriptionID != nil {
sub, err = r.GetSubscription(ctx, *old.SubscriptionID)
previousSubscription, err = r.GetSubscription(ctx, *old.SubscriptionID)
if err != nil {
return stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo")
}
if sub == nil {
if previousSubscription == 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 := subscriptionIsImplicitAndOnlyAttachedToOIR(ctx, r, &id, previousSubscription)
if err != nil {
return stacktrace.Propagate(err, "Could not determine if Subscription can be removed")
}

// Find Subscriptions that may overlap the OperationalIntent's Volume4D
Expand Down Expand Up @@ -130,7 +151,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")
}
Expand Down Expand Up @@ -437,9 +458,9 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid state for initial version: `%s`", params.State)
}

subscriptionID := dssmodels.ID("")
specifiedSubscriptionID := dssmodels.ID("")
if params.SubscriptionId != nil {
subscriptionID, err = dssmodels.IDFromOptionalString(string(*params.SubscriptionId))
specifiedSubscriptionID, err = dssmodels.IDFromOptionalString(string(*params.SubscriptionId))
if err != nil {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Invalid ID format for Subscription ID: `%s`", *params.SubscriptionId)
}
Expand All @@ -448,7 +469,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
// Check if a subscription is required for this request:
// OIRs in an accepted state do not need a subscription.
if state.RequiresSubscription() &&
subscriptionID.Empty() &&
specifiedSubscriptionID.Empty() &&
(params.NewSubscription == nil ||
params.NewSubscription.UssBaseUrl == "") {
return nil, nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, "Provided Operational Intent Reference state `%s` requires either a subscription ID or information to create an implicit subscription", state)
Expand All @@ -471,6 +492,8 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
if err != nil {
return stacktrace.Propagate(err, "Could not get OperationalIntent from repo")
}

var previousSubscription *scdmodels.Subscription
if old != nil {
if old.Manager != manager {
return stacktrace.NewErrorWithCode(dsserr.PermissionDenied,
Expand All @@ -482,6 +505,13 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
}

version = int32(old.Version)

if old.SubscriptionID != nil {
previousSubscription, err = r.GetSubscription(ctx, *old.SubscriptionID)
if err != nil {
return stacktrace.Propagate(err, "Unable to get OperationalIntent's Subscription from repo")
}
}
} else {
if ovn != "" {
return stacktrace.NewErrorWithCode(dsserr.NotFound, "OperationalIntent does not exist and therefore is not version %s", ovn)
Expand All @@ -490,12 +520,23 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
version = 0
}

var sub *scdmodels.Subscription
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,
// an error will have been returned earlier.
// If they are not set at this point, continue without creating an implicit subscription.
// Determine if the previous subscription needs to be cleaned up
previousSubIsBeingReplaced := previousSubscription != nil && specifiedSubscriptionID != previousSubscription.ID
removePreviousImplicitSubscription := false
if previousSubIsBeingReplaced {
removePreviousImplicitSubscription, err = subscriptionIsImplicitAndOnlyAttachedToOIR(ctx, r, &id, previousSubscription)
if err != nil {
return stacktrace.Propagate(err, "Could not determine if previous Subscription can be removed")
}
}

// effectiveSubscription is the subscription that will end up being attached to the OIR
// it defaults to the previous subscription, but may be updated if required by the parameters
effectiveSubscription := previousSubscription
if specifiedSubscriptionID.Empty() {
// No subscription ID was provided:
// We will check if parameters to create a new implicit subscription were provided,
// otherwise we will do nothing.
if params.NewSubscription != nil && params.NewSubscription.UssBaseUrl != "" {
if !a.AllowHTTPBaseUrls {
err := scdmodels.ValidateUSSBaseURL(string(params.NewSubscription.UssBaseUrl))
Expand All @@ -504,6 +545,9 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
}
}

// Note: parameters for a new implicit subscription have been passed, so we will create
// a new implicit subscription even if another subscription was attaches to this OIR before,
// (and regardless of whether it was an implicit subscription or not).
subToUpsert := scdmodels.Subscription{
ID: dssmodels.ID(uuid.New().String()),
Manager: manager,
Expand All @@ -520,53 +564,57 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
subToUpsert.NotifyForConstraints = *params.NewSubscription.NotifyForConstraints
}

sub, err = r.UpsertSubscription(ctx, &subToUpsert)
effectiveSubscription, err = r.UpsertSubscription(ctx, &subToUpsert)
if err != nil {
return stacktrace.Propagate(err, "Failed to create implicit subscription")
}
}

} else {
// Use existing Subscription
sub, err = r.GetSubscription(ctx, subscriptionID)
if err != nil {
return stacktrace.Propagate(err, "Unable to get Subscription")
}
if sub == nil {
return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Specified Subscription %s does not exist", subscriptionID)
// A subscription has been specified:
// 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.
if effectiveSubscription == nil || previousSubIsBeingReplaced {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

effectiveSubscription == nil will always be true here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually now that I look at it again, I'm also not completely sure why this change here is needed? Was there previously an issue that is fixed by this change? Or is it just an optimization to avoid fetching twice the same subscription?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

effectiveSubscription == nil will always be true here.

Thanks for catching. A previous revision was pre-setting the value and that ended up being even more confusing than it is now.

Actually now that I look at it again, I'm also not completely sure why this change here is needed? Was there previously an issue that is fixed by this change? Or is it just an optimization to avoid fetching twice the same subscription?

This is to avoid re-fetching the same subscription already fetched at line 496 in the if old.SubscriptionID != nil { block if that subscription is not being replaced. In cases where it is being replaced, we need to fetch the subscription that will eventually be attached to the OIR to verify that it properly covers the OIR.

It indeed ensures that we don't do unnecessary roundtrips.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching. A previous revision was pre-setting the value and that ended up being even more confusing than it is now.

(Well, as a good illustration of how convoluted the control logic got, my 'simplification' was too much and needs a rework)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to avoid re-fetching the same subscription already fetched at line 496 in the if old.SubscriptionID != nil { block if that subscription is not being replaced. In cases where it is being replaced, we need to fetch the subscription that will eventually be attached to the OIR to verify that it properly covers the OIR.

It indeed ensures that we don't do unnecessary roundtrips.

Could you add a comment mentioning that? That clarifies that this is not actually a separate case, but an optimization. Ideally that could be hidden behind a function but not sure if that's doable or makes sense here.

effectiveSubscription, err = r.GetSubscription(ctx, specifiedSubscriptionID)
if err != nil {
return stacktrace.Propagate(err, "Unable to get requested Subscription from store")
}
if effectiveSubscription == nil {
return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Specified Subscription %s does not exist", specifiedSubscriptionID)
}
}
if sub.Manager != dssmodels.Manager(manager) {
if effectiveSubscription.Manager != dssmodels.Manager(manager) {
return stacktrace.Propagate(
stacktrace.NewErrorWithCode(dsserr.PermissionDenied, "Specificed Subscription is owned by different client"),
"Subscription %s owned by %s, but %s attempted to use it for an OperationalIntent", subscriptionID, sub.Manager, manager)
"Subscription %s owned by %s, but %s attempted to use it for an OperationalIntent", specifiedSubscriptionID, effectiveSubscription.Manager, manager)
}
updateSub := false
if sub.StartTime != nil && sub.StartTime.After(*uExtent.StartTime) {
if sub.ImplicitSubscription {
sub.StartTime = uExtent.StartTime
if effectiveSubscription.StartTime != nil && effectiveSubscription.StartTime.After(*uExtent.StartTime) {
if effectiveSubscription.ImplicitSubscription {
effectiveSubscription.StartTime = uExtent.StartTime
updateSub = true
} else {
return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not begin until after the OperationalIntent starts")
}
}
if sub.EndTime != nil && sub.EndTime.Before(*uExtent.EndTime) {
if sub.ImplicitSubscription {
sub.EndTime = uExtent.EndTime
if effectiveSubscription.EndTime != nil && effectiveSubscription.EndTime.Before(*uExtent.EndTime) {
if effectiveSubscription.ImplicitSubscription {
effectiveSubscription.EndTime = uExtent.EndTime
updateSub = true
} else {
return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription ends before the OperationalIntent ends")
}
}
if !sub.Cells.Contains(cells) {
if sub.ImplicitSubscription {
sub.Cells = s2.CellUnionFromUnion(sub.Cells, cells)
if !effectiveSubscription.Cells.Contains(cells) {
if effectiveSubscription.ImplicitSubscription {
effectiveSubscription.Cells = s2.CellUnionFromUnion(effectiveSubscription.Cells, cells)
updateSub = true
} else {
return stacktrace.NewErrorWithCode(dsserr.BadRequest, "Subscription does not cover entire spatial area of the OperationalIntent")
}
}
if updateSub {
sub, err = r.UpsertSubscription(ctx, sub)
effectiveSubscription, err = r.UpsertSubscription(ctx, effectiveSubscription)
if err != nil {
return stacktrace.Propagate(err, "Failed to update existing Subscription")
}
Expand Down Expand Up @@ -601,7 +649,7 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize

// Identify Constraints missing from the key
var missingConstraints []*scdmodels.Constraint
if sub != nil && sub.NotifyForConstraints {
if effectiveSubscription != nil && effectiveSubscription.NotifyForConstraints {
constraints, err := r.SearchConstraints(ctx, uExtent)
if err != nil {
return stacktrace.Propagate(err, "Unable to SearchConstraints")
Expand Down Expand Up @@ -654,8 +702,8 @@ func (a *Server) upsertOperationalIntentReference(ctx context.Context, authorize
// in such cases the subscription ID on scdmodels.OperationalIntent will be nil
// and will be replaced with the 'NullV4UUID' when sent over to a client.
var subID *dssmodels.ID = nil
if sub != nil {
subID = &sub.ID
if effectiveSubscription != nil {
subID = &effectiveSubscription.ID
}

// Construct the new OperationalIntent
Expand Down Expand Up @@ -706,6 +754,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 removed
mickmis marked this conversation as resolved.
Show resolved Hide resolved
if removePreviousImplicitSubscription {
err = r.DeleteSubscription(ctx, previousSubscription.ID)
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 {
Expand Down
Loading