From 1ffb7ab3fb0dffb0e530fd3a8a411c7ad8c72a6a Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 11 Mar 2025 11:06:57 -0400 Subject: [PATCH] dynamic host volumes: allow plugins to return an error message (#25341) Errors from `volume create` or `volume delete` only get logged by the client agent, which may make it harder for volume authors to debug these tasks if they are not also the cluster administrator with access to host logs. Allow plugins to include an optional error message in their response. Because we can't count on receiving this response (the error could come before the plugin executes), we parse this message optimistically and include it only if available. Ref: https://hashicorp.atlassian.net/browse/NET-12087 --- api/host_volumes.go | 11 +++-- .../hostvolumemanager/host_volume_plugin.go | 42 +++++++++++++++---- .../host_volume_plugin_test.go | 4 +- .../test_fixtures/test_plugin_sad.sh | 2 +- command/agent/host_volume_endpoint.go | 2 +- command/agent/host_volume_endpoint_test.go | 3 +- command/volume_delete.go | 2 +- .../concepts/plugins/storage/host-volumes.mdx | 18 ++++++++ 8 files changed, 66 insertions(+), 18 deletions(-) diff --git a/api/host_volumes.go b/api/host_volumes.go index 576e2fbd6..591cee1d2 100644 --- a/api/host_volumes.go +++ b/api/host_volumes.go @@ -177,6 +177,8 @@ type HostVolumeDeleteRequest struct { ID string } +type HostVolumeDeleteResponse struct{} + // Create forwards to client agents so a host volume can be created on those // hosts, and registers the volume with Nomad servers. func (hv *HostVolumes) Create(req *HostVolumeCreateRequest, opts *WriteOptions) (*HostVolumeCreateResponse, *WriteMeta, error) { @@ -236,11 +238,12 @@ func (hv *HostVolumes) List(req *HostVolumeListRequest, opts *QueryOptions) ([]* } // Delete deletes a host volume -func (hv *HostVolumes) Delete(req *HostVolumeDeleteRequest, opts *WriteOptions) (*WriteMeta, error) { +func (hv *HostVolumes) Delete(req *HostVolumeDeleteRequest, opts *WriteOptions) (*HostVolumeDeleteResponse, *WriteMeta, error) { + var resp *HostVolumeDeleteResponse path, err := url.JoinPath("/v1/volume/host/", url.PathEscape(req.ID)) if err != nil { - return nil, err + return nil, nil, err } - wm, err := hv.client.delete(path, nil, nil, opts) - return wm, err + wm, err := hv.client.delete(path, nil, resp, opts) + return resp, wm, err } diff --git a/client/hostvolumemanager/host_volume_plugin.go b/client/hostvolumemanager/host_volume_plugin.go index c51336b41..364201176 100644 --- a/client/hostvolumemanager/host_volume_plugin.go +++ b/client/hostvolumemanager/host_volume_plugin.go @@ -52,12 +52,20 @@ 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. +// HostVolumePluginCreateResponse returns values to the server that may be shown +// to the user or stored 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"` + Error string `json:"error"` +} + +// HostVolumePluginDeleteResponse returns values to the server that may be shown +// to the user or stored in server state. Plugins are expected to respond to +// 'delete' calls with json that unmarshals to this struct. +type HostVolumePluginDeleteResponse struct { + Error string `json:"error"` } const HostVolumePluginMkdirID = "mkdir" @@ -255,13 +263,21 @@ func (p *HostVolumePluginExternal) Create(ctx context.Context, fmt.Sprintf("%s=%s", EnvParameters, params), } + var pluginResp HostVolumePluginCreateResponse log := p.log.With("volume_name", req.Name, "volume_id", req.ID) stdout, _, err := p.runPlugin(ctx, log, "create", envVars) if err != nil { - return nil, fmt.Errorf("error creating volume %q with plugin %q: %w", req.ID, p.ID, err) + jsonErr := json.Unmarshal(stdout, &pluginResp) + if jsonErr != nil { + // if we got an error, we can't actually count on getting JSON, so + // optimistically look for it and return the original error + // otherwise + return nil, fmt.Errorf( + "error creating volume %q with plugin %q: %w", req.ID, p.ID, err) + } + return nil, fmt.Errorf("error creating volume %q with plugin %q: %w: %s", + req.ID, p.ID, err, pluginResp.Error) } - - var pluginResp HostVolumePluginCreateResponse err = json.Unmarshal(stdout, &pluginResp) if err != nil { // note: if a plugin does not return valid json, a volume may be @@ -313,10 +329,20 @@ func (p *HostVolumePluginExternal) Delete(ctx context.Context, } log := p.log.With("volume_name", req.Name, "volume_id", req.ID) - _, _, err = p.runPlugin(ctx, log, "delete", envVars) + stdout, _, err := p.runPlugin(ctx, log, "delete", envVars) if err != nil { - return fmt.Errorf("error deleting volume %q with plugin %q: %w", req.ID, p.ID, err) + var pluginResp HostVolumePluginDeleteResponse + jsonErr := json.Unmarshal(stdout, &pluginResp) + if jsonErr != nil { + // if we got an error, we can't actually count on getting JSON, so + // optimistically look for it and return the original error + // otherwise + return fmt.Errorf("error reading plugin response when deleting volume %q with plugin %q: original error: %w", req.ID, p.ID, err) + } + return fmt.Errorf("error deleting volume %q with plugin %q: %w: %s", + req.ID, p.ID, err, pluginResp.Error) } + return nil } diff --git a/client/hostvolumemanager/host_volume_plugin_test.go b/client/hostvolumemanager/host_volume_plugin_test.go index 826f7b943..cebdb0498 100644 --- a/client/hostvolumemanager/host_volume_plugin_test.go +++ b/client/hostvolumemanager/host_volume_plugin_test.go @@ -188,7 +188,7 @@ func TestHostVolumePluginExternal(t *testing.T) { &cstructs.ClientHostVolumeCreateRequest{ ID: volID, }) - must.EqError(t, err, `error creating volume "test-vol-id" with plugin "test_plugin_sad.sh": exit status 1`) + must.EqError(t, err, `error creating volume "test-vol-id" with plugin "test_plugin_sad.sh": exit status 1: create: sad plugin is sad`) must.Nil(t, resp) logged = getLogs() must.StrContains(t, logged, "create: sad plugin is sad") @@ -201,7 +201,7 @@ func TestHostVolumePluginExternal(t *testing.T) { &cstructs.ClientHostVolumeDeleteRequest{ ID: volID, }) - must.EqError(t, err, `error deleting volume "test-vol-id" with plugin "test_plugin_sad.sh": exit status 1`) + must.EqError(t, err, `error deleting volume "test-vol-id" with plugin "test_plugin_sad.sh": exit status 1: delete: sad plugin is sad`) logged = getLogs() must.StrContains(t, logged, "delete: sad plugin is sad") must.StrContains(t, logged, "delete: it tells you all about it in stderr") diff --git a/client/hostvolumemanager/test_fixtures/test_plugin_sad.sh b/client/hostvolumemanager/test_fixtures/test_plugin_sad.sh index 6f883297a..9af277e01 100755 --- a/client/hostvolumemanager/test_fixtures/test_plugin_sad.sh +++ b/client/hostvolumemanager/test_fixtures/test_plugin_sad.sh @@ -2,6 +2,6 @@ # Copyright (c) HashiCorp, Inc. # SPDX-License-Identifier: BUSL-1.1 -echo "$1: sad plugin is sad" +printf '{"error": "%s: sad plugin is sad"}' $1 echo "$1: it tells you all about it in stderr" 1>&2 exit 1 diff --git a/command/agent/host_volume_endpoint.go b/command/agent/host_volume_endpoint.go index db12cca92..a3791dee6 100644 --- a/command/agent/host_volume_endpoint.go +++ b/command/agent/host_volume_endpoint.go @@ -139,5 +139,5 @@ func (s *HTTPServer) hostVolumeDelete(id string, resp http.ResponseWriter, req * setIndex(resp, out.Index) - return nil, nil + return out, nil } diff --git a/command/agent/host_volume_endpoint_test.go b/command/agent/host_volume_endpoint_test.go index ddff7a33f..abb51524e 100644 --- a/command/agent/host_volume_endpoint_test.go +++ b/command/agent/host_volume_endpoint_test.go @@ -87,8 +87,9 @@ func TestHostVolumeEndpoint_CRUD(t *testing.T) { req, err = http.NewRequest(http.MethodDelete, fmt.Sprintf("/v1/volume/host/%s", volID), nil) must.NoError(t, err) - _, err = s.Server.HostVolumeSpecificRequest(respW, req) + obj, err = s.Server.HostVolumeSpecificRequest(respW, req) must.NoError(t, err) + must.NotNil(t, obj) // Verify volume was deleted diff --git a/command/volume_delete.go b/command/volume_delete.go index e23e02f6f..427fd9c59 100644 --- a/command/volume_delete.go +++ b/command/volume_delete.go @@ -195,7 +195,7 @@ func (c *VolumeDeleteCommand) deleteHostVolume(client *api.Client, volID string) c.namespace = stub.Namespace } - _, err := client.HostVolumes().Delete(&api.HostVolumeDeleteRequest{ID: volID}, nil) + _, _, err := client.HostVolumes().Delete(&api.HostVolumeDeleteRequest{ID: volID}, nil) if err != nil { c.Ui.Error(fmt.Sprintf("Error deleting volume: %s", err)) return 1 diff --git a/website/content/docs/concepts/plugins/storage/host-volumes.mdx b/website/content/docs/concepts/plugins/storage/host-volumes.mdx index 709df40f5..719bbbdf8 100644 --- a/website/content/docs/concepts/plugins/storage/host-volumes.mdx +++ b/website/content/docs/concepts/plugins/storage/host-volumes.mdx @@ -291,6 +291,15 @@ DHV_PARAMETERS={stringified json of parameters from the volume spec} {"path": "/path/to/created/volume", "bytes": 50000000} ``` +**Expected stdout on error:** + +``` +{"error": "error message"} +``` + +Returning an error message is optional. Nomad returns the error message in any +error returned to the user. + **Requirements:** * Must complete within 60 seconds, or Nomad will kill it. @@ -333,6 +342,15 @@ DHV_PARAMETERS={stringified json of parameters from the volume spec} **Expected stdout:** none (stdout is discarded) +**Expected stdout on error:** + +``` +{"error": "error message"} +``` + +Returning an error message is optional. Nomad returns the error message in any +error returned to the user. + **Requirements:** * Must complete within 60 seconds, or Nomad will kill it.