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] DSS0210,2 check op-intent references are synchronized #530

Closed
wants to merge 2 commits into from

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Mar 5, 2024

First round of checks regarding op-intent reference synchronization.

(This currently excludes DSS0210,2e which will come in a later PR, as well as constraint references, which will have their dedicated scenario)

@Shastick Shastick force-pushed the dss0210-basics-entities branch 5 times, most recently from 1ae27db to f87ebed Compare March 5, 2024 14:25
@Shastick Shastick marked this pull request as ready for review March 7, 2024 14:27
Copy link
Member

@BenjaminPelletier BenjaminPelletier left a comment

Choose a reason for hiding this comment

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

Whenever possible, let's try to target small PRs. It's great to make progress by finishing a lot of development, but the same amount of progress split over multiple PRs is likely to be easier and better than waiting until the big final product. In addition to our standard guidance linked above, I find Google's guidelines to be very useful and helpful as well.

Comment on lines 53 to 67
## ⚠️ Non-mutated operational intent reference keeps the same version check

If the version of the operational intent reference is updated without there having been any mutation of the operational intent reference, the DSS is in violation of **[astm.f3548.v21.DSS0005,1](../../../../../requirements/astm/f3548/v21.md)**.

## ⚠️ Non-mutated operational intent reference keeps the same OVN check

If the OVN of the operational intent reference is updated without there having been any mutation of the operational intent reference, the DSS is in violation of **[astm.f3548.v21.DSS0005,1](../../../../../requirements/astm/f3548/v21.md)**.

## ⚠️ Mutated operational intent reference version is updated check

Following a mutation, the DSS needs to update the operational intent reference version, otherwise it is in violation of **[astm.f3548.v21.DSS0005,1](../../../../../requirements/astm/f3548/v21.md)**.

## ⚠️ Mutated operational intent reference OVN is updated check

Following a mutation, the DSS needs to update the operational intent reference OVN, otherwise it is in violation of **[astm.f3548.v21.DSS0005,1](../../../../../requirements/astm/f3548/v21.md)**.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should split these up. We don't want to attach a giant list of checks to every step where we perform only some of those giant checks in each step because then we lose traceability of what should (as opposed to could) actually happen where. We don't always have to perform every potential check, but I think we should usually target having every check documented for a test step at least sometimes being performed in that step. Since checking a mutated op intent is mutually exclusive from checking a non-mutated op intent, I think these should be separated out. It shouldn't hurt to have multiple fragments per step since we have that support now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fragments separated to multiple files, and split that change out to #535


### Create OIR validation test step

#### [Create OIR](../fragments/oir_crud.md)
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment as above -- we should definitely not pull in all the RUD checks to the test step if we're only going to perform the C checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I'll split things out to separate fragments for crud and validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fragments separated to multiple files, and split that change out to #535


When queried for an operational intent reference that was created via another DSS, a DSS instance is expected to provide a valid operational intent reference.

If it does not, it might be in violation of **[astm.f3548.v21.DSS0005,1](../../../../../requirements/astm/f3548/v21.md)**.
Copy link
Member

Choose a reason for hiding this comment

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

I see that we have many of these "might" phrasings, but I think we'll need to fix that -- if we indicate that a check has failed and the documentation for that check references a requirement, failing the check should necessarily mean the requirement was not met. If we fail the check and the requirement is met, that's a bug in our implementation. So, hopefully we can make the claim:

Suggested change
If it does not, it might be in violation of **[astm.f3548.v21.DSS0005,1](../../../../../requirements/astm/f3548/v21.md)**.
If it does not, it is in violation of **[astm.f3548.v21.DSS0005,1](../../../../../requirements/astm/f3548/v21.md)**.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the wording should be non-conditional whenever possible:

In this particular situation, the failure might also be caused by the DSS failing DSS0215 (ie, returning the API call before committing the change to the distributed storage.

(We can move that discussion to PR #533 as it introduces the relevant wording and reference to DSS0215 and DSS0020 to the subscription synchronization scenario)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Wording updated to is for the moment, as this check references a single requirement only)

## Overview

Verifies that all CRUD operations on operational intent references performed on a given DSS instance
are properly propagated to every other DSS instances participating in the deployment.
Copy link
Member

Choose a reason for hiding this comment

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

nit: instance

Suggested change
are properly propagated to every other DSS instances participating in the deployment.
are properly propagated to every other DSS instance participating in the deployment.

@Shastick Shastick marked this pull request as draft March 8, 2024 09:59
@Shastick
Copy link
Contributor Author

Shastick commented Mar 8, 2024

Taking to draft while splitting fragments. Might elect to split things out to separate PRs

def query_timestamps(self) -> List[datetime.datetime]:
"""Returns the timestamps of all queries present in this QueryError."""
return [q.request.timestamp for q in self.queries]

Copy link
Contributor Author

@Shastick Shastick Mar 8, 2024

Choose a reason for hiding this comment

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

Update to this file split out to #536, for any discussion/updates we can discuss over there

@Shastick Shastick force-pushed the dss0210-basics-entities branch from 0cb119e to f9a95c7 Compare March 8, 2024 12:40
@Shastick Shastick marked this pull request as ready for review March 8, 2024 12:40
@Shastick
Copy link
Contributor Author

Shastick commented Mar 8, 2024

Ready for another round of review.

This PR sits on top of #535, the range of two last commits on the present PR represent its intended changes.

return

with self.check(
"Create operational intent reference response is correct",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here for the usage of the check mentioned in this comment

@Shastick Shastick marked this pull request as draft March 12, 2024 16:52
@Shastick Shastick force-pushed the dss0210-basics-entities branch 3 times, most recently from fa7589d to b30627e Compare March 12, 2024 17:04
@Shastick Shastick marked this pull request as ready for review March 12, 2024 17:05
@Shastick Shastick force-pushed the dss0210-basics-entities branch from b30627e to 9cc18fe Compare March 12, 2024 17:12
@Shastick Shastick marked this pull request as draft March 14, 2024 07:50
@Shastick
Copy link
Contributor Author

Shastick commented Mar 14, 2024

Moved to draft while we reword and restructure the test fragments, and we make sure that format & content check logic is as it should be.

@Shastick Shastick force-pushed the dss0210-basics-entities branch 3 times, most recently from c4be07f to 12ccf79 Compare March 15, 2024 10:53
@Shastick Shastick marked this pull request as ready for review March 15, 2024 10:54
@Shastick Shastick marked this pull request as draft March 15, 2024 19:28
@Shastick Shastick force-pushed the dss0210-basics-entities branch 5 times, most recently from da4db1c to 0cfed7f Compare March 20, 2024 21:42
@Shastick Shastick marked this pull request as ready for review March 20, 2024 21:44
PR comment integration
@Shastick Shastick force-pushed the dss0210-basics-entities branch from 0cfed7f to f765598 Compare March 21, 2024 07:44
@Shastick
Copy link
Contributor Author

Shastick commented Mar 21, 2024

Closing to reopen a clean PR, as things have diverged a lot from the original. New PR at #573

@Shastick Shastick closed this Mar 21, 2024
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