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] DSS0030 port ISA-subscriptions interactions test #291

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Oct 25, 2023

This ports the existing test_subscription_isa_interaction.py prober test to a generic ISASubscriptionInteractions scenario.

This also adds a delete_any_subscription function to monitoring/uss_qualifier/scenarios/astm/netrid/common/dss/utils.py

A few API methods have been added to requirements/interuss/f3411/dss_endpoints.md, as this scenario depends on implementation details specified in the A4.1 annex that are not mentioned in the standard.

@Shastick Shastick force-pushed the dss0030-isa-sub-interact branch from 1d0bad1 to 0a1e85d Compare October 26, 2023 05:51
Comment on lines 187 to 236
# For checking the notifications, we ignore the request we made for the subscription that we created.
if self._isa.base_url not in subscriber_url:
with self.check("Notified subscriber", [subscriber_url]) as check:
# TODO: Find a better way to identify a subscriber who couldn't be notified:
# as-is the subscriber url causes the test-suite to crash when it writes its report
Copy link
Contributor Author

@Shastick Shastick Oct 26, 2023

Choose a reason for hiding this comment

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

Two notes/questions here:

  • I'm not checking the notification query to our own subscription because it fails, as it assumes https is available. As we are querying ourselves anyway, failure or success on that query does not give us any additional information about the DSS we are testing. Happy to dig further into this if required. (see log excerpt below)

  • subscriber_url is a bad stand-in for identifying a failing participant here: it will cause the qualifier to fail when writing test reports, when it attempts to find a directory that has the faulty URL in its path (I kept things as-is for now as this is done this way in every place that notifies ISA subscribers and deserves a separate PR to properly fix it)

For reference, here's a warning we get when failing to notify ourselves:

2023-10-26 16:32:44.610 | WARNING | monitoring.monitorlib.fetch:query_and_describe:361 - query_and_describe attempt 1 from PID 1 to POST https://uss_qualifier.test.utm/dummy_base_url/v1/uss/identification_service_areas/00000172-e36d-40be-d38b-eca6ca350000 failed with non-retryable RequestException ConnectionError: HTTPSConnectionPool(host='uss_qualifier.test.utm', port=443): Max retries exceeded with url: /dummy_base_url/v1/uss/identification_service_areas/00000172-e36d-40be-d38b-eca6ca350000 (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0xffff4ab2a130>: Failed to establish a new connection: [Errno -2] Name or service not known'))

And here's the exception the qualifier fails with at the end when we use a URL as a participant_id:

