From a11bd0f755ca036f891eac5d8853c278a656c755 Mon Sep 17 00:00:00 2001 From: Lou Date: Thu, 9 Jan 2020 21:06:07 +0800 Subject: [PATCH 1/5] delete individual volume Signed-off-by: Lou --- iscsi/iscsi.go | 79 ++++++++++++++++++-- iscsi/iscsi_test.go | 171 ++++++++++++++++++++++++++++++++++++++++++++ iscsi/multipath.go | 87 ++++++++++++++++++++++ 3 files changed, 331 insertions(+), 6 deletions(-) create mode 100644 iscsi/multipath.go diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 3f9a0471..1ee5a550 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -19,8 +19,9 @@ import ( const defaultPort = "3260" var ( - debug *log.Logger - execCommand = exec.Command + debug *log.Logger + execCommand = exec.Command + execWithTimeout = ExecWithTimeout ) type statFunc func(string) (os.FileInfo, error) @@ -50,10 +51,14 @@ type Connector struct { SessionSecrets Secrets `json:"session_secrets"` Interface string `json:"interface"` Multipath bool `json:"multipath"` - RetryCount int32 `json:"retry_count"` - CheckInterval int32 `json:"check_interval"` - DoDiscovery bool `json:"do_discovery"` - DoCHAPDiscovery bool `json:"do_chap_discovery"` + + // DevicePath is dm-x for a multipath device, and sdx for a normal device. + DevicePath string `json:"device_path"` + + RetryCount int32 `json:"retry_count"` + CheckInterval int32 `json:"check_interval"` + DoDiscovery bool `json:"do_discovery"` + DoCHAPDiscovery bool `json:"do_chap_discovery"` } func init() { @@ -350,6 +355,68 @@ func Disconnect(tgtIqn string, portals []string) error { return err } +// DisconnectVolume removes a volume from a Linux host. +func DisconnectVolume(c Connector) error { + // Steps to safely remove an iSCSI storage volume from a Linux host are as following: + // 1. Unmount the disk from a filesystem on the system. + // 2. Flush the multipath map for the disk we’re removing (if multipath is enabled). + // 3. Remove the physical disk entities that Linux maintains. + // 4. Take the storage volume (disk) offline on the storage subsystem. + // 5. Rescan the iSCSI sessions. + // + // DisconnectVolume focuses on step 2 and 3. + // Note: make sure the volume is already unmounted before calling this method. + if c.Multipath { + err := Flush(c.DevicePath) + if err != nil { + return err + } + devices, err := GetSysDevicesFromMultipathDevice(c.DevicePath) + if err != nil { + return err + } + return RemovePhysicalDevice(devices...) + } + return RemovePhysicalDevice(c.DevicePath) +} + +// RemovePhysicalDevice removes device(s) sdx from a Linux host. +func RemovePhysicalDevice(devices ...string) error { + debug.Printf("Removing scsi device %v.\n", devices) + var errs []error + for _, deviceName := range devices { + if deviceName == "" { + continue + } + + debug.Printf("Delete scsi device %v.\n", deviceName) + // Remove a scsi device by executing 'echo "1" > /sys/block/sdx/device/delete + filename := filepath.Join(sysBlockPath, deviceName, "device", "delete") + if f, err := os.OpenFile(filename, os.O_APPEND|os.O_WRONLY, 0200); err != nil { + if os.IsNotExist(err) { + continue + } else { + debug.Printf("Error while opening file %v: %v\n", filename, err) + errs = append(errs, err) + continue + } + } else { + defer f.Close() + if _, err := f.WriteString("1"); err != nil { + debug.Printf("Error while writing to file %v: %v", filename, err) + errs = append(errs, err) + continue + } + } + } + + if len(errs) > 0 { + return errs[0] + } + debug.Println("Finshed to remove SCSI devices.") + return nil +} + // PersistConnector persists the provided Connector to the specified file (ie /var/lib/pfile/myConnector.json) func PersistConnector(c *Connector, filePath string) error { //file := path.Join("mnt", c.VolumeName+".json") diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index b2c8879b..118264c5 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -1,12 +1,16 @@ package iscsi import ( + "context" "fmt" + "io/ioutil" "os" "os/exec" + "path/filepath" "reflect" "strconv" "testing" + "time" ) var nodeDB = ` @@ -79,10 +83,16 @@ node.conn[0].iscsi.OFMarker = No var emptyTransportName = "iface.transport_name = \n" var emptyDbRecord = "\n\n\n" var testCmdOutput = "" +var testCmdTimeout = false var testCmdError error +var testExecWithTimeoutError error var mockedExitStatus = 0 var mockedStdout string +var normalDevice = "sda" +var multipathDevice = "dm-1" +var slaves = []string{"sdb", "sdc"} + type testCmdRunner struct{} func fakeExecCommand(command string, args ...string) *exec.Cmd { @@ -96,6 +106,13 @@ func fakeExecCommand(command string, args ...string) *exec.Cmd { return cmd } +func fakeExecWithTimeout(command string, args []string, timeout time.Duration) ([]byte, error) { + if testCmdTimeout { + return nil, context.DeadlineExceeded + } + return []byte(testCmdOutput), testExecWithTimeoutError +} + func TestExecCommandHelper(t *testing.T) { if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { return @@ -111,6 +128,33 @@ func (tr testCmdRunner) execCmd(cmd string, args ...string) (string, error) { } +func preparePaths(sysBlockPath string) error { + for _, d := range append(slaves, normalDevice) { + devicePath := filepath.Join(sysBlockPath, d, "device") + err := os.MkdirAll(devicePath, os.ModePerm) + if err != nil { + return err + } + err = ioutil.WriteFile(filepath.Join(devicePath, "delete"), []byte(""), 0600) + if err != nil { + return err + } + } + for _, s := range slaves { + err := os.MkdirAll(filepath.Join(sysBlockPath, multipathDevice, "slaves", s), os.ModePerm) + if err != nil { + return err + } + } + + err := os.MkdirAll(filepath.Join(sysBlockPath, "dev", multipathDevice), os.ModePerm) + if err != nil { + return err + } + return nil + +} + func Test_parseSessions(t *testing.T) { var sessions []iscsiSession output := "tcp: [2] 192.168.1.107:3260,1 iqn.2010-10.org.openstack:volume-eb393993-73d0-4e39-9ef4-b5841e244ced (non-flash)\n" + @@ -226,3 +270,130 @@ func Test_sessionExists(t *testing.T) { }) } } + +func Test_DisconnectNormalVolume(t *testing.T) { + + tmpDir, err := ioutil.TempDir("", "") + if err != nil { + t.Errorf("can not create temp directory: %v", err) + return + } + sysBlockPath = tmpDir + defer os.RemoveAll(tmpDir) + + err = preparePaths(tmpDir) + if err != nil { + t.Errorf("can not create temp directories and files: %v", err) + return + } + + execWithTimeout = fakeExecWithTimeout + devicePath := normalDevice + + tests := []struct { + name string + removeDevice bool + wantErr bool + }{ + {"DisconnectNormalVolume", false, false}, + {"DisconnectNonexistentNormalVolume", true, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.removeDevice { + os.RemoveAll(filepath.Join(sysBlockPath, devicePath)) + } + c := Connector{Multipath: false, DevicePath: devicePath} + err := DisconnectVolume(c) + if (err != nil) != tt.wantErr { + t.Errorf("DisconnectVolume() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if !tt.removeDevice { + deleteFile := filepath.Join(sysBlockPath, devicePath, "device", "delete") + out, err := ioutil.ReadFile(deleteFile) + if err != nil { + t.Errorf("can not read file %v: %v", deleteFile, err) + return + } + if string(out) != "1" { + t.Errorf("file content mismatch, got = %s, want = 1", string(out)) + return + } + } + }) + } +} + +func Test_DisconnectMultipathVolume(t *testing.T) { + + tmpDir, err := ioutil.TempDir("", "") + if err != nil { + t.Errorf("can not create temp directory: %v", err) + return + } + sysBlockPath = tmpDir + devPath = filepath.Join(tmpDir, "dev") + defer os.RemoveAll(tmpDir) + + err = preparePaths(tmpDir) + if err != nil { + t.Errorf("can not create temp directories and files: %v", err) + return + } + + execWithTimeout = fakeExecWithTimeout + devicePath := multipathDevice + + tests := []struct { + name string + timeout bool + removeDevice bool + wantErr bool + cmdError error + }{ + {"DisconnectMultipathVolume", false, false, false, nil}, + {"DisconnectMultipathVolumeFlushFailed", false, false, true, fmt.Errorf("error")}, + {"DisconnectMultipathVolumeFlushTimeout", true, false, true, nil}, + {"DisconnectNonexistentMultipathVolume", false, false, true, fmt.Errorf("error")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testExecWithTimeoutError = tt.cmdError + testCmdTimeout = tt.timeout + if tt.removeDevice { + os.RemoveAll(filepath.Join(sysBlockPath, devicePath)) + os.RemoveAll(devPath) + } + c := Connector{Multipath: true, DevicePath: devicePath} + err := DisconnectVolume(c) + if (err != nil) != tt.wantErr { + t.Errorf("DisconnectVolume() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.timeout { + if err != context.DeadlineExceeded { + t.Errorf("DisconnectVolume() error = %v, wantErr %v", err, context.DeadlineExceeded) + return + } + } + + if !tt.removeDevice { + for _, s := range slaves { + deleteFile := filepath.Join(sysBlockPath, s, "device", "delete") + out, err := ioutil.ReadFile(deleteFile) + if err != nil { + t.Errorf("can not read file %v: %v", deleteFile, err) + return + } + if string(out) != "1" { + t.Errorf("file content mismatch, got = %s, want = 1", string(out)) + return + } + } + + } + }) + } +} diff --git a/iscsi/multipath.go b/iscsi/multipath.go new file mode 100644 index 00000000..bccfb695 --- /dev/null +++ b/iscsi/multipath.go @@ -0,0 +1,87 @@ +package iscsi + +import ( + "context" + "io/ioutil" + "os" + "os/exec" + "path/filepath" + "time" +) + +var sysBlockPath = "/sys/block" +var devPath = "/dev" + +func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]byte, error) { + debug.Printf("Executing command '%v' with args: '%v'.\n", command, args) + + // Create a new context and add a timeout to it + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + // Create command with context + cmd := exec.CommandContext(ctx, command, args...) + + // This time we can simply use Output() to get the result. + out, err := cmd.Output() + + // We want to check the context error to see if the timeout was executed. + // The error returned by cmd.Output() will be OS specific based on what + // happens when a process is killed. + if ctx.Err() == context.DeadlineExceeded { + debug.Printf("Command '%s' timeout reached.\n", command) + return nil, ctx.Err() + } + + // If there's no context error, we know the command completed (or errored). + debug.Printf("Output from command: %s", string(out)) + if err != nil { + debug.Printf("Non-zero exit code: %s\n", err) + } + + debug.Println("Finished executing command.") + return out, err +} + +// GetSysDevicesFromMultipathDevice gets all slaves for multipath device dm-x +// in /sys/block/dm-x/slaves/ +func GetSysDevicesFromMultipathDevice(device string) ([]string, error) { + debug.Printf("Getting all slaves for multipath device %s.\n", device) + deviceSlavePath := filepath.Join(sysBlockPath, device, "slaves") + slaves, err := ioutil.ReadDir(deviceSlavePath) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + debug.Printf("An error occured while looking for slaves: %v\n", err) + return nil, err + } + + var s []string + for _, slave := range slaves { + s = append(s, slave.Name()) + } + debug.Printf("Found slaves: %v.\n", s) + return s, nil +} + +// Flush flushes a multipath device dm-x with command multipath -f /dev/dm-x +func Flush(device string) error { + debug.Printf("Flushing multipath device '%v'.\n", device) + + fullDevice := filepath.Join(devPath, device) + timeout := 5 * time.Second + _, err := execWithTimeout("multipath", []string{"-f", fullDevice}, timeout) + + if err != nil { + if _, e := os.Stat(fullDevice); os.IsNotExist(e) { + debug.Printf("Multipath device %v was deleted.\n", device) + } else { + debug.Printf("Command 'multipath -f %v' did not succeed to delete the device: %v\n", fullDevice, err) + return err + } + } + + debug.Printf("Finshed flushing multipath device %v.\n", device) + return nil +} From 6987264e1e94eb6837c2e3fc9d74ad8d66c558fa Mon Sep 17 00:00:00 2001 From: Lou Date: Fri, 10 Jan 2020 17:36:25 +0800 Subject: [PATCH 2/5] update after review Signed-off-by: Lou --- iscsi/iscsi.go | 21 +++++++++++++++++---- iscsi/multipath.go | 4 ++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 1ee5a550..8f031734 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -366,8 +366,11 @@ func DisconnectVolume(c Connector) error { // // DisconnectVolume focuses on step 2 and 3. // Note: make sure the volume is already unmounted before calling this method. + + debug.Printf("Disconnecting volume in path %s.\n", c.DevicePath) if c.Multipath { - err := Flush(c.DevicePath) + debug.Printf("Removing multipath device.\n") + err := FlushMultipathDevice(c.DevicePath) if err != nil { return err } @@ -375,9 +378,19 @@ func DisconnectVolume(c Connector) error { if err != nil { return err } - return RemovePhysicalDevice(devices...) + debug.Printf("Found multipath slaves %v, removing all of them.\n", devices) + if err := RemovePhysicalDevice(devices...); err != nil { + return err + } + } else { + debug.Printf("Removing normal device.\n") + if err := RemovePhysicalDevice(c.DevicePath); err != nil { + return err + } } - return RemovePhysicalDevice(c.DevicePath) + + debug.Printf("Finished disconnecting volume.\n") + return nil } // RemovePhysicalDevice removes device(s) sdx from a Linux host. @@ -413,7 +426,7 @@ func RemovePhysicalDevice(devices ...string) error { if len(errs) > 0 { return errs[0] } - debug.Println("Finshed to remove SCSI devices.") + debug.Println("Finshed removing SCSI devices.") return nil } diff --git a/iscsi/multipath.go b/iscsi/multipath.go index bccfb695..3a93b123 100644 --- a/iscsi/multipath.go +++ b/iscsi/multipath.go @@ -65,8 +65,8 @@ func GetSysDevicesFromMultipathDevice(device string) ([]string, error) { return s, nil } -// Flush flushes a multipath device dm-x with command multipath -f /dev/dm-x -func Flush(device string) error { +// FlushMultipathDevice flushes a multipath device dm-x with command multipath -f /dev/dm-x +func FlushMultipathDevice(device string) error { debug.Printf("Flushing multipath device '%v'.\n", device) fullDevice := filepath.Join(devPath, device) From 77ef9f5c73d2bb3e0fd8c6ec6f4d057aa6541b0f Mon Sep 17 00:00:00 2001 From: Lou Date: Mon, 13 Jan 2020 17:01:45 +0800 Subject: [PATCH 3/5] use TRUNC instead of APPEND Signed-off-by: Lou --- iscsi/iscsi.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 8f031734..c96df124 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -405,7 +405,7 @@ func RemovePhysicalDevice(devices ...string) error { debug.Printf("Delete scsi device %v.\n", deviceName) // Remove a scsi device by executing 'echo "1" > /sys/block/sdx/device/delete filename := filepath.Join(sysBlockPath, deviceName, "device", "delete") - if f, err := os.OpenFile(filename, os.O_APPEND|os.O_WRONLY, 0200); err != nil { + if f, err := os.OpenFile(filename, os.O_TRUNC|os.O_WRONLY, 0200); err != nil { if os.IsNotExist(err) { continue } else { From da1b94e79a4ccb2084318c2d9b1a4a789af81c21 Mon Sep 17 00:00:00 2001 From: Lou Date: Mon, 13 Jan 2020 19:58:36 +0800 Subject: [PATCH 4/5] update ut Signed-off-by: Lou --- iscsi/iscsi_test.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index 118264c5..dceec27c 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -328,21 +328,6 @@ func Test_DisconnectNormalVolume(t *testing.T) { func Test_DisconnectMultipathVolume(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "") - if err != nil { - t.Errorf("can not create temp directory: %v", err) - return - } - sysBlockPath = tmpDir - devPath = filepath.Join(tmpDir, "dev") - defer os.RemoveAll(tmpDir) - - err = preparePaths(tmpDir) - if err != nil { - t.Errorf("can not create temp directories and files: %v", err) - return - } - execWithTimeout = fakeExecWithTimeout devicePath := multipathDevice @@ -356,10 +341,25 @@ func Test_DisconnectMultipathVolume(t *testing.T) { {"DisconnectMultipathVolume", false, false, false, nil}, {"DisconnectMultipathVolumeFlushFailed", false, false, true, fmt.Errorf("error")}, {"DisconnectMultipathVolumeFlushTimeout", true, false, true, nil}, - {"DisconnectNonexistentMultipathVolume", false, false, true, fmt.Errorf("error")}, + {"DisconnectNonexistentMultipathVolume", false, true, false, fmt.Errorf("error")}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + + tmpDir, err := ioutil.TempDir("", "") + if err != nil { + t.Errorf("can not create temp directory: %v", err) + return + } + sysBlockPath = tmpDir + devPath = filepath.Join(tmpDir, "dev") + defer os.RemoveAll(tmpDir) + + err = preparePaths(tmpDir) + if err != nil { + t.Errorf("can not create temp directories and files: %v", err) + return + } testExecWithTimeoutError = tt.cmdError testCmdTimeout = tt.timeout if tt.removeDevice { @@ -367,7 +367,7 @@ func Test_DisconnectMultipathVolume(t *testing.T) { os.RemoveAll(devPath) } c := Connector{Multipath: true, DevicePath: devicePath} - err := DisconnectVolume(c) + err = DisconnectVolume(c) if (err != nil) != tt.wantErr { t.Errorf("DisconnectVolume() error = %v, wantErr %v", err, tt.wantErr) return @@ -379,7 +379,7 @@ func Test_DisconnectMultipathVolume(t *testing.T) { } } - if !tt.removeDevice { + if !tt.removeDevice && !tt.wantErr { for _, s := range slaves { deleteFile := filepath.Join(sysBlockPath, s, "device", "delete") out, err := ioutil.ReadFile(deleteFile) From c725e6b53c7c37ff99d02757f19183fa6ed380ac Mon Sep 17 00:00:00 2001 From: Lou Date: Sun, 1 Mar 2020 16:09:36 +0800 Subject: [PATCH 5/5] update after revieww Signed-off-by: Lou --- iscsi/iscsi.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index c96df124..f45c52bd 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -370,11 +370,11 @@ func DisconnectVolume(c Connector) error { debug.Printf("Disconnecting volume in path %s.\n", c.DevicePath) if c.Multipath { debug.Printf("Removing multipath device.\n") - err := FlushMultipathDevice(c.DevicePath) + devices, err := GetSysDevicesFromMultipathDevice(c.DevicePath) if err != nil { return err } - devices, err := GetSysDevicesFromMultipathDevice(c.DevicePath) + err := FlushMultipathDevice(c.DevicePath) if err != nil { return err }