From bc6ad1ad9fed0c92a42b9e7f8572502d76ca8d5b Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Wed, 23 Aug 2017 14:56:51 +0200 Subject: [PATCH] iscsi: Use VolumeHost.GetExec() to execute stuff in volume plugins --- pkg/volume/iscsi/BUILD | 3 - pkg/volume/iscsi/iscsi.go | 13 ++- pkg/volume/iscsi/iscsi_util.go | 38 ++++----- pkg/volume/iscsi/iscsi_util_test.go | 119 ++++++++++++++-------------- 4 files changed, 83 insertions(+), 90 deletions(-) diff --git a/pkg/volume/iscsi/BUILD b/pkg/volume/iscsi/BUILD index 14348017bf..50e1ca3e42 100644 --- a/pkg/volume/iscsi/BUILD +++ b/pkg/volume/iscsi/BUILD @@ -22,7 +22,6 @@ go_library( "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", - "//vendor/k8s.io/utils/exec:go_default_library", ], ) @@ -42,8 +41,6 @@ go_test( "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/client-go/kubernetes/fake:go_default_library", "//vendor/k8s.io/client-go/util/testing:go_default_library", - "//vendor/k8s.io/utils/exec:go_default_library", - "//vendor/k8s.io/utils/exec/testing:go_default_library", ], ) diff --git a/pkg/volume/iscsi/iscsi.go b/pkg/volume/iscsi/iscsi.go index b2a5295894..bf66c66adb 100644 --- a/pkg/volume/iscsi/iscsi.go +++ b/pkg/volume/iscsi/iscsi.go @@ -28,17 +28,15 @@ import ( utilstrings "k8s.io/kubernetes/pkg/util/strings" "k8s.io/kubernetes/pkg/volume" ioutil "k8s.io/kubernetes/pkg/volume/util" - "k8s.io/utils/exec" ) // This is the primary entrypoint for volume plugins. func ProbeVolumePlugins() []volume.VolumePlugin { - return []volume.VolumePlugin{&iscsiPlugin{nil, exec.New()}} + return []volume.VolumePlugin{&iscsiPlugin{nil}} } type iscsiPlugin struct { host volume.VolumeHost - exe exec.Interface } var _ volume.VolumePlugin = &iscsiPlugin{} @@ -154,6 +152,7 @@ func (plugin *iscsiPlugin) newMounterInternal(spec *volume.Spec, podUID types.UI fsType: iscsi.FSType, readOnly: readOnly, mounter: &mount.SafeFormatAndMount{Interface: mounter, Exec: exec}, + exec: exec, deviceUtil: ioutil.NewDeviceHandler(ioutil.NewIOHandler()), mountOptions: volume.MountOptionFromSpec(spec), }, nil @@ -173,14 +172,10 @@ func (plugin *iscsiPlugin) newUnmounterInternal(volName string, podUID types.UID plugin: plugin, }, mounter: mounter, + exec: exec, }, nil } -func (plugin *iscsiPlugin) execCommand(command string, args []string) ([]byte, error) { - cmd := plugin.exe.Command(command, args...) - return cmd.CombinedOutput() -} - func (plugin *iscsiPlugin) ConstructVolumeSpec(volumeName, mountPath string) (*volume.Spec, error) { iscsiVolume := &v1.Volume{ Name: volumeName, @@ -222,6 +217,7 @@ type iscsiDiskMounter struct { readOnly bool fsType string mounter *mount.SafeFormatAndMount + exec mount.Exec deviceUtil ioutil.DeviceUtil mountOptions []string } @@ -259,6 +255,7 @@ func (b *iscsiDiskMounter) SetUpAt(dir string, fsGroup *int64) error { type iscsiDiskUnmounter struct { *iscsiDisk mounter mount.Interface + exec mount.Exec } var _ volume.Unmounter = &iscsiDiskUnmounter{} diff --git a/pkg/volume/iscsi/iscsi_util.go b/pkg/volume/iscsi/iscsi_util.go index 21e6cdebde..ddac19bd85 100755 --- a/pkg/volume/iscsi/iscsi_util.go +++ b/pkg/volume/iscsi/iscsi_util.go @@ -46,7 +46,7 @@ var ( func updateISCSIDiscoverydb(b iscsiDiskMounter, tp string) error { if b.chap_discovery { - out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "update", "-n", "discovery.sendtargets.auth.authmethod", "-v", "CHAP"}) + out, err := b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "update", "-n", "discovery.sendtargets.auth.authmethod", "-v", "CHAP") if err != nil { return fmt.Errorf("iscsi: failed to update discoverydb with CHAP, output: %v", string(out)) } @@ -54,7 +54,7 @@ func updateISCSIDiscoverydb(b iscsiDiskMounter, tp string) error { for _, k := range chap_st { v := b.secret[k] if len(v) > 0 { - out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "update", "-n", k, "-v", v}) + out, err := b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "update", "-n", k, "-v", v) if err != nil { return fmt.Errorf("iscsi: failed to update discoverydb key %q with value %q error: %v", k, v, string(out)) } @@ -66,7 +66,7 @@ func updateISCSIDiscoverydb(b iscsiDiskMounter, tp string) error { func updateISCSINode(b iscsiDiskMounter, tp string) error { if b.chap_session { - out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "-o", "update", "-n", "node.session.auth.authmethod", "-v", "CHAP"}) + out, err := b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "-o", "update", "-n", "node.session.auth.authmethod", "-v", "CHAP") if err != nil { return fmt.Errorf("iscsi: failed to update node with CHAP, output: %v", string(out)) } @@ -74,7 +74,7 @@ func updateISCSINode(b iscsiDiskMounter, tp string) error { for _, k := range chap_sess { v := b.secret[k] if len(v) > 0 { - out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "-o", "update", "-n", k, "-v", v}) + out, err := b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "-o", "update", "-n", k, "-v", v) if err != nil { return fmt.Errorf("iscsi: failed to update node session key %q with value %q error: %v", k, v, string(out)) } @@ -196,7 +196,7 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error { var iscsiTransport string var lastErr error - out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "iface", "-I", b.Iface, "-o", "show"}) + out, err := b.exec.Run("iscsiadm", "-m", "iface", "-I", b.Iface, "-o", "show") if err != nil { glog.Errorf("iscsi: could not read iface %s error: %s", b.Iface, string(out)) return err @@ -222,7 +222,7 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error { for _, tp := range bkpPortal { // Rescan sessions to discover newly mapped LUNs. Do not specify the interface when rescanning // to avoid establishing additional sessions to the same target. - out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", tp, "-T", b.Iqn, "-R"}) + out, err := b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-R") if err != nil { glog.Errorf("iscsi: failed to rescan session with error: %s (%v)", string(out), err) } @@ -238,17 +238,17 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error { exist := waitForPathToExist(&devicePath, 1, iscsiTransport) if exist == false { // build discoverydb and discover iscsi target - b.plugin.execCommand("iscsiadm", []string{"-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "new"}) + b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "new") // update discoverydb with CHAP secret err = updateISCSIDiscoverydb(b, tp) if err != nil { lastErr = fmt.Errorf("iscsi: failed to update discoverydb to portal %s error: %v", tp, err) continue } - out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "--discover"}) + out, err := b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "--discover") if err != nil { // delete discoverydb record - b.plugin.execCommand("iscsiadm", []string{"-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "delete"}) + b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "delete") lastErr = fmt.Errorf("iscsi: failed to sendtargets to portal %s output: %s, err %v", tp, string(out), err) continue } @@ -259,10 +259,10 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error { continue } // login to iscsi target - out, err = b.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "--login"}) + out, err = b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "--login") if err != nil { // delete the node record from database - b.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", tp, "-I", b.Iface, "-T", b.Iqn, "-o", "delete"}) + b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-I", b.Iface, "-T", b.Iqn, "-o", "delete") lastErr = fmt.Errorf("iscsi: failed to attach disk: Error: %s (%v)", string(out), err) continue } @@ -283,7 +283,7 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error { if len(devicePaths) == 0 { // delete cloned iface - b.plugin.execCommand("iscsiadm", []string{"-m", "iface", "-I", b.Iface, "-o", "delete"}) + b.exec.Run("iscsiadm", "-m", "iface", "-I", b.Iface, "-o", "delete") glog.Errorf("iscsi: failed to get any path for iscsi disk, last err seen:\n%v", lastErr) return fmt.Errorf("failed to get any path for iscsi disk, last err seen:\n%v", lastErr) } @@ -381,13 +381,13 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error { delete = append(delete, []string{"-I", iface}...) } glog.Infof("iscsi: log out target %s iqn %s iface %s", portal, iqn, iface) - out, err := c.plugin.execCommand("iscsiadm", logout) + out, err := c.exec.Run("iscsiadm", logout...) if err != nil { glog.Errorf("iscsi: failed to detach disk Error: %s", string(out)) } // Delete the node record glog.Infof("iscsi: delete node record target %s iqn %s", portal, iqn) - out, err = c.plugin.execCommand("iscsiadm", delete) + out, err = c.exec.Run("iscsiadm", delete...) if err != nil { glog.Errorf("iscsi: failed to delete node record Error: %s", string(out)) } @@ -396,7 +396,7 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error { // If the iface is not created via iscsi plugin, skip to delete if initiatorName != "" && found && iface == (portals[0]+":"+c.volName) { delete := []string{"-m", "iface", "-I", iface, "-o", "delete"} - out, err := c.plugin.execCommand("iscsiadm", delete) + out, err := c.exec.Run("iscsiadm", delete...) if err != nil { glog.Errorf("iscsi: failed to delete iface Error: %s", string(out)) } @@ -504,7 +504,7 @@ func parseIscsiadmShow(output string) (map[string]string, error) { func cloneIface(b iscsiDiskMounter, newIface string) error { var lastErr error // get pre-configured iface records - out, err := b.plugin.execCommand("iscsiadm", []string{"-m", "iface", "-I", b.Iface, "-o", "show"}) + out, err := b.exec.Run("iscsiadm", "-m", "iface", "-I", b.Iface, "-o", "show") if err != nil { lastErr = fmt.Errorf("iscsi: failed to show iface records: %s (%v)", string(out), err) return lastErr @@ -518,16 +518,16 @@ func cloneIface(b iscsiDiskMounter, newIface string) error { // update initiatorname params["iface.initiatorname"] = b.InitiatorName // create new iface - out, err = b.plugin.execCommand("iscsiadm", []string{"-m", "iface", "-I", newIface, "-o", "new"}) + out, err = b.exec.Run("iscsiadm", "-m", "iface", "-I", newIface, "-o", "new") if err != nil { lastErr = fmt.Errorf("iscsi: failed to create new iface: %s (%v)", string(out), err) return lastErr } // update new iface records for key, val := range params { - _, err = b.plugin.execCommand("iscsiadm", []string{"-m", "iface", "-I", newIface, "-o", "update", "-n", key, "-v", val}) + _, err = b.exec.Run("iscsiadm", "-m", "iface", "-I", newIface, "-o", "update", "-n", key, "-v", val) if err != nil { - b.plugin.execCommand("iscsiadm", []string{"-m", "iface", "-I", newIface, "-o", "delete"}) + b.exec.Run("iscsiadm", "-m", "iface", "-I", newIface, "-o", "delete") lastErr = fmt.Errorf("iscsi: failed to update iface records: %s (%v). iface(%s) will be used", string(out), err, b.Iface) break } diff --git a/pkg/volume/iscsi/iscsi_util_test.go b/pkg/volume/iscsi/iscsi_util_test.go index cab0baf39b..30eb7f9063 100755 --- a/pkg/volume/iscsi/iscsi_util_test.go +++ b/pkg/volume/iscsi/iscsi_util_test.go @@ -18,6 +18,7 @@ package iscsi import ( "errors" + "fmt" "os" "path/filepath" "reflect" @@ -25,8 +26,6 @@ import ( "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/volume" - "k8s.io/utils/exec" - fakeexec "k8s.io/utils/exec/testing" ) func TestGetDevicePrefixRefCount(t *testing.T) { @@ -247,117 +246,117 @@ func TestParseIscsiadmShow(t *testing.T) { } func TestClonedIface(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ + cmdCount := 0 + fakeExec := mount.NewFakeExec(func(cmd string, args ...string) ([]byte, error) { + cmdCount++ + if cmd != "iscsiadm" { + t.Errorf("iscsiadm command expected, got %q", cmd) + } + switch cmdCount { + case 1: // iscsiadm -m iface -I -o show - func() ([]byte, error) { - return []byte("iface.ipaddress = \niface.transport_name = tcp\niface.initiatorname = \n"), nil - }, + return []byte("iface.ipaddress = \niface.transport_name = tcp\niface.initiatorname = \n"), nil + + case 2: // iscsiadm -m iface -I -o new - func() ([]byte, error) { return []byte("New interface 192.168.1.10:pv0001 added"), nil }, + return []byte("New interface 192.168.1.10:pv0001 added"), nil + case 3: // iscsiadm -m iface -I -o update -n -v - func() ([]byte, error) { return []byte(""), nil }, - func() ([]byte, error) { return []byte(""), nil }, - }, - } - fexec := fakeexec.FakeExec{ - CommandScript: []fakeexec.FakeCommandAction{ - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } + return []byte(""), nil + case 4: + return []byte(""), nil + } + return nil, fmt.Errorf("Unexpected exec call nr %d: %s", cmdCount, cmd) + }) plugins := []volume.VolumePlugin{ &iscsiPlugin{ host: nil, - exe: &fexec, }, } plugin := plugins[0] fakeMounter := iscsiDiskMounter{ iscsiDisk: &iscsiDisk{ plugin: plugin.(*iscsiPlugin)}, + exec: fakeExec, } newIface := "192.168.1.10:pv0001" cloneIface(fakeMounter, newIface) - if fcmd.CombinedOutputCalls != 4 { - t.Errorf("expected 4 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if cmdCount != 4 { + t.Errorf("expected 4 CombinedOutput() calls, got %d", cmdCount) } } func TestClonedIfaceShowError(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ - // iscsiadm -m iface -I -o show, return test error - func() ([]byte, error) { return []byte(""), errors.New("test error") }, - }, - } - fexec := fakeexec.FakeExec{ - CommandScript: []fakeexec.FakeCommandAction{ - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } + cmdCount := 0 + fakeExec := mount.NewFakeExec(func(cmd string, args ...string) ([]byte, error) { + cmdCount++ + if cmd != "iscsiadm" { + t.Errorf("iscsiadm command expected, got %q", cmd) + } + // iscsiadm -m iface -I -o show, return test error + return []byte(""), errors.New("test error") + }) plugins := []volume.VolumePlugin{ &iscsiPlugin{ host: nil, - exe: &fexec, }, } plugin := plugins[0] fakeMounter := iscsiDiskMounter{ iscsiDisk: &iscsiDisk{ plugin: plugin.(*iscsiPlugin)}, + exec: fakeExec, } newIface := "192.168.1.10:pv0001" cloneIface(fakeMounter, newIface) - if fcmd.CombinedOutputCalls != 1 { - t.Errorf("expected 1 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if cmdCount != 1 { + t.Errorf("expected 1 CombinedOutput() calls, got %d", cmdCount) } } func TestClonedIfaceUpdateError(t *testing.T) { - fcmd := fakeexec.FakeCmd{ - CombinedOutputScript: []fakeexec.FakeCombinedOutputAction{ + cmdCount := 0 + fakeExec := mount.NewFakeExec(func(cmd string, args ...string) ([]byte, error) { + cmdCount++ + if cmd != "iscsiadm" { + t.Errorf("iscsiadm command expected, got %q", cmd) + } + switch cmdCount { + case 1: // iscsiadm -m iface -I -o show - func() ([]byte, error) { - return []byte("iface.ipaddress = \niface.transport_name = tcp\niface.initiatorname = \n"), nil - }, + return []byte("iface.ipaddress = \niface.transport_name = tcp\niface.initiatorname = \n"), nil + + case 2: // iscsiadm -m iface -I -o new - func() ([]byte, error) { return []byte("New interface 192.168.1.10:pv0001 added"), nil }, + return []byte("New interface 192.168.1.10:pv0001 added"), nil + case 3: // iscsiadm -m iface -I -o update -n -v - func() ([]byte, error) { return []byte(""), nil }, - func() ([]byte, error) { return []byte(""), errors.New("test error") }, + return []byte(""), nil + case 4: + return []byte(""), errors.New("test error") + case 5: // iscsiadm -m iface -I -o delete - func() ([]byte, error) { return []byte(""), nil }, - }, - } - fexec := fakeexec.FakeExec{ - CommandScript: []fakeexec.FakeCommandAction{ - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - func(cmd string, args ...string) exec.Cmd { return fakeexec.InitFakeCmd(&fcmd, cmd, args...) }, - }, - } + return []byte(""), nil + } + return nil, fmt.Errorf("Unexpected exec call nr %d: %s", cmdCount, cmd) + }) plugins := []volume.VolumePlugin{ &iscsiPlugin{ host: nil, - exe: &fexec, }, } plugin := plugins[0] fakeMounter := iscsiDiskMounter{ iscsiDisk: &iscsiDisk{ plugin: plugin.(*iscsiPlugin)}, + exec: fakeExec, } newIface := "192.168.1.10:pv0001" cloneIface(fakeMounter, newIface) - if fcmd.CombinedOutputCalls != 5 { - t.Errorf("expected 5 CombinedOutput() calls, got %d", fcmd.CombinedOutputCalls) + if cmdCount != 5 { + t.Errorf("expected 5 CombinedOutput() calls, got %d", cmdCount) } }