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.

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

[uss_qualifier/scenarios/netrid/dss_interoperability] Avoid timeouts in notifications to dummy subscriptions #518

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Feb 23, 2024

Don't attempt to notify the short-lived subscriptions that the dss_interoperability scenario created, as these result in requests to example.com that time out and cause the scenario's assumptions to break.

(This uses the recently introduced do_not_notify parameter.)

This also updates the dummy uss_base_url from example.com to example.interuss.org to take a step forward regarding #275

Evidence this seems to work:

See #517

Comment on lines 601 to 605
LONG_WAIT_SEC,
"Subscriptions needs to expire so we can check they don't trigger notifications",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notes:

  • description string updated from my understanding of the code.
  • the longer wait time aims at making sure that the subscriptions created at step9 above have expired

@@ -515,7 +518,9 @@ def step9(self):
created_sub = dss.put_sub(
check,
sub_id=sub_2.uuid,
**_default_params(datetime.timedelta(seconds=SHORT_WAIT_SEC)),
**_default_params(
datetime.timedelta(seconds=SHORT_SUBSCRIPTION_DURATION_SEC)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we run into problems again in the future, or if users report that this is brittle when testing real deployments, we could consider:

  • making this a configurable part of the scenario, giving users the possibility to trade off execution speed vs delay sensibility
  • computing these delays according to the number of DSS's, based on allowed p95 latencies

@Shastick Shastick marked this pull request as draft February 23, 2024 11:48
@Shastick
Copy link
Contributor Author

Will reopen when the root-cause is addressed

@Shastick Shastick force-pushed the dss-interop-fixes branch 3 times, most recently from e6e5956 to 55b4925 Compare February 23, 2024 12:02
@Shastick Shastick marked this pull request as ready for review February 23, 2024 12:46
@Shastick Shastick marked this pull request as draft February 23, 2024 12:49
@Shastick
Copy link
Contributor Author

This seems to be consistently passing again.

@Shastick Shastick marked this pull request as ready for review February 23, 2024 12:58
@Shastick Shastick changed the title [qualifier] make dss_interop more resilient to possible api delays [qualifier] avoid timeouts in notifications to dummy subscriptions in dss_interop Feb 23, 2024
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

LGTM
FYI I'm taking the liberty to modify your description so that this PR won't close automatically the related PR.

@mickmis mickmis changed the title [qualifier] avoid timeouts in notifications to dummy subscriptions in dss_interop [uss_qualifier/scenarios/netrid/dss_interoperability] Avoid timeouts in notifications to dummy subscriptions Feb 23, 2024
@mickmis mickmis merged commit 8c81f0c into interuss:main Feb 23, 2024
9 checks passed
@Shastick Shastick deleted the dss-interop-fixes branch February 23, 2024 15:40
github-actions bot added a commit that referenced this pull request Feb 23, 2024
…in notifications to dummy subscriptions (#518)

[qualifier] make dss_interop more resilient to possible api delays 8c81f0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants