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.

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

volumes: return better error messages for unsupported task drivers #8030

Merged
merged 7 commits into from
May 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/allocrunner/alloc_runner_hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func (ar *allocRunner) initRunnerHooks(config *clientconfig.Config) error {
logger: hookLogger,
}),
newConsulSockHook(hookLogger, alloc, ar.allocDir, config.ConsulConfig),
newCSIHook(hookLogger, alloc, ar.rpcClient, ar.csiManager, hrs),
newCSIHook(ar, hookLogger, alloc, ar.rpcClient, ar.csiManager, hrs),
}

return nil
Expand Down
19 changes: 18 additions & 1 deletion client/allocrunner/csi_hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import (
hclog "github.com/hashicorp/go-hclog"
"github.com/hashicorp/nomad/client/pluginmanager/csimanager"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/hashicorp/nomad/plugins/drivers"
)

// csiHook will wait for remote csi volumes to be attached to the host before
// continuing.
//
// It is a noop for allocs that do not depend on CSI Volumes.
type csiHook struct {
ar *allocRunner
alloc *structs.Allocation
logger hclog.Logger
csimanager csimanager.Manager
Expand Down Expand Up @@ -88,7 +90,21 @@ func (c *csiHook) claimVolumesFromAlloc() (map[string]*volumeAndRequest, error)

// Initially, populate the result map with all of the requests
for alias, volumeRequest := range tg.Volumes {

if volumeRequest.Type == structs.VolumeTypeCSI {

for _, task := range tg.Tasks {
caps, err := c.ar.GetTaskDriverCapabilities(task.Name)
if err != nil {
return nil, fmt.Errorf("could not validate task driver capabilities: %v", err)
}

if caps.MountConfigs == drivers.MountConfigSupportNone {
return nil, fmt.Errorf(
"task driver %q for %q does not support CSI", task.Driver, task.Name)
}
}

result[alias] = &volumeAndRequest{request: volumeRequest}
}
}
Expand Down Expand Up @@ -125,8 +141,9 @@ func (c *csiHook) claimVolumesFromAlloc() (map[string]*volumeAndRequest, error)
return result, nil
}

func newCSIHook(logger hclog.Logger, alloc *structs.Allocation, rpcClient RPCer, csi csimanager.Manager, updater hookResourceSetter) *csiHook {
func newCSIHook(ar *allocRunner, logger hclog.Logger, alloc *structs.Allocation, rpcClient RPCer, csi csimanager.Manager, updater hookResourceSetter) *csiHook {
return &csiHook{
ar: ar,
alloc: alloc,
logger: logger.Named("csi_hook"),
rpcClient: rpcClient,
Expand Down
24 changes: 24 additions & 0 deletions client/allocrunner/taskrunner/volume_hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,18 @@ func (h *volumeHook) prepareHostVolumes(req *interfaces.TaskPrestartRequest, vol
return nil, err
}

if len(hostVolumeMounts) > 0 {
caps, err := h.runner.DriverCapabilities()
if err != nil {
return nil, fmt.Errorf("could not validate task driver capabilities: %v", err)
}
if caps.MountConfigs == drivers.MountConfigSupportNone {
return nil, fmt.Errorf(
"task driver %q for %q does not support host volumes",
h.runner.task.Driver, h.runner.task.Name)
}
}

return hostVolumeMounts, nil
}

Expand Down Expand Up @@ -167,6 +179,18 @@ func (h *volumeHook) prepareCSIVolumes(req *interfaces.TaskPrestartRequest, volu
}
}

if len(mounts) > 0 {
caps, err := h.runner.DriverCapabilities()
if err != nil {
return nil, fmt.Errorf("could not validate task driver capabilities: %v", err)
}
if caps.MountConfigs == drivers.MountConfigSupportNone {
return nil, fmt.Errorf(
"task driver %q for %q does not support CSI",
h.runner.task.Driver, h.runner.task.Name)
}
}

return mounts, nil
}

Expand Down
78 changes: 61 additions & 17 deletions client/allocrunner/taskrunner/volume_hook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/hashicorp/nomad/nomad/mock"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/hashicorp/nomad/plugins/drivers"
dtu "github.com/hashicorp/nomad/plugins/drivers/testutils"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -67,8 +68,11 @@ func TestVolumeHook_PartitionMountsByVolume_Works(t *testing.T) {
}

