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

[scd] cr deletion now checks for OVN correctness #1083

Merged

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Aug 28, 2024

Add a check for the provided OVN before deleting constraint references.

Once #1082 is merged, this will close #1081

Test plan: a DSS built from this PR was used as a target for this scenario and behaved properly.

@Shastick
Copy link
Contributor Author

Shastick commented Aug 28, 2024

@BenjaminPelletier, following our discussion at the last community meeting about maintaining and updating the qualifier configuration used in the CI for the DSS repo, should we maybe change the way we release scenarios that 'break' with the current DSS implementation?

So far we have been doing this:

  1. Add scenario (or fix existing one) that reveals an issue with the DSS
  2. Fix the issue on the DSS side, manually test the PR against the new scenario
  3. Release new DSS version
  4. update DSS version on the monitoring repo, obtain a green CI
  5. merge new scenario
  6. release monitoring image
  7. update monitoring image on the DSS repo
  8. update CI qualifier configuration on the DSS repo

This works, but is a bit ad-hoc and requires trusting the manual step in 2) above. The main reason we do this is that we usually directly update the qualifier configurations when we add a scenario, which means CI runs will necessarily break if the DSS has an open issue.

An alternative process could be:

  1. Add scenario that reveals issue with the DSS, but do not update the default configurations yet
  2. Open PR, get it reviewed and merged
  3. Release new version of the monitoring image
  4. Within a PR on the DSS:
  • Fix the issue on the DSS,
  • update the qualifier config used in the DSS CI to use the new scenario,
  • get a green CI that confirms the issue is fixed and merge
  1. release the DSS image
  2. Open a PR on the monitoring repo that:
  • updates to the latest released DSS
  • updates the configurations to test the newly fixed features

The second process is technically cleaner, albeit a little bit more involved, and introduces the risk to forget to add a relevant configuration to our defaults in step 6). On the other hand, it gets us closer to properly using the qualifier and forces us to go through the gymnastics of properly updating configurations on the DSS side. (Under the current process, we risk forgetting updating the monitoring image or the qualifier configuration on the DSS repo, which is guaranteed to be impossible with the alternative process)

I will try the second approach for the set of scenarios that check that OVNs are being verified to see if it is even practical: for entirely new scenarios it should be easy, but for updates to existing scenarios it will be less so.

Note that there will still be situations that require an ad-hoc approach.

@Shastick
Copy link
Contributor Author

@BenjaminPelletier this is ready for review: if we're interested in moving quickly, the two PRs fixing OVN handling can be merged and a release can be done so we may proceed with the qualifier scenarios interuss/monitoring#761 and interuss/monitoring#762

@BenjaminPelletier
Copy link
Member

I've added this issue to track that discussion separate from this PR.

@BenjaminPelletier BenjaminPelletier merged commit a667c89 into interuss:master Aug 30, 2024
6 checks passed
@Shastick Shastick deleted the 1081-ovn-validation-crs branch August 30, 2024 06:34
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.

[scd] deletion of entity ref. accepts any OVN for deletion.
2 participants