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

[ci] run the uss-qualifier as part of the CI #1045

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Jun 19, 2024

To increase test coverage, we add a step to the CI that will run the qualifier against the DSS version built from the current PR.

Notes:

  • this currently only runs "single instance tests", as, to the best of my understanding, only a single DSS instance is started in CI (therefore, only a single DSS is configured)
  • this does not run any CRDB-specific checks (they are skipped)
  • the configuration was obtained by running the qualifier with the --config-output option, with the following modifications:
    • removal of the second DSS
    • add has_private_address: true to dss resource specifications (otherwise the qualifier fails on resource initialisation)
    • set uss1: all_astm_dss_requirements in the participant_requirements artifacts section

Test plan:
This worked both locally when running the qualifier from a docker image built from a recent release of the interuss/monitoring image, and passes on the CI when running with the monitoring release v0.8.0

See this successful test run for example

repository: interuss/monitoring
submodules: true
path: monitoring-repo
ref: main # consider checking out a tag instead? Downside is that we'd need to update it with each release
Copy link
Member

Choose a reason for hiding this comment

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

We certainly want to pin our dependencies -- not pinning means that any change in a dependency can break our system without any change in our system.

cd monitoring-repo
# Set the docker-compose file to use the locally built DSS instance
# Note that on OS X you'll need to use `sed -i ''` instead of `sed -i`
sed -i 's|interuss/dss:[^ ]*|interuss-local/dss:latest|g' build/dev/docker-compose.yaml
Copy link
Member

Choose a reason for hiding this comment

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

The ability to bring up a local DSS instance should certainly exist in the DSS repo -- we shouldn't need to rely on the content of another repository to do that.

Since uss_qualifier is designed to test systems however they're deployed, we should tell it how our DSS system is deployed when we want to test it. That means we should have a test configuration for testing the DSS tracked in this repo. The configuration will probably look a lot like this one, but we should remove the $refs because the targets exist in another repo and we don't want to be dependent on them for the configuration to be valid. To do this, we can run uss_qualifier with --config-output.

Once we have a locally-deployed system to test and a configuration specifying how to test it, we should be able to just run uss_qualifier directly similar to how we run prober directly. monitoring's run_locally.sh is probably a good hint on what that command to run uss_qualifier should look like, but we shouldn't need to run the script in the other repo since that's just an example and not the generally-expected usage of uss-qualifier.

It is probably also a good idea to (separately) modernize the local DSS deployment approach in this repo to look more like the one in the monitoring repo, but the two repos should not be directly coupled.

@@ -65,3 +65,22 @@ jobs:
run: make probe-locally
- name: Bring down local DSS instance
run: make down-locally
- name: Checkout monitoring repository
Copy link
Member

Choose a reason for hiding this comment

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

We should be using uss_qualifier as a standalone tool that is released separately from the DSS. When this is the case, we should simply be able to use the released monitoring docker image (like the prober check does) and not check out the repo at all. If we really need to use a specific commit of that repo that is not released, that probably indicates a problem in our release strategy since if this user needs that specific unreleased commit, it's likely others will want to use that specific unreleased commit as well, so we should make a monitoring release that includes that commit.

make restart-all
cd monitoring/uss_qualifier/
# Run the qualifier
./run_locally.sh configurations.dev.noop,configurations.dev.uspace,configurations.dev.geoawareness_cis,configurations.dev.generate_rid_test_data,configurations.dev.geospatial_comprehension,configurations.dev.general_flight_auth,configurations.dev.message_signing,configurations.dev.dss_probing,configurations.dev.f3548_self_contained,configurations.dev.netrid_v22a,configurations.dev.netrid_v19
Copy link
Member

Choose a reason for hiding this comment

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

Most of these configurations are not valid for testing the behavior of the DSS product itself. A configuration very similar to dss_probing in the monitoring repo should be all we need to verify compliance of this DSS implementation to relevant requirements. If that test suite is missing tests to adequately verify compliance of a DSS implementation, we should add those missing tests to it rather than relying on other test suites not directly testing a DSS implementation to incidentally cover those tests.

@Shastick Shastick marked this pull request as draft August 16, 2024 06:49
@Shastick Shastick force-pushed the run-qualifier-in-ci branch 2 times, most recently from 7afbb0e to 1b30fa4 Compare August 21, 2024 16:35
@Shastick Shastick changed the title [ci] checkout and run the uss-qualifier as part of the CI [ci] run the uss-qualifier as part of the CI Aug 23, 2024
@Shastick Shastick marked this pull request as ready for review August 23, 2024 13:06
@Shastick
Copy link
Contributor Author

The step is run and succeeded.

@BenjaminPelletier BenjaminPelletier merged commit 092dd73 into interuss:master Aug 26, 2024
6 checks passed
@Shastick Shastick deleted the run-qualifier-in-ci branch August 28, 2024 07:05
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