feat: Use flintlock unique id #327

yitsushi · 2021-12-17T11:17:41Z

What this PR does / why we need it:

This commit message will be a QA like commit message

Why in the spec and not somewhere else?

Historically we are using the same spec in create request an responses,
we do not return with status on create request because it's mostly
empty. The uuid could be there, but on create there is no status yet.

Why is it set on create request?

We can't return the uuid in the response if it's not generated and
without the uuid they can't delete the resource, it would be a disaster
if capmvm sends a request to create ns1/mvm1, then it wants to delete it
even before we created the resources. If the delete request would be
only ns1/mvm1 it would delete the old one, or basically any other
microvms that has the same name and namespace (coming from other
management clusters).

Why is it global and not microvm specific?

It seems logical and generating and maintaining a unique id shouldn't be
the job of the microvm provider, their responsibility should be to
manage microvms, metadata and other stuffs are provider independent.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #291

Special notes for your reviewer:

Checklist:

squashed commits into logical changes
includes documentation
adds unit tests
adds or updates e2e tests

codecov-commenter · 2022-01-10T10:37:38Z

Codecov Report

Merging #327 (eec3be9) into main (3807bed) will decrease coverage by 0.46%.
The diff coverage is 63.04%.

❗ Current head eec3be9 differs from pull request most recent head 97182d0. Consider uploading reports for the commit 97182d0 to get more accurate results

@@            Coverage Diff             @@
##             main     #327      +/-   ##
==========================================
- Coverage   57.76%   57.30%   -0.47%     
==========================================
  Files          51       51              
  Lines        2517     2546      +29     
==========================================
+ Hits         1454     1459       +5     
- Misses        945      964      +19     
- Partials      118      123       +5

