Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

csi/api: populate ReadAllocs/WriteAllocs fields #9377

Merged
merged 2 commits into from
Nov 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 9 additions & 21 deletions api/csi.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ func (v *CSIVolumes) Info(id string, q *QueryOptions) (*CSIVolume, *QueryMeta, e
return nil, nil, err
}

// Cleanup allocation representation for the ui
resp.allocs()

return &resp, qm, nil
}

Expand Down Expand Up @@ -110,11 +107,17 @@ type CSIVolume struct {
Parameters map[string]string `hcl:"parameters"`
Context map[string]string `hcl:"context"`

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

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Combine structs.{Read,Write}Allocs
// Allocations is a combined list of readers and writers
Allocations []*AllocationListStub

// Schedulable is true if all the denormalized plugin health fields are true
Expand All @@ -136,21 +139,6 @@ type CSIVolume struct {
ExtraKeysHCL []string `hcl1:",unusedKeys" json:"-"`
}

// allocs is called after we query the volume (creating this CSIVolume struct) to collapse
// allocations for the UI
func (v *CSIVolume) allocs() {
for _, a := range v.WriteAllocs {
if a != nil {
v.Allocations = append(v.Allocations, a.Stub())
}
}
for _, a := range v.ReadAllocs {
if a != nil {
v.Allocations = append(v.Allocations, a.Stub())
}
}
}

type CSIVolumeIndexSort []*CSIVolumeListStub

func (v CSIVolumeIndexSort) Len() int {
Expand Down
25 changes: 21 additions & 4 deletions command/agent/csi_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ func structsCSIVolumeToApi(vol *structs.CSIVolume) *api.CSIVolume {
return nil
}

allocs := len(vol.WriteAllocs) + len(vol.ReadAllocs)
allocCount := len(vol.ReadAllocs) + len(vol.WriteAllocs)

out := &api.CSIVolume{
ID: vol.ID,
Expand All @@ -326,7 +326,11 @@ func structsCSIVolumeToApi(vol *structs.CSIVolume) *api.CSIVolume {
Context: vol.Context,

// Allocations is the collapsed list of both read and write allocs
Allocations: make([]*api.AllocationListStub, 0, allocs),
Allocations: make([]*api.AllocationListStub, 0, allocCount),

// tracking of volume claims
ReadAllocs: map[string]*api.Allocation{},
WriteAllocs: map[string]*api.Allocation{},

Schedulable: vol.Schedulable,
PluginID: vol.PluginID,
Expand All @@ -342,15 +346,28 @@ func structsCSIVolumeToApi(vol *structs.CSIVolume) *api.CSIVolume {
ModifyIndex: vol.ModifyIndex,
}

// WriteAllocs and ReadAllocs will only ever contain the Allocation ID,
// with a null value for the Allocation; these IDs are mapped to
// allocation stubs in the Allocations field. This indirection is so the
// API can support both the UI and CLI consumer in a safely backwards
// compatible way
for _, a := range vol.WriteAllocs {
if a != nil {
out.Allocations = append(out.Allocations, structsAllocListStubToApi(a.Stub(nil)))
alloc := structsAllocListStubToApi(a.Stub(nil))
if alloc != nil {
out.WriteAllocs[alloc.ID] = nil
out.Allocations = append(out.Allocations, alloc)
}
}
}

for _, a := range vol.ReadAllocs {
if a != nil {
out.Allocations = append(out.Allocations, structsAllocListStubToApi(a.Stub(nil)))
alloc := structsAllocListStubToApi(a.Stub(nil))
if alloc != nil {
out.ReadAllocs[alloc.ID] = nil
out.Allocations = append(out.Allocations, alloc)
}
}
}

Expand Down
26 changes: 17 additions & 9 deletions ui/app/serializers/volume.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,25 @@ export default class VolumeSerializer extends ApplicationSerializer {
hash.ID = JSON.stringify([`csi/${hash.ID}`, hash.NamespaceID || 'default']);
hash.PluginID = `csi/${hash.PluginID}`;

// Convert hash-based allocation embeds to lists
// Populate read/write allocation lists from aggregate allocation list
const readAllocs = hash.ReadAllocs || {};
const writeAllocs = hash.WriteAllocs || {};
const bindIDToAlloc = hash => id => {
const alloc = hash[id];
alloc.ID = id;
return alloc;
};

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

hash.ReadAllocations = [];
hash.WriteAllocations = [];

if (hash.Allocations) {
hash.Allocations.forEach(function(alloc) {
const id = alloc.ID;
if (id in readAllocs) {
hash.ReadAllocations.push(alloc);
}
if (id in writeAllocs) {
hash.WriteAllocations.push(alloc);
}
});
delete hash.Allocations;
}

const normalizedHash = super.normalize(typeHash, hash);
return this.extractEmbeddedRecords(this, this.store, typeHash, normalizedHash);
Expand Down
1 change: 1 addition & 0 deletions ui/mirage/models/csi-volume.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ export default Model.extend({
plugin: belongsTo('csi-plugin'),
writeAllocs: hasMany('allocation'),
readAllocs: hasMany('allocation'),
allocations: hasMany('allocation'),
});
2 changes: 1 addition & 1 deletion ui/mirage/serializers/csi-volume.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const groupBy = (list, attr) => {

export default ApplicationSerializer.extend({
embed: true,
include: ['writeAllocs', 'readAllocs'],
include: ['writeAllocs', 'readAllocs', 'allocations'],

serialize() {
var json = ApplicationSerializer.prototype.serialize.apply(this, arguments);
Expand Down
2 changes: 2 additions & 0 deletions ui/tests/acceptance/volume-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ import VolumeDetail from 'nomad-ui/tests/pages/storage/volumes/detail';

const assignWriteAlloc = (volume, alloc) => {
volume.writeAllocs.add(alloc);
volume.allocations.add(alloc);
volume.save();
};

const assignReadAlloc = (volume, alloc) => {
volume.readAllocs.add(alloc);
volume.allocations.add(alloc);
volume.save();
};

Expand Down
2 changes: 2 additions & 0 deletions ui/tests/acceptance/volumes-list-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ import Layout from 'nomad-ui/tests/pages/layout';

const assignWriteAlloc = (volume, alloc) => {
volume.writeAllocs.add(alloc);
volume.allocations.add(alloc);
volume.save();
};

const assignReadAlloc = (volume, alloc) => {
volume.readAllocs.add(alloc);
volume.allocations.add(alloc);
volume.save();
};

Expand Down
20 changes: 14 additions & 6 deletions ui/tests/unit/serializers/volume-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,30 +173,38 @@ module('Unit | Serializer | Volume', function(hooks) {
NodesExpected: 2,
CreateIndex: 1,
ModifyIndex: 38,
WriteAllocs: {
'alloc-id-1': {
Allocations: [
{
ID: 'alloc-id-1',
TaskGroup: 'foobar',
CreateTime: +REF_DATE * 1000000,
ModifyTime: +REF_DATE * 1000000,
JobID: 'the-job',
Namespace: 'namespace-2',
},
'alloc-id-2': {
{
ID: 'alloc-id-2',
TaskGroup: 'write-here',
CreateTime: +REF_DATE * 1000000,
ModifyTime: +REF_DATE * 1000000,
JobID: 'the-job',
Namespace: 'namespace-2',
},
},
ReadAllocs: {
'alloc-id-3': {
{
ID: 'alloc-id-3',
TaskGroup: 'look-if-you-must',
CreateTime: +REF_DATE * 1000000,
ModifyTime: +REF_DATE * 1000000,
JobID: 'the-job',
Namespace: 'namespace-2',
},
],
WriteAllocs: {
'alloc-id-1': null,
'alloc-id-2': null,
},
ReadAllocs: {
'alloc-id-3': null,
},
},
out: {
Expand Down
30 changes: 9 additions & 21 deletions vendor/github.com/hashicorp/nomad/api/csi.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions website/pages/api-docs/volumes.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ $ curl \
"ModifyTime": 1495747371794276400
}
],
"ReadAllocs": {
"a8198d79-cfdb-6593-a999-1e9adabcba2e": null
},
"WriteAllocs": {},
"Schedulable": true,
"PluginID": "plugin-id1",
"Provider": "ebs",
Expand Down