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] rid: dss0130 - check information relative to intersecting entities is properly sync'd #839

Merged

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Nov 3, 2024

Probe the secondary DSS's to check that they have all required information to return the proper subscriptions relevant to an ISA.

Builds on top of #838, please only review the last commit.

Part of the effort on #754

@Shastick Shastick force-pushed the dss-0130-sub-interaction-across-dss branch 3 times, most recently from 5f77be2 to 4fa7a58 Compare November 4, 2024 09:21
@Shastick Shastick marked this pull request as ready for review November 4, 2024 10:09
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.

This also opportunistically implements a check that was left over as a TODO

Please split it away, this does not help for review velocity, and that's unfortunately a bottleneck these days :/

@Shastick Shastick force-pushed the dss-0130-sub-interaction-across-dss branch from 4fa7a58 to 7801228 Compare November 5, 2024 11:08
@Shastick
Copy link
Contributor Author

Shastick commented Nov 5, 2024

Reverted the open TODO

@Shastick Shastick requested a review from mickmis November 5, 2024 11:09
@Shastick Shastick force-pushed the dss-0130-sub-interaction-across-dss branch from 7801228 to 7e93100 Compare November 5, 2024 11:10
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.

Reverted the open TODO

Thanks!

Reviewed 7e93100


#### ⚠️ ISA modification on secondary DSS triggers subscription notification requests check

**[astm.f3411.v19.DSS0130,A2-6-1,3c](../../../../requirements/astm/f3411/v19.md)** and **[astm.f3411.v19.DSS0130,2,e](../../../../requirements/astm/f3411/v19.md)**
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it is fair to fail DSS0130,2,e here, there could a number of other reasons than the area not being synchronized.
Why not validating that simply by querying secondary DSSs in a different place?

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 this check fails, we may not be able to tell exactly why. We've had checks fail multiple requirements that are pretty different (some related to API while others are related to synchronization) for SCD (see eg here).

The wording could be improved here, though I'd be happy to do that in a separate PR and make this whole page more verbose.

Why not validating that simply by querying secondary DSSs in a different place?

That might be a valid additional check that might partially cover the requirement: if we create an ISA in another area we would expect to not find the subscriptions not covering it, but that would still not allows us to conclude that the subscription's area has been properly synced?

Possibly the check needs rephrasing and we should use or instead of and, but creating a subscription on DSS A then doing something on DSS B in a similar area and expect to run into the subscription is pretty much how we said we would test this requirement at the community meeting ~10 days ago.

Comment on lines 467 to 494
subs_to_notify_sec = []
for subscriber in mutated_isa_sec.subscribers:
for s in subscriber.raw.subscriptions:
subs_to_notify_sec.append(s.subscription_id)

with self.check(
"ISA modification on secondary DSS triggers subscription notification requests",
[self._dss_primary.participant_id],
) as check:
if sub_1_0.uuid not in subs_to_notify_sec:
check.record_failed(
summary=f"Subscription {sub_1_0.uuid} was not notified of ISA modification",
details=f"Subscription {sub_1_0.uuid} was created on the primary DSS and should have been notified of the ISA modification that happened on the primary DSS, but was not.",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This here will take into account only the last secondary DSS. Why not:

  • put that inside the for loop
  • add the secondary DSS as participant in the check
  • add in check details both primary and secondary DSSs participant IDs

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 catch, I think that this was the original intention, will fix.

@Shastick Shastick force-pushed the dss-0130-sub-interaction-across-dss branch 4 times, most recently from 378724d to 3320566 Compare November 18, 2024 16:36
@Shastick Shastick requested a review from mickmis November 18, 2024 16:36
@Shastick Shastick force-pushed the dss-0130-sub-interaction-across-dss branch 3 times, most recently from 41331d2 to 02f094d Compare November 21, 2024 08:45
@mickmis mickmis force-pushed the dss-0130-sub-interaction-across-dss branch from 02f094d to cb41723 Compare December 3, 2024 14:21
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
(I fixed the formatting issue that made the CI fail)

@mickmis mickmis merged commit 346b780 into interuss:main Dec 3, 2024
20 checks passed
@mickmis mickmis deleted the dss-0130-sub-interaction-across-dss branch December 3, 2024 14:32
github-actions bot added a commit that referenced this pull request Dec 3, 2024
…ting entities is properly sync'd (#839)

Co-authored-by: Mickaël Misbach <[email protected]> 346b780
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