Impacted Files	Coverage Δ
core/application/errors.go	`0.00% <0.00%> (ø)`
core/application/reconcile.go	`0.00% <0.00%> (ø)`
infrastructure/firecracker/provider.go	`0.00% <0.00%> (ø)`
infrastructure/firecracker/state.go	`0.00% <0.00%> (ø)`
core/models/vmid.go	`61.11% <43.47%> (-24.26%)`	⬇️
infrastructure/grpc/convert.go	`41.98% <60.00%> (+0.18%)`	⬆️
infrastructure/containerd/repo.go	`74.25% <80.00%> (+0.01%)`	⬆️
core/application/commands.go	`71.64% <85.71%> (-1.38%)`	⬇️
core/application/query.go	`100.00% <100.00%> (ø)`
core/plans/microvm_delete.go	`60.37% <100.00%> (-0.74%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3807bed...97182d0. Read the comment docs.

jmickey

LGTM

richardcase · 2022-01-19T06:33:44Z

api/services/microvm/v1alpha1/microvms.proto

@@ -58,11 +58,13 @@ message CreateMicroVMResponse {
 message DeleteMicroVMRequest {
  string id = 1;
  string namespace = 2;
+  string uuid = 3;


I'm not sure about this. I think we need 2 versions of the Delete/Get API methods:

Delete/Get(name, namespace)

DeleteById/GetById(id)

I still think its valid to get or delete a microvm based on its namespace/name only

This would probably then mean changing id to name throughout flintlock

I wouldn't let users to delete by ns/name only because in some cases it will not be only one, same for get. The get endpoint returns one spec, but without the uid, it's not guaranteed that's the only one.

Or we have to update the Get return a list of specs.

I would even go remove everything from the delete and get and leave it only the uid. So you can Get (single spec response) only with uid and delete the same way.

Yes i think perhaps you are right. Get and delete should only accept the id. I think we should also implement (in a different PR) a Find that we can use to find by the ns/name.....wdyt?

richardcase · 2022-01-19T06:42:46Z

core/application/commands.go

 		if err != nil {
 			return nil, fmt.Errorf("creating vmid: %w", err)
 		}

 		mvm.ID = *vmid
 	}

+	uuid, err := a.ports.IdentifierService.GenerateRandom()


The infrastructure implementation that we use for the identifier service doesn't generate a UUID but instead a ULID. It does say a ULID is compatible with a UUID but its not formatted as a UUID with this call.

Maybe we need to refer to this as a uid instead of uuid to indicate that its not a UUID? Or something similar.

actually, it's even more confusing now. Turned out i used a.ports.IdentifierService.GenerateRandom() here and in the grpc server uuid.New().String() which one should we keep?

i'll use ulid to be more consistent with the rest of the system.

Yeah ulid from the identifier service is the best.

This commit message will be a QA like ocmmit message Why in the spec and not somewhere else? Historically we are using the same spec in create request an responses, we do not return with status on create request because it's mostly empty. The uuid could be there, but on create there is no status yet. Why is it set on create request? We can't return the uuid in the response if it's not generated and without the uuid they can't delete the resource, it would be a disaster if capmvm sends a request to create ns1/mvm1, then it wants to delete it even before we created the resources. If the delete request would be only ns1/mvm1 it would delete the old one, or basically any other microvms that has the same name and namespace (coming from other management clusters). Why is it global and not microvm specific? It seems logical and generating and maintaining a unique id shouldn't be the job of the microvm provider, their responsibility should be to manage microvms, metadata and other stuffs are provider independent. Fixes liquidmetal-dev#291 rename field to be uid instead of uuid because it is an ulid now remove id and ns from get and delete endpoints

core/application/commands.go

Callisto13 · 2022-01-25T14:07:21Z

hack/scripts/payload/DeleteMicroVM.json

@@ -1,4 +1,5 @@
 {
  "id": "mvm1",
-  "namespace": "ns1"
+  "namespace": "ns1",
+  "uid": "b1c0aa3c-ebba-498a-a7c7-903d91758d83"


should the other 2 be removed?

But that "tooling" has to be refined after Find.

Callisto13 · 2022-01-25T14:10:10Z

infrastructure/grpc/server.go

+	/*
+		if req.Microvm.Uid == nil {
+			newUID := uuid.New().String()
+			req.Microvm.Uid = &newUID
+		}
+	*/


this meant to be here still?

Co-authored-by: Claudia <claudiaberesford@gmail.com>

* e2e path was not updated. * Get tried to use empty name and namespace as a filter, not it does not use those values if they are empty. * UpdateMicroVM required all fields. Related to #291 (issue) Related to #327 (pr)

* fix: uid related issues * e2e path was not updated. * Get tried to use empty name and namespace as a filter, not it does not use those values if they are empty. * UpdateMicroVM required all fields. Related to #291 (issue) Related to #327 (pr) * fix test

yitsushi added the kind/feature New feature or request label Dec 17, 2021

yitsushi force-pushed the 291-uuid branch from 9599599 to 8561175 Compare December 21, 2021 11:20

yitsushi marked this pull request as ready for review December 21, 2021 12:19

yitsushi force-pushed the 291-uuid branch from d331720 to 4eb2593 Compare January 10, 2022 10:37

yitsushi force-pushed the 291-uuid branch 2 times, most recently from 84d9839 to 841dcf3 Compare January 14, 2022 09:29

jmickey previously approved these changes Jan 14, 2022

View reviewed changes

yitsushi requested a review from richardcase January 17, 2022 08:54

richardcase requested changes Jan 19, 2022

View reviewed changes

yitsushi dismissed jmickey’s stale review via 5dc5b3f January 20, 2022 10:23

yitsushi force-pushed the 291-uuid branch from 5dc5b3f to 3ab6f19 Compare January 20, 2022 10:25

Callisto13 mentioned this pull request Jan 24, 2022

Creating an MD with 2+ hosts makes duplicate mvms across each of them liquidmetal-dev/cluster-api-provider-microvm#104

Closed

yitsushi force-pushed the 291-uuid branch 2 times, most recently from 845cf75 to 86d92cd Compare January 25, 2022 11:16

yitsushi added 2 commits January 25, 2022 12:30

fix e2e

f3d71e3

yitsushi force-pushed the 291-uuid branch from 39ff2f9 to f3d71e3 Compare January 25, 2022 11:31

Callisto13 previously approved these changes Jan 25, 2022

View reviewed changes

richardcase previously approved these changes Jan 25, 2022

View reviewed changes

Update core/application/commands.go

979ea2e

Co-authored-by: Claudia <claudiaberesford@gmail.com>

yitsushi dismissed stale reviews from richardcase and Callisto13 via 979ea2e January 25, 2022 15:54

yitsushi added 2 commits January 25, 2022 16:55

address comments

dbf622c

Merge branch '291-uuid' of github.com:yitsushi/flintlock into 291-uuid

f898160

Callisto13 approved these changes Jan 25, 2022

View reviewed changes

Merge branch 'main' into 291-uuid

97182d0

yitsushi merged commit 054c6c3 into liquidmetal-dev:main Jan 26, 2022

yitsushi mentioned this pull request Jan 26, 2022

fix: uid related issues #367

Merged

4 tasks

yitsushi mentioned this pull request Jan 26, 2022

Ability to list microvms based on namespace and name #368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use flintlock unique id #327

feat: Use flintlock unique id #327

yitsushi commented Dec 17, 2021 •

edited

Loading

codecov-commenter commented Jan 10, 2022 •

edited

Loading

jmickey left a comment

richardcase Jan 19, 2022 •

edited

Loading

richardcase Jan 19, 2022

yitsushi Jan 19, 2022

yitsushi Jan 19, 2022

yitsushi Jan 20, 2022

richardcase Jan 22, 2022

richardcase Jan 19, 2022

yitsushi Jan 20, 2022

yitsushi Jan 20, 2022 •

edited

Loading

richardcase Jan 22, 2022

Callisto13 Jan 25, 2022

yitsushi Jan 25, 2022

yitsushi Jan 25, 2022

Callisto13 Jan 25, 2022

yitsushi Jan 25, 2022

feat: Use flintlock unique id #327

feat: Use flintlock unique id #327

Conversation

yitsushi commented Dec 17, 2021 • edited Loading

codecov-commenter commented Jan 10, 2022 • edited Loading

Codecov Report

jmickey left a comment

Choose a reason for hiding this comment

richardcase Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi commented Dec 17, 2021 •

edited

Loading

codecov-commenter commented Jan 10, 2022 •

edited

Loading

richardcase Jan 19, 2022 •

edited

Loading

yitsushi Jan 20, 2022 •

edited

Loading