func TestVolumeHook_prepareCSIVolumes(t *testing.T) {

req := &interfaces.TaskPrestartRequest{
Task: &structs.Task{
Name: "test",
Driver: "mock",
VolumeMounts: []*structs.VolumeMount{
{
Volume: "foo",
Expand All @@ -85,31 +89,71 @@ func TestVolumeHook_prepareCSIVolumes(t *testing.T) {
},
}

tr := &TaskRunner{
allocHookResources: &cstructs.AllocHookResources{
CSIMounts: map[string]*csimanager.MountInfo{
"foo": {
Source: "/mnt/my-test-volume",
cases := []struct {
Name string
Driver drivers.DriverPlugin
Expected []*drivers.MountConfig
ExpectedError string
}{
{
Name: "supported driver",
Driver: &dtu.MockDriver{
CapabilitiesF: func() (*drivers.Capabilities, error) {
return &drivers.Capabilities{
MountConfigs: drivers.MountConfigSupportAll,
}, nil
},
},
Expected: []*drivers.MountConfig{
{
HostPath: "/mnt/my-test-volume",
TaskPath: "/bar",
},
},
},
}

expected := []*drivers.MountConfig{
{
HostPath: "/mnt/my-test-volume",
TaskPath: "/bar",
Name: "unsupported driver",
Driver: &dtu.MockDriver{
CapabilitiesF: func() (*drivers.Capabilities, error) {
return &drivers.Capabilities{
MountConfigs: drivers.MountConfigSupportNone,
}, nil
},
},
ExpectedError: "task driver \"mock\" for \"test\" does not support CSI",
},
}

hook := &volumeHook{
logger: testlog.HCLogger(t),
alloc: structs.MockAlloc(),
runner: tr,
for _, tc := range cases {
t.Run(tc.Name, func(t *testing.T) {

tr := &TaskRunner{
task: req.Task,
driver: tc.Driver,
allocHookResources: &cstructs.AllocHookResources{
CSIMounts: map[string]*csimanager.MountInfo{
"foo": {
Source: "/mnt/my-test-volume",
},
},
},
}

hook := &volumeHook{
logger: testlog.HCLogger(t),
alloc: structs.MockAlloc(),
runner: tr,
}
mounts, err := hook.prepareCSIVolumes(req, volumes)

if tc.ExpectedError != "" {
require.EqualError(t, err, tc.ExpectedError)
} else {
require.NoError(t, err)
}
require.Equal(t, tc.Expected, mounts)
})
}
mounts, err := hook.prepareCSIVolumes(req, volumes)
require.NoError(t, err)
require.Equal(t, expected, mounts)
}

func TestVolumeHook_Interpolation(t *testing.T) {
Expand Down
1 change: 1 addition & 0 deletions drivers/docker/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ var (
drivers.NetIsolationModeTask,
},
MustInitiateNetwork: true,
MountConfigs: drivers.MountConfigSupportAll,
}
)

Expand Down
1 change: 1 addition & 0 deletions drivers/exec/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ var (
drivers.NetIsolationModeHost,
drivers.NetIsolationModeGroup,
},
MountConfigs: drivers.MountConfigSupportAll,
}
)

Expand Down
2 changes: 2 additions & 0 deletions drivers/java/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ var (
drivers.NetIsolationModeHost,
drivers.NetIsolationModeGroup,
},
MountConfigs: drivers.MountConfigSupportNone,
}

_ drivers.DriverPlugin = (*Driver)(nil)
Expand All @@ -95,6 +96,7 @@ var (
func init() {
if runtime.GOOS == "linux" {
capabilities.FSIsolation = drivers.FSIsolationChroot
capabilities.MountConfigs = drivers.MountConfigSupportAll
}
}

Expand Down
7 changes: 4 additions & 3 deletions drivers/mock/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,10 @@ func NewMockDriver(logger hclog.Logger) drivers.DriverPlugin {
logger = logger.Named(pluginName)

capabilities := &drivers.Capabilities{
SendSignals: true,
Exec: true,
FSIsolation: drivers.FSIsolationNone,
SendSignals: true,
Exec: true,
FSIsolation: drivers.FSIsolationNone,
MountConfigs: drivers.MountConfigSupportNone,
}

return &Driver{
Expand Down
7 changes: 4 additions & 3 deletions drivers/qemu/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,10 @@ var (
// capabilities is returned by the Capabilities RPC and indicates what
// optional features this driver supports
capabilities = &drivers.Capabilities{
SendSignals: false,
Exec: false,
FSIsolation: drivers.FSIsolationImage,
SendSignals: false,
Exec: false,
FSIsolation: drivers.FSIsolationImage,
MountConfigs: drivers.MountConfigSupportNone,
}

_ drivers.DriverPlugin = (*Driver)(nil)
Expand Down
1 change: 1 addition & 0 deletions drivers/rawexec/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ var (
drivers.NetIsolationModeHost,
drivers.NetIsolationModeGroup,
},
MountConfigs: drivers.MountConfigSupportNone,
}
)

Expand Down
2 changes: 2 additions & 0 deletions plugins/drivers/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ func (d *driverPluginClient) Capabilities() (*Capabilities, error) {
default:
caps.FSIsolation = FSIsolationNone
}

caps.MountConfigs = MountConfigSupport(resp.Capabilities.MountConfigs)
}

return caps, nil
Expand Down
12 changes: 12 additions & 0 deletions plugins/drivers/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ type Capabilities struct {
// MustInitiateNetwork tells Nomad that the driver must create the network
// namespace and that the CreateNetwork and DestroyNetwork RPCs are implemented.
MustInitiateNetwork bool

// MountConfigs tells Nomad which mounting config options the driver supports.
MountConfigs MountConfigSupport
}

func (c *Capabilities) HasNetIsolationMode(m NetIsolationMode) bool {
Expand Down Expand Up @@ -197,6 +200,15 @@ type NetworkIsolationSpec struct {
Labels map[string]string
}

// MountConfigSupport is an enum that defaults to "all" for backwards
// compatibility with community drivers.
type MountConfigSupport int32

const (
MountConfigSupportAll MountConfigSupport = iota
MountConfigSupportNone
)

type TerminalSize struct {
Height int
Width int
Expand Down
Loading