more comments, mostly docstrings

client/hostvolumemanager/host_volume_plugin.go

@@ -20,16 +20,23 @@ import (
 	"github.com/hashicorp/nomad/helper"
 )
 
+// 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
 }
 
+// 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"`
@@ -40,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
@@ -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 should
+// be idempotent, and safe to be called concurrently per volume ID.
+// 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,6 +183,22 @@ 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) {
 
@@ -191,6 +231,16 @@ func (p *HostVolumePluginExternal) Create(ctx context.Context,
 	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 {
 
@@ -201,6 +251,7 @@ func (p *HostVolumePluginExternal) Delete(ctx context.Context,
 	}
 	envVars := []string{
 		"NODE_ID=" + req.NodeID,
+		"VOLUME_NAME=" + req.Name,
 		"PARAMETERS=" + string(params),
 	}
 
@@ -211,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) {
 
@@ -243,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_volumes.go

@@ -21,12 +21,14 @@ var (
 	ErrPluginNotExecutable = errors.New("plugin not executable")
 )
 
+// HostVolumeStateManager manages the lifecycle of volumes in client state.
 type HostVolumeStateManager interface {
 	PutDynamicHostVolume(*cstructs.HostVolumeState) error
 	GetDynamicHostVolumes() ([]*cstructs.HostVolumeState, error)
 	DeleteDynamicHostVolume(string) error
 }
 
+// Config is used to configure a HostVolumeManager.
 type Config struct {
 	// PluginDir is where external plugins may be found.
 	PluginDir string
@@ -43,6 +45,8 @@ type Config struct {
 	UpdateNodeVols HostVolumeNodeUpdater
 }
 
+// HostVolumeManager executes plugins, manages volume metadata in client state,
+// and registers volumes with the client node.
 type HostVolumeManager struct {
 	pluginDir      string
 	sharedMountDir string
@@ -52,6 +56,7 @@ type HostVolumeManager struct {
 	log            hclog.Logger
 }
 
+// NewHostVolumeManager includes default builtin plugins.
 func NewHostVolumeManager(logger hclog.Logger, config Config) *HostVolumeManager {
 	return &HostVolumeManager{
 		pluginDir:      config.PluginDir,
@@ -69,6 +74,8 @@ func NewHostVolumeManager(logger hclog.Logger, config Config) *HostVolumeManager
 	}
 }
 
+// Create runs the appropriate plugin for the given request, saves the request
+// to state, and updates the node with the volume.
 func (hvm *HostVolumeManager) Create(ctx context.Context,
 	req *cstructs.ClientHostVolumeCreateRequest) (*cstructs.ClientHostVolumeCreateResponse, error) {
 
@@ -116,6 +123,8 @@ func (hvm *HostVolumeManager) Create(ctx context.Context,
 	return resp, nil
 }
 
+// Delete runs the appropriate plugin for the given request, removes it from
+// state, and updates the node to remove the volume.
 func (hvm *HostVolumeManager) Delete(ctx context.Context,
 	req *cstructs.ClientHostVolumeDeleteRequest) (*cstructs.ClientHostVolumeDeleteResponse, error) {
 
@@ -144,6 +153,7 @@ func (hvm *HostVolumeManager) Delete(ctx context.Context,
 	return resp, nil
 }
 
+// getPlugin finds either a built-in plugin or an external plugin.
 func (hvm *HostVolumeManager) getPlugin(id string) (HostVolumePlugin, error) {
 	if plug, ok := hvm.builtIns[id]; ok {
 		return plug, nil
@@ -153,6 +163,8 @@ func (hvm *HostVolumeManager) getPlugin(id string) (HostVolumePlugin, error) {
 	return NewHostVolumePluginExternal(log, id, path, hvm.sharedMountDir)
 }
 
+// restoreFromState loads all volumes from client state and runs Create for
+// each one, so volumes are restored upon agent restart or host reboot.
 func (hvm *HostVolumeManager) restoreFromState(ctx context.Context) (VolumeMap, error) {
 	vols, err := hvm.stateMgr.GetDynamicHostVolumes()
 	if err != nil {
@@ -194,6 +206,8 @@ func (hvm *HostVolumeManager) restoreFromState(ctx context.Context) (VolumeMap,
 	return volumes, helper.FlattenMultierror(mErr.ErrorOrNil())
 }
 
+// genVolConfig generates the host volume config for the node to report as
+// available to the servers for job scheduling.
 func genVolConfig(req *cstructs.ClientHostVolumeCreateRequest, resp *HostVolumePluginCreateResponse) *structs.ClientHostVolumeConfig {
 	return &structs.ClientHostVolumeConfig{
 		Name: req.Name,

client/hostvolumemanager/host_volumes_test.go

@@ -181,7 +181,7 @@ func (p *fakePlugin) Delete(_ context.Context, req *cstructs.ClientHostVolumeDel
 	return nil
 }
 
-func TestHostVolumeManager_restoreState(t *testing.T) {
+func TestHostVolumeManager_restoreFromState(t *testing.T) {
 	log := testlog.HCLogger(t)
 	vol := &cstructs.HostVolumeState{
 		ID: "test-vol-id",
@@ -278,8 +278,10 @@ func TestHostVolumeManager_restoreState(t *testing.T) {
 		hvm.builtIns["test-plugin"] = plug
 
 		vols, err := hvm.restoreFromState(timeout(t))
+		// error during restore should not halt the whole client
 		must.NoError(t, err)
 		must.NotNil(t, vols)
+		// but it should log
 		logs := getLogs()
 		must.StrContains(t, logs, "[ERROR]")
 		must.StrContains(t, logs, `failed to restore: plugin_id=test-plugin volume_id=test-volume error="sad create"`)

client/hostvolumemanager/volume_fingerprint.go

@@ -41,6 +41,7 @@ func UpdateVolumeMap(volumes VolumeMap, name string, vol *structs.ClientHostVolu
 }
 
 // WaitForFirstFingerprint implements client.FingerprintingPluginManager
+// so any existing volumes are added to the client node on agent start.
 func (hvm *HostVolumeManager) WaitForFirstFingerprint(ctx context.Context) <-chan struct{} {
 	// the fingerprint manager puts batchFirstFingerprintsTimeout (50 seconds)
 	// on the context that it sends to us here so we don't need another

command/agent/config.go

@@ -230,7 +230,6 @@ type ClientConfig struct {
 	AllocMountsDir string `hcl:"alloc_mounts_dir"`
 
 	// HostVolumePluginDir directory contains dynamic host volume plugins
-	// db TODO(1.10.0): document default directory is alongside alloc_mounts
 	HostVolumePluginDir string `hcl:"host_volume_plugin_dir"`
 
 	// Servers is a list of known server addresses. These are as "host:port"

demo/hostvolume/_test-plugin.sh

@@ -56,7 +56,6 @@ case $op in
     export CAPACITY_MAX_BYTES=50000000 # 50mb
     export CAPACITY_MIN_BYTES=50000000 # 50mb
     export PARAMETERS='{"a": "ayy"}'
-    # db TODO(1.10.0): check stdout
     ;;
 
   delete)

demo/hostvolume/example-plugin-mkfs

@@ -2,8 +2,6 @@
 # Copyright (c) HashiCorp, Inc.
 # SPDX-License-Identifier: BUSL-1.1
 
-# db TODO(1.10.0): where does PATH come from here? somewhere implicit? /sbin/ and /bin/ and ...?
-
 set -euo pipefail
 
 version='0.0.1'