csi/api: populate ReadAllocs/WriteAllocs fields #9377

tgross · 2020-11-17T15:45:10Z

The API is missing values for ReadAllocs and WriteAllocs fields, resulting
in allocation claims not being populated in the web UI. These fields mirror
the fields in nomad/structs.CSIVolume. Returning a separate list of stubs
for read and write would be ideal, but this can't be done without either
bloating the API response with repeated full Allocation data, or causing a
panic in previous versions of the CLI.

The nomad/structs fields are persisted with nil values and are populated
during RPC, so we'll do the same in the HTTP API and populate the ReadAllocs
and WriteAllocs fields with a map of allocation IDs, but with null
values. The web UI will then create its ReadAllocations and
WriteAllocations fields by mapping from those IDs to the values in
Allocations, instead of flattening the map into a list.

I've tested this out via the web UI and verified nomad volume status, nomad alloc status, and nomad node status aren't broken, including with a v0.12.8 version of the command line.

tgross · 2020-11-17T15:48:23Z

ui/app/serializers/volume.js

-    hash.ReadAllocations = Object.keys(readAllocs).map(bindIDToAlloc(readAllocs));
-    hash.WriteAllocations = Object.keys(writeAllocs).map(bindIDToAlloc(writeAllocs));
+    hash.ReadAllocations = Object.keys(readAllocs).map(bindIDToAlloc(hash.Allocations));
+    hash.WriteAllocations = Object.keys(writeAllocs).map(bindIDToAlloc(hash.Allocations));


Question for reviewers: would it save browser memory if we nulled-out the hash.Allocations, hash.ReadAllocs and hash.WriteAllocs fields after we're done with them here? Or is this object so ephemeral that it's meaningless to do so?

I don't know off the top of my head if hash sticks around or if a copy is what gets inserted into the store. It wouldn't hurt to add a delete hash.Allocations just to err on the side of caution.

github-actions · 2020-11-17T15:52:43Z

Ember Asset Size action

As of efdce68

Files that got Bigger 🚨:

File	raw	gzip
nomad-ui.js	+50 B	+8 B

Files that stayed the same size 🤷‍:

File	raw	gzip
vendor.js	0 B	0 B
nomad-ui.css	0 B	0 B
vendor.css	0 B	0 B

github-actions · 2020-11-17T16:19:47Z

Ember Test Audit comparison

	master	`efdce68`	change
passes	1536	1536	0
failures	0	0	0
flaky	0	0	0
duration	7m 40s 364ms	8m 06s 396ms	+26s 032ms

notnoop

The golang side is LGTM.

notnoop · 2020-11-18T14:29:54Z

api/csi.go

+
+	// WriteAllocs is a map of allocation IDs for tracking writer claim
+	// status. The Allocation value will always be nil; clients can populate
+	// this data by iterating over the Allocations field.
 	WriteAllocs map[string]*Allocation


Curious why we chose this representation? If the map value is never a populated alloc, how does it compare to using map[string]bool (explicitly meaningless value) or a plain slice []string (avoid value but losing uniqueness)? Not advocating for changing it, specially if we are constrained by compatibility.

It's solely constrained by compatibility at this point, unfortunately.

With 20/20 hindsight, we would have had the nomad/structs version store only the CSIVolumeClaims and not allocations (which has been a huge complication with respect to garbage collected jobs and allocations), and then had this API struct return WriteAllocs map[string]*AllocationListStub and ReadAllocs map[string]*AllocationListStub, without an Allocations field at all. See #8734 for more

DingoEatingFuzz

I had a note on performance, but I'll let you decide if it's worth changing.

ui/app/serializers/volume.js

DingoEatingFuzz · 2020-11-25T19:13:12Z

ui/app/serializers/volume.js

-    hash.ReadAllocations = Object.keys(readAllocs).map(bindIDToAlloc(readAllocs));
-    hash.WriteAllocations = Object.keys(writeAllocs).map(bindIDToAlloc(writeAllocs));
+    hash.ReadAllocations = Object.keys(readAllocs).map(bindIDToAlloc(hash.Allocations));
+    hash.WriteAllocations = Object.keys(writeAllocs).map(bindIDToAlloc(hash.Allocations));


I don't know off the top of my head if hash sticks around or if a copy is what gets inserted into the store. It wouldn't hurt to add a delete hash.Allocations just to err on the side of caution.

