[deploy][scd/crdb] Add columns uss_requested_ovn and past_ovns to scd_operations table; Bump schema_version to v3.2.0 #1095
Conversation
| if operation.OVN != "" { | ||
| ovn = pgtype.Text{ | ||
| String: operation.OVN.String(), | ||
| Valid: true, | ||
| } | ||
| } |
There was a problem hiding this comment.
Side question: if we are going to set the ovn field only when it was specified by a client, should we possibly name it client_specified_ovn?
Alternatively, could we set it in any case and compute the time-based OVN once at insertion time instead of when it is retrieved from the database? Or is it important to be able to distinguish between client-provided and server-generated OVN, and to do this via a null ovn field?
There was a problem hiding this comment.
Alternatively, could we set it in any case and compute the time-based OVN once at insertion time instead of when it is retrieved from the database? Or is it important to be able to distinguish between client-provided and server-generated OVN, and to do this via a null ovn field?
I did consider that initially, and at the time of making this PR the assumption was that if the provided suffix is not valid, the DSS just ignores it and do as usual.
However that assumption changed so maybe that's to be reconsidered.
There was a problem hiding this comment.
I've looked into it, but unfortunately there isn't a simple way (i.e. w/o data migration) of computing the OVN at insertion time instead of when it is retrieved. The risk is that with the DSS update the previous OVNs cease to be valid which is of course not acceptable.
I will update the column however to make it more explicit that this is not the OVN in all cases however.
| CHECK ( | ||
| array_position(past_ovns, NULL) IS NULL AND | ||
| array_position(past_ovns, '') IS NULL AND | ||
| array_position(past_ovns, ovn) IS NULL | ||
| ); -- past_ovns must not contain NULL elements, empty strings or current ovn |
There was a problem hiding this comment.
Side thought: how big do we expect these arrays to grow?
I don't believe that enforcing the constraint will ever become a problem, but this still makes me wonder how many modifications of an OIR (which is what will cause this array to grow) we should expect in the worst case.
There was a problem hiding this comment.
I do not know. But given that we don't use it to query anything, I don't expect its size to have any significant impact.
mickmis
force-pushed
the
1078/crdb
branch
2 times, most recently
from
c1a38c0
to
c8c57f5
Compare
…ons table; Bump schema_version to v3.2.0
mickmis
changed the title
There was a problem hiding this comment.
Small remark about the expected commit message / PR title.
Do note that I've only tested this change by deploying locally the DSS and by validating that the CI passes. I've not tested deployment to the cloud: @barroco should this be tested manually before merging this PR or it this not necessary?
Full scale deployment to the cloud will be tested before releasing the next stable version manually.
There is an action to deploy a DSS instance to AWS if you wish to run it on specific commits. (Only covers helm and aws for the moment)
mickmis
changed the title
| ALTER TABLE scd_operations | ||
| ADD COLUMN IF NOT EXISTS uss_requested_ovn STRING | ||
| CHECK (uss_requested_ovn != ''), -- uss_requested_ovn must be NULL if unspecified, not an empty string | ||
| ADD COLUMN IF NOT EXISTS past_ovns STRING[] NOT NULL | ||
| DEFAULT ARRAY []::STRING[] | ||
| CHECK ( | ||
| array_position(past_ovns, NULL) IS NULL AND | ||
| array_position(past_ovns, '') IS NULL AND | ||
| array_position(past_ovns, uss_requested_ovn) IS NULL | ||
| ); -- past_ovns must not contain NULL elements, empty strings or current uss_requested_ovn |
There was a problem hiding this comment.
@BenjaminPelletier @barroco FTR: the only modification this data migration is doing is to add columns. Given that the queries are only ever referencing explicitly columns (as opposed to a wildcard selection), I think it is safe to conclude that previous versions of the DSS runtime are compatible with this newer version of the schema.
(follow-up to the discussion last tuesday about upgrades during the InterUSS weekly)
First step of #1078: this adds to the cockroach DB
scd_operationstable theovnandpast_ovnscolumns.This change only concerns the DB and does not introduce any behavior change.
Note the
README.mdupdates: removal of*_bootstrapper.sh(latestversion is specified there) and addition ofschema_manager.yaml.CRDB schema
A
NULLvalue in theovncolumn signifies that the operational intent uses the existing DSS-generated OVN, which is derived from the timestamp of the last update. If it has a non-NULLvalue, it may not be an empty string, and it signifies that the OVN in force was proposed by the USS.The
past_ovnscolumn is intended to contain all past OVNs of the operational intent, both DSS-generated and USS-proposed. This mechanism will be implemented in the next PR.Testing
Do note that I've only tested this change by deploying locally the DSS and by validating that the CI passes. I've not tested deployment to the cloud: @barroco should this be tested manually before merging this PR or it this not necessary?