warn if a static vol is added after a dynamic one

with the same name -- the static vol is discarded,
dynamic takes precedence, and log to warn the user

client/client.go

@@ -408,6 +408,7 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulProxie
 	}
 
 	c.batchNodeUpdates = newBatchNodeUpdates(
+		c.logger,
 		c.updateNodeFromDriver,
 		c.updateNodeFromDevices,
 		c.updateNodeFromCSI,

client/hostvolumemanager/host_volumes_test.go

@@ -27,7 +27,7 @@ func TestHostVolumeManager(t *testing.T) {
 	tmp := t.TempDir()
 	errDB := &cstate.ErrDB{}
 	memDB := cstate.NewMemDB(log)
-	node := newFakeNode()
+	node := newFakeNode(t)
 
 	hvm := NewHostVolumeManager(log, Config{
 		PluginDir:      "./test_fixtures",
@@ -231,7 +231,7 @@ func TestHostVolumeManager_restoreFromState(t *testing.T) {
 			PluginID: "mkdir",
 		},
 	}
-	node := newFakeNode()
+	node := newFakeNode(t)
 
 	t.Run("no vols", func(t *testing.T) {
 		state := cstate.NewMemDB(log)
@@ -332,15 +332,17 @@ func TestHostVolumeManager_restoreFromState(t *testing.T) {
 
 type fakeNode struct {
 	vols VolumeMap
+	log  hclog.Logger
 }
 
 func (n *fakeNode) updateVol(name string, volume *structs.ClientHostVolumeConfig) {
-	UpdateVolumeMap(n.vols, name, volume)
+	UpdateVolumeMap(n.log, n.vols, name, volume)
 }
 
-func newFakeNode() *fakeNode {
+func newFakeNode(t *testing.T) *fakeNode {
 	return &fakeNode{
 		vols: make(VolumeMap),
+		log:  testlog.HCLogger(t),
 	}
 }
 

client/hostvolumemanager/volume_fingerprint.go

@@ -6,6 +6,7 @@ package hostvolumemanager
 import (
 	"context"
 
+	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/nomad/nomad/structs"
 )
 
@@ -24,14 +25,20 @@ type VolumeMap map[string]*structs.ClientHostVolumeConfig
 //
 // Since it may mutate the map, the caller should make a copy
 // or acquire a lock as appropriate for their context.
-func UpdateVolumeMap(volumes VolumeMap, name string, vol *structs.ClientHostVolumeConfig) (changed bool) {
+func UpdateVolumeMap(log hclog.Logger, volumes VolumeMap, name string, vol *structs.ClientHostVolumeConfig) (changed bool) {
 	current, exists := volumes[name]
 	if vol == nil {
 		if exists {
 			delete(volumes, name)
 			changed = true
 		}
 	} else {
+		// if the volume already exists with no ID, it will be because it was
+		// added to client agent config after having been previously created
+		// as a dynamic vol. dynamic takes precedence, but log a warning.
+		if exists && current.ID == "" {
+			log.Warn("overriding static host volume with dynamic", "name", name, "id", vol.ID)
+		}
 		if !exists || !vol.Equal(current) {
 			volumes[name] = vol
 			changed = true

client/hostvolumemanager/volume_fingerprint_test.go

@@ -23,6 +23,8 @@ func TestUpdateVolumeMap(t *testing.T) {
 
 		expectMap    VolumeMap
 		expectChange bool
+
+		expectLog string
 	}{
 		{
 			name:         "delete absent",
@@ -57,20 +59,33 @@ func TestUpdateVolumeMap(t *testing.T) {
 			expectChange: false,
 		},
 		{
-			// this should not happen, but test anyway
+			// this should not happen with dynamic vols, but test anyway
 			name:         "change present",
-			vols:         VolumeMap{"changeme": {Path: "before"}},
+			vols:         VolumeMap{"changeme": {ID: "before"}},
 			volName:      "changeme",
-			vol:          &structs.ClientHostVolumeConfig{Path: "after"},
-			expectMap:    VolumeMap{"changeme": {Path: "after"}},
+			vol:          &structs.ClientHostVolumeConfig{ID: "after"},
+			expectMap:    VolumeMap{"changeme": {ID: "after"}},
+			expectChange: true,
+		},
+		{
+			// this should only happen during agent start, if a static vol has
+			// been added to config after a previous dynamic vol was created
+			// with the same name.
+			name:         "override static",
+			vols:         VolumeMap{"overrideme": {ID: ""}}, // static vols have no ID
+			volName:      "overrideme",
+			vol:          &structs.ClientHostVolumeConfig{ID: "dynamic-vol-id"},
+			expectMap:    VolumeMap{"overrideme": {ID: "dynamic-vol-id"}},
 			expectChange: true,
+			expectLog:    "overriding static host volume with dynamic: name=overrideme id=dynamic-vol-id",
 		},
 	}
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
+			log, getLogs := logRecorder(t)
 
-			changed := UpdateVolumeMap(tc.vols, tc.volName, tc.vol)
+			changed := UpdateVolumeMap(log, tc.vols, tc.volName, tc.vol)
 			must.Eq(t, tc.expectMap, tc.vols)
 
 			if tc.expectChange {
@@ -79,6 +94,7 @@ func TestUpdateVolumeMap(t *testing.T) {
 				must.False(t, changed, must.Sprint("expect volume not to have been changed"))
 			}
 
+			must.StrContains(t, getLogs(), tc.expectLog)
 		})
 	}
 }
@@ -87,7 +103,7 @@ func TestWaitForFirstFingerprint(t *testing.T) {
 	log := testlog.HCLogger(t)
 	tmp := t.TempDir()
 	memDB := state.NewMemDB(log)
-	node := newFakeNode()
+	node := newFakeNode(t)
 	hvm := NewHostVolumeManager(log, Config{
 		PluginDir:      "",
 		SharedMountDir: tmp,

client/node_updater.go

@@ -9,6 +9,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/nomad/client/devicemanager"
 	hvm "github.com/hashicorp/nomad/client/hostvolumemanager"
 	"github.com/hashicorp/nomad/client/pluginmanager/csimanager"
@@ -50,7 +51,8 @@ SEND_BATCH:
 	// host volume updates
 	var hostVolChanged bool
 	c.batchNodeUpdates.batchHostVolumeUpdates(func(name string, vol *structs.ClientHostVolumeConfig) {
-		hostVolChanged = hvm.UpdateVolumeMap(newConfig.Node.HostVolumes, name, vol)
+		hostVolChanged = hvm.UpdateVolumeMap(c.logger.Named("node_updater").With("method", "batchFirstFingerprint"),
+			newConfig.Node.HostVolumes, name, vol)
 	})
 
 	// csi updates
@@ -140,7 +142,8 @@ func (c *Client) updateNodeFromHostVol(name string, vol *structs.ClientHostVolum
 		newConfig.Node.HostVolumes = make(map[string]*structs.ClientHostVolumeConfig)
 	}
 
-	changed := hvm.UpdateVolumeMap(newConfig.Node.HostVolumes, name, vol)
+	changed := hvm.UpdateVolumeMap(c.logger.Named("node_updater").With("method", "updateNodeFromHostVol"),
+		newConfig.Node.HostVolumes, name, vol)
 	if changed {
 		c.config = newConfig
 		c.updateNode()
@@ -342,6 +345,8 @@ func (c *Client) updateNodeFromDevicesLocked(devices []*structs.NodeDeviceResour
 // Once ready, the batches can be flushed and toggled to stop batching and forward
 // all updates to a configured callback to be performed incrementally
 type batchNodeUpdates struct {
+	logger hclog.Logger
+
 	// access to driver fields must hold driversMu lock
 	drivers        map[string]*structs.DriverInfo
 	driversBatched bool
@@ -368,12 +373,14 @@ type batchNodeUpdates struct {
 }
 
 func newBatchNodeUpdates(
+	logger hclog.Logger,
 	driverCB drivermanager.UpdateNodeDriverInfoFn,
 	devicesCB devicemanager.UpdateNodeDevicesFn,
 	csiCB csimanager.UpdateNodeCSIInfoFunc,
 	hostVolumeCB hvm.HostVolumeNodeUpdater) *batchNodeUpdates {
 
 	return &batchNodeUpdates{
+		logger:               logger,
 		drivers:              make(map[string]*structs.DriverInfo),
 		driverCB:             driverCB,
 		devices:              []*structs.NodeDeviceResource{},
@@ -394,7 +401,8 @@ func (b *batchNodeUpdates) updateNodeFromHostVolume(name string, vol *structs.Cl
 		b.hostVolumeCB(name, vol) // => Client.updateNodeFromHostVol()
 		return
 	}
-	hvm.UpdateVolumeMap(b.hostVolumes, name, vol)
+	hvm.UpdateVolumeMap(b.logger.Named("node_updater").With("method", "updateNodeFromHostVolume"),
+		b.hostVolumes, name, vol)
 }
 
 // this one runs on client start