Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

csi: loosen ValidateVolumeCapability requirements #9049

Merged
merged 3 commits into from
Oct 8, 2020
Merged

csi: loosen ValidateVolumeCapability requirements #9049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ebff972
csi: loosen ValidateVolumeCapability requirements
tgross Oct 8, 2020
d3136e1
fix mispellings
tgross Oct 8, 2020
848cd1e
add changelog entry
tgross Oct 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]

Expand Down
47 changes: 30 additions & 17 deletions plugins/csi/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: I've changed the wording of these error messages so that they match what the operator has submitted in the volume spec or jobspec, rather than matching the CSI internal nomenclature.

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
}
Expand All @@ -442,6 +454,7 @@ NEXT_CAP:
continue NEXT_CAP
}
}

return nil
}
return err.ErrorOrNil()
Expand Down
114 changes: 107 additions & 7 deletions plugins/csi/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand All @@ -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{},
Expand All @@ -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{},
Expand All @@ -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 {
Expand All @@ -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"},
Expand Down
2 changes: 1 addition & 1 deletion website/pages/docs/commands/volume/register.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: I dunno why these slipped by the docs PR but triggered the linter here. Maybe we don't check spelling there?


- `fs_type`: file system type (ex. `"ext4"`)
- `mount_flags`: the flags passed to `mount` (ex. `"ro,noatime"`)
Expand Down
2 changes: 1 addition & 1 deletion website/pages/docs/job-specification/volume.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`)
Expand Down