From 4cdfa19b1e33a90930344fbeb0fb8f6005bd5398 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Mon, 24 Feb 2025 11:38:07 -0500 Subject: [PATCH] volume status: default type to show both DHV and CSI volumes (#25185) The `-type` option for `volume status` is a UX papercut because for many clusters there will be only one sort of volume in use. Update the CLI so that the default behavior is to query CSI and/or DHV. This behavior is subtly different when the user provides an ID or not. If the user doesn't provide an ID, we query both CSI and DHV and show both tables. If the user provides an ID, we query DHV first and then CSI, and show only the appropriate volume. Because DHV IDs are UUIDs, we're sure we won't have collisions between the two. We only show errors if both queries return an error. Fixes: https://hashicorp.atlassian.net/browse/NET-12214 --- command/helpers.go | 4 +- command/helpers_test.go | 4 +- command/scaling_policy_info_test.go | 4 +- command/volume_status.go | 58 ++++++++++++++- command/volume_status_csi.go | 74 ++++++++----------- command/volume_status_host.go | 70 ++++++++---------- command/volume_status_host_test.go | 2 +- .../content/docs/commands/volume/status.mdx | 44 ++++++----- 8 files changed, 147 insertions(+), 113 deletions(-) diff --git a/command/helpers.go b/command/helpers.go index d2d2a7d36..36bb69fd7 100644 --- a/command/helpers.go +++ b/command/helpers.go @@ -753,11 +753,11 @@ func getByPrefix[T any]( ) (*T, []*T, error) { objs, _, err := queryFn(opts) if err != nil { - return nil, nil, fmt.Errorf("Error querying %s: %s", objName, err) + return nil, nil, fmt.Errorf("error querying %s: %s", objName, err) } switch len(objs) { case 0: - return nil, nil, fmt.Errorf("No %s with prefix or ID %q found", objName, opts.Prefix) + return nil, nil, fmt.Errorf("no %s with prefix or ID %q found", objName, opts.Prefix) case 1: return objs[0], nil, nil default: diff --git a/command/helpers_test.go b/command/helpers_test.go index c6b0c4dae..3959c39bc 100644 --- a/command/helpers_test.go +++ b/command/helpers_test.go @@ -673,7 +673,7 @@ func TestHelperGetByPrefix(t *testing.T) { { name: "query error", queryErr: errors.New("foo"), - expectErr: "Error querying stubs: foo", + expectErr: "error querying stubs: foo", }, { name: "multiple prefix matches with exact match", @@ -697,7 +697,7 @@ func TestHelperGetByPrefix(t *testing.T) { name: "no matches", queryObjs: []*testStub{}, queryPrefix: "test", - expectErr: "No stubs with prefix or ID \"test\" found", + expectErr: "no stubs with prefix or ID \"test\" found", }, } diff --git a/command/scaling_policy_info_test.go b/command/scaling_policy_info_test.go index 44d76a0ff..62c1292af 100644 --- a/command/scaling_policy_info_test.go +++ b/command/scaling_policy_info_test.go @@ -58,8 +58,8 @@ func TestScalingPolicyInfoCommand_Run(t *testing.T) { if code := cmd.Run([]string{"-address=" + url, "scaling_policy_info"}); code != 1 { t.Fatalf("expected cmd run exit code 1, got: %d", code) } - if out := ui.ErrorWriter.String(); !strings.Contains(out, `No scaling policies with prefix or ID "scaling_policy_inf" found`) { - t.Fatalf("expected 'No scaling policies with prefix' within output: %v", out) + if out := ui.ErrorWriter.String(); !strings.Contains(out, `no scaling policies with prefix or ID "scaling_policy_inf" found`) { + t.Fatalf("expected 'no scaling policies with prefix' within output: %v", out) } // Generate a test job. diff --git a/command/volume_status.go b/command/volume_status.go index d599e349e..36d2577bc 100644 --- a/command/volume_status.go +++ b/command/volume_status.go @@ -4,6 +4,7 @@ package command import ( + "errors" "fmt" "strings" @@ -38,7 +39,8 @@ General Options: Status Options: -type - List only volumes of type . + List only volumes of type (one of "host" or "csi"). If omitted, the + command will query for both dynamic host volumes and CSI volumes. -short Display short output. Used only when a single volume is being @@ -147,17 +149,65 @@ func (c *VolumeStatusCommand) Run(args []string) int { id = args[0] } + opts := formatOpts{ + verbose: c.verbose, + short: c.short, + length: c.length, + json: c.json, + template: c.template, + } + switch typeArg { - case "csi", "": + case "csi": if nodeID != "" || nodePool != "" { c.Ui.Error("-node and -node-pool can only be used with -type host") return 1 } - return c.csiStatus(client, id) + if err := c.csiVolumeStatus(client, id, opts); err != nil { + c.Ui.Error(err.Error()) + return 1 + } case "host": - return c.hostVolumeStatus(client, id, nodeID, nodePool) + if err := c.hostVolumeStatus(client, id, nodeID, nodePool, opts); err != nil { + c.Ui.Error(err.Error()) + return 1 + } + case "": + if id == "" { + // for list, we want to show both + dhvErr := c.hostVolumeList(client, nodeID, nodePool, opts) + if dhvErr != nil { + c.Ui.Error(dhvErr.Error()) + } + c.Ui.Output("") + csiErr := c.csiVolumesList(client, opts) + if csiErr != nil { + c.Ui.Error(csiErr.Error()) + } + if dhvErr == nil && csiErr == nil { + return 0 + } + return 1 + } else { + // for read, we only want to show whichever has results + hostErr := c.hostVolumeStatus(client, id, nodeID, nodePool, opts) + if hostErr != nil { + if !errors.Is(hostErr, hostVolumeListError) { + c.Ui.Error(hostErr.Error()) + return 1 // we found a host volume but had some other error + } + csiErr := c.csiVolumeStatus(client, id, opts) + if csiErr != nil { + c.Ui.Error(hostErr.Error()) + c.Ui.Error(csiErr.Error()) + return 1 + } + } + } default: c.Ui.Error(fmt.Sprintf("No such volume type %q", typeArg)) return 1 } + + return 0 } diff --git a/command/volume_status_csi.go b/command/volume_status_csi.go index 8b8b9a986..3303b2014 100644 --- a/command/volume_status_csi.go +++ b/command/volume_status_csi.go @@ -11,19 +11,13 @@ import ( "strings" "github.com/dustin/go-humanize" + multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/nomad/api" ) -func (c *VolumeStatusCommand) csiBanner() { - if !(c.json || len(c.template) > 0) { - c.Ui.Output(c.Colorize().Color("[bold]Container Storage Interface[reset]")) - } -} - -func (c *VolumeStatusCommand) csiStatus(client *api.Client, id string) int { - // Invoke list mode if no volume id +func (c *VolumeStatusCommand) csiVolumeStatus(client *api.Client, id string, opts formatOpts) error { if id == "" { - return c.listCSIVolumes(client) + return c.csiVolumesList(client, opts) } // get a CSI volume that matches the given prefix or a list of all matches if an @@ -35,71 +29,65 @@ func (c *VolumeStatusCommand) csiStatus(client *api.Client, id string) int { Namespace: c.namespace, }) if err != nil { - c.Ui.Error(fmt.Sprintf("Error listing volumes: %s", err)) - return 1 + return fmt.Errorf("Error listing CSI volumes: %w", err) } if len(possible) > 0 { out, err := csiFormatVolumes(possible, c.json, c.template) if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) - return 1 + return fmt.Errorf("Error formatting: %w", err) } - c.Ui.Error(fmt.Sprintf("Prefix matched multiple volumes\n\n%s", out)) - return 1 + return fmt.Errorf("Prefix matched multiple CSI volumes\n\n%s", out) } // Try querying the volume vol, _, err := client.CSIVolumes().Info(volStub.ID, nil) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying volume: %s", err)) - return 1 + return fmt.Errorf("Error querying CSI volume: %w", err) } str, err := c.formatCSIBasic(vol) if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting volume: %s", err)) - return 1 + return fmt.Errorf("Error formatting CSI volume: %w", err) } c.Ui.Output(str) - - return 0 + return nil } -func (c *VolumeStatusCommand) listCSIVolumes(client *api.Client) int { +func (c *VolumeStatusCommand) csiVolumesList(client *api.Client, opts formatOpts) error { + + if !(opts.json || len(opts.template) > 0) { + c.Ui.Output(c.Colorize().Color("[bold]Container Storage Interface[reset]")) + } - c.csiBanner() vols, _, err := client.CSIVolumes().List(nil) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying volumes: %s", err)) - return 1 + return fmt.Errorf("Error querying CSI volumes: %w", err) } if len(vols) == 0 { - // No output if we have no volumes c.Ui.Error("No CSI volumes") + return nil // not an empty is not an error } else { - str, err := csiFormatVolumes(vols, c.json, c.template) + str, err := csiFormatVolumes(vols, opts.json, opts.template) if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) - return 1 + return fmt.Errorf("Error formatting: %w", err) } c.Ui.Output(str) } - if !c.verbose { - return 0 + if !opts.verbose { + return nil } plugins, _, err := client.CSIPlugins().List(nil) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying CSI plugins: %s", err)) - return 1 + return fmt.Errorf("Error querying CSI plugins: %w", err) } if len(plugins) == 0 { - return 0 // No more output if we have no plugins + return nil // No more output if we have no plugins } - var code int + var mErr *multierror.Error q := &api.QueryOptions{PerPage: 30} // TODO: tune page size NEXT_PLUGIN: @@ -110,12 +98,11 @@ NEXT_PLUGIN: for { externalList, _, err := client.CSIVolumes().ListExternal(plugin.ID, q) if err != nil && !errors.Is(err, io.EOF) { - c.Ui.Error(fmt.Sprintf( - "Error querying CSI external volumes for plugin %q: %s", plugin.ID, err)) + mErr = multierror.Append(mErr, fmt.Errorf( + "Error querying CSI external volumes for plugin %q: %w", plugin.ID, err)) // we'll stop querying this plugin, but there may be more to // query, so report and set the error code but move on to the // next plugin - code = 1 continue NEXT_PLUGIN } if externalList == nil || len(externalList.Volumes) == 0 { @@ -123,6 +110,7 @@ NEXT_PLUGIN: // rather than an empty list continue NEXT_PLUGIN } + c.Ui.Output("") // force a newline rows := []string{"External ID|Condition|Nodes"} for _, v := range externalList.Volumes { condition := "OK" @@ -130,7 +118,7 @@ NEXT_PLUGIN: condition = fmt.Sprintf("Abnormal (%v)", v.Status) } rows = append(rows, fmt.Sprintf("%s|%s|%s", - limit(v.ExternalID, c.length), + limit(v.ExternalID, opts.length), limit(condition, 20), strings.Join(v.PublishedExternalNodeIDs, ","), )) @@ -147,7 +135,7 @@ NEXT_PLUGIN: } } - return code + return mErr.ErrorOrNil() } func csiFormatVolumes(vols []*api.CSIVolumeListStub, json bool, template string) (string, error) { @@ -157,7 +145,7 @@ func csiFormatVolumes(vols []*api.CSIVolumeListStub, json bool, template string) if json || len(template) > 0 { out, err := Format(json, template, vols) if err != nil { - return "", fmt.Errorf("format error: %v", err) + return "", fmt.Errorf("format error: %w", err) } return out, nil } @@ -186,7 +174,7 @@ func (c *VolumeStatusCommand) formatCSIBasic(vol *api.CSIVolume) (string, error) if c.json || len(c.template) > 0 { out, err := Format(c.json, c.template, vol) if err != nil { - return "", fmt.Errorf("format error: %v", err) + return "", fmt.Errorf("format error: %w", err) } return out, nil } @@ -224,7 +212,7 @@ func (c *VolumeStatusCommand) formatCSIBasic(vol *api.CSIVolume) (string, error) full = append(full, topo) } - banner := "\n[bold]Capabilities[reset]" + banner := c.Colorize().Color("\n[bold]Capabilities[reset]") caps := formatCSIVolumeCapabilities(vol.RequestedCapabilities) full = append(full, banner) full = append(full, caps) diff --git a/command/volume_status_host.go b/command/volume_status_host.go index 5f8f4232a..42f1ac48d 100644 --- a/command/volume_status_host.go +++ b/command/volume_status_host.go @@ -4,6 +4,7 @@ package command import ( + "errors" "fmt" "sort" "strings" @@ -12,83 +13,70 @@ import ( "github.com/hashicorp/nomad/api" ) -func (c *VolumeStatusCommand) hostVolumeStatus(client *api.Client, id, nodeID, nodePool string) int { +// hostVolumeListError is a non-fatal error for the 'volume status' command when +// used with the -type option unset, because we want to continue on to list CSI +// volumes +var hostVolumeListError = errors.New("Error listing host volumes") + +func (c *VolumeStatusCommand) hostVolumeStatus(client *api.Client, id, nodeID, nodePool string, opts formatOpts) error { if id == "" { - return c.listHostVolumes(client, nodeID, nodePool) + return c.hostVolumeList(client, nodeID, nodePool, opts) } - if nodeID != "" || nodePool != "" { - c.Ui.Error("-node or -node-pool options can only be used when no ID is provided") - return 1 - } - - opts := formatOpts{ - verbose: c.verbose, - short: c.short, - length: c.length, - json: c.json, - template: c.template, + return errors.New("-node or -node-pool options can only be used when no ID is provided") } // get a host volume that matches the given prefix or a list of all matches // if an exact match is not found. note we can't use the shared getByPrefix // helper here because the List API doesn't match the required signature - volStub, possible, err := getHostVolumeByPrefix(client, id, c.namespace) if err != nil { - c.Ui.Error(fmt.Sprintf("Error listing volumes: %s", err)) - return 1 + return fmt.Errorf("%w: %w", hostVolumeListError, err) } if len(possible) > 0 { out, err := formatHostVolumes(possible, opts) if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting: %s", err)) - return 1 + return fmt.Errorf("Error formatting: %w", err) } - c.Ui.Error(fmt.Sprintf("Prefix matched multiple volumes\n\n%s", out)) - return 1 + return fmt.Errorf("Prefix matched multiple host volumes\n\n%s", out) } vol, _, err := client.HostVolumes().Get(volStub.ID, nil) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying volume: %s", err)) - return 1 + return fmt.Errorf("Error querying host volume: %w", err) } str, err := formatHostVolume(vol, opts) if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting volume: %s", err)) - return 1 + return fmt.Errorf("Error formatting host volume: %w", err) } c.Ui.Output(c.Colorize().Color(str)) - return 0 + return nil } -func (c *VolumeStatusCommand) listHostVolumes(client *api.Client, nodeID, nodePool string) int { +func (c *VolumeStatusCommand) hostVolumeList(client *api.Client, nodeID, nodePool string, opts formatOpts) error { + if !(opts.json || len(opts.template) > 0) { + c.Ui.Output(c.Colorize().Color("[bold]Dynamic Host Volumes[reset]")) + } + vols, _, err := client.HostVolumes().List(&api.HostVolumeListRequest{ NodeID: nodeID, NodePool: nodePool, }, nil) if err != nil { - c.Ui.Error(fmt.Sprintf("Error querying volumes: %s", err)) - return 1 + return fmt.Errorf("Error querying host volumes: %w", err) } - - opts := formatOpts{ - verbose: c.verbose, - short: c.short, - length: c.length, - json: c.json, - template: c.template, + if len(vols) == 0 { + c.Ui.Error("No dynamic host volumes") + return nil // empty is not an error } str, err := formatHostVolumes(vols, opts) if err != nil { - c.Ui.Error(fmt.Sprintf("Error formatting volumes: %s", err)) - return 1 + return fmt.Errorf("Error formatting host volumes: %w", err) } c.Ui.Output(c.Colorize().Color(str)) - return 0 + return nil } func getHostVolumeByPrefix(client *api.Client, prefix, ns string) (*api.HostVolumeStub, []*api.HostVolumeStub, error) { @@ -98,7 +86,7 @@ func getHostVolumeByPrefix(client *api.Client, prefix, ns string) (*api.HostVolu }) if err != nil { - return nil, nil, fmt.Errorf("error querying volumes: %s", err) + return nil, nil, fmt.Errorf("error querying volumes: %w", err) } switch len(vols) { case 0: @@ -127,7 +115,7 @@ func formatHostVolume(vol *api.HostVolume, opts formatOpts) (string, error) { if opts.json || len(opts.template) > 0 { out, err := Format(opts.json, opts.template, vol) if err != nil { - return "", fmt.Errorf("format error: %v", err) + return "", fmt.Errorf("format error: %w", err) } return out, nil } @@ -181,7 +169,7 @@ func formatHostVolumes(vols []*api.HostVolumeStub, opts formatOpts) (string, err if opts.json || len(opts.template) > 0 { out, err := Format(opts.json, opts.template, vols) if err != nil { - return "", fmt.Errorf("format error: %v", err) + return "", fmt.Errorf("format error: %w", err) } return out, nil } diff --git a/command/volume_status_host_test.go b/command/volume_status_host_test.go index 8df4c11ef..b3494f463 100644 --- a/command/volume_status_host_test.go +++ b/command/volume_status_host_test.go @@ -161,7 +161,7 @@ capability { code = cmd.Run(args) must.Eq(t, 1, code) must.StrContains(t, ui.ErrorWriter.String(), - "Error listing volumes: no volumes with prefix or ID") + "Error listing host volumes: no volumes with prefix or ID") ui.ErrorWriter.Reset() args = []string{"-address", url, "-type", "host", "-namespace", "prod", id} diff --git a/website/content/docs/commands/volume/status.mdx b/website/content/docs/commands/volume/status.mdx index 1a93d7f6c..8d17818e8 100644 --- a/website/content/docs/commands/volume/status.mdx +++ b/website/content/docs/commands/volume/status.mdx @@ -8,7 +8,7 @@ description: | # Command: volume status The `volume status` command displays status information for [Container -Storage Interface (CSI)][csi] volumes. +Storage Interface (CSI)][csi] volumes or [dynamic host volumes][dhv]. ## Usage @@ -16,17 +16,18 @@ Storage Interface (CSI)][csi] volumes. nomad volume status [options] [volume] ``` -This command accepts an optional volume ID or prefix as the sole argument. If there -is an exact match based on the provided volume ID or prefix, then information about -the specific volume is queried and displayed. Otherwise, a list of matching volumes -and information will be displayed. +This command accepts an optional volume ID or prefix as the sole argument. If +there is an exact match based on the provided volume ID or prefix, then +information about the specific volume is queried and displayed. Otherwise, a +list of matching volumes and information will be displayed. -If the ID is omitted, the command lists out all of the existing volumes and a few -of the most useful status fields for each. +If the ID is omitted, the command lists out all of the existing volumes and a +few of the most useful status fields for each. -When ACLs are enabled, this command requires a token with the -`csi-read-volume` and `csi-list-volumes` capability for the volume's -namespace. +When ACLs are enabled, this command requires a token with the appropriate +capability in the volume's namespace: the `csi-read-volume` and +`csi-list-volumes` capability for CSI volumes, or `host-volume-read` for dynamic +host volumes. ## General Options @@ -34,9 +35,9 @@ namespace. ## Status Options -- `-type`: Display only volumes of a particular type. Currently only - the `csi` type is supported, so this option can be omitted when - querying the status of CSI volumes. +- `-type`: Display only volumes of a particular type (one of `"host"` or + `"csi"`). If omitted, the command will query for both dynamic host volumes and + CSI volumes. - `-plugin_id`: Display only volumes managed by a particular [CSI plugin][csi_plugin]. @@ -57,15 +58,21 @@ namespace. List of all volumes: ```shell-session -$ nomad volume [-type csi] status +$ nomad volume status +Dynamic Host Volumes +ID Name Namespace Plugin ID Node ID Node Pool State +070dc8ca internal-plugin default mkdir c3c27514 default ready +7ce38017 external-plugin default example-plugin-mkfs c3c27514 default ready + +Container Storage Interface ID Name Namespace Plugin ID Schedulable Access Mode ebs_prod_db1 database default ebs-prod true single-node-writer ``` -List of all volumes, with external provider info: +List of all CSI volumes, with external provider info: ```shell-session -$ nomad volume [-type csi] status -verbose +$ nomad volume -type csi status -verbose ID Name Namespace Plugin ID Schedulable Access Mode ebs_prod_db1 database default ebs-prod true single-node-writer @@ -77,7 +84,7 @@ vol-cd46df Abnormal (provider message here) i-14a12df13 Short view of a specific volume: ```shell-session -$ nomad volume status [-verbose] [-plugin=ebs-prod] ebs_prod_db1 +$ nomad volume status ebs_prod_db1 ID = ebs_prod_db1 Name = database Type = csi @@ -100,7 +107,7 @@ Namespace = default Full status information of a volume: ```shell-session -$ nomad volume status [-verbose] [-plugin=ebs-prod] ebs_prod_db1 +$ nomad volume status -verbose ebs_prod_db1 ID = ebs_prod_db1 Name = database Type = csi @@ -127,3 +134,4 @@ b00fa322 28be17d5 write csi 0 run [csi]: https://github.com/container-storage-interface/spec [csi_plugin]: /nomad/docs/job-specification/csi_plugin [`volume create`]: /nomad/docs/commands/volume/create +[dhv]: /nomad/docs/other-specifications/volume/host