rekey all cosmos data from .id=UUID to .id=resourceID#3651
rekey all cosmos data from .id=UUID to .id=resourceID#3651openshift-merge-bot[bot] merged 8 commits intomainfrom
Conversation
deads2k
force-pushed
the
cs-76-standardize-metadata
branch
from
4f8d363 to
56657dd
Compare
|
/retest |
deads2k
force-pushed
the
cs-76-standardize-metadata
branch
4 times, most recently
from
c04ac35 to
2ac4245
Compare
|
/retest |
deads2k
force-pushed
the
cs-76-standardize-metadata
branch
4 times, most recently
from
7b3a225 to
9e0d385
Compare
|
ha! it worked! |
deads2k
force-pushed
the
cs-76-standardize-metadata
branch
from
9e0d385 to
906d49f
Compare
deads2k
changed the title
deads2k
force-pushed
the
cs-76-standardize-metadata
branch
from
906d49f to
7173fff
Compare
|
/retest |
|
setup /retest |
| } | ||
| _, err = transaction.Execute(ctx, nil) | ||
| if err != nil { | ||
| logger.Error("failed executing transaction", "transaction", transaction) |
There was a problem hiding this comment.
is it useful to emit the error here in the log as well, or we'll have it tracked as part of utils.TrackError()
There was a problem hiding this comment.
is it useful to emit the error here in the log as well, or we'll have it tracked as part of
utils.TrackError()
Doing it here allows us to include the entire transaction in the structured output.
bennerv
left a comment
bennerv
left a comment
There was a problem hiding this comment.
/hold
/lgtm
Took a few passes and it makes sense to me. Had a couple open questions, but feel free to remove the hold so it merges.
| for _, subscription := range subscriptionIterator.Items(ctx) { | ||
| if _, err := cosmosClient.Subscriptions().Get(ctx, subscription.ResourceID.Name); err != nil { | ||
| panic(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we use subscription.ResourceID.Name over the subscription.ResourceID.SubscriptionID for keying into cosmosdb? Fundamentally there's no difference, but wondering if there's a specific reason.
There was a problem hiding this comment.
inconsistency when I wrote it.
test-integration/utils/databasemutationhelpers/resource_crud_test_util.go
Show resolved
Hide resolved
| LastUpdated int `json:"-"` | ||
|
|
||
| // CosmosUID is used to keep track of whether we have transitioned to a new cosmosUID scheme for this item | ||
| CosmosUID string `json:"-"` |
There was a problem hiding this comment.
why do we not marshal this, but do so for operation types and other types?
There was a problem hiding this comment.
why do we not marshal this, but do so for operation types and other types?
I think you're right. Sounds like via slack I can take this an IOU to clear the stack and deliver it.
| } | ||
|
|
||
| // some pieces of data conflict with standard fields. We may evolve over time, but for now avoid persisting those. | ||
| cosmosObj.InternalState.CosmosUID = "" |
There was a problem hiding this comment.
is this unnecessary since the field using the json tag: json:"-" for the subscription CosmosUID field?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bennerv, deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I agree to the IOU /hold cancel |
|
/retest |
This change performs a data migration on read to switch from reading based on a cosmos search of fields matching a pattern to direct item reads based on key. On a failure to lookup by cosmosID and success reading based on existing search, we create a new item and delete the original. Since old frontends use the cosmosUID as fully opaque, the new ID doesn't cause problems for old frontends even on rollback. When the frontend starts, we find all subscriptions, then search for all resources in each subscription using the untyped client listRecursive, then read each item to trigger the migration.
The iterator should remain cosmosID (unless we decide to redefine as resourceID), but for now keeping that meaning the same.
This will allow debugger errors like "(wrapped at cluster.go:674) transaction step 1 of 3 failed with 404 Not Found".
deads2k
force-pushed
the
cs-76-standardize-metadata
branch
from
4e54fcd to
9234f40
Compare
|
New changes are detected. LGTM label has been removed. |
|
simple rebase/unit test tweak to #3218 |
|
2025 seed /retest |
4 participants
This change performs a data migration on read to switch from reading based on a cosmos search of fields matching a pattern to direct item reads based on key. On a failure to lookup by cosmosID and success reading based on existing search, we create a new item and delete the original.
Since old frontends use the cosmosUID as fully opaque, the new ID doesn't cause problems for old frontends even on rollback.
When the frontend starts, we find all subscriptions, then search for all resources in each subscription using the untyped client listRecursive, then read each item to trigger the migration.
If the create succeeds and the delete fails, then resulting GETs will produce an ambiguous result error (a pre-existing handled error, so old frontends are ok) and we'll have to resolve the conflict manually.
Still needs more test coverage for types, but since we merged #3662 we can see this works properly for clusters and operations. Need to add coverage (in a separate PR first to prove it works) for nodepools and externalauths.