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.

Sign up for GitHub

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: don't overwrite context with empty value from request #24922

Merged
merged 1 commit into from
Jan 23, 2025
Merged

CSI: don't overwrite context with empty value from request #24922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions .changelog/24922.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
csi: Fixed a bug where volume context from the plugin would be erased on volume updates
```
10 changes: 7 additions & 3 deletions nomad/structs/csi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,9 +846,13 @@ func (v *CSIVolume) Merge(other *CSIVolume) error {
"volume parameters cannot be updated"))
}

// Context is mutable and will be used during controller
// validation
v.Context = other.Context
// Context is mutable and will be used during controller validation, but we
// need to ensure we don't remove context that's been previously stored
// server-side if the user has submitted an update without adding it to the
// spec manually (which we should not require)
if len(other.Context) != 0 {
v.Context = other.Context
}
Comment on lines +853 to +855
Copy link
Member

Choose a reason for hiding this comment

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

this makes good sense for create, where the controller plugin generates the context dynamically, but I wonder about this edge case:

what if a user registers a volume with some context, but later wants to remove the context entirely? is that even a plausible concern? I suppose one could deregister the volume then re-register it with no context, since the actual volume out in the world would be unaffected.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only time that would happen is if they updated the plugin and the new version of the plugin requires no context. In that case, you're almost certainly better off deregistering and re-registering

return errs.ErrorOrNil()
}

Expand Down
15 changes: 15 additions & 0 deletions nomad/structs/csi_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,13 @@ func TestCSIVolume_Merge(t *testing.T) {
{Segments: map[string]string{"rack": "R2"}},
},
},
Context: map[string]string{
// a typical context for democratic-csi
"provisioner_driver": "nfs-client",
"node_attach_driver": "nfs",
"server": "192.168.1.170",
"share": "/srv/nfs_data/v/csi-volume-nfs",
},
},
update: &CSIVolume{
Topologies: []*CSITopology{
Expand Down Expand Up @@ -745,6 +752,14 @@ func TestCSIVolume_Merge(t *testing.T) {
test.Sprint("should add 2 requested capabilities"))
test.Eq(t, []string{"noatime", "another"}, v.MountOptions.MountFlags,
test.Sprint("should add mount flag"))
test.Eq(t, map[string]string{
"provisioner_driver": "nfs-client",
"node_attach_driver": "nfs",
"server": "192.168.1.170",
"share": "/srv/nfs_data/v/csi-volume-nfs",
}, v.Context,
test.Sprint("context should not be overwritten with empty update"))

},
},
}
Expand Down
Loading