ROX-33608: add EnsureVirtualMachineExists to VM v2 datastore#19551
ROX-33608: add EnsureVirtualMachineExists to VM v2 datastore#19551dashrews78 wants to merge 3 commits intomasterfrom
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
23b41b9 to
ae3cb10
Compare
|
Images are ready for the commit at 8d6e9a2. To use with deploy scripts, first |
07beb4c to
3e3239d
Compare
ae3cb10 to
13ce987
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19551 +/- ##
==========================================
- Coverage 49.32% 49.31% -0.01%
==========================================
Files 2737 2737
Lines 206445 206462 +17
==========================================
+ Hits 101823 101827 +4
- Misses 97075 97090 +15
+ Partials 7547 7545 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3e3239d to
2fc5dd0
Compare
The scan index pipeline needs to insert a minimal VM record before upserting scan data to satisfy FK constraints (VirtualMachineScanV2.VmV2Id → VirtualMachineV2.Id). EnsureVirtualMachineExists checks if the VM row exists; if not, it inserts a minimal placeholder with just id and cluster_id, avoiding overwriting richer metadata from the VM pipeline. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
13ce987 to
504a110
Compare
Clarify that the upsert creates a minimal VM record with only the ID and cluster association when the VM doesn't yet exist. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ajheflin
left a comment
There was a problem hiding this comment.
I assume this is going to be actually used somewhere in another PR
The next one in fact. |
|
/retest-required |
| return ds.store.GetMany(ctx, ids) | ||
| } | ||
|
|
||
| func (ds *datastoreImpl) EnsureVirtualMachineExists(ctx context.Context, vmID string, clusterID string) error { |
There was a problem hiding this comment.
Do we need to validate the clusterID isn't empty like we're doing for vmID?
|
@dashrews78: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| return errors.New("cannot ensure VM exists without an id") | ||
| } | ||
|
|
||
| _, exists, err := ds.store.Get(ctx, vmID) |
There was a problem hiding this comment.
nit : Use count.
Get is probably fine too though. Since Scan is separated out, the proto won't be that big.
Description
The scan index pipeline needs to insert a minimal VM record before
upserting scan data to satisfy FK constraints (VirtualMachineScanV2.VmV2Id
→ VirtualMachineV2.Id). EnsureVirtualMachineExists checks if the VM row
exists; if not, it inserts a minimal placeholder with just id and
cluster_id, avoiding overwriting richer metadata from the VM pipeline.
Partially generated by AI.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
User-facing documentation
Testing and quality
Automated testing
How I validated my change
new unit tests.