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

Remove controller node plugin driver dependency for non-attachable fl… #47503

Merged
merged 1 commit into from
Jun 22, 2017
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
5 changes: 1 addition & 4 deletions examples/volumes/flexvolume/nfs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ unmount() {
op=$1

if [ "$op" = "init" ]; then
log "{\"status\": \"Success\"}"
log "{\"status\": \"Success\", \"capabilities\": {\"attach\": false}}"
exit 0
fi

Expand All @@ -100,9 +100,6 @@ case "$op" in
unmount)
unmount $*
;;
getvolumename)
getvolumename $*
;;
*)
log "{ \"status\": \"Not supported\" }"
exit 0
Expand Down
16 changes: 4 additions & 12 deletions pkg/volume/flexvolume/attacher-defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ limitations under the License.
package flexvolume

import (
"fmt"
"path"
"time"

"github.com/golang/glog"
Expand All @@ -33,30 +31,24 @@ type attacherDefaults flexVolumeAttacher

// Attach is part of the volume.Attacher interface
func (a *attacherDefaults) Attach(spec *volume.Spec, hostName types.NodeName) (string, error) {
glog.Warning(logPrefix(a.plugin), "using default Attach for volume ", spec.Name, ", host ", hostName)
glog.Warning(logPrefix(a.plugin.flexVolumePlugin), "using default Attach for volume ", spec.Name, ", host ", hostName)
return "", nil
}

// WaitForAttach is part of the volume.Attacher interface
func (a *attacherDefaults) WaitForAttach(spec *volume.Spec, devicePath string, timeout time.Duration) (string, error) {
glog.Warning(logPrefix(a.plugin), "using default WaitForAttach for volume ", spec.Name, ", device ", devicePath)
glog.Warning(logPrefix(a.plugin.flexVolumePlugin), "using default WaitForAttach for volume ", spec.Name, ", device ", devicePath)
return devicePath, nil
}

// GetDeviceMountPath is part of the volume.Attacher interface
func (a *attacherDefaults) GetDeviceMountPath(spec *volume.Spec, mountsDir string) (string, error) {
glog.Warning(logPrefix(a.plugin), "using default GetDeviceMountPath for volume ", spec.Name, ", mountsDir ", mountsDir)
volumeName, err := a.plugin.GetVolumeName(spec)
if err != nil {
return "", fmt.Errorf("GetVolumeName failed from GetDeviceMountPath: %s", err)
}

return path.Join(mountsDir, volumeName), nil
return a.plugin.getDeviceMountPath(spec)
}

// MountDevice is part of the volume.Attacher interface
func (a *attacherDefaults) MountDevice(spec *volume.Spec, devicePath string, deviceMountPath string, mounter mount.Interface) error {
glog.Warning(logPrefix(a.plugin), "using default MountDevice for volume ", spec.Name, ", device ", devicePath, ", deviceMountPath ", deviceMountPath)
glog.Warning(logPrefix(a.plugin.flexVolumePlugin), "using default MountDevice for volume ", spec.Name, ", device ", devicePath, ", deviceMountPath ", deviceMountPath)
volSource, readOnly := getVolumeSource(spec)

options := make([]string, 0)
Expand Down
7 changes: 2 additions & 5 deletions pkg/volume/flexvolume/attacher.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package flexvolume

import (
"path"
"time"

"github.com/golang/glog"
Expand All @@ -26,7 +25,7 @@ import (
)

type flexVolumeAttacher struct {
plugin *flexVolumePlugin
plugin *flexVolumeAttachablePlugin
}

var _ volume.Attacher = &flexVolumeAttacher{}
Expand Down Expand Up @@ -64,9 +63,7 @@ func (a *flexVolumeAttacher) WaitForAttach(spec *volume.Spec, devicePath string,

// GetDeviceMountPath is part of the volume.Attacher interface
func (a *flexVolumeAttacher) GetDeviceMountPath(spec *volume.Spec) (string, error) {
mountsDir := path.Join(a.plugin.host.GetPluginDir(flexVolumePluginName), a.plugin.driverName, "mounts")

return (*attacherDefaults)(a).GetDeviceMountPath(spec, mountsDir)
return a.plugin.getDeviceMountPath(spec)
}

// MountDevice is part of the volume.Attacher interface
Expand Down
20 changes: 11 additions & 9 deletions pkg/volume/flexvolume/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,18 @@ import (
volumetesting "k8s.io/kubernetes/pkg/volume/testing"
)

func testPlugin() (*flexVolumePlugin, string) {
func testPlugin() (*flexVolumeAttachablePlugin, string) {
rootDir, err := utiltesting.MkTmpdir("flexvolume_test")
if err != nil {
panic("error creating temp dir: " + err.Error())
}
return &flexVolumePlugin{
driverName: "test",
execPath: "/plugin",
host: volumetesting.NewFakeVolumeHost(rootDir, nil, nil),
unsupportedCommands: []string{},
return &flexVolumeAttachablePlugin{
flexVolumePlugin: &flexVolumePlugin{
driverName: "test",
execPath: "/plugin",
host: volumetesting.NewFakeVolumeHost(rootDir, nil, nil),
unsupportedCommands: []string{},
},
}, rootDir
}

Expand Down Expand Up @@ -77,11 +79,11 @@ func fakeResultOutput(result interface{}) exec.FakeCombinedOutputAction {
}

func successOutput() exec.FakeCombinedOutputAction {
return fakeResultOutput(&DriverStatus{StatusSuccess, "", "", "", true})
return fakeResultOutput(&DriverStatus{StatusSuccess, "", "", "", true, nil})
}

func notSupportedOutput() exec.FakeCombinedOutputAction {
return fakeResultOutput(&DriverStatus{StatusNotSupported, "", "", "", false})
return fakeResultOutput(&DriverStatus{StatusNotSupported, "", "", "", false, nil})
}

func sameArgs(args, expectedArgs []string) bool {
Expand Down Expand Up @@ -126,7 +128,7 @@ func fakePersistentVolumeSpec() *volume.Spec {
return volume.NewSpecFromPersistentVolume(vol, false)
}

func specJson(plugin *flexVolumePlugin, spec *volume.Spec, extraOptions map[string]string) string {
func specJson(plugin *flexVolumeAttachablePlugin, spec *volume.Spec, extraOptions map[string]string) string {
o, err := NewOptionsForDriver(spec, plugin.host, extraOptions)
if err != nil {
panic("Failed to convert spec: " + err.Error())
Expand Down
6 changes: 3 additions & 3 deletions pkg/volume/flexvolume/detacher-defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,18 @@ type detacherDefaults flexVolumeDetacher

// Detach is part of the volume.Detacher interface.
func (d *detacherDefaults) Detach(deviceName string, hostName types.NodeName) error {
glog.Warning(logPrefix(d.plugin), "using default Detach for device ", deviceName, ", host ", hostName)
glog.Warning(logPrefix(d.plugin.flexVolumePlugin), "using default Detach for device ", deviceName, ", host ", hostName)
return nil
}

// WaitForDetach is part of the volume.Detacher interface.
func (d *detacherDefaults) WaitForDetach(devicePath string, timeout time.Duration) error {
glog.Warning(logPrefix(d.plugin), "using default WaitForDetach for device ", devicePath)
glog.Warning(logPrefix(d.plugin.flexVolumePlugin), "using default WaitForDetach for device ", devicePath)
return nil
}

// UnmountDevice is part of the volume.Detacher interface.
func (d *detacherDefaults) UnmountDevice(deviceMountPath string) error {
glog.Warning(logPrefix(d.plugin), "using default UnmountDevice for device mount path ", deviceMountPath)
glog.Warning(logPrefix(d.plugin.flexVolumePlugin), "using default UnmountDevice for device mount path ", deviceMountPath)
return util.UnmountPath(deviceMountPath, d.plugin.host.GetMounter())
}
2 changes: 1 addition & 1 deletion pkg/volume/flexvolume/detacher.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
)

type flexVolumeDetacher struct {
plugin *flexVolumePlugin
plugin *flexVolumeAttachablePlugin
}

var _ volume.Detacher = &flexVolumeDetacher{}
Expand Down
6 changes: 6 additions & 0 deletions pkg/volume/flexvolume/driver-call.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ const (
optionKeyPodUID = "kubernetes.io/pod.uid"

optionKeyServiceAccountName = "kubernetes.io/serviceAccount.name"

attachCapability = "attach"
)

const (
Expand Down Expand Up @@ -201,6 +203,10 @@ type DriverStatus struct {
VolumeName string `json:"volumeName,omitempty"`
// Represents volume is attached on the node
Attached bool `json:"attached,omitempty"`
// Returns capabilities of the driver.
// By default we assume all the capabilities are supported.
// If the plugin does not support a capability, it can return false for that capability.
Capabilities map[string]bool
}

// isCmdNotSupportedErr checks if the error corresponds to command not supported by
Expand Down
10 changes: 2 additions & 8 deletions pkg/volume/flexvolume/mounter-defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ limitations under the License.
package flexvolume

import (
"fmt"

"github.com/golang/glog"

"k8s.io/apimachinery/pkg/types"
Expand All @@ -32,13 +30,9 @@ type mounterDefaults flexVolumeMounter
func (f *mounterDefaults) SetUpAt(dir string, fsGroup *types.UnixGroupID) error {
glog.Warning(logPrefix(f.plugin), "using default SetUpAt to ", dir)

a, err := f.plugin.NewAttacher()
if err != nil {
return fmt.Errorf("NewAttacher failed: %v", err)
}
src, err := a.GetDeviceMountPath(f.spec)
src, err := f.plugin.getDeviceMountPath(f.spec)
if err != nil {
return fmt.Errorf("GetDeviceMountPath failed: %v", err)
return err
}

if err := doMount(f.mounter, src, dir, "auto", []string{"bind"}); err != nil {
Expand Down
65 changes: 62 additions & 3 deletions pkg/volume/flexvolume/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package flexvolume

import (
"fmt"
"path"
"strings"
"sync"
Expand All @@ -27,6 +28,7 @@ import (
api "k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/util/exec"
"k8s.io/kubernetes/pkg/util/mount"
utilstrings "k8s.io/kubernetes/pkg/util/strings"
"k8s.io/kubernetes/pkg/volume"
)

Expand All @@ -43,9 +45,56 @@ type flexVolumePlugin struct {
unsupportedCommands []string
}

var _ volume.AttachableVolumePlugin = &flexVolumePlugin{}
type flexVolumeAttachablePlugin struct {
*flexVolumePlugin
}

var _ volume.AttachableVolumePlugin = &flexVolumeAttachablePlugin{}
var _ volume.PersistentVolumePlugin = &flexVolumePlugin{}

func NewFlexVolumePlugin(pluginDir, name string) (volume.VolumePlugin, error) {
execPath := path.Join(pluginDir, name)

driverName := utilstrings.UnescapePluginName(name)

flexPlugin := &flexVolumePlugin{
driverName: driverName,
execPath: execPath,
runner: exec.New(),
unsupportedCommands: []string{},
}

// Check whether the plugin is attachable.
ok, err := isAttachable(flexPlugin)
if err != nil {
return nil, err
}

if ok {
// Plugin supports attach/detach, so return flexVolumeAttachablePlugin
return &flexVolumeAttachablePlugin{flexVolumePlugin: flexPlugin}, nil
} else {
return flexPlugin, nil
}
}

func isAttachable(plugin *flexVolumePlugin) (bool, error) {
call := plugin.NewDriverCall(initCmd)
res, err := call.Run()
if err != nil {
return false, err
}

// By default all plugins are attachable, unless they report otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

How about making the default false, aka non-attachable? And if a plugin implements attach, then they must explicitly also set capability to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is going to break existing plugins. That is the only reason. Doesn't benefit too much either-way.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify it would break existing post-1.6 drivers right? Pre-1.6 drivers would continue to work as is if the default was false, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. For Post 1.6 drivers. Users have to modify their drivers to work for 1.6+ anyways, because of change mount callout.

Copy link
Member

Choose a reason for hiding this comment

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

Ack. Ok fine to leave as is.

cap, ok := res.Capabilities[attachCapability]
if ok {
// cap is false, so plugin does not support attach/detach calls.
return cap, nil
}

return true, nil
}

// Init is part of the volume.VolumePlugin interface.
func (plugin *flexVolumePlugin) Init(host volume.VolumeHost) error {
plugin.host = host
Expand Down Expand Up @@ -155,12 +204,12 @@ func (plugin *flexVolumePlugin) newUnmounterInternal(volName string, podUID type
}

// NewAttacher is part of the volume.AttachableVolumePlugin interface.
func (plugin *flexVolumePlugin) NewAttacher() (volume.Attacher, error) {
func (plugin *flexVolumeAttachablePlugin) NewAttacher() (volume.Attacher, error) {
return &flexVolumeAttacher{plugin}, nil
}

// NewDetacher is part of the volume.AttachableVolumePlugin interface.
func (plugin *flexVolumePlugin) NewDetacher() (volume.Detacher, error) {
func (plugin *flexVolumeAttachablePlugin) NewDetacher() (volume.Detacher, error) {
return &flexVolumeDetacher{plugin}, nil
}

Expand Down Expand Up @@ -208,3 +257,13 @@ func (plugin *flexVolumePlugin) GetDeviceMountRefs(deviceMountPath string) ([]st
mounter := plugin.host.GetMounter()
return mount.GetMountRefs(mounter, deviceMountPath)
}

func (plugin *flexVolumePlugin) getDeviceMountPath(spec *volume.Spec) (string, error) {
volumeName, err := plugin.GetVolumeName(spec)
if err != nil {
return "", fmt.Errorf("GetVolumeName failed from getDeviceMountPath: %s", err)
}

mountsDir := path.Join(plugin.host.GetPluginDir(flexVolumePluginName), plugin.driverName, "mounts")
return path.Join(mountsDir, volumeName), nil
}
16 changes: 6 additions & 10 deletions pkg/volume/flexvolume/probe.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@ package flexvolume

import (
"io/ioutil"
"path"

"k8s.io/kubernetes/pkg/util/exec"
utilstrings "k8s.io/kubernetes/pkg/util/strings"
"k8s.io/kubernetes/pkg/volume"
)

Expand All @@ -37,13 +34,12 @@ func ProbeVolumePlugins(pluginDir string) []volume.VolumePlugin {
// e.g. dirname = vendor~cifs
// then, executable will be pluginDir/dirname/cifs
if f.IsDir() {
execPath := path.Join(pluginDir, f.Name())
plugins = append(plugins, &flexVolumePlugin{
driverName: utilstrings.UnescapePluginName(f.Name()),
execPath: execPath,
runner: exec.New(),
unsupportedCommands: []string{},
})
plugin, err := NewFlexVolumePlugin(pluginDir, f.Name())
Copy link
Member

Choose a reason for hiding this comment

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

Beautiful! Thanks for refactoring, so much cleaner!

if err != nil {
continue
}

plugins = append(plugins, plugin)
}
}
return plugins
Expand Down