Skip to content

Commit

Permalink
csi: allow more than 1 writer claim for multi-writer mode (#9040)
Browse files Browse the repository at this point in the history
Fixes a bug where CSI volumes with the `MULTI_NODE_MULTI_WRITER` access mode
were using the same logic as `MULTI_NODE_SINGLE_WRITER` to determine whether
the volume had writer claims available for scheduling.

Extends CSI claim endpoint test to exercise multi-reader and make sure `WriteFreeClaims`
is exercised for multi-writer in feasibility test.
  • Loading branch information
tgross committed Oct 7, 2020
1 parent 98a7834 commit 7cff2de
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ BUG FIXES:

* core: Fixed a bug where blocking queries would not include the query's maximum wait time when calculating whether it was safe to retry. [[GH-8921](https://github.com/hashicorp/nomad/issues/8921)]
* consul: Fixed a bug to correctly validate task when using script-checks in group-level services [[GH-8952](https://github.com/hashicorp/nomad/issues/8952)]
* csi: Fixed a bug where multi-writer volumes were allowed only 1 write claim. [[GH-9040](https://github.com/hashicorp/nomad/issues/9040)]

## 0.12.5 (September 17, 2020)

Expand Down
19 changes: 19 additions & 0 deletions nomad/csi_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,25 @@ func TestCSIVolumeEndpoint_Claim(t *testing.T) {
require.Equal(t, id0, volGetResp.Volume.ID)
require.Len(t, volGetResp.Volume.ReadAllocs, 1)
require.Len(t, volGetResp.Volume.WriteAllocs, 1)

// Make a second reader claim
alloc3 := mock.Alloc()
alloc3.JobID = uuid.Generate()
summary = mock.JobSummary(alloc3.JobID)
index++
require.NoError(t, state.UpsertJobSummary(index, summary))
index++
require.NoError(t, state.UpsertAllocs(index, []*structs.Allocation{alloc3}))
claimReq.AllocationID = alloc3.ID
err = msgpackrpc.CallWithCodec(codec, "CSIVolume.Claim", claimReq, claimResp)
require.NoError(t, err)

// Verify the new claim was set
err = msgpackrpc.CallWithCodec(codec, "CSIVolume.Get", volGetReq, volGetResp)
require.NoError(t, err)
require.Equal(t, id0, volGetResp.Volume.ID)
require.Len(t, volGetResp.Volume.ReadAllocs, 2)
require.Len(t, volGetResp.Volume.WriteAllocs, 1)
}

// TestCSIVolumeEndpoint_ClaimWithController exercises the VolumeClaim RPC
Expand Down
7 changes: 6 additions & 1 deletion nomad/structs/csi.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,13 @@ func (v *CSIVolume) WriteSchedulable() bool {
// WriteFreeClaims determines if there are any free write claims available
func (v *CSIVolume) WriteFreeClaims() bool {
switch v.AccessMode {
case CSIVolumeAccessModeSingleNodeWriter, CSIVolumeAccessModeMultiNodeSingleWriter, CSIVolumeAccessModeMultiNodeMultiWriter:
case CSIVolumeAccessModeSingleNodeWriter, CSIVolumeAccessModeMultiNodeSingleWriter:
return len(v.WriteAllocs) == 0
case CSIVolumeAccessModeMultiNodeMultiWriter:
// the CSI spec doesn't allow for setting a max number of writers.
// we track node resource exhaustion through v.ResourceExhausted
// which is checked in WriteSchedulable
return true
default:
return false
}
Expand Down
5 changes: 5 additions & 0 deletions nomad/structs/csi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ func TestCSIVolumeClaim(t *testing.T) {
vol.ClaimRelease(claim)
require.True(t, vol.ReadSchedulable())
require.True(t, vol.WriteFreeClaims())

vol.AccessMode = CSIVolumeAccessModeMultiNodeMultiWriter
require.NoError(t, vol.ClaimWrite(claim, alloc))
require.NoError(t, vol.ClaimWrite(claim, alloc))
require.True(t, vol.WriteFreeClaims())
}

func TestCSIPluginJobs(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion scheduler/feasible_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ func TestCSIVolumeChecker(t *testing.T) {
vol := structs.NewCSIVolume(vid, index)
vol.PluginID = "foo"
vol.Namespace = structs.DefaultNamespace
vol.AccessMode = structs.CSIVolumeAccessModeMultiNodeSingleWriter
vol.AccessMode = structs.CSIVolumeAccessModeMultiNodeMultiWriter
vol.AttachmentMode = structs.CSIVolumeAttachmentModeFilesystem
err := state.CSIVolumeRegister(index, []*structs.CSIVolume{vol})
require.NoError(t, err)
Expand Down

1 comment on commit 7cff2de

@vercel
Copy link

@vercel vercel bot commented on 7cff2de Oct 7, 2020

Choose a reason for hiding this comment

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

Please sign in to comment.