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

[mock_uss/flight_planning] Exclude OVN from key when USS is down #351

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

mickmis
Copy link
Contributor

@mickmis mickmis commented Nov 16, 2023

Because if included in the key as is, the DSS will reject the request.
The DSS does not reject the request.

@BenjaminPelletier
Copy link
Member

Because if included in the key as is, the DSS will reject the request.

Does the DSS actually reject the request? I had thought providing extra OVNs would not cause rejection since the USS doesn't know exactly the area the DSS is considering when checking OVNs and therefore is likely to often have extras (e.g., a USS uses a large planning area to obtain information and then targets a small flight within that area). I think this PR is good regardless, but I would have expected the sole OVN-related criterion for DSS acceptance would be that all required OVNs are provided (regardless of how many extras are provided). OVNs are "proof of knowledge" and the DSS is just verifying that the client knows everything they're supposed to know about the airspace before changing it ('synchronization' part of its duties).

@mickmis
Copy link
Contributor Author

mickmis commented Nov 16, 2023

Does the DSS actually reject the request? I had thought providing extra OVNs would not cause rejection since the USS doesn't know exactly the area the DSS is considering when checking OVNs and therefore is likely to often have extras (e.g., a USS uses a large planning area to obtain information and then targets a small flight within that area). I think this PR is good regardless, but I would have expected the sole OVN-related criterion for DSS acceptance would be that all required OVNs are provided (regardless of how many extras are provided). OVNs are "proof of knowledge" and the DSS is just verifying that the client knows everything they're supposed to know about the airspace before changing it ('synchronization' part of its duties).

I'm not 100% sure why now, but I was convinced the DSS rejected the OVN because it was malformed or did not exist. I think I got a bit mixed up at some point because I had a very trial-and-error approach fixing up locally the DSS and the mock USS to get the test scenario running with success. Sorry about that :/ I will update the PR description.

@BenjaminPelletier
Copy link
Member

I'm not 100% sure why now, but I was convinced the DSS rejected the OVN because it was malformed or did not exist. I think I got a bit mixed up at some point because I had a very trial-and-error approach fixing up locally the DSS and the mock USS to get the test scenario running with success. Sorry about that :/ I will update the PR description.

I don't know that that belief was incorrect in fact -- it's not intended behavior (intended behavior = accept request when all required OVNs are provided, regardless of other OVNs provided), but the actual behavior may very well not match intended behavior (in which case we'd want to fix).

This PR still looks good as-is regardless though -- feel free to merge at your discretion.

@mickmis
Copy link
Contributor Author

mickmis commented Nov 17, 2023

but the actual behavior may very well not match intended behavior (in which case we'd want to fix).

I did try to reproduce the (imaginary) issue locally, but in fact I could not. I will merge the PR. Thanks for the review!

@mickmis mickmis merged commit 1fd5bbc into interuss:main Nov 17, 2023
9 checks passed
@mickmis mickmis deleted the mock_uss/ovnexclusion branch November 17, 2023 08:33
Shastick pushed a commit to Orbitalize/monitoring that referenced this pull request Nov 17, 2023
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