From 701da2e3b2ac427b7b5f1f9d4b452a432b4c377d Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 17 Nov 2020 10:10:55 -0500 Subject: [PATCH 1/2] csi/api: populate ReadAllocs/WriteAllocs fields 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. --- api/csi.go | 30 ++++++-------------- command/agent/csi_endpoint.go | 25 +++++++++++++--- ui/app/serializers/volume.js | 11 ++++--- ui/mirage/models/csi-volume.js | 1 + ui/mirage/serializers/csi-volume.js | 2 +- ui/tests/acceptance/volume-detail-test.js | 2 ++ ui/tests/acceptance/volumes-list-test.js | 2 ++ ui/tests/unit/serializers/volume-test.js | 20 +++++++++---- vendor/github.com/hashicorp/nomad/api/csi.go | 30 ++++++-------------- website/pages/api-docs/volumes.mdx | 4 +++ 10 files changed, 68 insertions(+), 59 deletions(-) diff --git a/api/csi.go b/api/csi.go index 3c657f557e90..66ce2731d880 100644 --- a/api/csi.go +++ b/api/csi.go @@ -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 } @@ -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 - // 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 @@ -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 { diff --git a/command/agent/csi_endpoint.go b/command/agent/csi_endpoint.go index 028a5cceed59..15d09893586a 100644 --- a/command/agent/csi_endpoint.go +++ b/command/agent/csi_endpoint.go @@ -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, @@ -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, @@ -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) + } } } diff --git a/ui/app/serializers/volume.js b/ui/app/serializers/volume.js index 2b6e055f1492..8f2acad3b0e8 100644 --- a/ui/app/serializers/volume.js +++ b/ui/app/serializers/volume.js @@ -23,17 +23,16 @@ 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; + const bindIDToAlloc = allocList => id => { + const alloc = allocList.find(a => a.ID === id); return alloc; }; - 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)); const normalizedHash = super.normalize(typeHash, hash); return this.extractEmbeddedRecords(this, this.store, typeHash, normalizedHash); diff --git a/ui/mirage/models/csi-volume.js b/ui/mirage/models/csi-volume.js index 256bb03c682f..0df3de4eaf08 100644 --- a/ui/mirage/models/csi-volume.js +++ b/ui/mirage/models/csi-volume.js @@ -4,4 +4,5 @@ export default Model.extend({ plugin: belongsTo('csi-plugin'), writeAllocs: hasMany('allocation'), readAllocs: hasMany('allocation'), + allocations: hasMany('allocation'), }); diff --git a/ui/mirage/serializers/csi-volume.js b/ui/mirage/serializers/csi-volume.js index 9cc22c10d82a..2785a4bff513 100644 --- a/ui/mirage/serializers/csi-volume.js +++ b/ui/mirage/serializers/csi-volume.js @@ -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); diff --git a/ui/tests/acceptance/volume-detail-test.js b/ui/tests/acceptance/volume-detail-test.js index c277dc4bba98..b414d4a475d7 100644 --- a/ui/tests/acceptance/volume-detail-test.js +++ b/ui/tests/acceptance/volume-detail-test.js @@ -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(); }; diff --git a/ui/tests/acceptance/volumes-list-test.js b/ui/tests/acceptance/volumes-list-test.js index 7adf77364740..5ef601da3b8d 100644 --- a/ui/tests/acceptance/volumes-list-test.js +++ b/ui/tests/acceptance/volumes-list-test.js @@ -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(); }; diff --git a/ui/tests/unit/serializers/volume-test.js b/ui/tests/unit/serializers/volume-test.js index c98999932343..72822cf4950b 100644 --- a/ui/tests/unit/serializers/volume-test.js +++ b/ui/tests/unit/serializers/volume-test.js @@ -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: { diff --git a/vendor/github.com/hashicorp/nomad/api/csi.go b/vendor/github.com/hashicorp/nomad/api/csi.go index 3c657f557e90..66ce2731d880 100644 --- a/vendor/github.com/hashicorp/nomad/api/csi.go +++ b/vendor/github.com/hashicorp/nomad/api/csi.go @@ -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 } @@ -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 - // 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 @@ -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 { diff --git a/website/pages/api-docs/volumes.mdx b/website/pages/api-docs/volumes.mdx index a56d6d84a143..204ac135bf1d 100644 --- a/website/pages/api-docs/volumes.mdx +++ b/website/pages/api-docs/volumes.mdx @@ -230,6 +230,10 @@ $ curl \ "ModifyTime": 1495747371794276400 } ], + "ReadAllocs": { + "a8198d79-cfdb-6593-a999-1e9adabcba2e": null + }, + "WriteAllocs": {}, "Schedulable": true, "PluginID": "plugin-id1", "Provider": "ebs", From efdce6859e9ded2b7c652263c22195621e57ad6f Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 25 Nov 2020 15:47:33 -0500 Subject: [PATCH 2/2] fix accidentally quadratic behavior --- ui/app/serializers/volume.js | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/ui/app/serializers/volume.js b/ui/app/serializers/volume.js index 8f2acad3b0e8..c3aa3a57dc43 100644 --- a/ui/app/serializers/volume.js +++ b/ui/app/serializers/volume.js @@ -26,13 +26,22 @@ export default class VolumeSerializer extends ApplicationSerializer { // Populate read/write allocation lists from aggregate allocation list const readAllocs = hash.ReadAllocs || {}; const writeAllocs = hash.WriteAllocs || {}; - const bindIDToAlloc = allocList => id => { - const alloc = allocList.find(a => a.ID === id); - return alloc; - }; - hash.ReadAllocations = Object.keys(readAllocs).map(bindIDToAlloc(hash.Allocations)); - hash.WriteAllocations = Object.keys(writeAllocs).map(bindIDToAlloc(hash.Allocations)); + 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);