From ea44e34543aff6eb1221f8e9c09532b60fef879e Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 8 Oct 2020 12:53:24 -0400 Subject: [PATCH] csi: loosen ValidateVolumeCapability requirements (#9049) The CSI specification for `ValidateVolumeCapability` says that we shall "reconcile successful capability-validation responses by comparing the validated capabilities with those that it had originally requested" but leaves the details of that reconcilation unspecified. This API is not implemented in Kubernetes, so controller plugins don't have a real-world implementation to verify their behavior against. We have found that CSI plugins in the wild may return "successful" but incomplete `VolumeCapability` responses, so we can't require that all capabilities we expect have been validated, only that the ones that have been validated match. This appears to violate the CSI specification but until that's been resolved in upstream we have to loosen our validation requirements. The tradeoff is that we're more likely to have runtime errors during `NodeStageVolume` instead of at the time of volume registration. --- CHANGELOG.md | 1 + plugins/csi/client.go | 47 +++++--- plugins/csi/client_test.go | 114 ++++++++++++++++-- .../pages/docs/commands/volume/register.mdx | 2 +- .../pages/docs/job-specification/volume.mdx | 2 +- 5 files changed, 140 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37c0333348fe..32b59d6ab42d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ IMPROVEMENTS: * client: Added support for fingerprinting the client node's Consul segment. [[GH-7214](https://github.com/hashicorp/nomad/issues/7214)] * client: Updated consul-template to v0.25.0 - config function_blacklist deprecated and replaced with function_denylist [[GH-8988](https://github.com/hashicorp/nomad/pull/8988)] * consul: Support Consul namespace (Consul Enterprise) in client configuration. [[GH-8849](https://github.com/hashicorp/nomad/pull/8849)] + * csi: Relaxed validation requirements when checking volume capabilities with controller plugins, to accommodate existing plugin behaviors. [[GH-9049](https://github.com/hashicorp/nomad/issues/9049)] * driver/docker: Upgrade pause container and detect architecture [[GH-8957](https://github.com/hashicorp/nomad/pull/8957)] * jobspec: Lowered minimum CPU allowed from 10 to 1. [[GH-8996](https://github.com/hashicorp/nomad/issues/8996)] diff --git a/plugins/csi/client.go b/plugins/csi/client.go index bfff2848d508..d0bcb4ebd3e8 100644 --- a/plugins/csi/client.go +++ b/plugins/csi/client.go @@ -383,8 +383,16 @@ func (c *client) ControllerValidateCapabilities(ctx context.Context, req *Contro return nil } -// compareCapabilities returns an error if the 'got' capabilities does not -// contain the 'expected' capability +// compareCapabilities returns an error if the 'got' capabilities aren't found +// within the 'expected' capability. +// +// Note that plugins in the wild are known to return incomplete +// VolumeCapability responses, so we can't require that all capabilities we +// expect have been validated, only that the ones that have been validated +// match. This appears to violate the CSI specification but until that's been +// resolved in upstream we have to loosen our validation requirements. The +// tradeoff is that we're more likely to have runtime errors during +// NodeStageVolume. func compareCapabilities(expected *csipbv1.VolumeCapability, got []*csipbv1.VolumeCapability) error { var err multierror.Error NEXT_CAP: @@ -393,36 +401,40 @@ NEXT_CAP: expectedMode := expected.GetAccessMode().GetMode() capMode := cap.GetAccessMode().GetMode() - if expectedMode != capMode { - multierror.Append(&err, - fmt.Errorf("requested AccessMode %v, got %v", expectedMode, capMode)) - continue NEXT_CAP + // The plugin may not validate AccessMode, in which case we'll + // get UNKNOWN as our response + if capMode != csipbv1.VolumeCapability_AccessMode_UNKNOWN { + if expectedMode != capMode { + multierror.Append(&err, + fmt.Errorf("requested access mode %v, got %v", expectedMode, capMode)) + continue NEXT_CAP + } } - // AccessType Block is an empty struct even if set, so the - // only way to test for it is to check that the AccessType - // isn't Mount. - expectedMount := expected.GetMount() + capBlock := cap.GetBlock() capMount := cap.GetMount() + expectedBlock := expected.GetBlock() + expectedMount := expected.GetMount() - if expectedMount == nil { - if capMount == nil { - return nil - } + if capBlock != nil && expectedBlock == nil { multierror.Append(&err, fmt.Errorf( - "requested AccessType Block but got AccessType Mount")) + "'block-device' access type was not requested but was validated by the controller")) continue NEXT_CAP } if capMount == nil { + continue NEXT_CAP + } + + if expectedMount == nil { multierror.Append(&err, fmt.Errorf( - "requested AccessType Mount but got AccessType Block")) + "'file-system' access type was not requested but was validated by the controller")) continue NEXT_CAP } if expectedMount.FsType != capMount.FsType { multierror.Append(&err, fmt.Errorf( - "requested AccessType mount filesystem type %v, got %v", + "requested filesystem type %v, got %v", expectedMount.FsType, capMount.FsType)) continue NEXT_CAP } @@ -442,6 +454,7 @@ NEXT_CAP: continue NEXT_CAP } } + return nil } return err.ErrorOrNil() diff --git a/plugins/csi/client_test.go b/plugins/csi/client_test.go index 265d234b83a9..53a1042fcb9a 100644 --- a/plugins/csi/client_test.go +++ b/plugins/csi/client_test.go @@ -480,23 +480,31 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { cases := []struct { Name string + AccessType VolumeAccessType + AccessMode VolumeAccessMode ResponseErr error Response *csipbv1.ValidateVolumeCapabilitiesResponse ExpectedErr error }{ { Name: "handles underlying grpc errors", + AccessType: VolumeAccessTypeMount, + AccessMode: VolumeAccessModeMultiNodeMultiWriter, ResponseErr: status.Errorf(codes.Internal, "some grpc error"), ExpectedErr: fmt.Errorf("controller plugin returned an internal error, check the plugin allocation logs for more information: rpc error: code = Internal desc = some grpc error"), }, { - Name: "handles empty success", + Name: "handles success empty capabilities", + AccessType: VolumeAccessTypeMount, + AccessMode: VolumeAccessModeMultiNodeMultiWriter, Response: &csipbv1.ValidateVolumeCapabilitiesResponse{}, ResponseErr: nil, ExpectedErr: nil, }, { - Name: "handles validate success", + Name: "handles success exact match MountVolume", + AccessType: VolumeAccessTypeMount, + AccessMode: VolumeAccessModeMultiNodeMultiWriter, Response: &csipbv1.ValidateVolumeCapabilitiesResponse{ Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{ VolumeContext: map[string]string{}, @@ -518,8 +526,33 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { ResponseErr: nil, ExpectedErr: nil, }, + + { + Name: "handles success exact match BlockVolume", + AccessType: VolumeAccessTypeBlock, + AccessMode: VolumeAccessModeMultiNodeMultiWriter, + Response: &csipbv1.ValidateVolumeCapabilitiesResponse{ + Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{ + VolumeCapabilities: []*csipbv1.VolumeCapability{ + { + AccessType: &csipbv1.VolumeCapability_Block{ + Block: &csipbv1.VolumeCapability_BlockVolume{}, + }, + + AccessMode: &csipbv1.VolumeCapability_AccessMode{ + Mode: csipbv1.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER, + }, + }, + }, + }, + }, + ResponseErr: nil, + ExpectedErr: nil, + }, + { - Name: "handles validation failure block mismatch", + Name: "handles failure AccessMode mismatch", + AccessMode: VolumeAccessModeMultiNodeMultiWriter, Response: &csipbv1.ValidateVolumeCapabilitiesResponse{ Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{ VolumeContext: map[string]string{}, @@ -537,11 +570,13 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { }, ResponseErr: nil, // this is a multierror - ExpectedErr: fmt.Errorf("volume capability validation failed: 1 error occurred:\n\t* requested AccessMode MULTI_NODE_MULTI_WRITER, got SINGLE_NODE_WRITER\n\n"), + ExpectedErr: fmt.Errorf("volume capability validation failed: 1 error occurred:\n\t* requested access mode MULTI_NODE_MULTI_WRITER, got SINGLE_NODE_WRITER\n\n"), }, { - Name: "handles validation failure mount flags", + Name: "handles failure MountFlags mismatch", + AccessType: VolumeAccessTypeMount, + AccessMode: VolumeAccessModeMultiNodeMultiWriter, Response: &csipbv1.ValidateVolumeCapabilitiesResponse{ Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{ VolumeContext: map[string]string{}, @@ -564,6 +599,71 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { // this is a multierror ExpectedErr: fmt.Errorf("volume capability validation failed: 1 error occurred:\n\t* requested mount flags did not match available capabilities\n\n"), }, + + { + Name: "handles failure MountFlags with Block", + AccessType: VolumeAccessTypeBlock, + AccessMode: VolumeAccessModeMultiNodeMultiWriter, + Response: &csipbv1.ValidateVolumeCapabilitiesResponse{ + Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{ + VolumeContext: map[string]string{}, + VolumeCapabilities: []*csipbv1.VolumeCapability{ + { + AccessType: &csipbv1.VolumeCapability_Mount{ + Mount: &csipbv1.VolumeCapability_MountVolume{ + FsType: "ext4", + MountFlags: []string{}, + }, + }, + AccessMode: &csipbv1.VolumeCapability_AccessMode{ + Mode: csipbv1.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER, + }, + }, + }, + }, + }, + ResponseErr: nil, + // this is a multierror + ExpectedErr: fmt.Errorf("volume capability validation failed: 1 error occurred:\n\t* 'file-system' access type was not requested but was validated by the controller\n\n"), + }, + + { + Name: "handles success incomplete no AccessType", + AccessType: VolumeAccessTypeMount, + AccessMode: VolumeAccessModeMultiNodeMultiWriter, + Response: &csipbv1.ValidateVolumeCapabilitiesResponse{ + Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{ + VolumeCapabilities: []*csipbv1.VolumeCapability{ + { + AccessMode: &csipbv1.VolumeCapability_AccessMode{ + Mode: csipbv1.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER, + }, + }, + }, + }, + }, + ResponseErr: nil, + ExpectedErr: nil, + }, + + { + Name: "handles success incomplete no AccessMode", + AccessType: VolumeAccessTypeBlock, + AccessMode: VolumeAccessModeMultiNodeMultiWriter, + Response: &csipbv1.ValidateVolumeCapabilitiesResponse{ + Confirmed: &csipbv1.ValidateVolumeCapabilitiesResponse_Confirmed{ + VolumeCapabilities: []*csipbv1.VolumeCapability{ + { + AccessType: &csipbv1.VolumeCapability_Block{ + Block: &csipbv1.VolumeCapability_BlockVolume{}, + }, + }, + }, + }, + }, + ResponseErr: nil, + ExpectedErr: nil, + }, } for _, tc := range cases { @@ -572,8 +672,8 @@ func TestClient_RPC_ControllerValidateVolume(t *testing.T) { defer client.Close() requestedCaps := &VolumeCapability{ - AccessType: VolumeAccessTypeMount, - AccessMode: VolumeAccessModeMultiNodeMultiWriter, + AccessType: tc.AccessType, + AccessMode: tc.AccessMode, MountVolume: &structs.CSIMountOptions{ // should be ignored FSType: "ext4", MountFlags: []string{"noatime", "errors=remount-ro"}, diff --git a/website/pages/docs/commands/volume/register.mdx b/website/pages/docs/commands/volume/register.mdx index efb1db5a88b6..7c295a79b8a5 100644 --- a/website/pages/docs/commands/volume/register.mdx +++ b/website/pages/docs/commands/volume/register.mdx @@ -91,7 +91,7 @@ context { - `mount_options` - Options for mounting `file-system` volumes that don't already have a pre-formatted file system. Consult the documentation for your storage provider and CSI plugin as to whether these options are required or - neccessary. + necessary. - `fs_type`: file system type (ex. `"ext4"`) - `mount_flags`: the flags passed to `mount` (ex. `"ro,noatime"`) diff --git a/website/pages/docs/job-specification/volume.mdx b/website/pages/docs/job-specification/volume.mdx index cb4307d4ed42..8c45ed849887 100644 --- a/website/pages/docs/job-specification/volume.mdx +++ b/website/pages/docs/job-specification/volume.mdx @@ -59,7 +59,7 @@ the [volume_mount][volume_mount] stanza in the `task` configuration. `file-system` [attachment mode]. These options override the `mount_options` field from [volume registration]. Consult the documentation for your storage provider and CSI plugin as to whether these options are required or - neccessary. + necessary. - `fs_type`: file system type (ex. `"ext4"`) - `mount_flags`: the flags passed to `mount` (ex. `"ro,noatime"`)