Traceback (most recent call last):
  File "main.py", line 299, in <module>
    sys.exit(main())
  File "main.py", line 284, in main
    run_config(
  File "main.py", line 231, in run_config
    generate_tested_requirements(redacted_report, tested_reqs_config, path)
  File "/app/monitoring/uss_qualifier/reports/tested_requirements.py", line 243, in generate_tested_requirements
    with open(participant_file, "w") as f:
FileNotFoundError: [Errno 2] No such file or directory: 'output/netrid_v19/requirements/https://uss_qualifier.test.utm/dummy_base_url/v1/uss/identification_service_areas.html'

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we'd want to create new ParticipantIDs on the fly -- they're supposed to refer to participants in the test rather than, for instance, bystanders that the test happened to interact with. I do think we want to identify the participant affecting a query or check whenever we reasonably can, but I don't think that's possible in all cases. When it's not currently feasible to link a query or check with a test-designer-specified participant (or, perhaps, a "special" participant like the otherwise-unnamed test designer) I think we'd just not specify participant IDs for those actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, in that case I'll defer to the query's participant_id field.

This moves the responsibility for finding out which participant is being queried to mutate/rid.py (here).

Should I look into how to best do this, and fix any other place that uses the qualifier URL as the participant ID?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we should only have ParticipantIDs by the IDs specified by the test designer, or perhaps in special cases, identify a few special ParticipantIDs (e.g., "uss_qualifier"/"test_runner"/"test_owner"/"test_designer"). When we have ParticipantIDs that aren't in this set, yes, I think we should probably adjust them.

In general, we should definitely prioritize fixing any place where the participant ID is set to the wrong participant (it would be bad to attribute failures or successes to the wrong participant). It would be good to attribute all feasible checks to a particular participant (it's hard to know what to do when something fails but no one's responsible). Attributing queries to particular participants is nice and we should do it when possible, but I think we should only go out of our ways to update existing code when that update might affect the outcome of a check (e.g., a performance requirement). Attributing things to a participant the test designer doesn't recognize can be confusing -- along the same kind of lines as unattributed checks & queries since the test designer will have to do some manual analysis to understand who might be responsible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good, then I'll do a quick pass on the places that were setting a URL as the participant ID without changing anything else: now that some scenarios are creating subscriptions, crashes of the qualifier become much more likely.

@Shastick Shastick force-pushed the dss0030-isa-sub-interact branch from 0a1e85d to da32cba Compare October 26, 2023 07:31

#### Successful subscription query check

**[astm.f3411.v19.DSS0030,f](../../../../../requirements/astm/f3411/v19.md)** requires the implementation of the DSS endpoint to allow callers to retrieve the subscriptions they created.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side question:

the standard does not mention anything related to an area being queried. However, in order to list the subscriptions from the DSS, we need to provide an area in which to search (the Swagger spec from the A4.1 annex mentions that the area is a required parameter).

For reference, DSS0030-f reads:

GET Subscriptions—this interface enables a Net-RID Display Provider to retrieve the details of all existing subscriptions it created.

A valid interpretation thereof is that there should be a way to list all created subscriptions without specifying an area.

As the swagger spec makes the requirement a bit stricter, should we add an entry to /requirements/interuss/f3411/dss_endpoints.md and mention it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that I had other reasons to update dss_endpoints.md I went ahead and added a SearchSubscriptionsentry there.

Copy link
Member

Choose a reason for hiding this comment

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

When the preface is included, I think the requirement should be fairly clear:

(3) A DSS implementation shall (DSS0030) minimally
include the following interfaces for use by Net-RID Service
Providers and Display Providers, in accordance with the DSS
portion of the OpenAPI specification presented in Annex A4:
[...]
(f) GET Subscriptions—this interface enables a Net-RID
Display Provider to retrieve the details of all existing subscrip-
tions it created.

The (f) text doesn't specify the need to provide an area, but the preface says each of the subparts (including f) is in accordance with Annex A4, and Annex A4 requires an area. So, I think DSS0030,f should be fine and we don't need an additional requirement. We do need an additional requirement to, e.g., retrieve one specific ISA (GET by ID) since only PUT and DELETE are listed as requirements in the standard (I think this is a simple oversight, but regardless, it doesn't appear to be a requirement in F3411-22a).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks. Looks like I took "the requirement needs to be listed in the standard" a bit too literally :) I'll update the relevant docs.

@Shastick Shastick force-pushed the dss0030-isa-sub-interact branch 3 times, most recently from 161044b to 1ab70e6 Compare October 27, 2023 07:39
@Shastick Shastick marked this pull request as ready for review October 27, 2023 07:59
@Shastick Shastick changed the title [uss-qualifier] DSS0030 port ISA-subscriptions interactions test [uss_qualifier] DSS0030 port ISA-subscriptions interactions test Oct 27, 2023
@Shastick Shastick force-pushed the dss0030-isa-sub-interact branch from 2ec986c to 3f64133 Compare October 29, 2023 21:00
@Shastick Shastick force-pushed the dss0030-isa-sub-interact branch from 3f64133 to 558f64f Compare October 30, 2023 17:07
Comment on lines +68 to +91
def delete_any_subscription(
scenario: GenericTestScenario,
dss_wrapper: DSSWrapper,
area: List[LatLngPoint],
):
"""
Deletes any subscription that is returned for the passed area.

Args:
scenario: the scenario instance that will provide the checks
dss_wrapper: the dss on which to delete subscriptions
area: the area for which subscriptions are to be deleted
"""
with scenario.check(
"Successful subscription query", [dss_wrapper.participant_id]
) as check:
fetched = dss_wrapper.search_subs(
check, [vertex.as_s2sphere() for vertex in area]
)
for sub_id in fetched.subscriptions.keys():
with scenario.check(
"Successful subscription deletion", [dss_wrapper.participant_id]
) as check:
dss_wrapper.cleanup_sub(check, sub_id=sub_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Several of the subscription scenarios have a need to clean up: I made a note to go over them once they are merged to see if this logic can be regrouped a bit.

@BenjaminPelletier BenjaminPelletier merged commit 7f528d5 into interuss:main Oct 31, 2023
8 checks passed
github-actions bot added a commit that referenced this pull request Oct 31, 2023
* [uss-qualifier] DSS0030 port ISA-subscriptions interactions test

* Add notification_index checks

* rename CreateSubscription to PutSubscription for consistency

* Reference the standard directly where relevant 7f528d5
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