dynamic host volumes: client-side tests, comments, tidying (#24747)

client/hostvolumemanager/host_volume_plugin.go

@@ -20,17 +20,23 @@ import (
 	"github.com/hashicorp/nomad/helper"
 )
 
-type PluginFingerprint struct {
-	Version *version.Version `json:"version"`
-}
-
+// HostVolumePlugin manages the lifecycle of volumes.
 type HostVolumePlugin interface {
 	Fingerprint(ctx context.Context) (*PluginFingerprint, error)
 	Create(ctx context.Context, req *cstructs.ClientHostVolumeCreateRequest) (*HostVolumePluginCreateResponse, error)
 	Delete(ctx context.Context, req *cstructs.ClientHostVolumeDeleteRequest) error
-	// db TODO(1.10.0): update? resize? ??
 }
 
+// PluginFingerprint gets set on the node for volume scheduling.
+// Plugins are expected to respond to 'fingerprint' calls with json that
+// unmarshals to this struct.
+type PluginFingerprint struct {
+	Version *version.Version `json:"version"`
+}
+
+// HostVolumePluginCreateResponse gets stored on the volume in server state.
+// Plugins are expected to respond to 'create' calls with json that
+// unmarshals to this struct.
 type HostVolumePluginCreateResponse struct {
 	Path      string `json:"path"`
 	SizeBytes int64  `json:"bytes"`
@@ -41,6 +47,8 @@ const HostVolumePluginMkdirVersion = "0.0.1"
 
 var _ HostVolumePlugin = &HostVolumePluginMkdir{}
 
+// HostVolumePluginMkdir is a plugin that creates a directory within the
+// specified TargetPath. It is built-in to Nomad, so is always available.
 type HostVolumePluginMkdir struct {
 	ID         string
 	TargetPath string
@@ -66,7 +74,8 @@ func (p *HostVolumePluginMkdir) Create(_ context.Context,
 	log.Debug("running plugin")
 
 	resp := &HostVolumePluginCreateResponse{
-		Path:      path,
+		Path: path,
+		// "mkdir" volumes, being simple directories, have unrestricted size
 		SizeBytes: 0,
 	}
 
@@ -109,6 +118,8 @@ func (p *HostVolumePluginMkdir) Delete(_ context.Context, req *cstructs.ClientHo
 
 var _ HostVolumePlugin = &HostVolumePluginExternal{}
 
+// NewHostVolumePluginExternal returns an external host volume plugin
+// if the specified executable exists on disk.
 func NewHostVolumePluginExternal(log hclog.Logger,
 	id, executable, targetPath string) (*HostVolumePluginExternal, error) {
 	// this should only be called with already-detected executables,
@@ -132,6 +143,10 @@ func NewHostVolumePluginExternal(log hclog.Logger,
 	}, nil
 }
 
+// HostVolumePluginExternal calls an executable on disk. All operations
+// *must* be idempotent, and safe to be called concurrently per volume.
+// For each call, the executable's stdout and stderr may be logged, so plugin
+// authors should not include any sensitive information in their plugin outputs.
 type HostVolumePluginExternal struct {
 	ID         string
 	Executable string
@@ -140,6 +155,15 @@ type HostVolumePluginExternal struct {
 	log hclog.Logger
 }
 
+// Fingerprint calls the executable with the following parameters:
+// arguments: fingerprint
+// environment:
+// OPERATION=fingerprint
+//
+// Response should be valid JSON on stdout, with a "version" key, e.g.:
+// {"version": "0.0.1"}
+// The version value should be a valid version number as allowed by
+// version.NewVersion()
 func (p *HostVolumePluginExternal) Fingerprint(ctx context.Context) (*PluginFingerprint, error) {
 	cmd := exec.CommandContext(ctx, p.Executable, "fingerprint")
 	cmd.Env = []string{"OPERATION=fingerprint"}
@@ -159,12 +183,28 @@ func (p *HostVolumePluginExternal) Fingerprint(ctx context.Context) (*PluginFing
 	return fprint, nil
 }
 
+// Create calls the executable with the following parameters:
+// arguments: create {path to create}
+// environment:
+// OPERATION=create
+// HOST_PATH={path to create}
+// NODE_ID={Nomad node ID}
+// VOLUME_NAME={name from the volume specification}
+// CAPACITY_MIN_BYTES={capacity_min from the volume spec}
+// CAPACITY_MAX_BYTES={capacity_max from the volume spec}
+// PARAMETERS={json of parameters from the volume spec}
+//
+// Response should be valid JSON on stdout with "path" and "bytes", e.g.:
+// {"path": $HOST_PATH, "bytes": 50000000}
+// "path" must be provided to confirm that the requested path is what was
+// created by the plugin. "bytes" is the actual size of the volume created
+// by the plugin; if excluded, it will default to 0.
 func (p *HostVolumePluginExternal) Create(ctx context.Context,
 	req *cstructs.ClientHostVolumeCreateRequest) (*HostVolumePluginCreateResponse, error) {
 
-	params, err := json.Marshal(req.Parameters) // db TODO(1.10.0): document if this is nil, then PARAMETERS env will be "null"
+	params, err := json.Marshal(req.Parameters)
 	if err != nil {
-		// this is a proper error, because users can set this in the volume spec
+		// should never happen; req.Parameters is a simple map[string]string
 		return nil, fmt.Errorf("error marshaling volume pramaters: %w", err)
 	}
 	envVars := []string{
@@ -181,22 +221,37 @@ func (p *HostVolumePluginExternal) Create(ctx context.Context,
 	}
 
 	var pluginResp HostVolumePluginCreateResponse
-	err = json.Unmarshal(stdout, &pluginResp) // db TODO(1.10.0): if this fails, then the volume may have been created, according to the plugin, but Nomad will not save it
+	err = json.Unmarshal(stdout, &pluginResp)
 	if err != nil {
+		// note: if a plugin does not return valid json, a volume may be
+		// created without any respective state in Nomad, since we return
+		// an error here after the plugin has done who-knows-what.
 		return nil, err
 	}
 	return &pluginResp, nil
 }
 
+// Delete calls the executable with the following parameters:
+// arguments: delete {path to create}
+// environment:
+// OPERATION=delete
+// HOST_PATH={path to create}
+// NODE_ID={Nomad node ID}
+// VOLUME_NAME={name from the volume specification}
+// PARAMETERS={json of parameters from the volume spec}
+//
+// Response on stdout is discarded.
 func (p *HostVolumePluginExternal) Delete(ctx context.Context,
 	req *cstructs.ClientHostVolumeDeleteRequest) error {
 
 	params, err := json.Marshal(req.Parameters)
 	if err != nil {
+		// should never happen; req.Parameters is a simple map[string]string
 		return fmt.Errorf("error marshaling volume pramaters: %w", err)
 	}
 	envVars := []string{
 		"NODE_ID=" + req.NodeID,
+		"VOLUME_NAME=" + req.Name,
 		"PARAMETERS=" + string(params),
 	}
 
@@ -207,6 +262,9 @@ func (p *HostVolumePluginExternal) Delete(ctx context.Context,
 	return nil
 }
 
+// runPlugin executes the... executable with these additional env vars:
+// OPERATION={op}
+// HOST_PATH={p.TargetPath/volID}
 func (p *HostVolumePluginExternal) runPlugin(ctx context.Context,
 	op, volID string, env []string) (stdout, stderr []byte, err error) {
 
@@ -239,6 +297,7 @@ func (p *HostVolumePluginExternal) runPlugin(ctx context.Context,
 	return stdout, stderr, nil
 }
 
+// runCommand executes the provided Cmd and captures stdout and stderr.
 func runCommand(cmd *exec.Cmd) (stdout, stderr []byte, err error) {
 	var errBuf bytes.Buffer
 	cmd.Stderr = io.Writer(&errBuf)

client/hostvolumemanager/host_volume_plugin_test.go

@@ -4,19 +4,13 @@
 package hostvolumemanager
 
 import (
-	"bytes"
-	"context"
-	"io"
 	"path/filepath"
 	"runtime"
 	"testing"
-	"time"
 
-	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-version"
 	cstructs "github.com/hashicorp/nomad/client/structs"
 	"github.com/hashicorp/nomad/helper/testlog"
-	"github.com/shoenig/test"
 	"github.com/shoenig/test/must"
 )
 
@@ -37,23 +31,29 @@ func TestHostVolumePluginMkdir(t *testing.T) {
 	must.NoError(t, err)
 
 	t.Run("happy", func(t *testing.T) {
-		resp, err := plug.Create(timeout(t),
-			&cstructs.ClientHostVolumeCreateRequest{
-				ID: volID, // minimum required by this plugin
-			})
-		must.NoError(t, err)
-		must.Eq(t, &HostVolumePluginCreateResponse{
-			Path:      target,
-			SizeBytes: 0,
-		}, resp)
-		must.DirExists(t, target)
+		// run multiple times, should be idempotent
+		for range 2 {
+			resp, err := plug.Create(timeout(t),
+				&cstructs.ClientHostVolumeCreateRequest{
+					ID: volID, // minimum required by this plugin
+				})
+			must.NoError(t, err)
+			must.Eq(t, &HostVolumePluginCreateResponse{
+				Path:      target,
+				SizeBytes: 0,
+			}, resp)
+			must.DirExists(t, target)
+		}
 
-		err = plug.Delete(timeout(t),
-			&cstructs.ClientHostVolumeDeleteRequest{
-				ID: volID,
-			})
-		must.NoError(t, err)
-		must.DirNotExists(t, target)
+		// delete should be idempotent, too
+		for range 2 {
+			err = plug.Delete(timeout(t),
+				&cstructs.ClientHostVolumeDeleteRequest{
+					ID: volID,
+				})
+			must.NoError(t, err)
+			must.DirNotExists(t, target)
+		}
 	})
 
 	t.Run("sad", func(t *testing.T) {
@@ -75,6 +75,31 @@ func TestHostVolumePluginMkdir(t *testing.T) {
 	})
 }
 
+func TestNewHostVolumePluginExternal(t *testing.T) {
+	log := testlog.HCLogger(t)
+	var err error
+
+	_, err = NewHostVolumePluginExternal(log, "test-id", "non-existent", "target")
+	must.ErrorIs(t, err, ErrPluginNotExists)
+
+	_, err = NewHostVolumePluginExternal(log, "test-id", "host_volume_plugin_test.go", "target")
+	must.ErrorIs(t, err, ErrPluginNotExecutable)
+
+	t.Run("unix", func(t *testing.T) {
+		if runtime.GOOS == "windows" {
+			t.Skip("skipped because windows") // db TODO(1.10.0)
+		}
+		p, err := NewHostVolumePluginExternal(log, "test-id", "./test_fixtures/test_plugin.sh", "test-target")
+		must.NoError(t, err)
+		must.Eq(t, &HostVolumePluginExternal{
+			ID:         "test-id",
+			Executable: "./test_fixtures/test_plugin.sh",
+			TargetPath: "test-target",
+			log:        log,
+		}, p)
+	})
+}
+
 func TestHostVolumePluginExternal(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("skipped because windows") // db TODO(1.10.0)
@@ -187,29 +212,3 @@ func TestHostVolumePluginExternal(t *testing.T) {
 		must.StrContains(t, logged, "delete: it tells you all about it in stderr")
 	})
 }
-
-// timeout provides a context that times out in 1 second
-func timeout(t *testing.T) context.Context {
-	t.Helper()
-	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
-	t.Cleanup(cancel)
-	return ctx
-}
-
-// logRecorder is here so we can assert that stdout/stderr appear in logs
-func logRecorder(t *testing.T) (hclog.Logger, func() string) {
-	t.Helper()
-	buf := &bytes.Buffer{}
-	logger := hclog.New(&hclog.LoggerOptions{
-		Name:            "log-recorder",
-		Output:          buf,
-		Level:           hclog.Debug,
-		IncludeLocation: true,
-		DisableTime:     true,
-	})
-	return logger, func() string {
-		bts, err := io.ReadAll(buf)
-		test.NoError(t, err)
-		return string(bts)
-	}
-}