The API is missing values for `ReadAllocs` and `WriteAllocs` fields, resulting in allocation claims not being populated in the web UI. These fields mirror the fields in `nomad/structs.CSIVolume`. Returning a separate list of stubs for read and write would be ideal, but this can't be done without either bloating the API response with repeated full `Allocation` data, or causing a panic in previous versions of the CLI. The `nomad/structs` fields are persisted with nil values and are populated during RPC, so we'll do the same in the HTTP API and populate the `ReadAllocs` and `WriteAllocs` fields with a map of allocation IDs, but with null values. The web UI will then create its `ReadAllocations` and `WriteAllocations` fields by mapping from those IDs to the values in `Allocations`, instead of flattening the map into a list.

github-actions · 2022-12-08T02:18:17Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

tgross commented Nov 17, 2020

View reviewed changes

vercel bot temporarily deployed to Preview November 17, 2020 20:16 Inactive

tgross force-pushed the b-csi-api-readwriteallocs branch from 80cd829 to 6b246ee Compare November 17, 2020 20:32

vercel bot deployed to Preview November 17, 2020 20:32 View deployment

tgross marked this pull request as ready for review November 17, 2020 20:43

tgross requested review from DingoEatingFuzz and notnoop November 17, 2020 20:43

notnoop approved these changes Nov 18, 2020

View reviewed changes

tgross added this to the 1.0 milestone Nov 18, 2020

tgross requested a review from backspace November 23, 2020 15:07

tgross mentioned this pull request Nov 25, 2020

changelog entries for CSI #9444

Merged

3 tasks

DingoEatingFuzz approved these changes Nov 25, 2020

View reviewed changes

tgross force-pushed the b-csi-api-readwriteallocs branch from 6b246ee to 198264e Compare November 25, 2020 20:47

vercel bot temporarily deployed to Preview November 25, 2020 20:48 Inactive

tgross force-pushed the b-csi-api-readwriteallocs branch from 198264e to d095329 Compare November 25, 2020 20:52

vercel bot temporarily deployed to Preview November 25, 2020 20:52 Inactive

fix accidentally quadratic behavior

efdce68

tgross force-pushed the b-csi-api-readwriteallocs branch from d095329 to efdce68 Compare November 25, 2020 21:11

vercel bot temporarily deployed to Preview November 25, 2020 21:11 Inactive

tgross merged commit 8351c3f into master Nov 25, 2020

tgross deleted the b-csi-api-readwriteallocs branch November 25, 2020 21:44

tgross mentioned this pull request Nov 25, 2020

Nomad CSI volume status UI page never shows allocations depending on the volume #9215

Closed

tgross mentioned this pull request Dec 3, 2020

CSI volume allocation count not updated properly in UI #9495

Closed

github-actions bot locked as resolved and limited conversation to collaborators Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

csi/api: populate ReadAllocs/WriteAllocs fields #9377

csi/api: populate ReadAllocs/WriteAllocs fields #9377

tgross commented Nov 17, 2020 •

edited

Loading

tgross Nov 17, 2020 •

edited

Loading

DingoEatingFuzz Nov 25, 2020

github-actions bot commented Nov 17, 2020 •

edited

Loading

github-actions bot commented Nov 17, 2020 •

edited

Loading

notnoop left a comment

notnoop Nov 18, 2020

tgross Nov 18, 2020

DingoEatingFuzz left a comment

DingoEatingFuzz Nov 25, 2020

github-actions bot commented Dec 8, 2022

csi/api: populate ReadAllocs/WriteAllocs fields #9377

csi/api: populate ReadAllocs/WriteAllocs fields #9377

Conversation

tgross commented Nov 17, 2020 • edited Loading

tgross Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

DingoEatingFuzz Nov 25, 2020

Choose a reason for hiding this comment

github-actions bot commented Nov 17, 2020 • edited Loading

Ember Asset Size action

github-actions bot commented Nov 17, 2020 • edited Loading

Ember Test Audit comparison

notnoop left a comment

Choose a reason for hiding this comment

notnoop Nov 18, 2020

Choose a reason for hiding this comment

tgross Nov 18, 2020

Choose a reason for hiding this comment

DingoEatingFuzz left a comment

Choose a reason for hiding this comment

DingoEatingFuzz Nov 25, 2020

Choose a reason for hiding this comment

github-actions bot commented Dec 8, 2022

tgross commented Nov 17, 2020 •

edited

Loading

tgross Nov 17, 2020 •

edited

Loading

github-actions bot commented Nov 17, 2020 •

edited

Loading

github-actions bot commented Nov 17, 2020 •

edited

Loading