From 121455e04d3fc8b64db0e5e3370f2e2f2f2302a3 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Thu, 5 Nov 2020 14:53:54 +0100 Subject: [PATCH 01/19] fix example compilation - fix example/main.go - add go.mod to simplify local development --- example/main.go | 22 ++++++++++++++++------ go.mod | 3 +++ 2 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 go.mod diff --git a/example/main.go b/example/main.go index dc0f8eb3..656c7c1d 100644 --- a/example/main.go +++ b/example/main.go @@ -22,20 +22,30 @@ var ( func main() { flag.Parse() - tgtp := strings.Split(*portals, ",") + tgtps := strings.Split(*portals, ",") if *debug { iscsi.EnableDebugLogging(os.Stdout) } + var targets []iscsi.TargetInfo + + for _, tgtp := range tgtps { + parts := strings.Split(tgtp, ":") + targets = append(targets, iscsi.TargetInfo{ + // Specify the target iqn we're dealing with + Iqn: *iqn, + Portal: parts[0], + Port: parts[1], + }) + } + // You can utilize the iscsiadm calls directly if you wish, but by creating a Connector // you can simplify interactions to simple calls like "Connect" and "Disconnect" c := iscsi.Connector{ // Our example uses chap AuthType: "chap", - // Specify the target iqn we're dealing with - TargetIqn: *iqn, - // List of portals must be >= 1 (>1 signals multipath/mpio) - TargetPortals: tgtp, + // List of targets must be >= 1 (>1 signals multipath/mpio) + Targets: targets, // CHAP can be setup up for discovery as well as sessions, our example // device only uses CHAP security for sessions, for those that use Discovery // as well, we'd add a DiscoverySecrets entry the same way @@ -71,5 +81,5 @@ func main() { // Disconnect is easy as well, we don't need the full Connector any more, just the Target IQN and the Portals /// this should disconnect the volume as well as clear out the iscsi DB entries associated with it - iscsi.Disconnect(c.TargetIqn, c.TargetPortals) + iscsi.Disconnect(*iqn, tgtps) } diff --git a/go.mod b/go.mod new file mode 100644 index 00000000..a7d03b82 --- /dev/null +++ b/go.mod @@ -0,0 +1,3 @@ +module github.com/kubernetes-csi/csi-lib-iscsi + +go 1.15 From fca97c2313693aea072e48d17d0e75d027f364db Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Wed, 18 Nov 2020 11:32:25 +0100 Subject: [PATCH 02/19] properly disconnect volume Following Red Hat Customer Portal recommendations: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/storage_administration_guide/removing_devices --- iscsi/iscsi.go | 140 +++++++++++++++++++++++++++++++++----------- iscsi/iscsi_test.go | 1 - iscsi/iscsiadm.go | 3 +- iscsi/multipath.go | 73 ++++++++++++++++++----- 4 files changed, 166 insertions(+), 51 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index b70c9ce1..8b4b30cd 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -27,6 +27,7 @@ var ( type statFunc func(string) (os.FileInfo, error) type globFunc func(string) ([]string, error) +// iscsiSession contains information avout an iSCSI session type iscsiSession struct { Protocol string ID int32 @@ -41,7 +42,16 @@ type TargetInfo struct { Port string `json:"port"` } -//Connector provides a struct to hold all of the needed parameters to make our iscsi connection +type scsiInfo struct { + BlockDevices []ScsiDevice +} + +type ScsiDevice struct { + Name string `json:"name"` + Hctl string `json:"hctl"` +} + +// Connector provides a struct to hold all of the needed parameters to make our iscsi connection type Connector struct { VolumeName string `json:"volume_name"` Targets []TargetInfo `json:"targets"` @@ -55,11 +65,11 @@ type Connector struct { // 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"` - TargetIqn string `json:"target_iqn"` + RetryCount int32 `json:"retry_count"` + CheckInterval int32 `json:"check_interval"` + DoDiscovery bool `json:"do_discovery"` + DoCHAPDiscovery bool `json:"do_chap_discovery"` + TargetIqn string `json:"target_iqn"` TargetPortals []string `json:"target_portals"` } @@ -104,6 +114,7 @@ func parseSessions(lines string) []iscsiSession { return sessions } +// sessionExists checks if an iSCSI session exists func sessionExists(tgtPortal, tgtIQN string) (bool, error) { sessions, err := getCurrentSessions() if err != nil { @@ -117,6 +128,7 @@ func sessionExists(tgtPortal, tgtIQN string) (bool, error) { return false, nil } +// extractTransportName returns a transport_name from getCurrentSessions output func extractTransportName(output string) string { res := regexp.MustCompile(`iface.transport_name = (.*)\n`).FindStringSubmatch(output) if res == nil { @@ -128,8 +140,8 @@ func extractTransportName(output string) string { return res[1] } +// getCurrentSessions list current iSCSI sessions func getCurrentSessions() ([]iscsiSession, error) { - out, err := GetSessions() if err != nil { exitErr, ok := err.(*exec.ExitError) @@ -366,7 +378,7 @@ func DisconnectVolume(c Connector) error { // 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. + // 5. Rescan the iSCSI sessions (after unmapping only). // // DisconnectVolume focuses on step 2 and 3. // Note: make sure the volume is already unmounted before calling this method. @@ -374,7 +386,7 @@ func DisconnectVolume(c Connector) error { debug.Printf("Disconnecting volume in path %s.\n", c.DevicePath) if c.Multipath { debug.Printf("Removing multipath device.\n") - devices, err := GetSysDevicesFromMultipathDevice(c.DevicePath) + devices, err := GetSCSIDevicesFromMultipathDevice(c.DevicePath) if err != nil { return err } @@ -383,12 +395,21 @@ func DisconnectVolume(c Connector) error { return err } debug.Printf("Found multipath slaves %v, removing all of them.\n", devices) - if err := RemovePhysicalDevice(devices...); err != nil { + scsiDevices, err := GetSCSIDevices(devices...) + if err != nil { + return err + } + + if err := RemoveSCSIDevices(scsiDevices...); err != nil { return err } } else { debug.Printf("Removing normal device.\n") - if err := RemovePhysicalDevice(c.DevicePath); err != nil { + scsiDevice, err := GetSCSIDevice(c.DevicePath) + if err != nil { + return err + } + if err = RemoveSCSIDevices(*scsiDevice); err != nil { return err } } @@ -397,33 +418,84 @@ func DisconnectVolume(c Connector) error { return nil } -// RemovePhysicalDevice removes device(s) sdx from a Linux host. -func RemovePhysicalDevice(devices ...string) error { - debug.Printf("Removing scsi device %v.\n", devices) +func GetSCSIDevice(device string) (*ScsiDevice, error) { + scsiDevices, err := GetSCSIDevices(device) + if err != nil { + return nil, err + } + return &scsiDevices[0], nil +} + +// GetSCSIDevices get scsi devices from a device names +func GetSCSIDevices(devices ...string) ([]ScsiDevice, error) { + debug.Printf("Getting info about SCSI devices %s.\n", devices) + + var devicePaths []string + for _, device := range devices { + devicePaths = append(devicePaths, filepath.Join(devPath, device)) + } + + out, err := exec.Command("lsblk", append([]string{"-SJ"}, devicePaths...)...).Output() + if err != nil { + debug.Printf("An error occured while looking info about SCSI devices: %v\n", err) + return nil, err + } + + var scsiInfo scsiInfo + json.Unmarshal(out, &scsiInfo) + return scsiInfo.BlockDevices, nil +} + +func writeInSCSIDeviceFile(hctl string, file string, content string) (bool, error) { + filename := filepath.Join(sysScsiPath, hctl, "device", file) + debug.Printf("Write %q in %q.\n", content, filename) + if f, err := os.OpenFile(filename, os.O_TRUNC|os.O_WRONLY, 0200); err != nil { + if os.IsNotExist(err) { + return true, nil + } else { + debug.Printf("Error while opening file %v: %v\n", filename, err) + return false, err + } + } else { + defer f.Close() + if _, err := f.WriteString(content); err != nil { + debug.Printf("Error while writing to file %v: %v", filename, err) + return false, err + } + } + return false, nil +} + +// RemovePhysicalDevice removes scsi device(s) from a Linux host. +func RemoveSCSIDevices(devices ...ScsiDevice) error { + debug.Printf("Removing scsi devices %v.\n", devices) + var errs []error - for _, deviceName := range devices { - if deviceName == "" { + for _, device := range devices { + fullDevice := filepath.Join(devPath, device.Name) + debug.Printf("Flush scsi device %v.\n", device.Name) + err := exec.Command("blockdev", "--flushbufs", fullDevice).Run() + if err != nil { + debug.Printf("Command 'blockdev --flushbufs %v' did not succeed to flush the device: %v\n", fullDevice, err) + return err + } + + debug.Printf("Put scsi device %v offline.\n", device.Name) + skip, err := writeInSCSIDeviceFile(device.Hctl, "state", "offline\n") + if err != nil { + errs = append(errs, err) + continue + } else if skip { 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_TRUNC|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 - } + debug.Printf("Delete scsi device %v.\n", device.Name) + skip, err = writeInSCSIDeviceFile(device.Hctl, "delete", "1") // Remove a scsi device by echo 1 > /sys/class/scsi_device/h:c:t:l/device/delete + if err != nil { + errs = append(errs, err) + continue + } else if skip { + continue } } diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index dceec27c..40663b57 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -272,7 +272,6 @@ 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) diff --git a/iscsi/iscsiadm.go b/iscsi/iscsiadm.go index a75f218c..44648ee0 100644 --- a/iscsi/iscsiadm.go +++ b/iscsi/iscsiadm.go @@ -36,6 +36,7 @@ func (e *CmdError) Error() string { func iscsiCmd(args ...string) (string, error) { cmd := execCommand("iscsiadm", args...) + debug.Printf("Run iscsiadm command: %s", strings.Join(append([]string{"iscsiadm"}, args...), " ")) var stdout bytes.Buffer var iscsiadmError error cmd.Stdout = &stdout @@ -91,7 +92,7 @@ func ShowInterface(iface string) (string, error) { func CreateDBEntry(tgtIQN, portal, iFace string, discoverySecrets, sessionSecrets Secrets) error { debug.Println("Begin CreateDBEntry...") baseArgs := []string{"-m", "node", "-T", tgtIQN, "-p", portal} - _, err := iscsiCmd(append(baseArgs, []string{"-I", iFace, "-o", "new"}...)...) + _, err := iscsiCmd(append(baseArgs, "-I", iFace, "-o", "new")...) if err != nil { return err } diff --git a/iscsi/multipath.go b/iscsi/multipath.go index 3a93b123..71df053c 100644 --- a/iscsi/multipath.go +++ b/iscsi/multipath.go @@ -2,16 +2,36 @@ package iscsi import ( "context" - "io/ioutil" "os" "os/exec" "path/filepath" "time" + "encoding/json" ) var sysBlockPath = "/sys/block" +var sysScsiPath = "/sys/class/scsi_device" var devPath = "/dev" +type multipathDeviceMap struct { + Map multipathMap `json:"map"` +} + +type multipathMap struct { + Name string `json:"name"` + Uuid string `json:"uuid"` + Sysfs string `json:"sysfs"` + PathGroups []pathGroup `json:"path_groups"` +} + +type pathGroup struct { + Paths []path `json:"paths"` +} + +type path struct { + Device string `json:"dev"` +} + func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]byte, error) { debug.Printf("Executing command '%v' with args: '%v'.\n", command, args) @@ -43,26 +63,49 @@ func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]by 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) +func getMultipathMap(device string) (*multipathDeviceMap, error) { + debug.Printf("Getting multipath map for device %s.\n", device[1:]) + + cmd := exec.Command("multipathd", "show", "map", device[1:], "json") + out, err := cmd.Output() + // debug.Printf(string(out)) if err != nil { - if os.IsNotExist(err) { - return nil, nil + debug.Printf("An error occured while looking for multipath device map: %v\n", err) + return nil, err + } + + var deviceMap multipathDeviceMap; + json.Unmarshal(out, &deviceMap) + return &deviceMap, nil +} + +func (deviceMap *multipathDeviceMap) GetSlaves() []string { + var slaves []string + + for _, pathGroup := range deviceMap.Map.PathGroups { + for _, path := range pathGroup.Paths { + slaves = append(slaves, path.Device) } + } + + return slaves +} + +// GetScsiDevicesFromMultipathDevice gets all ScsiDevice for multipath device dm-x +func GetScsiDevicesFromMultipathDevice(device string) ([]string, error) { + debug.Printf("Getting all slaves for multipath device %s.\n", device) + + deviceMap, err := getMultipathMap(device) + + if err != 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 + slaves := deviceMap.GetSlaves() + + debug.Printf("Found slaves: %v.\n", slaves) + return slaves, nil } // FlushMultipathDevice flushes a multipath device dm-x with command multipath -f /dev/dm-x From efce096f60692fd17bdc45dfe53e56876f8a2429 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Fri, 29 Oct 2021 11:07:07 +0200 Subject: [PATCH 03/19] standardize Device usage - always control devices through the Device class - add checks on devices - store all used devices instead of mounted one only - filter device by transport to keep iSCSI devices only --- example/main.go | 10 +- iscsi/iscsi.go | 343 ++++++++++++++++++++++++++------------------ iscsi/iscsi_test.go | 1 + iscsi/multipath.go | 58 +++----- 4 files changed, 228 insertions(+), 184 deletions(-) diff --git a/example/main.go b/example/main.go index 656c7c1d..21eb42b3 100644 --- a/example/main.go +++ b/example/main.go @@ -13,7 +13,6 @@ import ( var ( portals = flag.String("portals", "192.168.1.112:3260", "Comma delimited. Eg: 1.1.1.1,2.2.2.2") iqn = flag.String("iqn", "iqn.2010-10.org.openstack:volume-95739000-1557-44f8-9f40-e9d29fe6ec47", "") - multipath = flag.Bool("multipath", false, "") username = flag.String("username", "3aX7EEf3CEgvESQG75qh", "") password = flag.String("password", "eJBDC7Bt7WE3XFDq", "") lun = flag.Int("lun", 1, "") @@ -41,7 +40,7 @@ func main() { // You can utilize the iscsiadm calls directly if you wish, but by creating a Connector // you can simplify interactions to simple calls like "Connect" and "Disconnect" - c := iscsi.Connector{ + c := &iscsi.Connector{ // Our example uses chap AuthType: "chap", // List of targets must be >= 1 (>1 signals multipath/mpio) @@ -55,8 +54,6 @@ func main() { SecretsType: "chap"}, // Lun is the lun number the devices uses for exports Lun: int32(*lun), - // Multipath indicates that we want to configure this connection as a multipath device - Multipath: *multipath, // Number of times we check for device path, waiting for CheckInterval seconds inbetween each check (defaults to 10 if omitted) RetryCount: 11, // CheckInterval is the time in seconds to wait inbetween device path checks when logging in to a target @@ -71,11 +68,6 @@ func main() { os.Exit(1) } - if path == "" { - log.Printf("Failed to connect, didn't receive a path, but also no error!") - os.Exit(1) - } - log.Printf("Connected device at path: %s\n", path) time.Sleep(3 * time.Second) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 8b4b30cd..3584eb1e 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -36,22 +36,30 @@ type iscsiSession struct { Name string } +// TargetInfo contains connection information to connect to an iSCSI endpoint type TargetInfo struct { Iqn string `json:"iqn"` Portal string `json:"portal"` Port string `json:"port"` } -type scsiInfo struct { - BlockDevices []ScsiDevice +type deviceInfo struct { + BlockDevices []Device } -type ScsiDevice struct { - Name string `json:"name"` - Hctl string `json:"hctl"` +// Device contains informations about a device +type Device struct { + Name string `json:"name"` + Hctl string `json:"hctl"` + Children []Device `json:"children"` + Type string `json:"type"` + Vendor string `json:"vendor"` + Model string `json:"model"` + Revision string `json:"rev"` + Transport string `json:"tran"` } -// Connector provides a struct to hold all of the needed parameters to make our iscsi connection +// Connector provides a struct to hold all of the needed parameters to make our iSCSI connection type Connector struct { VolumeName string `json:"volume_name"` Targets []TargetInfo `json:"targets"` @@ -60,10 +68,9 @@ type Connector struct { DiscoverySecrets Secrets `json:"discovery_secrets"` SessionSecrets Secrets `json:"session_secrets"` Interface string `json:"interface"` - Multipath bool `json:"multipath"` - // DevicePath is dm-x for a multipath device, and sdx for a normal device. - DevicePath string `json:"device_path"` + MountTargetDevice *Device `json:"mount_target_device"` + Devices []Device `json:"devices"` RetryCount int32 `json:"retry_count"` CheckInterval int32 `json:"check_interval"` @@ -76,7 +83,6 @@ type Connector struct { func init() { // by default we don't log anything, EnableDebugLogging() can turn on some tracing debug = log.New(ioutil.Discard, "", 0) - } // EnableDebugLogging provides a mechanism to turn on debug logging for this package @@ -85,7 +91,7 @@ func EnableDebugLogging(writer io.Writer) { debug = log.New(writer, "DEBUG: ", log.Ldate|log.Ltime|log.Lshortfile) } -// parseSession takes the raw stdout from the iscsiadm -m session command and encodes it into an iscsi session type +// parseSession takes the raw stdout from the iscsiadm -m session command and encodes it into an iSCSI session type func parseSessions(lines string) []iscsiSession { entries := strings.Split(strings.TrimSpace(lines), "\n") r := strings.NewReplacer("[", "", @@ -181,7 +187,7 @@ func waitForPathToExistImpl(devicePath *string, maxRetries, intervalSeconds int, err = os.ErrNotExist } else { // There might be a case that fpath contains multiple device paths if - // multiple PCI devices connect to same iscsi target. We handle this + // multiple PCI devices connect to same iSCSI target. We handle this // case at subsequent logic. Pick up only first path here. *devicePath = fpath[0] } @@ -197,52 +203,47 @@ func waitForPathToExistImpl(devicePath *string, maxRetries, intervalSeconds int, return false, err } -func getMultipathDisk(path string) (string, error) { - // Follow link to destination directory - debug.Printf("Checking for multipath device for path: %s", path) - devicePath, err := os.Readlink(path) - if err != nil { - debug.Printf("Failed reading link for multipath disk: %s -- error: %s\n", path, err.Error()) - return "", err +// getMultipathDevice returns a multipath device for the configured targets if it exists +func getMultipathDevice(devices []Device) (*Device, error) { + var deviceInfo deviceInfo + var multipathDevice *Device + var devicePaths []string + + for _, device := range devices { + devicePaths = append(devicePaths, device.GetPath()) } - sdevice := filepath.Base(devicePath) - // If destination directory is already identified as a multipath device, - // just return its path - if strings.HasPrefix(sdevice, "dm-") { - debug.Printf("Already found multipath device: %s", sdevice) - return path, nil - } - // Fallback to iterating through all the entries under /sys/block/dm-* and - // check to see if any have an entry under /sys/block/dm-*/slaves matching - // the device the symlink was pointing at - dmPaths, err := filepath.Glob("/sys/block/dm-*") + out, err := lsblk("-J", devicePaths) if err != nil { - debug.Printf("Glob error: %s", err) - return "", err + return nil, err } - for _, dmPath := range dmPaths { - sdevices, err := filepath.Glob(filepath.Join(dmPath, "slaves", "*")) - if err != nil { - debug.Printf("Glob error: %s", err) + + if err = json.Unmarshal(out, &deviceInfo); err != nil { + return nil, err + } + + for _, device := range deviceInfo.BlockDevices { + if len(device.Children) != 1 { + return nil, fmt.Errorf("device is not mapped to exactly one multipath device: %v", device.Children) } - for _, spath := range sdevices { - s := filepath.Base(spath) - debug.Printf("Basepath: %s", s) - if sdevice == s { - // We've found a matching entry, return the path for the - // dm-* device it was found under - p := filepath.Join("/dev", filepath.Base(dmPath)) - debug.Printf("Found matching multipath device: %s under dm-* device path %s", sdevice, dmPath) - return p, nil - } + if multipathDevice != nil && device.Children[0].Name != multipathDevice.Name { + return nil, fmt.Errorf("devices don't share a common multipath device: %v", devices) } + multipathDevice = &device.Children[0] + } + + if multipathDevice == nil { + return nil, fmt.Errorf("multipath device not found") } - debug.Printf("Couldn't find dm-* path for path: %s, found non dm-* path: %s", path, devicePath) - return "", fmt.Errorf("couldn't find dm-* path for path: %s, found non dm-* path: %s", path, devicePath) + + if multipathDevice.Type != "mpath" { + return nil, fmt.Errorf("device is not of mpath type: %v", multipathDevice) + } + + return multipathDevice, nil } // Connect attempts to connect a volume to this node using the provided Connector info -func Connect(c Connector) (string, error) { +func Connect(c *Connector) (string, error) { var lastErr error if c.RetryCount == 0 { c.RetryCount = 10 @@ -255,7 +256,7 @@ func Connect(c Connector) (string, error) { return "", fmt.Errorf("invalid RetryCount and CheckInterval combination, both must be positive integers. "+ "RetryCount: %d, CheckInterval: %d", c.RetryCount, c.CheckInterval) } - var devicePaths []string + iFace := "default" if c.Interface != "" { iFace = c.Interface @@ -268,6 +269,7 @@ func Connect(c Connector) (string, error) { } iscsiTransport := extractTransportName(out) + var devicePaths []string for _, target := range c.Targets { debug.Printf("process targetIqn: %s, portal: %s\n", target.Iqn, target.Portal) baseArgs := []string{"-m", "node", "-T", target.Iqn, "-p", target.Portal} @@ -301,7 +303,7 @@ func Connect(c Connector) (string, error) { } if c.DoDiscovery { - // build discoverydb and discover iscsi target + // build discoverydb and discover iSCSI target if err := Discoverydb(p, iFace, c.DiscoverySecrets, c.DoCHAPDiscovery); err != nil { debug.Printf("Error in discovery of the target: %s\n", err.Error()) lastErr = err @@ -334,31 +336,32 @@ func Connect(c Connector) (string, error) { } } + // GetISCSIDevices returns all devices if no paths are given if len(devicePaths) < 1 { - iscsiCmd([]string{"-m", "iface", "-I", iFace, "-o", "delete"}...) - return "", fmt.Errorf("failed to find device path: %s, last error seen: %v", devicePaths, lastErr) + c.Devices = []Device{} + } else { + c.Devices, err = GetISCSIDevices(devicePaths) + if err != nil { + return "", err + } } - if lastErr != nil { - debug.Printf("Last error occured during iscsi init: \n%v", lastErr) + if len(c.Devices) < 1 { + iscsiCmd([]string{"-m", "iface", "-I", iFace, "-o", "delete"}...) + return "", fmt.Errorf("failed to find device path: %s, last error seen: %v", devicePaths, lastErr) } - for i, path := range devicePaths { - if path != "" { - if mappedDevicePath, err := getMultipathDisk(path); mappedDevicePath != "" { - devicePaths[i] = mappedDevicePath - if err != nil { - return "", err - } - } - } + mountTargetDevice, err := getMountTargetDevice(c) + c.MountTargetDevice = mountTargetDevice + if err != nil { + debug.Printf("Connect failed: %v", err) + RemoveSCSIDevices(c.Devices...) + c.MountTargetDevice = nil + c.Devices = []Device{} + return "", err } - debug.Printf("After connect we're returning devicePaths: %s", devicePaths) - if len(devicePaths) > 0 { - return devicePaths[0], err - } - return "", err + return c.MountTargetDevice.GetPath(), nil } //Disconnect performs a disconnect operation on a volume @@ -372,7 +375,7 @@ func Disconnect(tgtIqn string, portals []string) error { } // DisconnectVolume removes a volume from a Linux host. -func DisconnectVolume(c Connector) error { +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). @@ -383,33 +386,24 @@ 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 { - debug.Printf("Removing multipath device.\n") - devices, err := GetSCSIDevicesFromMultipathDevice(c.DevicePath) - if err != nil { - return err - } - err = FlushMultipathDevice(c.DevicePath) - if err != nil { - return err - } - debug.Printf("Found multipath slaves %v, removing all of them.\n", devices) - scsiDevices, err := GetSCSIDevices(devices...) + if len(c.Devices) > 1 { + debug.Printf("Removing multipath device in path %s.\n", c.MountTargetDevice.GetPath()) + err := FlushMultipathDevice(c.MountTargetDevice) if err != nil { return err } - if err := RemoveSCSIDevices(scsiDevices...); err != nil { + if err := RemoveSCSIDevices(c.Devices...); err != nil { return err } } else { - debug.Printf("Removing normal device.\n") - scsiDevice, err := GetSCSIDevice(c.DevicePath) + devicePath := c.MountTargetDevice.GetPath() + debug.Printf("Removing normal device in path %s.\n", devicePath) + device, err := GetISCSIDevice(devicePath) if err != nil { return err } - if err = RemoveSCSIDevices(*scsiDevice); err != nil { + if err = RemoveSCSIDevices(*device); err != nil { return err } } @@ -418,83 +412,133 @@ func DisconnectVolume(c Connector) error { return nil } -func GetSCSIDevice(device string) (*ScsiDevice, error) { - scsiDevices, err := GetSCSIDevices(device) +func getMountTargetDevice(c *Connector) (*Device, error) { + if len(c.Devices) > 1 { + multipathDevice, err := getMultipathDevice(c.Devices) + if err != nil { + debug.Printf("mount target is not a multipath device: %v", err) + return nil, err + } + debug.Printf("mount target is a multipath device") + return multipathDevice, nil + } + + if len(c.Devices) == 0 { + return nil, fmt.Errorf("could not find mount target device: connector does not contain any device") + } + + return &c.Devices[0], nil +} + +// GetISCSIDevice get an iSCSI device from a device name +func GetISCSIDevice(deviceName string) (*Device, error) { + iscsiDevices, err := GetISCSIDevices([]string{deviceName}) if err != nil { return nil, err } - return &scsiDevices[0], nil + if len(iscsiDevices) == 0 { + return nil, fmt.Errorf("device %q not found", deviceName) + } + return &iscsiDevices[0], nil } -// GetSCSIDevices get scsi devices from a device names -func GetSCSIDevices(devices ...string) ([]ScsiDevice, error) { - debug.Printf("Getting info about SCSI devices %s.\n", devices) +// GetSCSIDevices get SCSI devices from device paths +// It will returns all SCSI devices if no paths are given +func GetSCSIDevices(devicePaths []string) ([]Device, error) { + debug.Printf("Getting info about SCSI devices %s.\n", devicePaths) - var devicePaths []string - for _, device := range devices { - devicePaths = append(devicePaths, filepath.Join(devPath, device)) + out, err := lsblk("-JS", devicePaths) + if err != nil { + debug.Printf("An error occured while looking info about SCSI devices: %v", err) + return nil, err } - out, err := exec.Command("lsblk", append([]string{"-SJ"}, devicePaths...)...).Output() + var deviceInfo deviceInfo + err = json.Unmarshal(out, &deviceInfo) if err != nil { - debug.Printf("An error occured while looking info about SCSI devices: %v\n", err) return nil, err } - var scsiInfo scsiInfo - json.Unmarshal(out, &scsiInfo) - return scsiInfo.BlockDevices, nil + return deviceInfo.BlockDevices, nil } -func writeInSCSIDeviceFile(hctl string, file string, content string) (bool, error) { - filename := filepath.Join(sysScsiPath, hctl, "device", file) - debug.Printf("Write %q in %q.\n", content, filename) - if f, err := os.OpenFile(filename, os.O_TRUNC|os.O_WRONLY, 0200); err != nil { - if os.IsNotExist(err) { - return true, nil - } else { - debug.Printf("Error while opening file %v: %v\n", filename, err) - return false, err - } - } else { - defer f.Close() - if _, err := f.WriteString(content); err != nil { - debug.Printf("Error while writing to file %v: %v", filename, err) - return false, err +// GetISCSIDevices get iSCSI devices from device paths +// It will returns all iSCSI devices if no paths are given +func GetISCSIDevices(devicePaths []string) (devices []Device, err error) { + scsiDevices, err := GetSCSIDevices(devicePaths) + if err != nil { + return + } + + for i := range scsiDevices { + device := &scsiDevices[i] + if device.Transport == "iscsi" { + devices = append(devices, *device) } } - return false, nil + + return +} + +// lsblk execute the lsblk commands +func lsblk(flags string, devicePaths []string) ([]byte, error) { + out, err := exec.Command("lsblk", append([]string{flags}, devicePaths...)...).Output() + debug.Printf("lsblk %s %s", flags, strings.Join(devicePaths, " ")) + if err != nil { + return nil, fmt.Errorf("lsblk: %v", err) + } + + return out, nil +} + +// writeInSCSIDeviceFile write into special devices files to change devices state +func writeInSCSIDeviceFile(hctl string, file string, content string) error { + filename := filepath.Join("/sys/class/scsi_device", hctl, "device", file) + debug.Printf("Write %q in %q.\n", content, filename) + + f, err := os.OpenFile(filename, os.O_TRUNC|os.O_WRONLY, 0200) + if err != nil { + debug.Printf("Error while opening file %v: %v\n", filename, err) + return err + } + + defer f.Close() + if _, err := f.WriteString(content); err != nil { + debug.Printf("Error while writing to file %v: %v", filename, err) + return err + } + + return nil } -// RemovePhysicalDevice removes scsi device(s) from a Linux host. -func RemoveSCSIDevices(devices ...ScsiDevice) error { - debug.Printf("Removing scsi devices %v.\n", devices) +// RemoveSCSIDevices removes SCSI device(s) from a Linux host. +func RemoveSCSIDevices(devices ...Device) error { + debug.Printf("Removing SCSI devices %v.\n", devices) var errs []error for _, device := range devices { - fullDevice := filepath.Join(devPath, device.Name) - debug.Printf("Flush scsi device %v.\n", device.Name) - err := exec.Command("blockdev", "--flushbufs", fullDevice).Run() + debug.Printf("Flush SCSI device %v.\n", device.Name) + err := exec.Command("blockdev", "--flushbufs", device.GetPath()).Run() if err != nil { - debug.Printf("Command 'blockdev --flushbufs %v' did not succeed to flush the device: %v\n", fullDevice, err) + debug.Printf("Command 'blockdev --flushbufs %v' did not succeed to flush the device: %v\n", device.Name, err) return err } - debug.Printf("Put scsi device %v offline.\n", device.Name) - skip, err := writeInSCSIDeviceFile(device.Hctl, "state", "offline\n") + debug.Printf("Put SCSI device %v offline.\n", device.Name) + err = device.Shutdown() if err != nil { - errs = append(errs, err) - continue - } else if skip { + if !os.IsNotExist(err) { // Ignore device already removed + errs = append(errs, err) + } continue } - debug.Printf("Delete scsi device %v.\n", device.Name) - skip, err = writeInSCSIDeviceFile(device.Hctl, "delete", "1") // Remove a scsi device by echo 1 > /sys/class/scsi_device/h:c:t:l/device/delete + debug.Printf("Delete SCSI device %v.\n", device.Name) + err = device.Delete() if err != nil { - errs = append(errs, err) - continue - } else if skip { + if !os.IsNotExist(err) { // Ignore device already removed + errs = append(errs, err) + } continue } } @@ -511,7 +555,7 @@ func PersistConnector(c *Connector, filePath string) error { //file := path.Join("mnt", c.VolumeName+".json") f, err := os.Create(filePath) if err != nil { - return fmt.Errorf("error creating iscsi persistence file %s: %s", filePath, err) + return fmt.Errorf("error creating iSCSI persistence file %s: %s", filePath, err) } defer f.Close() encoder := json.NewEncoder(f) @@ -519,7 +563,6 @@ func PersistConnector(c *Connector, filePath string) error { return fmt.Errorf("error encoding connector: %v", err) } return nil - } // GetConnectorFromFile attempts to create a Connector using the specified json file (ie /var/lib/pfile/myConnector.json) @@ -536,5 +579,33 @@ func GetConnectorFromFile(filePath string) (*Connector, error) { } return &data, nil +} + +// GetPath returns the path of a device +func (d *Device) GetPath() string { + if d.Type == "mpath" { + return filepath.Join("/dev/mapper", d.Name) + } + + return filepath.Join("/dev", d.Name) +} + +// WriteDeviceFile write in a device file +func (d *Device) WriteDeviceFile(name string, content string) error { + return writeInSCSIDeviceFile(d.Hctl, name, content) +} + +// Shutdown turn off an SCSI device by writing offline\n in /sys/class/scsi_device/h:c:t:l/device/state +func (d *Device) Shutdown() error { + return writeInSCSIDeviceFile(d.Hctl, "state", "offline\n") +} + +// Delete detach an SCSI device by writing 1 in /sys/class/scsi_device/h:c:t:l/device/delete +func (d *Device) Delete() error { + return writeInSCSIDeviceFile(d.Hctl, "delete", "1") +} +// Rescan rescan an SCSI device by writing 1 in /sys/class/scsi_device/h:c:t:l/device/rescan +func (d *Device) Rescan() error { + return writeInSCSIDeviceFile(d.Hctl, "rescan", "1") } diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index 40663b57..70529a9e 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -89,6 +89,7 @@ var testExecWithTimeoutError error var mockedExitStatus = 0 var mockedStdout string +var sysBlockPath = "/sys/block" var normalDevice = "sda" var multipathDevice = "dm-1" var slaves = []string{"sdb", "sdc"} diff --git a/iscsi/multipath.go b/iscsi/multipath.go index 71df053c..fda1b9f8 100644 --- a/iscsi/multipath.go +++ b/iscsi/multipath.go @@ -2,25 +2,20 @@ package iscsi import ( "context" + "encoding/json" "os" "os/exec" - "path/filepath" "time" - "encoding/json" ) -var sysBlockPath = "/sys/block" -var sysScsiPath = "/sys/class/scsi_device" -var devPath = "/dev" - type multipathDeviceMap struct { Map multipathMap `json:"map"` } type multipathMap struct { - Name string `json:"name"` - Uuid string `json:"uuid"` - Sysfs string `json:"sysfs"` + Name string `json:"name"` + UUID string `json:"uuid"` + Sysfs string `json:"sysfs"` PathGroups []pathGroup `json:"path_groups"` } @@ -32,6 +27,7 @@ type path struct { Device string `json:"dev"` } +// ExecWithTimeout execute a command with a timeout and returns an error if timeout is excedeed func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]byte, error) { debug.Printf("Executing command '%v' with args: '%v'.\n", command, args) @@ -53,8 +49,6 @@ func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]by 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) } @@ -64,7 +58,7 @@ func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]by } func getMultipathMap(device string) (*multipathDeviceMap, error) { - debug.Printf("Getting multipath map for device %s.\n", device[1:]) + debug.Printf("Getting multipath map for device %s.\n", device) cmd := exec.Command("multipathd", "show", "map", device[1:], "json") out, err := cmd.Output() @@ -74,8 +68,11 @@ func getMultipathMap(device string) (*multipathDeviceMap, error) { return nil, err } - var deviceMap multipathDeviceMap; - json.Unmarshal(out, &deviceMap) + var deviceMap multipathDeviceMap + err = json.Unmarshal(out, &deviceMap) + if err != nil { + return nil, err + } return &deviceMap, nil } @@ -91,40 +88,23 @@ func (deviceMap *multipathDeviceMap) GetSlaves() []string { return slaves } -// GetScsiDevicesFromMultipathDevice gets all ScsiDevice for multipath device dm-x -func GetScsiDevicesFromMultipathDevice(device string) ([]string, error) { - debug.Printf("Getting all slaves for multipath device %s.\n", device) - - deviceMap, err := getMultipathMap(device) - - if err != nil { - debug.Printf("An error occured while looking for slaves: %v\n", err) - return nil, err - } - - slaves := deviceMap.GetSlaves() - - debug.Printf("Found slaves: %v.\n", slaves) - return slaves, nil -} - // 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) +func FlushMultipathDevice(device *Device) error { + devicePath := device.GetPath() + debug.Printf("Flushing multipath device '%v'.\n", devicePath) - fullDevice := filepath.Join(devPath, device) timeout := 5 * time.Second - _, err := execWithTimeout("multipath", []string{"-f", fullDevice}, timeout) + _, err := execWithTimeout("multipath", []string{"-f", devicePath}, timeout) if err != nil { - if _, e := os.Stat(fullDevice); os.IsNotExist(e) { - debug.Printf("Multipath device %v was deleted.\n", device) + if _, e := os.Stat(devicePath); os.IsNotExist(e) { + debug.Printf("Multipath device %v has been removed.\n", devicePath) } else { - debug.Printf("Command 'multipath -f %v' did not succeed to delete the device: %v\n", fullDevice, err) + debug.Printf("Command 'multipath -f %v' did not succeed to delete the device: %v\n", devicePath, err) return err } } - debug.Printf("Finshed flushing multipath device %v.\n", device) + debug.Printf("Finshed flushing multipath device %v.\n", devicePath) return nil } From 8f2b4d0de1c359207dfa0315f5a3b9d6bf7ad438 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Mon, 11 Jan 2021 17:23:03 +0100 Subject: [PATCH 04/19] improve error reporting on command execution --- iscsi/iscsi.go | 9 +++++---- iscsi/multipath.go | 14 +++++++++++--- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 3584eb1e..ef631508 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -2,6 +2,7 @@ package iscsi import ( "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -482,10 +483,10 @@ func GetISCSIDevices(devicePaths []string) (devices []Device, err error) { // lsblk execute the lsblk commands func lsblk(flags string, devicePaths []string) ([]byte, error) { - out, err := exec.Command("lsblk", append([]string{flags}, devicePaths...)...).Output() + out, err := exec.Command("lsblk", append([]string{flags}, devicePaths...)...).CombinedOutput() debug.Printf("lsblk %s %s", flags, strings.Join(devicePaths, " ")) if err != nil { - return nil, fmt.Errorf("lsblk: %v", err) + return nil, fmt.Errorf("lsblk: %s", string(out)) } return out, nil @@ -518,10 +519,10 @@ func RemoveSCSIDevices(devices ...Device) error { var errs []error for _, device := range devices { debug.Printf("Flush SCSI device %v.\n", device.Name) - err := exec.Command("blockdev", "--flushbufs", device.GetPath()).Run() + out, err := exec.Command("blockdev", "--flushbufs", device.GetPath()).CombinedOutput() if err != nil { debug.Printf("Command 'blockdev --flushbufs %v' did not succeed to flush the device: %v\n", device.Name, err) - return err + return errors.New(string(out)) } debug.Printf("Put SCSI device %v offline.\n", device.Name) diff --git a/iscsi/multipath.go b/iscsi/multipath.go index fda1b9f8..46f9c56c 100644 --- a/iscsi/multipath.go +++ b/iscsi/multipath.go @@ -3,8 +3,10 @@ package iscsi import ( "context" "encoding/json" + "fmt" "os" "os/exec" + "strings" "time" ) @@ -50,7 +52,10 @@ func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]by } if err != nil { - debug.Printf("Non-zero exit code: %s\n", err) + if ee, ok := err.(*exec.ExitError); ok { + debug.Printf("Non-zero exit code: %s\n", err) + err = fmt.Errorf("%s", ee.Stderr) + } } debug.Println("Finished executing command.") @@ -61,10 +66,10 @@ func getMultipathMap(device string) (*multipathDeviceMap, error) { debug.Printf("Getting multipath map for device %s.\n", device) cmd := exec.Command("multipathd", "show", "map", device[1:], "json") - out, err := cmd.Output() + out, err := cmd.CombinedOutput() // debug.Printf(string(out)) if err != nil { - debug.Printf("An error occured while looking for multipath device map: %v\n", err) + debug.Printf("An error occured while looking for multipath device map: %s\n", out) return nil, err } @@ -100,6 +105,9 @@ func FlushMultipathDevice(device *Device) error { if _, e := os.Stat(devicePath); os.IsNotExist(e) { debug.Printf("Multipath device %v has been removed.\n", devicePath) } else { + if strings.Contains(err.Error(), "map in use") { + err = fmt.Errorf("device is probably still in use somewhere else: %v", err) + } debug.Printf("Command 'multipath -f %v' did not succeed to delete the device: %v\n", devicePath, err) return err } From d23b324b8a368fd62342d9471f296fc3482ba0f6 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Fri, 29 Oct 2021 13:17:31 +0200 Subject: [PATCH 05/19] refactor and simplify Connector - refactor connector's functions to use it as a receiver - refactor waitForPathToExists - refactor Connect to make it smaller and more readable --- iscsi/iscsi.go | 273 +++++++++++++++++++++++++--------------------- iscsi/iscsiadm.go | 20 ++-- 2 files changed, 159 insertions(+), 134 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index ef631508..cb992b2d 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -73,12 +73,10 @@ type Connector struct { MountTargetDevice *Device `json:"mount_target_device"` Devices []Device `json:"devices"` - RetryCount int32 `json:"retry_count"` - CheckInterval int32 `json:"check_interval"` - DoDiscovery bool `json:"do_discovery"` - DoCHAPDiscovery bool `json:"do_chap_discovery"` - TargetIqn string `json:"target_iqn"` - TargetPortals []string `json:"target_portals"` + RetryCount uint `json:"retry_count"` + CheckInterval uint `json:"check_interval"` + DoDiscovery bool `json:"do_discovery"` + DoCHAPDiscovery bool `json:"do_chap_discovery"` } func init() { @@ -161,47 +159,60 @@ func getCurrentSessions() ([]iscsiSession, error) { return sessions, err } -func waitForPathToExist(devicePath *string, maxRetries, intervalSeconds int, deviceTransport string) (bool, error) { +// waitForPathToExist wait for a file at a path to exists on disk +func waitForPathToExist(devicePath *string, maxRetries, intervalSeconds uint, deviceTransport string) error { return waitForPathToExistImpl(devicePath, maxRetries, intervalSeconds, deviceTransport, os.Stat, filepath.Glob) } -func waitForPathToExistImpl(devicePath *string, maxRetries, intervalSeconds int, deviceTransport string, osStat statFunc, filepathGlob globFunc) (bool, error) { +func waitForPathToExistImpl(devicePath *string, maxRetries, intervalSeconds uint, deviceTransport string, osStat statFunc, filepathGlob globFunc) error { if devicePath == nil { - return false, fmt.Errorf("unable to check unspecified devicePath") + return fmt.Errorf("unable to check unspecified devicePath") } - var err error - for i := 0; i < maxRetries; i++ { - err = nil - if deviceTransport == "tcp" { - _, err = osStat(*devicePath) - if err != nil && !os.IsNotExist(err) { - debug.Printf("Error attempting to stat device: %s", err.Error()) - return false, err - } else if err != nil { - debug.Printf("Device not found for: %s", *devicePath) - } + for i := uint(0); i <= maxRetries; i++ { + if i != 0 { + debug.Printf("Device path %q doesn't exists yet, retrying in %d seconds (%d/%d)", *devicePath, intervalSeconds, i, maxRetries) + time.Sleep(time.Second * time.Duration(intervalSeconds)) + } - } else { - fpath, _ := filepathGlob(*devicePath) - if fpath == nil { - err = os.ErrNotExist - } else { - // There might be a case that fpath contains multiple device paths if - // multiple PCI devices connect to same iSCSI target. We handle this - // case at subsequent logic. Pick up only first path here. - *devicePath = fpath[0] - } + if err := pathExistsImpl(devicePath, deviceTransport, osStat, filepathGlob); err == nil { + return nil + } else if !os.IsNotExist(err) { + return err } - if err == nil { - return true, nil + } + + return os.ErrNotExist +} + +// pathExists checks if a file at a path exists on disk +func pathExists(devicePath *string, deviceTransport string) error { + return pathExistsImpl(devicePath, deviceTransport, os.Stat, filepath.Glob) +} + +func pathExistsImpl(devicePath *string, deviceTransport string, osStat statFunc, filepathGlob globFunc) error { + if deviceTransport == "tcp" { + _, err := osStat(*devicePath) + if err != nil { + if !os.IsNotExist(err) { + debug.Printf("Error attempting to stat device: %s", err.Error()) + return err + } + debug.Printf("Device not found for: %s", *devicePath) + return err } - if i == maxRetries-1 { - break + } else { + fpath, _ := filepathGlob(*devicePath) + if fpath == nil { + return os.ErrNotExist } - time.Sleep(time.Second * time.Duration(intervalSeconds)) + // There might be a case that fpath contains multiple device paths if + // multiple PCI devices connect to same iscsi target. We handle this + // case at subsequent logic. Pick up only first path here. + *devicePath = fpath[0] } - return false, err + + return nil } // getMultipathDevice returns a multipath device for the configured targets if it exists @@ -244,8 +255,7 @@ func getMultipathDevice(devices []Device) (*Device, error) { } // Connect attempts to connect a volume to this node using the provided Connector info -func Connect(c *Connector) (string, error) { - var lastErr error +func (c *Connector) Connect() (string, error) { if c.RetryCount == 0 { c.RetryCount = 10 } @@ -253,11 +263,6 @@ func Connect(c *Connector) (string, error) { c.CheckInterval = 1 } - if c.RetryCount < 0 || c.CheckInterval < 0 { - return "", fmt.Errorf("invalid RetryCount and CheckInterval combination, both must be positive integers. "+ - "RetryCount: %d, CheckInterval: %d", c.RetryCount, c.CheckInterval) - } - iFace := "default" if c.Interface != "" { iFace = c.Interface @@ -270,81 +275,23 @@ func Connect(c *Connector) (string, error) { } iscsiTransport := extractTransportName(out) + var lastErr error var devicePaths []string for _, target := range c.Targets { - debug.Printf("process targetIqn: %s, portal: %s\n", target.Iqn, target.Portal) - baseArgs := []string{"-m", "node", "-T", target.Iqn, "-p", target.Portal} - // Rescan sessions to discover newly mapped LUNs. Do not specify the interface when rescanning - // to avoid establishing additional sessions to the same target. - if _, err := iscsiCmd(append(baseArgs, []string{"-R"}...)...); err != nil { - debug.Printf("failed to rescan session, err: %v", err) - } - - // create our devicePath that we'll be looking for based on the transport being used - port := defaultPort - if target.Port != "" { - port = target.Port - } - // portal with port - p := strings.Join([]string{target.Portal, port}, ":") - devicePath := strings.Join([]string{"/dev/disk/by-path/ip", p, "iscsi", target.Iqn, "lun", fmt.Sprint(c.Lun)}, "-") - if iscsiTransport != "tcp" { - devicePath = strings.Join([]string{"/dev/disk/by-path/pci", "*", "ip", p, "iscsi", target.Iqn, "lun", fmt.Sprint(c.Lun)}, "-") - } - - exists, _ := sessionExists(p, target.Iqn) - if exists { - if exists, err := waitForPathToExist(&devicePath, 1, 1, iscsiTransport); exists { - debug.Printf("Appending device path: %s", devicePath) - devicePaths = append(devicePaths, devicePath) - continue - } else if err != nil { - return "", err - } - } - - if c.DoDiscovery { - // build discoverydb and discover iSCSI target - if err := Discoverydb(p, iFace, c.DiscoverySecrets, c.DoCHAPDiscovery); err != nil { - debug.Printf("Error in discovery of the target: %s\n", err.Error()) - lastErr = err - continue - } - } - - if c.DoCHAPDiscovery { - // Make sure we don't log the secrets - err := CreateDBEntry(target.Iqn, p, iFace, c.DiscoverySecrets, c.SessionSecrets) - if err != nil { - debug.Printf("Error creating db entry: %s\n", err.Error()) - continue - } - } - - // perform the login - err = Login(target.Iqn, p) + devicePath, err := c.connectTarget(&target, iFace, iscsiTransport) if err != nil { - debug.Printf("failed to login, err: %v", err) lastErr = err - continue - } - retries := int(c.RetryCount / c.CheckInterval) - if exists, err := waitForPathToExist(&devicePath, retries, int(c.CheckInterval), iscsiTransport); exists { + } else { + debug.Printf("Appending device path: %s", devicePath) devicePaths = append(devicePaths, devicePath) - continue - } else if err != nil { - lastErr = fmt.Errorf("couldn't attach disk, err: %v", err) } } // GetISCSIDevices returns all devices if no paths are given if len(devicePaths) < 1 { c.Devices = []Device{} - } else { - c.Devices, err = GetISCSIDevices(devicePaths) - if err != nil { - return "", err - } + } else if c.Devices, err = GetISCSIDevices(devicePaths); err != nil { + return "", err } if len(c.Devices) < 1 { @@ -352,7 +299,7 @@ func Connect(c *Connector) (string, error) { return "", fmt.Errorf("failed to find device path: %s, last error seen: %v", devicePaths, lastErr) } - mountTargetDevice, err := getMountTargetDevice(c) + mountTargetDevice, err := c.getMountTargetDevice() c.MountTargetDevice = mountTargetDevice if err != nil { debug.Printf("Connect failed: %v", err) @@ -365,18 +312,95 @@ func Connect(c *Connector) (string, error) { return c.MountTargetDevice.GetPath(), nil } -//Disconnect performs a disconnect operation on a volume -func Disconnect(tgtIqn string, portals []string) error { - err := Logout(tgtIqn, portals) +func (c *Connector) connectTarget(target *TargetInfo, iFace string, iscsiTransport string) (string, error) { + debug.Printf("Process targetIqn: %s, portal: %s\n", target.Iqn, target.Portal) + baseArgs := []string{"-m", "node", "-T", target.Iqn, "-p", target.Portal} + // Rescan sessions to discover newly mapped LUNs. Do not specify the interface when rescanning + // to avoid establishing additional sessions to the same target. + if _, err := iscsiCmd(append(baseArgs, []string{"-R"}...)...); err != nil { + debug.Printf("Failed to rescan session, err: %v", err) + } + + // create our devicePath that we'll be looking for based on the transport being used + port := defaultPort + if target.Port != "" { + port = target.Port + } + // portal with port + portal := strings.Join([]string{target.Portal, port}, ":") + devicePath := strings.Join([]string{"/dev/disk/by-path/ip", portal, "iscsi", target.Iqn, "lun", fmt.Sprint(c.Lun)}, "-") + if iscsiTransport != "tcp" { + devicePath = strings.Join([]string{"/dev/disk/by-path/pci", "*", "ip", portal, "iscsi", target.Iqn, "lun", fmt.Sprint(c.Lun)}, "-") + } + + exists, _ := sessionExists(portal, target.Iqn) + if exists { + debug.Printf("Session already exists, checking if device path %q exists", devicePath) + if err := waitForPathToExist(&devicePath, c.RetryCount, c.CheckInterval, iscsiTransport); err != nil { + return "", err + } + return devicePath, nil + } + + if err := c.discoverTarget(target, iFace, portal); err != nil { + return "", err + } + + // perform the login + err := Login(target.Iqn, portal) if err != nil { - return err + debug.Printf("Failed to login: %v", err) + return "", err + } + + debug.Printf("Waiting for device path %q to exist", devicePath) + if err := waitForPathToExist(&devicePath, c.RetryCount, c.CheckInterval, iscsiTransport); err != nil { + return "", err + } + + return devicePath, nil +} + +func (c *Connector) discoverTarget(target *TargetInfo, iFace string, portal string) error { + if c.DoDiscovery { + // build discoverydb and discover iscsi target + if err := Discoverydb(portal, iFace, c.DiscoverySecrets, c.DoCHAPDiscovery); err != nil { + debug.Printf("Error in discovery of the target: %s\n", err.Error()) + return err + } + } + + if c.DoCHAPDiscovery { + // Make sure we don't log the secrets + err := CreateDBEntry(target.Iqn, portal, iFace, c.DiscoverySecrets, c.SessionSecrets) + if err != nil { + debug.Printf("Error creating db entry: %s\n", err.Error()) + return err + } + } + + return nil +} + +// Disconnect performs a disconnect operation from an appliance. +// Be sure to disconnect all deivces properly before doing this as it can result in data loss. +func (c *Connector) Disconnect() { + for _, target := range c.Targets { + Logout(target.Iqn, target.Portal) + } + + deleted := map[string]bool{} + for _, target := range c.Targets { + if _, ok := deleted[target.Iqn]; ok { + continue + } + deleted[target.Iqn] = true + DeleteDBEntry(target.Iqn) } - err = DeleteDBEntry(tgtIqn) - return err } // DisconnectVolume removes a volume from a Linux host. -func DisconnectVolume(c *Connector) error { +func (c *Connector) DisconnectVolume() 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). @@ -387,13 +411,12 @@ func DisconnectVolume(c *Connector) error { // DisconnectVolume focuses on step 2 and 3. // Note: make sure the volume is already unmounted before calling this method. - if len(c.Devices) > 1 { + if c.IsMultipathEnabled() { debug.Printf("Removing multipath device in path %s.\n", c.MountTargetDevice.GetPath()) err := FlushMultipathDevice(c.MountTargetDevice) if err != nil { return err } - if err := RemoveSCSIDevices(c.Devices...); err != nil { return err } @@ -413,8 +436,9 @@ func DisconnectVolume(c *Connector) error { return nil } -func getMountTargetDevice(c *Connector) (*Device, error) { - if len(c.Devices) > 1 { +// getMountTargetDevice returns the device to be mounted among the configured devices +func (c *Connector) getMountTargetDevice() (*Device, error) { + if c.IsMultipathEnabled() { multipathDevice, err := getMultipathDevice(c.Devices) if err != nil { debug.Printf("mount target is not a multipath device: %v", err) @@ -431,7 +455,12 @@ func getMountTargetDevice(c *Connector) (*Device, error) { return &c.Devices[0], nil } -// GetISCSIDevice get an iSCSI device from a device name +// IsMultipathEnabled check if multipath is enabled on devices handled by this connector +func (c *Connector) IsMultipathEnabled() bool { + return len(c.Devices) > 1 +} + +// GetISCSIDevice get an SCSI device from a device name func GetISCSIDevice(deviceName string) (*Device, error) { iscsiDevices, err := GetISCSIDevices([]string{deviceName}) if err != nil { @@ -551,8 +580,8 @@ func RemoveSCSIDevices(devices ...Device) error { return nil } -// PersistConnector persists the provided Connector to the specified file (ie /var/lib/pfile/myConnector.json) -func PersistConnector(c *Connector, filePath string) error { +// Persist persists the Connector to the specified file (ie /var/lib/pfile/myConnector.json) +func (c *Connector) Persist(filePath string) error { //file := path.Join("mnt", c.VolumeName+".json") f, err := os.Create(filePath) if err != nil { diff --git a/iscsi/iscsiadm.go b/iscsi/iscsiadm.go index 44648ee0..f6f096da 100644 --- a/iscsi/iscsiadm.go +++ b/iscsi/iscsiadm.go @@ -181,29 +181,25 @@ func GetSessions() (string, error) { // Login performs an iscsi login for the specified target func Login(tgtIQN, portal string) error { + debug.Println("Begin Login...") baseArgs := []string{"-m", "node", "-T", tgtIQN, "-p", portal} - _, err := iscsiCmd(append(baseArgs, []string{"-l"}...)...) - if err != nil { + if _, err := iscsiCmd(append(baseArgs, []string{"-l"}...)...); err != nil { //delete the node record from database iscsiCmd(append(baseArgs, []string{"-o", "delete"}...)...) return fmt.Errorf("failed to sendtargets to portal %s, err: %v", portal, err) } - return err + return nil } -// Logout logs out the specified target, if the target is not logged in it's not considered an error -func Logout(tgtIQN string, portals []string) error { +// Logout logs out the specified target +func Logout(tgtIQN, portal string) error { debug.Println("Begin Logout...") - baseArgs := []string{"-m", "node", "-T", tgtIQN} - for _, p := range portals { - debug.Printf("attempting logout for portal: %s", p) - args := append(baseArgs, []string{"-p", p, "-u"}...) - iscsiCmd(args...) - } + args := []string{"-m", "node", "-T", tgtIQN, "-p", portal, "-u"} + iscsiCmd(args...) return nil } -// DeleteDBEntry deletes the iscsi db entry fo rthe specified target +// DeleteDBEntry deletes the iscsi db entry for the specified target func DeleteDBEntry(tgtIQN string) error { debug.Println("Begin DeleteDBEntry...") args := []string{"-m", "node", "-T", tgtIQN, "-o", "delete"} From 7b2d8ee1aa7fe2e4fc96eede165dd7f5f2f2db2d Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Mon, 1 Feb 2021 20:49:20 +0700 Subject: [PATCH 06/19] add function to resize multipath map --- iscsi/multipath.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/iscsi/multipath.go b/iscsi/multipath.go index 46f9c56c..2d727981 100644 --- a/iscsi/multipath.go +++ b/iscsi/multipath.go @@ -116,3 +116,14 @@ func FlushMultipathDevice(device *Device) error { debug.Printf("Finshed flushing multipath device %v.\n", devicePath) return nil } + +// ResizeMultipathDevice resize a multipath device based on its underlying devices +func ResizeMultipathDevice(device *Device) error { + debug.Printf("Resizing multipath device %s\n", device.GetPath()) + + if output, err := exec.Command("multipathd", "resize", "map", device.Name).CombinedOutput(); err != nil { + return fmt.Errorf("could not resize multipath device: %s (%v)", output, err) + } + + return nil +} From 898cec5de0dcf16430bbd90657665949c287f45a Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Tue, 16 Feb 2021 14:46:29 +0700 Subject: [PATCH 07/19] check if device exists before flushing it --- iscsi/iscsi.go | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index cb992b2d..beaf5c95 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -548,14 +548,18 @@ func RemoveSCSIDevices(devices ...Device) error { var errs []error for _, device := range devices { debug.Printf("Flush SCSI device %v.\n", device.Name) - out, err := exec.Command("blockdev", "--flushbufs", device.GetPath()).CombinedOutput() - if err != nil { - debug.Printf("Command 'blockdev --flushbufs %v' did not succeed to flush the device: %v\n", device.Name, err) - return errors.New(string(out)) + if err := device.Exists(); err == nil { + out, err := exec.Command("blockdev", "--flushbufs", device.GetPath()).CombinedOutput() + if err != nil { + debug.Printf("Command 'blockdev --flushbufs %s' did not succeed to flush the device: %v\n", device.GetPath(), err) + return errors.New(string(out)) + } + } else if !os.IsNotExist(err) { + return err } - debug.Printf("Put SCSI device %v offline.\n", device.Name) - err = device.Shutdown() + debug.Printf("Put SCSI device %q offline.\n", device.Name) + err := device.Shutdown() if err != nil { if !os.IsNotExist(err) { // Ignore device already removed errs = append(errs, err) @@ -563,7 +567,7 @@ func RemoveSCSIDevices(devices ...Device) error { continue } - debug.Printf("Delete SCSI device %v.\n", device.Name) + debug.Printf("Delete SCSI device %q.\n", device.Name) err = device.Delete() if err != nil { if !os.IsNotExist(err) { // Ignore device already removed @@ -611,6 +615,12 @@ func GetConnectorFromFile(filePath string) (*Connector, error) { return &data, nil } +// Exists check if the device exists at its path and returns an error otherwise +func (d *Device) Exists() error { + _, err := os.Stat(d.GetPath()) + return err +} + // GetPath returns the path of a device func (d *Device) GetPath() string { if d.Type == "mpath" { From df4b0a3eb994a27785e8e9535723265256ac2ec0 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Wed, 17 Feb 2021 17:25:53 +0700 Subject: [PATCH 08/19] fix tests and example --- example/main.go | 31 ++++---- iscsi/iscsi.go | 49 ++++-------- iscsi/iscsi_test.go | 177 +++++++++++++++++++++----------------------- iscsi/multipath.go | 8 +- 4 files changed, 119 insertions(+), 146 deletions(-) diff --git a/example/main.go b/example/main.go index 21eb42b3..327a19eb 100644 --- a/example/main.go +++ b/example/main.go @@ -11,12 +11,12 @@ import ( ) var ( - portals = flag.String("portals", "192.168.1.112:3260", "Comma delimited. Eg: 1.1.1.1,2.2.2.2") - iqn = flag.String("iqn", "iqn.2010-10.org.openstack:volume-95739000-1557-44f8-9f40-e9d29fe6ec47", "") - username = flag.String("username", "3aX7EEf3CEgvESQG75qh", "") - password = flag.String("password", "eJBDC7Bt7WE3XFDq", "") - lun = flag.Int("lun", 1, "") - debug = flag.Bool("debug", false, "enable logging") + portals = flag.String("portals", "192.168.1.112:3260", "Comma delimited. Eg: 1.1.1.1,2.2.2.2") + iqn = flag.String("iqn", "iqn.2010-10.org.openstack:volume-95739000-1557-44f8-9f40-e9d29fe6ec47", "") + username = flag.String("username", "3aX7EEf3CEgvESQG75qh", "") + password = flag.String("password", "eJBDC7Bt7WE3XFDq", "") + lun = flag.Int("lun", 1, "") + debug = flag.Bool("debug", false, "enable logging") ) func main() { @@ -32,9 +32,9 @@ func main() { parts := strings.Split(tgtp, ":") targets = append(targets, iscsi.TargetInfo{ // Specify the target iqn we're dealing with - Iqn: *iqn, + Iqn: *iqn, Portal: parts[0], - Port: parts[1], + Port: parts[1], }) } @@ -62,16 +62,21 @@ func main() { // Now we can just issue a connection request using our Connector // A succesful connection will include the device path to access our iscsi volume - path, err := iscsi.Connect(c) + path, err := c.Connect() if err != nil { - log.Printf("Error returned from iscsi.Connect: %s", err.Error()) + log.Printf("Error returned from c.Connect: %s", err.Error()) os.Exit(1) } log.Printf("Connected device at path: %s\n", path) time.Sleep(3 * time.Second) - // Disconnect is easy as well, we don't need the full Connector any more, just the Target IQN and the Portals - /// this should disconnect the volume as well as clear out the iscsi DB entries associated with it - iscsi.Disconnect(*iqn, tgtps) + // This will disconnect the volume + if err := c.DisconnectVolume(); err != nil { + log.Printf("Error returned from c.DisconnectVolume: %s", err.Error()) + os.Exit(1) + } + + // This will disconnect the session as well as clear out the iscsi DB entries associated with it + c.Disconnect() } diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index beaf5c95..e5f38d5f 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -20,14 +20,15 @@ import ( const defaultPort = "3260" var ( - debug *log.Logger - execCommand = exec.Command - execWithTimeout = ExecWithTimeout + debug *log.Logger + execCommand = exec.Command + execCommandContext = exec.CommandContext + execWithTimeout = ExecWithTimeout + osStat = os.Stat + filepathGlob = filepath.Glob + osOpenFile = os.OpenFile ) -type statFunc func(string) (os.FileInfo, error) -type globFunc func(string) ([]string, error) - // iscsiSession contains information avout an iSCSI session type iscsiSession struct { Protocol string @@ -161,10 +162,6 @@ func getCurrentSessions() ([]iscsiSession, error) { // waitForPathToExist wait for a file at a path to exists on disk func waitForPathToExist(devicePath *string, maxRetries, intervalSeconds uint, deviceTransport string) error { - return waitForPathToExistImpl(devicePath, maxRetries, intervalSeconds, deviceTransport, os.Stat, filepath.Glob) -} - -func waitForPathToExistImpl(devicePath *string, maxRetries, intervalSeconds uint, deviceTransport string, osStat statFunc, filepathGlob globFunc) error { if devicePath == nil { return fmt.Errorf("unable to check unspecified devicePath") } @@ -175,7 +172,7 @@ func waitForPathToExistImpl(devicePath *string, maxRetries, intervalSeconds uint time.Sleep(time.Second * time.Duration(intervalSeconds)) } - if err := pathExistsImpl(devicePath, deviceTransport, osStat, filepathGlob); err == nil { + if err := pathExists(devicePath, deviceTransport); err == nil { return nil } else if !os.IsNotExist(err) { return err @@ -187,10 +184,6 @@ func waitForPathToExistImpl(devicePath *string, maxRetries, intervalSeconds uint // pathExists checks if a file at a path exists on disk func pathExists(devicePath *string, deviceTransport string) error { - return pathExistsImpl(devicePath, deviceTransport, os.Stat, filepath.Glob) -} - -func pathExistsImpl(devicePath *string, deviceTransport string, osStat statFunc, filepathGlob globFunc) error { if deviceTransport == "tcp" { _, err := osStat(*devicePath) if err != nil { @@ -423,11 +416,7 @@ func (c *Connector) DisconnectVolume() error { } else { devicePath := c.MountTargetDevice.GetPath() debug.Printf("Removing normal device in path %s.\n", devicePath) - device, err := GetISCSIDevice(devicePath) - if err != nil { - return err - } - if err = RemoveSCSIDevices(*device); err != nil { + if err := RemoveSCSIDevices(*c.MountTargetDevice); err != nil { return err } } @@ -460,18 +449,6 @@ func (c *Connector) IsMultipathEnabled() bool { return len(c.Devices) > 1 } -// GetISCSIDevice get an SCSI device from a device name -func GetISCSIDevice(deviceName string) (*Device, error) { - iscsiDevices, err := GetISCSIDevices([]string{deviceName}) - if err != nil { - return nil, err - } - if len(iscsiDevices) == 0 { - return nil, fmt.Errorf("device %q not found", deviceName) - } - return &iscsiDevices[0], nil -} - // GetSCSIDevices get SCSI devices from device paths // It will returns all SCSI devices if no paths are given func GetSCSIDevices(devicePaths []string) ([]Device, error) { @@ -512,7 +489,7 @@ func GetISCSIDevices(devicePaths []string) (devices []Device, err error) { // lsblk execute the lsblk commands func lsblk(flags string, devicePaths []string) ([]byte, error) { - out, err := exec.Command("lsblk", append([]string{flags}, devicePaths...)...).CombinedOutput() + out, err := execCommand("lsblk", append([]string{flags}, devicePaths...)...).CombinedOutput() debug.Printf("lsblk %s %s", flags, strings.Join(devicePaths, " ")) if err != nil { return nil, fmt.Errorf("lsblk: %s", string(out)) @@ -526,7 +503,7 @@ func writeInSCSIDeviceFile(hctl string, file string, content string) error { filename := filepath.Join("/sys/class/scsi_device", hctl, "device", file) debug.Printf("Write %q in %q.\n", content, filename) - f, err := os.OpenFile(filename, os.O_TRUNC|os.O_WRONLY, 0200) + f, err := osOpenFile(filename, os.O_TRUNC|os.O_WRONLY, 0200) if err != nil { debug.Printf("Error while opening file %v: %v\n", filename, err) return err @@ -549,7 +526,7 @@ func RemoveSCSIDevices(devices ...Device) error { for _, device := range devices { debug.Printf("Flush SCSI device %v.\n", device.Name) if err := device.Exists(); err == nil { - out, err := exec.Command("blockdev", "--flushbufs", device.GetPath()).CombinedOutput() + out, err := execCommand("blockdev", "--flushbufs", device.GetPath()).CombinedOutput() if err != nil { debug.Printf("Command 'blockdev --flushbufs %s' did not succeed to flush the device: %v\n", device.GetPath(), err) return errors.New(string(out)) @@ -617,7 +594,7 @@ func GetConnectorFromFile(filePath string) (*Connector, error) { // Exists check if the device exists at its path and returns an error otherwise func (d *Device) Exists() error { - _, err := os.Stat(d.GetPath()) + _, err := osStat(d.GetPath()) return err } diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index 70529a9e..53daa114 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -89,10 +89,7 @@ var testExecWithTimeoutError error var mockedExitStatus = 0 var mockedStdout string -var sysBlockPath = "/sys/block" -var normalDevice = "sda" -var multipathDevice = "dm-1" -var slaves = []string{"sdb", "sdc"} +const testRootFS = "/tmp/iscsi-tests" type testCmdRunner struct{} @@ -129,31 +126,27 @@ 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 { +func getDevicePath(device *Device) string { + sysDevicePath := "/tmp/iscsi-tests/sys/class/scsi_device/" + return filepath.Join(sysDevicePath, device.Hctl, "device") +} + +func preparePaths(devices []Device) error { + for _, d := range devices { + devicePath := getDevicePath(&d) + + if err := os.MkdirAll(devicePath, os.ModePerm); err != nil { return err } - } - for _, s := range slaves { - err := os.MkdirAll(filepath.Join(sysBlockPath, multipathDevice, "slaves", s), os.ModePerm) - if err != nil { - return err + + for _, filename := range []string{"delete", "state"} { + if err := ioutil.WriteFile(filepath.Join(devicePath, filename), []byte(""), 0600); 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) { @@ -273,48 +266,41 @@ 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 + deleteDeviceFile := "/tmp/deleteDevice" + osOpenFile = func(name string, flag int, perm os.FileMode) (*os.File, error) { + fmt.Println(deleteDeviceFile) + return os.OpenFile(deleteDeviceFile, flag, perm) } - 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 + name string + withDeviceFile bool + wantErr bool }{ - {"DisconnectNormalVolume", false, false}, - {"DisconnectNonexistentNormalVolume", true, false}, + {"DisconnectNormalVolume", true, false}, + {"DisconnectNonexistentNormalVolume", false, false}, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.removeDevice { - os.RemoveAll(filepath.Join(sysBlockPath, devicePath)) + if tt.withDeviceFile { + os.Create(deleteDeviceFile) + } else { + os.RemoveAll(testRootFS) } - c := Connector{Multipath: false, DevicePath: devicePath} - err := DisconnectVolume(c) + + device := Device{Name: "test"} + c := Connector{Devices: []Device{device}, MountTargetDevice: &device} + err := c.DisconnectVolume() 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 tt.withDeviceFile { + out, err := ioutil.ReadFile(deleteDeviceFile) if err != nil { - t.Errorf("can not read file %v: %v", deleteFile, err) + t.Errorf("can not read file %v: %v", deleteDeviceFile, err) return } if string(out) != "1" { @@ -327,51 +313,54 @@ func Test_DisconnectNormalVolume(t *testing.T) { } func Test_DisconnectMultipathVolume(t *testing.T) { - execWithTimeout = fakeExecWithTimeout - devicePath := multipathDevice + mockedExitStatus = 0 + mockedStdout = "" + execCommand = fakeExecCommand + osStat = func(name string) (os.FileInfo, error) { + return nil, nil + } tests := []struct { - name string - timeout bool - removeDevice bool - wantErr bool - cmdError error + name string + timeout bool + withDeviceFile bool + wantErr bool + cmdError error }{ - {"DisconnectMultipathVolume", false, false, false, nil}, - {"DisconnectMultipathVolumeFlushFailed", false, false, true, fmt.Errorf("error")}, - {"DisconnectMultipathVolumeFlushTimeout", true, false, true, nil}, - {"DisconnectNonexistentMultipathVolume", false, true, false, fmt.Errorf("error")}, + {"DisconnectMultipathVolume", false, true, false, nil}, + {"DisconnectMultipathVolumeFlushTimeout", true, true, true, nil}, + {"DisconnectNonexistentMultipathVolume", false, false, false, nil}, } + 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 + testExecWithTimeoutError = tt.cmdError + testCmdTimeout = tt.timeout + c := Connector{ + Devices: []Device{{Hctl: "hctl1"}, {Hctl: "hctl2"}}, + MountTargetDevice: &Device{Type: "mpath"}, } - 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 + osOpenFile = func(name string, flag int, perm os.FileMode) (*os.File, error) { + return os.OpenFile(testRootFS+name, flag, perm) } - testExecWithTimeoutError = tt.cmdError - testCmdTimeout = tt.timeout - if tt.removeDevice { - os.RemoveAll(filepath.Join(sysBlockPath, devicePath)) - os.RemoveAll(devPath) + + if tt.withDeviceFile { + if err := preparePaths(c.Devices); err != nil { + t.Errorf("could not prepare paths: %v", err) + return + } + } else { + os.Remove(testRootFS) } - c := Connector{Multipath: true, DevicePath: devicePath} - err = DisconnectVolume(c) + + err := c.DisconnectVolume() 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) @@ -379,21 +368,23 @@ func Test_DisconnectMultipathVolume(t *testing.T) { } } - if !tt.removeDevice && !tt.wantErr { - 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 - } + if tt.withDeviceFile && !tt.wantErr { + for _, device := range c.Devices { + checkFileContents(t, getDevicePath(&device)+"/delete", "1") + checkFileContents(t, getDevicePath(&device)+"/state", "offline\n") } } }) } } + +func checkFileContents(t *testing.T, path string, contents string) { + if out, err := ioutil.ReadFile(path); err != nil { + t.Errorf("could not read file: %v", err) + return + } else if string(out) != contents { + t.Errorf("file content mismatch, got = %q, want = %q", string(out), contents) + return + } +} diff --git a/iscsi/multipath.go b/iscsi/multipath.go index 2d727981..a7a86c9c 100644 --- a/iscsi/multipath.go +++ b/iscsi/multipath.go @@ -38,7 +38,7 @@ func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]by defer cancel() // Create command with context - cmd := exec.CommandContext(ctx, command, args...) + cmd := execCommandContext(ctx, command, args...) // This time we can simply use Output() to get the result. out, err := cmd.Output() @@ -65,7 +65,7 @@ func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]by func getMultipathMap(device string) (*multipathDeviceMap, error) { debug.Printf("Getting multipath map for device %s.\n", device) - cmd := exec.Command("multipathd", "show", "map", device[1:], "json") + cmd := execCommand("multipathd", "show", "map", device[1:], "json") out, err := cmd.CombinedOutput() // debug.Printf(string(out)) if err != nil { @@ -102,7 +102,7 @@ func FlushMultipathDevice(device *Device) error { _, err := execWithTimeout("multipath", []string{"-f", devicePath}, timeout) if err != nil { - if _, e := os.Stat(devicePath); os.IsNotExist(e) { + if _, e := osStat(devicePath); os.IsNotExist(e) { debug.Printf("Multipath device %v has been removed.\n", devicePath) } else { if strings.Contains(err.Error(), "map in use") { @@ -121,7 +121,7 @@ func FlushMultipathDevice(device *Device) error { func ResizeMultipathDevice(device *Device) error { debug.Printf("Resizing multipath device %s\n", device.GetPath()) - if output, err := exec.Command("multipathd", "resize", "map", device.Name).CombinedOutput(); err != nil { + if output, err := execCommand("multipathd", "resize", "map", device.Name).CombinedOutput(); err != nil { return fmt.Errorf("could not resize multipath device: %s (%v)", output, err) } From 736e81c182584b740e1cb2dbca81d828fecf903a Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Thu, 18 Feb 2021 15:53:01 +0700 Subject: [PATCH 09/19] replace manual reassignation with stubs in tests --- go.mod | 2 + go.sum | 2 + iscsi/iscsi_test.go | 100 ++++++++++++++++++----------------------- iscsi/iscsiadm_test.go | 14 +++--- 4 files changed, 54 insertions(+), 64 deletions(-) create mode 100644 go.sum diff --git a/go.mod b/go.mod index a7d03b82..d5c5cdf9 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,5 @@ module github.com/kubernetes-csi/csi-lib-iscsi go 1.15 + +require github.com/prashantv/gostub v1.0.0 diff --git a/go.sum b/go.sum new file mode 100644 index 00000000..324091f3 --- /dev/null +++ b/go.sum @@ -0,0 +1,2 @@ +github.com/prashantv/gostub v1.0.0 h1:wTzvgO04xSS3gHuz6Vhuo0/kvWelyJxwNS0IRBPAwGY= +github.com/prashantv/gostub v1.0.0/go.mod h1:dP1v6T1QzyGJJKFocwAU0lSZKpfjstjH8TlhkEU0on0= diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index 53daa114..61521f31 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -11,9 +11,11 @@ import ( "strconv" "testing" "time" + + "github.com/prashantv/gostub" ) -var nodeDB = ` +const nodeDB = ` # BEGIN RECORD 6.2.0.874 node.name = iqn.2010-10.org.openstack:volume-eb393993-73d0-4e39-9ef4-b5841e244ced node.tpgt = -1 @@ -80,35 +82,30 @@ node.conn[0].iscsi.OFMarker = No # END RECORD ` -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 - +const emptyTransportName = "iface.transport_name = \n" +const emptyDbRecord = "\n\n\n" const testRootFS = "/tmp/iscsi-tests" -type testCmdRunner struct{} - -func fakeExecCommand(command string, args ...string) *exec.Cmd { - cs := []string{"-test.run=TestExecCommandHelper", "--", command} - cs = append(cs, args...) - cmd := exec.Command(os.Args[0], cs...) - es := strconv.Itoa(mockedExitStatus) - cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1", - "STDOUT=" + mockedStdout, - "EXIT_STATUS=" + es} - return cmd +func makeFakeExecCommand(exitStatus int, stdout string) func(string, ...string) *exec.Cmd { + return func(command string, args ...string) *exec.Cmd { + cs := []string{"-test.run=TestExecCommandHelper", "--", command} + cs = append(cs, args...) + cmd := exec.Command(os.Args[0], cs...) + es := strconv.Itoa(exitStatus) + cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1", + "STDOUT=" + stdout, + "EXIT_STATUS=" + es} + return cmd + } } -func fakeExecWithTimeout(command string, args []string, timeout time.Duration) ([]byte, error) { - if testCmdTimeout { - return nil, context.DeadlineExceeded +func makeFakeExecWithTimeout(testCmdTimeout bool, testExecWithTimeoutError error) func(string, []string, time.Duration) ([]byte, error) { + return func(command string, args []string, timeout time.Duration) ([]byte, error) { + if testCmdTimeout { + return nil, context.DeadlineExceeded + } + return []byte(""), testExecWithTimeoutError } - return []byte(testCmdOutput), testExecWithTimeoutError } func TestExecCommandHelper(t *testing.T) { @@ -121,11 +118,6 @@ func TestExecCommandHelper(t *testing.T) { os.Exit(i) } -func (tr testCmdRunner) execCmd(cmd string, args ...string) (string, error) { - return testCmdOutput, testCmdError - -} - func getDevicePath(device *Device) string { sysDevicePath := "/tmp/iscsi-tests/sys/class/scsi_device/" return filepath.Join(sysDevicePath, device.Hctl, "device") @@ -149,6 +141,16 @@ func preparePaths(devices []Device) error { return nil } +func checkFileContents(t *testing.T, path string, contents string) { + if out, err := ioutil.ReadFile(path); err != nil { + t.Errorf("could not read file: %v", err) + return + } else if string(out) != contents { + t.Errorf("file content mismatch, got = %q, want = %q", string(out), contents) + return + } +} + 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,9 +228,9 @@ func Test_extractTransportName(t *testing.T) { } func Test_sessionExists(t *testing.T) { - mockedExitStatus = 0 - mockedStdout = "tcp: [4] 192.168.1.107:3260,1 iqn.2010-10.org.openstack:volume-eb393993-73d0-4e39-9ef4-b5841e244ced (non-flash)\n" - execCommand = fakeExecCommand + fakeOutput := "tcp: [4] 192.168.1.107:3260,1 iqn.2010-10.org.openstack:volume-eb393993-73d0-4e39-9ef4-b5841e244ced (non-flash)\n" + defer gostub.Stub(&execCommand, makeFakeExecCommand(0, fakeOutput)).Reset() + type args struct { tgtPortal string tgtIQN string @@ -267,10 +269,9 @@ func Test_sessionExists(t *testing.T) { func Test_DisconnectNormalVolume(t *testing.T) { deleteDeviceFile := "/tmp/deleteDevice" - osOpenFile = func(name string, flag int, perm os.FileMode) (*os.File, error) { - fmt.Println(deleteDeviceFile) + defer gostub.Stub(&osOpenFile, func(name string, flag int, perm os.FileMode) (*os.File, error) { return os.OpenFile(deleteDeviceFile, flag, perm) - } + }).Reset() tests := []struct { name string @@ -313,13 +314,10 @@ func Test_DisconnectNormalVolume(t *testing.T) { } func Test_DisconnectMultipathVolume(t *testing.T) { - execWithTimeout = fakeExecWithTimeout - mockedExitStatus = 0 - mockedStdout = "" - execCommand = fakeExecCommand - osStat = func(name string) (os.FileInfo, error) { + defer gostub.Stub(&execCommand, makeFakeExecCommand(0, "")).Reset() + defer gostub.Stub(&osStat, func(name string) (os.FileInfo, error) { return nil, nil - } + }).Reset() tests := []struct { name string @@ -335,16 +333,15 @@ func Test_DisconnectMultipathVolume(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - testExecWithTimeoutError = tt.cmdError - testCmdTimeout = tt.timeout + defer gostub.Stub(&execWithTimeout, makeFakeExecWithTimeout(tt.timeout, tt.cmdError)).Reset() c := Connector{ Devices: []Device{{Hctl: "hctl1"}, {Hctl: "hctl2"}}, MountTargetDevice: &Device{Type: "mpath"}, } - osOpenFile = func(name string, flag int, perm os.FileMode) (*os.File, error) { + defer gostub.Stub(&osOpenFile, func(name string, flag int, perm os.FileMode) (*os.File, error) { return os.OpenFile(testRootFS+name, flag, perm) - } + }).Reset() if tt.withDeviceFile { if err := preparePaths(c.Devices); err != nil { @@ -373,18 +370,7 @@ func Test_DisconnectMultipathVolume(t *testing.T) { checkFileContents(t, getDevicePath(&device)+"/delete", "1") checkFileContents(t, getDevicePath(&device)+"/state", "offline\n") } - } }) } } - -func checkFileContents(t *testing.T, path string, contents string) { - if out, err := ioutil.ReadFile(path); err != nil { - t.Errorf("could not read file: %v", err) - return - } else if string(out) != contents { - t.Errorf("file content mismatch, got = %q, want = %q", string(out), contents) - return - } -} diff --git a/iscsi/iscsiadm_test.go b/iscsi/iscsiadm_test.go index 3d919766..9b924953 100644 --- a/iscsi/iscsiadm_test.go +++ b/iscsi/iscsiadm_test.go @@ -1,9 +1,12 @@ package iscsi -import "testing" +import ( + "testing" + + "github.com/prashantv/gostub" +) func TestDiscovery(t *testing.T) { - execCommand = fakeExecCommand tests := map[string]struct { tgtPortal string iface string @@ -63,8 +66,7 @@ iscsiadm: Could not perform SendTargets discovery.\n`, for name, tt := range tests { t.Run(name, func(t *testing.T) { - mockedExitStatus = tt.mockedExitStatus - mockedStdout = tt.mockedStdout + defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() err := Discoverydb(tt.tgtPortal, tt.iface, tt.discoverySecret, tt.chapDiscovery) if (err != nil) != tt.wantErr { t.Errorf("Discoverydb() error = %v, wantErr %v", err, tt.wantErr) @@ -75,7 +77,6 @@ iscsiadm: Could not perform SendTargets discovery.\n`, } func TestCreateDBEntry(t *testing.T) { - execCommand = fakeExecCommand tests := map[string]struct { tgtPortal string tgtIQN string @@ -115,8 +116,7 @@ func TestCreateDBEntry(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { - mockedExitStatus = tt.mockedExitStatus - mockedStdout = tt.mockedStdout + defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() err := CreateDBEntry(tt.tgtIQN, tt.tgtPortal, tt.iface, tt.discoverySecret, tt.sessionSecret) if (err != nil) != tt.wantErr { t.Errorf("CreateDBEntry() error = %v, wantErr %v", err, tt.wantErr) From c1dd9464733def02f7f5b9bfb2fbc68409d5d88f Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Fri, 19 Feb 2021 22:48:14 +0700 Subject: [PATCH 10/19] add tests and remove dead code --- Makefile | 5 +- go.mod | 5 +- go.sum | 10 ++ iscsi/iscsi.go | 49 ++++--- iscsi/iscsi_test.go | 275 ++++++++++++++++++++++++++++++++++++++++ iscsi/iscsiadm.go | 26 +--- iscsi/iscsiadm_test.go | 146 +++++++++++++++++++++ iscsi/multipath.go | 44 +------ iscsi/multipath_test.go | 79 ++++++++++++ 9 files changed, 551 insertions(+), 88 deletions(-) create mode 100644 iscsi/multipath_test.go diff --git a/Makefile b/Makefile index 26cf14c8..9e3fde2d 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: all build clean install +.PHONY: all build clean install test coverage all: clean build install @@ -16,3 +16,6 @@ install: test: go test ./iscsi/ +coverage: + go test ./iscsi -coverprofile=coverage.out + go tool cover -html=coverage.out diff --git a/go.mod b/go.mod index d5c5cdf9..c3584003 100644 --- a/go.mod +++ b/go.mod @@ -2,4 +2,7 @@ module github.com/kubernetes-csi/csi-lib-iscsi go 1.15 -require github.com/prashantv/gostub v1.0.0 +require ( + github.com/prashantv/gostub v1.0.0 + github.com/stretchr/testify v1.7.0 +) diff --git a/go.sum b/go.sum index 324091f3..5bb4efb7 100644 --- a/go.sum +++ b/go.sum @@ -1,2 +1,12 @@ +github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/prashantv/gostub v1.0.0 h1:wTzvgO04xSS3gHuz6Vhuo0/kvWelyJxwNS0IRBPAwGY= github.com/prashantv/gostub v1.0.0/go.mod h1:dP1v6T1QzyGJJKFocwAU0lSZKpfjstjH8TlhkEU0on0= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index e5f38d5f..c688a294 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -27,6 +27,7 @@ var ( osStat = os.Stat filepathGlob = filepath.Glob osOpenFile = os.OpenFile + sleep = time.Sleep ) // iscsiSession contains information avout an iSCSI session @@ -162,14 +163,14 @@ func getCurrentSessions() ([]iscsiSession, error) { // waitForPathToExist wait for a file at a path to exists on disk func waitForPathToExist(devicePath *string, maxRetries, intervalSeconds uint, deviceTransport string) error { - if devicePath == nil { + if devicePath == nil || *devicePath == "" { return fmt.Errorf("unable to check unspecified devicePath") } for i := uint(0); i <= maxRetries; i++ { if i != 0 { debug.Printf("Device path %q doesn't exists yet, retrying in %d seconds (%d/%d)", *devicePath, intervalSeconds, i, maxRetries) - time.Sleep(time.Second * time.Duration(intervalSeconds)) + sleep(time.Second * time.Duration(intervalSeconds)) } if err := pathExists(devicePath, deviceTransport); err == nil { @@ -195,7 +196,11 @@ func pathExists(devicePath *string, deviceTransport string) error { return err } } else { - fpath, _ := filepathGlob(*devicePath) + fpath, err := filepathGlob(*devicePath) + + if err != nil { + return err + } if fpath == nil { return os.ErrNotExist } @@ -210,22 +215,17 @@ func pathExists(devicePath *string, deviceTransport string) error { // getMultipathDevice returns a multipath device for the configured targets if it exists func getMultipathDevice(devices []Device) (*Device, error) { - var deviceInfo deviceInfo var multipathDevice *Device var devicePaths []string for _, device := range devices { devicePaths = append(devicePaths, device.GetPath()) } - out, err := lsblk("-J", devicePaths) + deviceInfo, err := lsblk("", devicePaths) if err != nil { return nil, err } - if err = json.Unmarshal(out, &deviceInfo); err != nil { - return nil, err - } - for _, device := range deviceInfo.BlockDevices { if len(device.Children) != 1 { return nil, fmt.Errorf("device is not mapped to exactly one multipath device: %v", device.Children) @@ -454,18 +454,12 @@ func (c *Connector) IsMultipathEnabled() bool { func GetSCSIDevices(devicePaths []string) ([]Device, error) { debug.Printf("Getting info about SCSI devices %s.\n", devicePaths) - out, err := lsblk("-JS", devicePaths) + deviceInfo, err := lsblk("-S", devicePaths) if err != nil { debug.Printf("An error occured while looking info about SCSI devices: %v", err) return nil, err } - var deviceInfo deviceInfo - err = json.Unmarshal(out, &deviceInfo) - if err != nil { - return nil, err - } - return deviceInfo.BlockDevices, nil } @@ -488,7 +482,7 @@ func GetISCSIDevices(devicePaths []string) (devices []Device, err error) { } // lsblk execute the lsblk commands -func lsblk(flags string, devicePaths []string) ([]byte, error) { +func lsblkRaw(flags string, devicePaths []string) ([]byte, error) { out, err := execCommand("lsblk", append([]string{flags}, devicePaths...)...).CombinedOutput() debug.Printf("lsblk %s %s", flags, strings.Join(devicePaths, " ")) if err != nil { @@ -498,6 +492,21 @@ func lsblk(flags string, devicePaths []string) ([]byte, error) { return out, nil } +func lsblk(flags string, devicePaths []string) (*deviceInfo, error) { + var deviceInfo deviceInfo + + out, err := lsblkRaw("-J "+flags, devicePaths) + if err != nil { + return nil, err + } + + if err = json.Unmarshal(out, &deviceInfo); err != nil { + return nil, err + } + + return &deviceInfo, nil +} + // writeInSCSIDeviceFile write into special devices files to change devices state func writeInSCSIDeviceFile(hctl string, file string, content string) error { filename := filepath.Join("/sys/class/scsi_device", hctl, "device", file) @@ -614,15 +623,15 @@ func (d *Device) WriteDeviceFile(name string, content string) error { // Shutdown turn off an SCSI device by writing offline\n in /sys/class/scsi_device/h:c:t:l/device/state func (d *Device) Shutdown() error { - return writeInSCSIDeviceFile(d.Hctl, "state", "offline\n") + return d.WriteDeviceFile("state", "offline\n") } // Delete detach an SCSI device by writing 1 in /sys/class/scsi_device/h:c:t:l/device/delete func (d *Device) Delete() error { - return writeInSCSIDeviceFile(d.Hctl, "delete", "1") + return d.WriteDeviceFile("delete", "1") } // Rescan rescan an SCSI device by writing 1 in /sys/class/scsi_device/h:c:t:l/device/rescan func (d *Device) Rescan() error { - return writeInSCSIDeviceFile(d.Hctl, "rescan", "1") + return d.WriteDeviceFile("rescan", "1") } diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index 61521f31..be117d59 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -2,6 +2,7 @@ package iscsi import ( "context" + "encoding/json" "fmt" "io/ioutil" "os" @@ -9,12 +10,23 @@ import ( "path/filepath" "reflect" "strconv" + "strings" "testing" "time" "github.com/prashantv/gostub" + "github.com/stretchr/testify/assert" ) +type testWriter struct { + data *[]byte +} + +func (w testWriter) Write(data []byte) (n int, err error) { + *w.data = append(*w.data, data...) + return len(data), nil +} + const nodeDB = ` # BEGIN RECORD 6.2.0.874 node.name = iqn.2010-10.org.openstack:volume-eb393993-73d0-4e39-9ef4-b5841e244ced @@ -99,6 +111,12 @@ func makeFakeExecCommand(exitStatus int, stdout string) func(string, ...string) } } +func makeFakeExecCommandContext(exitStatus int, stdout string) func(context.Context, string, ...string) *exec.Cmd { + return func(ctx context.Context, command string, args ...string) *exec.Cmd { + return makeFakeExecCommand(exitStatus, stdout)(command, args...) + } +} + func makeFakeExecWithTimeout(testCmdTimeout bool, testExecWithTimeoutError error) func(string, []string, time.Duration) ([]byte, error) { return func(command string, args []string, timeout time.Duration) ([]byte, error) { if testCmdTimeout { @@ -374,3 +392,260 @@ func Test_DisconnectMultipathVolume(t *testing.T) { }) } } + +func Test_EnableDebugLogging(t *testing.T) { + assert := assert.New(t) + data := []byte{} + writer := testWriter{data: &data} + EnableDebugLogging(writer) + + assert.Equal("", string(data)) + assert.Len(strings.Split(string(data), "\n"), 1) + + debug.Print("testing debug logs") + assert.Contains(string(data), "testing debug logs") + assert.Len(strings.Split(string(data), "\n"), 2) +} + +func Test_waitForPathToExist(t *testing.T) { + tests := map[string]struct { + attempts int + fileNotFound bool + withErr bool + transport string + }{ + "Basic": { + attempts: 1, + }, + "WithRetry": { + attempts: 2, + }, + "WithRetryFail": { + attempts: 3, + fileNotFound: true, + }, + "WithError": { + withErr: true, + }, + } + + for name, tt := range tests { + tt.transport = "tcp" + tests[name+"OverTCP"] = tt + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + attempts := 0 + maxRetries := tt.attempts - 1 + if tt.fileNotFound { + maxRetries-- + } + if maxRetries < 0 { + maxRetries = 0 + } + doAttempt := func(err error) error { + attempts++ + if tt.withErr { + return err + } + if attempts < tt.attempts { + return os.ErrNotExist + } + return nil + } + defer gostub.Stub(&osStat, func(name string) (os.FileInfo, error) { + if err := doAttempt(os.ErrPermission); err != nil { + return nil, err + } + return nil, nil + }).Reset() + defer gostub.Stub(&filepathGlob, func(name string) ([]string, error) { + if err := doAttempt(filepath.ErrBadPattern); err != nil { + return nil, err + } + return []string{"/somefilewithalongname"}, nil + }).Reset() + defer gostub.Stub(&sleep, func(_ time.Duration) {}).Reset() + path := "/somefile" + err := waitForPathToExist(&path, uint(maxRetries), 1, tt.transport) + + if tt.withErr { + if tt.transport == "tcp" { + assert.Equal(os.ErrPermission, err) + } else { + assert.Equal(filepath.ErrBadPattern, err) + } + return + } + if tt.fileNotFound { + assert.Equal(os.ErrNotExist, err) + assert.Equal(maxRetries, attempts-1) + } else { + assert.Nil(err) + assert.Equal(tt.attempts, attempts) + if tt.transport == "tcp" { + assert.Equal("/somefile", path) + } else { + assert.Equal("/somefilewithalongname", path) + } + } + }) + } + + t.Run("PathEmptyOrNil", func(t *testing.T) { + assert := assert.New(t) + path := "" + + err := waitForPathToExist(&path, 0, 0, "tcp") + assert.NotNil(err) + + err = waitForPathToExist(&path, 0, 0, "") + assert.NotNil(err) + + err = waitForPathToExist(nil, 0, 0, "tcp") + assert.NotNil(err) + + err = waitForPathToExist(nil, 0, 0, "") + assert.NotNil(err) + }) + + t.Run("PathNotFound", func(t *testing.T) { + assert := assert.New(t) + defer gostub.Stub(&filepathGlob, func(name string) ([]string, error) { + return nil, nil + }).Reset() + + path := "/test" + err := waitForPathToExist(&path, 0, 0, "") + assert.NotNil(err) + assert.Equal(os.ErrNotExist, err) + }) +} + +func Test_getMultipathDevice(t *testing.T) { + mpath1 := Device{Name: "3600c0ff0000000000000000000000000", Type: "mpath"} + mpath2 := Device{Name: "3600c0ff1111111111111111111111111", Type: "mpath"} + sda := Device{Name: "sda", Children: []Device{{Name: "sda1"}}} + sdb := Device{Name: "sdb", Children: []Device{mpath1}} + sdc := Device{Name: "sdc", Children: []Device{mpath1}} + sdd := Device{Name: "sdc", Children: []Device{mpath2}} + sde := Device{Name: "sdc", Children: []Device{mpath1, mpath2}} + + tests := map[string]struct { + mockedDevices deviceInfo + mockedStdout string + mockedExitStatus int + multipathDevice *Device + wantErr bool + }{ + "Basic": { + mockedDevices: deviceInfo{BlockDevices: []Device{sdb, sdc}}, + multipathDevice: &mpath1, + }, + "NotABlockDevice": { + mockedStdout: "lsblk: sdzz: not a block device", + mockedExitStatus: 32, + }, + "NotSharingTheSameMultipathDevice": { + mockedDevices: deviceInfo{BlockDevices: []Device{sdb, sdd}}, + wantErr: true, + }, + "MoreThanOneMultipathDevice": { + mockedDevices: deviceInfo{BlockDevices: []Device{sde}}, + wantErr: true, + }, + "NotAMultipathDevice": { + mockedDevices: deviceInfo{BlockDevices: []Device{sda}}, + wantErr: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + mockedStdout := tt.mockedStdout + if mockedStdout == "" { + out, err := json.Marshal(tt.mockedDevices) + assert.Nil(err, "could not setup test") + mockedStdout = string(out) + } + + gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, string(mockedStdout))) + multipathDevice, err := getMultipathDevice(tt.mockedDevices.BlockDevices) + + if tt.mockedExitStatus != 0 || tt.wantErr { + assert.Nil(multipathDevice) + assert.NotNil(err) + } else { + assert.Equal(tt.multipathDevice, multipathDevice) + assert.Nil(err) + } + }) + } +} + +func TestConnectorPersistance(t *testing.T) { + assert := assert.New(t) + + secret := Secrets{ + SecretsType: "fake secret type", + UserName: "fake username", + Password: "fake password", + UserNameIn: "fake username in", + PasswordIn: "fake password in", + } + childDevice := Device{ + Name: "child name", + Hctl: "child hctl", + Type: "child type", + Vendor: "child vendor", + Model: "child model", + Revision: "child revision", + Transport: "child transport", + } + device := Device{ + Name: "device name", + Hctl: "device hctl", + Children: []Device{childDevice}, + Type: "device type", + Vendor: "device vendor", + Model: "device model", + Revision: "device revision", + Transport: "device transport", + } + c := Connector{ + VolumeName: "fake volume name", + Targets: []TargetInfo{}, + Lun: 42, + AuthType: "fake auth type", + DiscoverySecrets: secret, + SessionSecrets: secret, + Interface: "fake interface", + MountTargetDevice: &device, + Devices: []Device{device, childDevice}, + RetryCount: 24, + CheckInterval: 13, + DoDiscovery: true, + DoCHAPDiscovery: true, + } + + c.Persist("/tmp/connector.json") + c2, err := GetConnectorFromFile("/tmp/connector.json") + assert.Nil(err) + assert.Equal(c, *c2) + + err = c.Persist("/tmp") + assert.NotNil(err) + + os.Remove("/tmp/shouldNotExists.json") + _, err = GetConnectorFromFile("/tmp/shouldNotExists.json") + assert.NotNil(err) + assert.IsType(&os.PathError{}, err) + + ioutil.WriteFile("/tmp/connector.json", []byte("not a connector"), 0600) + _, err = GetConnectorFromFile("/tmp/connector.json") + assert.NotNil(err) + assert.IsType(&json.SyntaxError{}, err) +} diff --git a/iscsi/iscsiadm.go b/iscsi/iscsiadm.go index f6f096da..a9fd6cf1 100644 --- a/iscsi/iscsiadm.go +++ b/iscsi/iscsiadm.go @@ -20,20 +20,6 @@ type Secrets struct { PasswordIn string `json:"passwordIn,omitempty"` } -// CmdError is a custom error to provide details including the command, stderr output and exit code. -// iscsiadm in some cases requires all of this info to determine success or failure -type CmdError struct { - CMD string - StdErr string - ExitCode int -} - -func (e *CmdError) Error() string { - // we don't output the command in the error string to avoid leaking any security info - // the command is still available in the error structure if the caller wants it though - return fmt.Sprintf("iscsiadm returned an error: %s, exit-code: %d", e.StdErr, e.ExitCode) -} - func iscsiCmd(args ...string) (string, error) { cmd := execCommand("iscsiadm", args...) debug.Printf("Run iscsiadm command: %s", strings.Join(append([]string{"iscsiadm"}, args...), " ")) @@ -45,18 +31,12 @@ func iscsiCmd(args ...string) (string, error) { // we're using Start and Wait because we want to grab exit codes err := cmd.Start() + if err == nil { + err = cmd.Wait() + } if err != nil { - // This is usually a cmd not found so we'll set our own error here formattedOutput := strings.Replace(string(stdout.Bytes()), "\n", "", -1) iscsiadmError = fmt.Errorf("iscsiadm error: %s (%s)", formattedOutput, err.Error()) - - } else { - err = cmd.Wait() - if err != nil { - formattedOutput := strings.Replace(string(stdout.Bytes()), "\n", "", -1) - iscsiadmError = fmt.Errorf("iscsiadm error: %s (%s)", formattedOutput, err.Error()) - - } } iscsiadmDebug(string(stdout.Bytes()), iscsiadmError) diff --git a/iscsi/iscsiadm_test.go b/iscsi/iscsiadm_test.go index 9b924953..a281bf58 100644 --- a/iscsi/iscsiadm_test.go +++ b/iscsi/iscsiadm_test.go @@ -4,8 +4,68 @@ import ( "testing" "github.com/prashantv/gostub" + "github.com/stretchr/testify/assert" ) +const defaultInterface = ` +# BEGIN RECORD 2.0-874 +iface.iscsi_ifacename = default +iface.net_ifacename = +iface.ipaddress = +iface.hwaddress = +iface.transport_name = tcp +iface.initiatorname = +iface.state = +iface.vlan_id = 0 +iface.vlan_priority = 0 +iface.vlan_state = +iface.iface_num = 0 +iface.mtu = 0 +iface.port = 0 +iface.bootproto = +iface.subnet_mask = +iface.gateway = +iface.dhcp_alt_client_id_state = +iface.dhcp_alt_client_id = +iface.dhcp_dns = +iface.dhcp_learn_iqn = +iface.dhcp_req_vendor_id_state = +iface.dhcp_vendor_id_state = +iface.dhcp_vendor_id = +iface.dhcp_slp_da = +iface.fragmentation = +iface.gratuitous_arp = +iface.incoming_forwarding = +iface.tos_state = +iface.tos = 0 +iface.ttl = 0 +iface.delayed_ack = +iface.tcp_nagle = +iface.tcp_wsf_state = +iface.tcp_wsf = 0 +iface.tcp_timer_scale = 0 +iface.tcp_timestamp = +iface.redirect = +iface.def_task_mgmt_timeout = 0 +iface.header_digest = +iface.data_digest = +iface.immediate_data = +iface.initial_r2t = +iface.data_seq_inorder = +iface.data_pdu_inorder = +iface.erl = 0 +iface.max_receive_data_len = 0 +iface.first_burst_len = 0 +iface.max_outstanding_r2t = 0 +iface.max_burst_len = 0 +iface.chap_auth = +iface.bidi_chap = +iface.strict_login_compliance = +iface.discovery_auth = +iface.discovery_logout = +# END RECORD +` + func TestDiscovery(t *testing.T) { tests := map[string]struct { tgtPortal string @@ -126,3 +186,89 @@ func TestCreateDBEntry(t *testing.T) { } } + +func TestListInterfaces(t *testing.T) { + tests := map[string]struct { + mockedStdout string + mockedExitStatus int + interfaces []string + wantErr bool + }{ + "EmptyOutput": { + mockedStdout: "", + mockedExitStatus: 0, + interfaces: []string{""}, + wantErr: false, + }, + "DefaultInterface": { + mockedStdout: "default", + mockedExitStatus: 0, + interfaces: []string{"default"}, + wantErr: false, + }, + "TwoInterface": { + mockedStdout: "default\ntest", + mockedExitStatus: 0, + interfaces: []string{"default", "test"}, + wantErr: false, + }, + "HasError": { + mockedStdout: "", + mockedExitStatus: 1, + interfaces: []string{}, + wantErr: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() + interfaces, err := ListInterfaces() + + if tt.wantErr { + assert.NotNil(err) + } else { + assert.Nil(err) + assert.Equal(interfaces, tt.interfaces) + } + }) + } +} + +func TestShowInterface(t *testing.T) { + tests := map[string]struct { + mockedStdout string + mockedExitStatus int + iFace string + wantErr bool + }{ + "DefaultInterface": { + mockedStdout: defaultInterface, + mockedExitStatus: 0, + iFace: defaultInterface, + wantErr: false, + }, + "HasError": { + mockedStdout: "", + mockedExitStatus: 1, + iFace: "", + wantErr: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() + interfaces, err := ShowInterface("default") + + if tt.wantErr { + assert.NotNil(err) + } else { + assert.Nil(err) + assert.Equal(interfaces, tt.iFace) + } + }) + } +} diff --git a/iscsi/multipath.go b/iscsi/multipath.go index a7a86c9c..347ae33b 100644 --- a/iscsi/multipath.go +++ b/iscsi/multipath.go @@ -2,7 +2,6 @@ package iscsi import ( "context" - "encoding/json" "fmt" "os" "os/exec" @@ -10,17 +9,6 @@ import ( "time" ) -type multipathDeviceMap struct { - Map multipathMap `json:"map"` -} - -type multipathMap struct { - Name string `json:"name"` - UUID string `json:"uuid"` - Sysfs string `json:"sysfs"` - PathGroups []pathGroup `json:"path_groups"` -} - type pathGroup struct { Paths []path `json:"paths"` } @@ -42,6 +30,7 @@ func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]by // This time we can simply use Output() to get the result. out, err := cmd.Output() + debug.Println(err) // 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 @@ -62,37 +51,6 @@ func ExecWithTimeout(command string, args []string, timeout time.Duration) ([]by return out, err } -func getMultipathMap(device string) (*multipathDeviceMap, error) { - debug.Printf("Getting multipath map for device %s.\n", device) - - cmd := execCommand("multipathd", "show", "map", device[1:], "json") - out, err := cmd.CombinedOutput() - // debug.Printf(string(out)) - if err != nil { - debug.Printf("An error occured while looking for multipath device map: %s\n", out) - return nil, err - } - - var deviceMap multipathDeviceMap - err = json.Unmarshal(out, &deviceMap) - if err != nil { - return nil, err - } - return &deviceMap, nil -} - -func (deviceMap *multipathDeviceMap) GetSlaves() []string { - var slaves []string - - for _, pathGroup := range deviceMap.Map.PathGroups { - for _, path := range pathGroup.Paths { - slaves = append(slaves, path.Device) - } - } - - return slaves -} - // FlushMultipathDevice flushes a multipath device dm-x with command multipath -f /dev/dm-x func FlushMultipathDevice(device *Device) error { devicePath := device.GetPath() diff --git a/iscsi/multipath_test.go b/iscsi/multipath_test.go new file mode 100644 index 00000000..abf52498 --- /dev/null +++ b/iscsi/multipath_test.go @@ -0,0 +1,79 @@ +package iscsi + +import ( + "context" + "os/exec" + "testing" + "time" + + "github.com/prashantv/gostub" + "github.com/stretchr/testify/assert" +) + +func TestExecWithTimeout(t *testing.T) { + tests := map[string]struct { + mockedStdout string + mockedExitStatus int + wantTimeout bool + }{ + "Success": { + mockedStdout: "some output", + mockedExitStatus: 0, + wantTimeout: false, + }, + "WithError": { + mockedStdout: "some\noutput", + mockedExitStatus: 1, + wantTimeout: false, + }, + "WithTimeout": { + mockedStdout: "", + mockedExitStatus: 0, + wantTimeout: true, + }, + "WithTimeoutAndOutput": { + mockedStdout: "should not be returned", + mockedExitStatus: 0, + wantTimeout: true, + }, + "WithTimeoutAndError": { + mockedStdout: "", + mockedExitStatus: 1, + wantTimeout: true, + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + + timeout := time.Second + if tt.wantTimeout { + timeout = time.Millisecond * 50 + } + + defer gostub.Stub(&execCommandContext, func(ctx context.Context, command string, args ...string) *exec.Cmd { + if tt.wantTimeout { + time.Sleep(timeout + time.Millisecond*10) + } + return makeFakeExecCommandContext(tt.mockedExitStatus, tt.mockedStdout)(ctx, command, args...) + }).Reset() + + out, err := ExecWithTimeout("dummy", []string{}, timeout) + + if tt.wantTimeout || tt.mockedExitStatus != 0 { + assert.NotNil(err) + if tt.wantTimeout { + assert.Equal(context.DeadlineExceeded, err) + } + } else { + assert.Nil(err) + } + + if tt.wantTimeout { + assert.Equal("", string(out)) + } else { + assert.Equal(tt.mockedStdout, string(out)) + } + }) + } +} From d1abc07f085603b06b1d5b6ea1b9369a883d4995 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Fri, 29 Oct 2021 12:11:34 +0200 Subject: [PATCH 11/19] refactor lsblk - include error code - remove unused properties - retrieve specific columns including device size - always output as JSON --- iscsi/iscsi.go | 64 +++++++++++++++++++--------------------- iscsi/iscsi_test.go | 72 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 96 insertions(+), 40 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index c688a294..8b095c5d 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -56,10 +56,8 @@ type Device struct { Hctl string `json:"hctl"` Children []Device `json:"children"` Type string `json:"type"` - Vendor string `json:"vendor"` - Model string `json:"model"` - Revision string `json:"rev"` Transport string `json:"tran"` + Size string `json:"size,omitempty"` } // Connector provides a struct to hold all of the needed parameters to make our iSCSI connection @@ -216,17 +214,8 @@ func pathExists(devicePath *string, deviceTransport string) error { // getMultipathDevice returns a multipath device for the configured targets if it exists func getMultipathDevice(devices []Device) (*Device, error) { var multipathDevice *Device - var devicePaths []string for _, device := range devices { - devicePaths = append(devicePaths, device.GetPath()) - } - deviceInfo, err := lsblk("", devicePaths) - if err != nil { - return nil, err - } - - for _, device := range deviceInfo.BlockDevices { if len(device.Children) != 1 { return nil, fmt.Errorf("device is not mapped to exactly one multipath device: %v", device.Children) } @@ -454,7 +443,7 @@ func (c *Connector) IsMultipathEnabled() bool { func GetSCSIDevices(devicePaths []string) ([]Device, error) { debug.Printf("Getting info about SCSI devices %s.\n", devicePaths) - deviceInfo, err := lsblk("-S", devicePaths) + deviceInfo, err := lsblk(devicePaths) if err != nil { debug.Printf("An error occured while looking info about SCSI devices: %v", err) return nil, err @@ -482,24 +471,19 @@ func GetISCSIDevices(devicePaths []string) (devices []Device, err error) { } // lsblk execute the lsblk commands -func lsblkRaw(flags string, devicePaths []string) ([]byte, error) { - out, err := execCommand("lsblk", append([]string{flags}, devicePaths...)...).CombinedOutput() - debug.Printf("lsblk %s %s", flags, strings.Join(devicePaths, " ")) - if err != nil { - return nil, fmt.Errorf("lsblk: %s", string(out)) - } - - return out, nil -} - -func lsblk(flags string, devicePaths []string) (*deviceInfo, error) { - var deviceInfo deviceInfo - - out, err := lsblkRaw("-J "+flags, devicePaths) +func lsblk(devicePaths []string) (*deviceInfo, error) { + flags := []string{"-J", "-o", "NAME,HCTL,TYPE,TRAN,SIZE"} + command := execCommand("lsblk", append(flags, devicePaths...)...) + debug.Println(command.String()) + out, err := command.Output() if err != nil { + if ee, ok := err.(*exec.ExitError); ok { + return nil, fmt.Errorf("%s, (%v)", strings.Trim(string(ee.Stderr), "\n"), ee) + } return nil, err } + var deviceInfo deviceInfo if err = json.Unmarshal(out, &deviceInfo); err != nil { return nil, err } @@ -589,16 +573,30 @@ func (c *Connector) Persist(filePath string) error { func GetConnectorFromFile(filePath string) (*Connector, error) { f, err := ioutil.ReadFile(filePath) if err != nil { - return &Connector{}, err - + return nil, err } - data := Connector{} - err = json.Unmarshal(f, &data) + c := Connector{} + err = json.Unmarshal([]byte(f), &c) if err != nil { - return &Connector{}, err + return nil, err + } + + devicePaths := []string{} + for _, device := range c.Devices { + devicePaths = append(devicePaths, device.GetPath()) + } + + if devices, err := GetSCSIDevices([]string{c.MountTargetDevice.GetPath()}); err != nil { + return nil, err + } else { + c.MountTargetDevice = &devices[0] + } + + if c.Devices, err = GetSCSIDevices(devicePaths); err != nil { + return nil, err } - return &data, nil + return &c, nil } // Exists check if the device exists at its path and returns an error otherwise diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index be117d59..a1225ac1 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -586,6 +586,58 @@ func Test_getMultipathDevice(t *testing.T) { } } +func Test_lsblk(t *testing.T) { + sda := Device{Name: "sda", Children: []Device{{}}} + + tests := map[string]struct { + devicePaths []string + mockedStdout string + mockedDevices []Device + mockedExitStatus int + wantErr bool + }{ + "Basic": { + devicePaths: []string{"/dev/sda"}, + mockedDevices: []Device{sda}, + }, + "NotABlockDevice": { + devicePaths: []string{"/dev/sdzz"}, + mockedStdout: "lsblk: sdzz: not a block device", + mockedExitStatus: 32, + }, + "InvalidJson": { + mockedStdout: "{", + mockedExitStatus: 0, + wantErr: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + + mockedStdout := tt.mockedStdout + if mockedStdout == "" { + out, err := json.Marshal(deviceInfo{BlockDevices: tt.mockedDevices}) + assert.Nil(err, "could not setup test") + mockedStdout = string(out) + } + + defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, mockedStdout)).Reset() + deviceInfo, err := lsblk(tt.devicePaths) + + if tt.mockedExitStatus != 0 || tt.wantErr { + assert.Nil(deviceInfo) + assert.NotNil(err) + } else { + assert.NotNil(deviceInfo) + assert.Equal(tt.mockedDevices, deviceInfo.BlockDevices) + assert.Nil(err) + } + }) + } +} + func TestConnectorPersistance(t *testing.T) { assert := assert.New(t) @@ -600,9 +652,6 @@ func TestConnectorPersistance(t *testing.T) { Name: "child name", Hctl: "child hctl", Type: "child type", - Vendor: "child vendor", - Model: "child model", - Revision: "child revision", Transport: "child transport", } device := Device{ @@ -610,9 +659,6 @@ func TestConnectorPersistance(t *testing.T) { Hctl: "device hctl", Children: []Device{childDevice}, Type: "device type", - Vendor: "device vendor", - Model: "device model", - Revision: "device revision", Transport: "device transport", } c := Connector{ @@ -624,13 +670,25 @@ func TestConnectorPersistance(t *testing.T) { SessionSecrets: secret, Interface: "fake interface", MountTargetDevice: &device, - Devices: []Device{device, childDevice}, + Devices: []Device{childDevice}, RetryCount: 24, CheckInterval: 13, DoDiscovery: true, DoCHAPDiscovery: true, } + defer gostub.Stub(&execCommand, func(cmd string, args ...string) *exec.Cmd { + mockedDevice := device + if args[3] == "/dev/child name" { + mockedDevice = childDevice + } + + mockedOutput, err := json.Marshal(deviceInfo{BlockDevices: []Device{mockedDevice}}) + assert.Nil(err, "could not setup test") + + return makeFakeExecCommand(0, string(mockedOutput))(cmd, args...) + }).Reset() + c.Persist("/tmp/connector.json") c2, err := GetConnectorFromFile("/tmp/connector.json") assert.Nil(err) From ae559b895bf57b5978423d69bb51e2453337fe3b Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Fri, 29 Oct 2021 12:54:26 +0200 Subject: [PATCH 12/19] check multipath consistency on connection/disconnection --- go.sum | 1 + iscsi/iscsi.go | 99 ++++++++++++++++++++++ iscsi/iscsi_test.go | 195 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 257 insertions(+), 38 deletions(-) diff --git a/go.sum b/go.sum index 5bb4efb7..f0297e0e 100644 --- a/go.sum +++ b/go.sum @@ -7,6 +7,7 @@ github.com/prashantv/gostub v1.0.0/go.mod h1:dP1v6T1QzyGJJKFocwAU0lSZKpfjstjH8Tl github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 8b095c5d..5fd873c1 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -60,6 +60,13 @@ type Device struct { Size string `json:"size,omitempty"` } +type HCTL struct { + HBA int + Channel int + Target int + LUN int +} + // Connector provides a struct to hold all of the needed parameters to make our iSCSI connection type Connector struct { VolumeName string `json:"volume_name"` @@ -291,6 +298,12 @@ func (c *Connector) Connect() (string, error) { return "", err } + if c.IsMultipathEnabled() { + if err := c.IsMultipathConsistent(); err != nil { + return "", fmt.Errorf("multipath is inconsistent: %v", err) + } + } + return c.MountTargetDevice.GetPath(), nil } @@ -394,6 +407,10 @@ func (c *Connector) DisconnectVolume() error { // Note: make sure the volume is already unmounted before calling this method. if c.IsMultipathEnabled() { + if err := c.IsMultipathConsistent(); err != nil { + return fmt.Errorf("multipath is inconsistent: %v", err) + } + debug.Printf("Removing multipath device in path %s.\n", c.MountTargetDevice.GetPath()) err := FlushMultipathDevice(c.MountTargetDevice) if err != nil { @@ -599,6 +616,52 @@ func GetConnectorFromFile(filePath string) (*Connector, error) { return &c, nil } +// IsMultipathConsistent check if the currently used device is using a consistent multipath mapping +func (c *Connector) IsMultipathConsistent() error { + devices := append([]Device{*c.MountTargetDevice}, c.Devices...) + + referenceLUN := struct { + LUN int + Name string + }{LUN: -1, Name: ""} + HBA := map[int]string{} + referenceDevice := devices[0] + for _, device := range devices { + if device.Size != referenceDevice.Size { + return fmt.Errorf("devices size differ: %s (%s) != %s (%s)", device.Name, device.Size, referenceDevice.Name, referenceDevice.Size) + } + + if device.Type != "mpath" { + hctl, err := device.HCTL() + if err != nil { + return err + } + if referenceLUN.LUN == -1 { + referenceLUN.LUN = hctl.LUN + referenceLUN.Name = device.Name + } else if hctl.LUN != referenceLUN.LUN { + return fmt.Errorf("devices LUNs differ: %s (%d) != %s (%d)", device.Name, hctl.LUN, referenceLUN.Name, referenceLUN.LUN) + } + + if name, ok := HBA[hctl.HBA]; !ok { + HBA[hctl.HBA] = device.Name + } else { + return fmt.Errorf("two devices are using the same controller (%d): %s and %s", hctl.HBA, device.Name, name) + } + } + + wwid, err := device.WWID() + if err != nil { + return fmt.Errorf("could not find WWID for device %s: %v", device.Name, err) + } + if wwid != referenceDevice.Name { + return fmt.Errorf("devices WWIDs differ: %s (wwid:%s) != %s (wwid:%s)", device.Name, wwid, referenceDevice.Name, referenceDevice.Name) + } + } + + return nil +} + // Exists check if the device exists at its path and returns an error otherwise func (d *Device) Exists() error { _, err := osStat(d.GetPath()) @@ -614,6 +677,42 @@ func (d *Device) GetPath() string { return filepath.Join("/dev", d.Name) } +// WWID returns the WWID of a device +func (d *Device) WWID() (string, error) { + timeout := 1 * time.Second + out, err := execWithTimeout("scsi_id", []string{"-g", "-u", d.GetPath()}, timeout) + if err != nil { + return "", err + } + + return string(out[:len(out)-1]), nil +} + +// HCTL returns the HCTL of a device +func (d *Device) HCTL() (*HCTL, error) { + var hctl []int + + for _, idstr := range strings.Split(d.Hctl, ":") { + id, err := strconv.Atoi(idstr) + if err != nil { + hctl = []int{} + break + } + hctl = append(hctl, id) + } + + if len(hctl) != 4 { + return nil, fmt.Errorf("invalid HCTL (%s) for device %q", d.Hctl, d.Name) + } + + return &HCTL{ + HBA: hctl[0], + Channel: hctl[1], + Target: hctl[2], + LUN: hctl[3], + }, nil +} + // WriteDeviceFile write in a device file func (d *Device) WriteDeviceFile(name string, content string) error { return writeInSCSIDeviceFile(d.Hctl, name, content) diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index a1225ac1..4498360f 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -3,6 +3,7 @@ package iscsi import ( "context" "encoding/json" + "errors" "fmt" "io/ioutil" "os" @@ -117,12 +118,12 @@ func makeFakeExecCommandContext(exitStatus int, stdout string) func(context.Cont } } -func makeFakeExecWithTimeout(testCmdTimeout bool, testExecWithTimeoutError error) func(string, []string, time.Duration) ([]byte, error) { +func makeFakeExecWithTimeout(withTimeout bool, output []byte, err error) func(string, []string, time.Duration) ([]byte, error) { return func(command string, args []string, timeout time.Duration) ([]byte, error) { - if testCmdTimeout { + if withTimeout { return nil, context.DeadlineExceeded } - return []byte(""), testExecWithTimeoutError + return output, err } } @@ -332,7 +333,6 @@ func Test_DisconnectNormalVolume(t *testing.T) { } func Test_DisconnectMultipathVolume(t *testing.T) { - defer gostub.Stub(&execCommand, makeFakeExecCommand(0, "")).Reset() defer gostub.Stub(&osStat, func(name string) (os.FileInfo, error) { return nil, nil }).Reset() @@ -342,25 +342,34 @@ func Test_DisconnectMultipathVolume(t *testing.T) { timeout bool withDeviceFile bool wantErr bool - cmdError error }{ - {"DisconnectMultipathVolume", false, true, false, nil}, - {"DisconnectMultipathVolumeFlushTimeout", true, true, true, nil}, - {"DisconnectNonexistentMultipathVolume", false, false, false, nil}, + {"DisconnectMultipathVolume", false, true, false}, + {"DisconnectMultipathVolumeFlushTimeout", true, true, true}, + {"DisconnectNonexistentMultipathVolume", false, false, false}, } + wwid := "3600c0ff0000000000000000000000000" + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - defer gostub.Stub(&execWithTimeout, makeFakeExecWithTimeout(tt.timeout, tt.cmdError)).Reset() + defer gostub.Stub(&execWithTimeout, func(cmd string, args []string, timeout time.Duration) ([]byte, error) { + mockedOutput := []byte("") + if cmd == "scsi_id" { + mockedOutput = []byte(wwid + "\n") + } + return makeFakeExecWithTimeout(tt.timeout, mockedOutput, nil)(cmd, args, timeout) + }).Reset() c := Connector{ - Devices: []Device{{Hctl: "hctl1"}, {Hctl: "hctl2"}}, - MountTargetDevice: &Device{Type: "mpath"}, + Devices: []Device{{Hctl: "0:0:0:0"}, {Hctl: "1:0:0:0"}}, + MountTargetDevice: &Device{Name: wwid, Type: "mpath"}, } defer gostub.Stub(&osOpenFile, func(name string, flag int, perm os.FileMode) (*os.File, error) { return os.OpenFile(testRootFS+name, flag, perm) }).Reset() + defer gostub.Stub(&execCommand, makeFakeExecCommand(0, wwid)).Reset() + if tt.withDeviceFile { if err := preparePaths(c.Devices); err != nil { t.Errorf("could not prepare paths: %v", err) @@ -377,10 +386,7 @@ func Test_DisconnectMultipathVolume(t *testing.T) { } if tt.timeout { - if err != context.DeadlineExceeded { - t.Errorf("DisconnectVolume() error = %v, wantErr %v", err, context.DeadlineExceeded) - return - } + assert.New(t).Contains(err.Error(), "context deadline exceeded") } if tt.withDeviceFile && !tt.wantErr { @@ -534,30 +540,24 @@ func Test_getMultipathDevice(t *testing.T) { sde := Device{Name: "sdc", Children: []Device{mpath1, mpath2}} tests := map[string]struct { - mockedDevices deviceInfo - mockedStdout string - mockedExitStatus int - multipathDevice *Device - wantErr bool + mockedDevices []Device + multipathDevice *Device + wantErr bool }{ "Basic": { - mockedDevices: deviceInfo{BlockDevices: []Device{sdb, sdc}}, + mockedDevices: []Device{sdb, sdc}, multipathDevice: &mpath1, }, - "NotABlockDevice": { - mockedStdout: "lsblk: sdzz: not a block device", - mockedExitStatus: 32, - }, "NotSharingTheSameMultipathDevice": { - mockedDevices: deviceInfo{BlockDevices: []Device{sdb, sdd}}, + mockedDevices: []Device{sdb, sdd}, wantErr: true, }, "MoreThanOneMultipathDevice": { - mockedDevices: deviceInfo{BlockDevices: []Device{sde}}, + mockedDevices: []Device{sde}, wantErr: true, }, "NotAMultipathDevice": { - mockedDevices: deviceInfo{BlockDevices: []Device{sda}}, + mockedDevices: []Device{sda}, wantErr: true, }, } @@ -565,17 +565,9 @@ func Test_getMultipathDevice(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) - mockedStdout := tt.mockedStdout - if mockedStdout == "" { - out, err := json.Marshal(tt.mockedDevices) - assert.Nil(err, "could not setup test") - mockedStdout = string(out) - } - - gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, string(mockedStdout))) - multipathDevice, err := getMultipathDevice(tt.mockedDevices.BlockDevices) + multipathDevice, err := getMultipathDevice(tt.mockedDevices) - if tt.mockedExitStatus != 0 || tt.wantErr { + if tt.wantErr { assert.Nil(multipathDevice) assert.NotNil(err) } else { @@ -676,6 +668,20 @@ func TestConnectorPersistance(t *testing.T) { DoDiscovery: true, DoCHAPDiscovery: true, } + devicesByPath := map[string]*Device{} + devicesByPath[childDevice.GetPath()] = &childDevice + devicesByPath[device.GetPath()] = &device + + defer gostub.Stub(&execCommand, func(name string, arg ...string) *exec.Cmd { + blockDevices := []Device{} + for _, path := range arg[3:] { + blockDevices = append(blockDevices, *devicesByPath[path]) + } + + out, err := json.Marshal(deviceInfo{BlockDevices: blockDevices}) + assert.Nil(err, "could not setup test") + return makeFakeExecCommand(0, string(out))(name, arg...) + }).Reset() defer gostub.Stub(&execCommand, func(cmd string, args ...string) *exec.Cmd { mockedDevice := device @@ -707,3 +713,116 @@ func TestConnectorPersistance(t *testing.T) { assert.NotNil(err) assert.IsType(&json.SyntaxError{}, err) } + +func Test_IsMultipathConsistent(t *testing.T) { + mpath1 := Device{Name: "3600c0ff0000000000000000000000000", Type: "mpath", Size: "10G", Hctl: "0:0:0:1"} + mpath2 := Device{Name: "3600c0ff0000000000000000000000042", Type: "mpath", Size: "5G", Hctl: "0:0:0:2"} + sda := Device{Name: "sda", Size: "10G", Hctl: "1:0:0:1"} + sdb := Device{Name: "sdb", Size: "10G", Hctl: "2:0:0:1"} + sdc := Device{Name: "sdc", Size: "5G", Hctl: "1:0:0:2"} + sdd := Device{Name: "sdd", Size: "5G", Hctl: "2:0:0:2"} + invalidHCTL := Device{Name: "sde", Size: "5G", Hctl: "2:b"} + sdf := Device{Name: "sdf", Size: "10G", Hctl: "2:0:0:3"} + sdg := Device{Name: "sdg", Size: "10G", Hctl: "1:0:0:1"} + devicesWWIDs := map[string]string{} + devicesWWIDs[mpath1.GetPath()] = "3600c0ff0000000000000000000000000" + devicesWWIDs[sda.GetPath()] = "3600c0ff0000000000000000000000000" + devicesWWIDs[sdb.GetPath()] = "3600c0ff0000000000000000000000000" + devicesWWIDs[sdg.GetPath()] = "3600c0ff0000000000000000000000024" + + tests := map[string]struct { + connector *Connector + wantErr bool + errContains string + }{ + "Basic": { + connector: &Connector{ + MountTargetDevice: &mpath1, + Devices: []Device{sda, sdb}, + }, + }, + "Different sizes 1": { + connector: &Connector{ + MountTargetDevice: &mpath1, + Devices: []Device{sda, sdc}, + }, + wantErr: true, + errContains: "size differ", + }, + "Different sizes 2": { + connector: &Connector{ + MountTargetDevice: &mpath1, + Devices: []Device{sdc, sdd}, + }, + wantErr: true, + errContains: "size differ", + }, + "Invalid HCTL": { + connector: &Connector{ + MountTargetDevice: &invalidHCTL, + Devices: []Device{}, + }, + wantErr: true, + errContains: "invalid HCTL", + }, + "LUNs differs": { + connector: &Connector{ + MountTargetDevice: &mpath1, + Devices: []Device{sda, sdf}, + }, + wantErr: true, + errContains: "LUNs differ", + }, + "Same controller": { + connector: &Connector{ + MountTargetDevice: &mpath1, + Devices: []Device{sda, sdg}, + }, + wantErr: true, + errContains: "same controller", + }, + "Missing WWID": { + connector: &Connector{ + MountTargetDevice: &mpath2, + Devices: []Device{sdc, sdd}, + }, + wantErr: true, + errContains: "could not find WWID", + }, + "WWIDs differ": { + connector: &Connector{ + MountTargetDevice: &mpath1, + Devices: []Device{sdb, sdg}, + }, + wantErr: true, + errContains: "WWIDs differ", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + c := tt.connector + + defer gostub.Stub(&execWithTimeout, func(_ string, args []string, _ time.Duration) ([]byte, error) { + devicePath := args[len(args)-1] + wwid, ok := devicesWWIDs[devicePath] + if !ok { + return []byte(""), errors.New("") + } + return []byte(wwid + "\n"), nil + }).Reset() + + err := c.IsMultipathConsistent() + + if tt.wantErr { + assert.Error(err) + if tt.errContains != "" { + assert.Contains(err.Error(), tt.errContains) + } + } else { + assert.Nil(err) + } + }) + } +} From 203e987cb4888514d7c9898188f14c5f560aedb3 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Wed, 9 Jun 2021 00:25:43 +0200 Subject: [PATCH 13/19] fix typo in logs --- iscsi/iscsi.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 5fd873c1..8c7b9db4 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -567,7 +567,7 @@ func RemoveSCSIDevices(devices ...Device) error { if len(errs) > 0 { return errs[0] } - debug.Println("Finshed removing SCSI devices.") + debug.Println("Finished removing SCSI devices.") return nil } From fb3c9e1cacd548725eb46447195cec236e8b5a12 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Wed, 9 Jun 2021 11:41:05 +0200 Subject: [PATCH 14/19] remove devices even if some are missing --- iscsi/iscsi.go | 29 +++++++++++++++++------------ iscsi/iscsi_test.go | 33 +++++++++++++++++++++++---------- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 8c7b9db4..34a35e9b 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -279,7 +279,7 @@ func (c *Connector) Connect() (string, error) { // GetISCSIDevices returns all devices if no paths are given if len(devicePaths) < 1 { c.Devices = []Device{} - } else if c.Devices, err = GetISCSIDevices(devicePaths); err != nil { + } else if c.Devices, err = GetISCSIDevices(devicePaths, true); err != nil { return "", err } @@ -457,10 +457,10 @@ func (c *Connector) IsMultipathEnabled() bool { // GetSCSIDevices get SCSI devices from device paths // It will returns all SCSI devices if no paths are given -func GetSCSIDevices(devicePaths []string) ([]Device, error) { +func GetSCSIDevices(devicePaths []string, strict bool) ([]Device, error) { debug.Printf("Getting info about SCSI devices %s.\n", devicePaths) - deviceInfo, err := lsblk(devicePaths) + deviceInfo, err := lsblk(devicePaths, strict) if err != nil { debug.Printf("An error occured while looking info about SCSI devices: %v", err) return nil, err @@ -471,8 +471,8 @@ func GetSCSIDevices(devicePaths []string) ([]Device, error) { // GetISCSIDevices get iSCSI devices from device paths // It will returns all iSCSI devices if no paths are given -func GetISCSIDevices(devicePaths []string) (devices []Device, err error) { - scsiDevices, err := GetSCSIDevices(devicePaths) +func GetISCSIDevices(devicePaths []string, strict bool) (devices []Device, err error) { + scsiDevices, err := GetSCSIDevices(devicePaths, strict) if err != nil { return } @@ -488,21 +488,26 @@ func GetISCSIDevices(devicePaths []string) (devices []Device, err error) { } // lsblk execute the lsblk commands -func lsblk(devicePaths []string) (*deviceInfo, error) { +func lsblk(devicePaths []string, strict bool) (*deviceInfo, error) { flags := []string{"-J", "-o", "NAME,HCTL,TYPE,TRAN,SIZE"} command := execCommand("lsblk", append(flags, devicePaths...)...) debug.Println(command.String()) out, err := command.Output() if err != nil { if ee, ok := err.(*exec.ExitError); ok { - return nil, fmt.Errorf("%s, (%v)", strings.Trim(string(ee.Stderr), "\n"), ee) + err = fmt.Errorf("%s, (%w)", strings.Trim(string(ee.Stderr), "\n"), ee) + if strict || ee.ExitCode() != 64 { // ignore the error if some devices have been found when not strict + return nil, err + } + debug.Printf("lsblk could find only some devices: %v", err) + } else { + return nil, err } - return nil, err } var deviceInfo deviceInfo - if err = json.Unmarshal(out, &deviceInfo); err != nil { - return nil, err + if jsonErr := json.Unmarshal(out, &deviceInfo); jsonErr != nil { + return nil, jsonErr } return &deviceInfo, nil @@ -603,13 +608,13 @@ func GetConnectorFromFile(filePath string) (*Connector, error) { devicePaths = append(devicePaths, device.GetPath()) } - if devices, err := GetSCSIDevices([]string{c.MountTargetDevice.GetPath()}); err != nil { + if devices, err := GetSCSIDevices([]string{c.MountTargetDevice.GetPath()}, false); err != nil { return nil, err } else { c.MountTargetDevice = &devices[0] } - if c.Devices, err = GetSCSIDevices(devicePaths); err != nil { + if c.Devices, err = GetSCSIDevices(devicePaths, false); err != nil { return nil, err } diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index 4498360f..20f8f4b0 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -581,8 +581,12 @@ func Test_getMultipathDevice(t *testing.T) { func Test_lsblk(t *testing.T) { sda := Device{Name: "sda", Children: []Device{{}}} + sdaOutput, err := json.Marshal(deviceInfo{BlockDevices: []Device{sda}}) + assert.New(t).Nil(err, "could not setup test") + tests := map[string]struct { devicePaths []string + strict bool mockedStdout string mockedDevices []Device mockedExitStatus int @@ -591,34 +595,43 @@ func Test_lsblk(t *testing.T) { "Basic": { devicePaths: []string{"/dev/sda"}, mockedDevices: []Device{sda}, + mockedStdout: string(sdaOutput), }, "NotABlockDevice": { devicePaths: []string{"/dev/sdzz"}, mockedStdout: "lsblk: sdzz: not a block device", mockedExitStatus: 32, + wantErr: true, }, "InvalidJson": { mockedStdout: "{", mockedExitStatus: 0, wantErr: true, }, + "StrictWithMissingDevices": { + devicePaths: []string{"/dev/sda", "/dev/sdb"}, + strict: true, + mockedDevices: []Device{sda}, + mockedStdout: string(sdaOutput), + mockedExitStatus: 64, + wantErr: true, + }, + "NotStrictWithMissingDevices": { + devicePaths: []string{"/dev/sda", "/dev/sdb"}, + mockedDevices: []Device{sda}, + mockedStdout: string(sdaOutput), + mockedExitStatus: 64, + }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) - mockedStdout := tt.mockedStdout - if mockedStdout == "" { - out, err := json.Marshal(deviceInfo{BlockDevices: tt.mockedDevices}) - assert.Nil(err, "could not setup test") - mockedStdout = string(out) - } - - defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, mockedStdout)).Reset() - deviceInfo, err := lsblk(tt.devicePaths) + defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() + deviceInfo, err := lsblk(tt.devicePaths, tt.strict) - if tt.mockedExitStatus != 0 || tt.wantErr { + if tt.wantErr { assert.Nil(deviceInfo) assert.NotNil(err) } else { From 52c573c23c0bda08e967a2e69580cf8e8659ac5a Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Wed, 9 Jun 2021 14:31:02 +0200 Subject: [PATCH 15/19] fix multipath detection on device lost --- iscsi/iscsi.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 34a35e9b..1849ae1e 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -433,7 +433,7 @@ func (c *Connector) DisconnectVolume() error { // getMountTargetDevice returns the device to be mounted among the configured devices func (c *Connector) getMountTargetDevice() (*Device, error) { - if c.IsMultipathEnabled() { + if len(c.Devices) > 1 { multipathDevice, err := getMultipathDevice(c.Devices) if err != nil { debug.Printf("mount target is not a multipath device: %v", err) @@ -452,7 +452,7 @@ func (c *Connector) getMountTargetDevice() (*Device, error) { // IsMultipathEnabled check if multipath is enabled on devices handled by this connector func (c *Connector) IsMultipathEnabled() bool { - return len(c.Devices) > 1 + return c.MountTargetDevice.Type == "mpath" } // GetSCSIDevices get SCSI devices from device paths @@ -499,7 +499,7 @@ func lsblk(devicePaths []string, strict bool) (*deviceInfo, error) { if strict || ee.ExitCode() != 64 { // ignore the error if some devices have been found when not strict return nil, err } - debug.Printf("lsblk could find only some devices: %v", err) + debug.Printf("Could find only some devices: %v", err) } else { return nil, err } From aac01d4424a31ffb171fbe2ee062542219dd342c Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Fri, 11 Jun 2021 14:53:48 +0200 Subject: [PATCH 16/19] add hint about multipathd not running in logs --- iscsi/iscsi.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index 1849ae1e..c9f11a7b 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -224,7 +224,11 @@ func getMultipathDevice(devices []Device) (*Device, error) { for _, device := range devices { if len(device.Children) != 1 { - return nil, fmt.Errorf("device is not mapped to exactly one multipath device: %v", device.Children) + multipathdNotRunning := "" + if len(device.Children) == 0 { + multipathdNotRunning = " (is multipathd running?)" + } + return nil, fmt.Errorf("device is not mapped to exactly one multipath device%s: %v", multipathdNotRunning, device.Children) } if multipathDevice != nil && device.Children[0].Name != multipathDevice.Name { return nil, fmt.Errorf("devices don't share a common multipath device: %v", devices) From d19252680054dc4a929c0add854e572bfeedb842 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Wed, 29 Sep 2021 11:27:16 +0200 Subject: [PATCH 17/19] add a timeout on iscsciadm commands --- iscsi/iscsi.go | 8 +++ iscsi/iscsi_test.go | 2 +- iscsi/iscsiadm.go | 24 ++----- iscsi/iscsiadm_test.go | 141 +++++++++++++++++++++-------------------- 4 files changed, 85 insertions(+), 90 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index c9f11a7b..d3cd12c2 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -318,6 +318,14 @@ func (c *Connector) connectTarget(target *TargetInfo, iFace string, iscsiTranspo // to avoid establishing additional sessions to the same target. if _, err := iscsiCmd(append(baseArgs, []string{"-R"}...)...); err != nil { debug.Printf("Failed to rescan session, err: %v", err) + if os.IsTimeout(err) { + debug.Printf("iscsiadm timeouted, logging out") + cmd := execCommand("iscsiadm", append(baseArgs, []string{"-u"}...)...) + out, err := cmd.CombinedOutput() + if err != nil { + return "", fmt.Errorf("could not logout from target: %s", out) + } + } } // create our devicePath that we'll be looking for based on the transport being used diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index 20f8f4b0..aaaf1355 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -248,7 +248,7 @@ func Test_extractTransportName(t *testing.T) { func Test_sessionExists(t *testing.T) { fakeOutput := "tcp: [4] 192.168.1.107:3260,1 iqn.2010-10.org.openstack:volume-eb393993-73d0-4e39-9ef4-b5841e244ced (non-flash)\n" - defer gostub.Stub(&execCommand, makeFakeExecCommand(0, fakeOutput)).Reset() + defer gostub.Stub(&execWithTimeout, makeFakeExecWithTimeout(false, []byte(fakeOutput), nil)).Reset() type args struct { tgtPortal string diff --git a/iscsi/iscsiadm.go b/iscsi/iscsiadm.go index a9fd6cf1..b87b5c66 100644 --- a/iscsi/iscsiadm.go +++ b/iscsi/iscsiadm.go @@ -1,9 +1,9 @@ package iscsi import ( - "bytes" "fmt" "strings" + "time" ) // Secrets provides optional iscsi security credentials (CHAP settings) @@ -21,26 +21,12 @@ type Secrets struct { } func iscsiCmd(args ...string) (string, error) { - cmd := execCommand("iscsiadm", args...) + stdout, err := execWithTimeout("iscsiadm", args, time.Second*3) + debug.Printf("Run iscsiadm command: %s", strings.Join(append([]string{"iscsiadm"}, args...), " ")) - var stdout bytes.Buffer - var iscsiadmError error - cmd.Stdout = &stdout - cmd.Stderr = &stdout - defer stdout.Reset() - - // we're using Start and Wait because we want to grab exit codes - err := cmd.Start() - if err == nil { - err = cmd.Wait() - } - if err != nil { - formattedOutput := strings.Replace(string(stdout.Bytes()), "\n", "", -1) - iscsiadmError = fmt.Errorf("iscsiadm error: %s (%s)", formattedOutput, err.Error()) - } + iscsiadmDebug(string(stdout), err) - iscsiadmDebug(string(stdout.Bytes()), iscsiadmError) - return string(stdout.Bytes()), iscsiadmError + return string(stdout), err } func iscsiadmDebug(output string, cmdError error) { diff --git a/iscsi/iscsiadm_test.go b/iscsi/iscsiadm_test.go index a281bf58..1d84d800 100644 --- a/iscsi/iscsiadm_test.go +++ b/iscsi/iscsiadm_test.go @@ -1,6 +1,7 @@ package iscsi import ( + "os/exec" "testing" "github.com/prashantv/gostub" @@ -68,20 +69,20 @@ iface.discovery_logout = func TestDiscovery(t *testing.T) { tests := map[string]struct { - tgtPortal string - iface string - discoverySecret Secrets - chapDiscovery bool - wantErr bool - mockedStdout string - mockedExitStatus int + tgtPortal string + iface string + discoverySecret Secrets + chapDiscovery bool + wantErr bool + mockedStdout string + mockedCmdError error }{ "DiscoverySuccess": { - tgtPortal: "172.18.0.2:3260", - iface: "default", - chapDiscovery: false, - mockedStdout: "172.18.0.2:3260,1 iqn.2016-09.com.openebs.jiva:store1\n", - mockedExitStatus: 0, + tgtPortal: "172.18.0.2:3260", + iface: "default", + chapDiscovery: false, + mockedStdout: "172.18.0.2:3260,1 iqn.2016-09.com.openebs.jiva:store1\n", + mockedCmdError: nil, }, "ConnectionFailure": { @@ -92,8 +93,8 @@ func TestDiscovery(t *testing.T) { iscsiadm: cannot make connection to 172.18.0.2: Connection refused iscsiadm: connection login retries (reopen_max) 5 exceeded iscsiadm: Could not perform SendTargets discovery: encountered connection failure\n`, - mockedExitStatus: 4, - wantErr: true, + mockedCmdError: exec.Command("exit", "4").Run(), + wantErr: true, }, "ChapEntrySuccess": { @@ -104,8 +105,8 @@ iscsiadm: Could not perform SendTargets discovery: encountered connection failur UserNameIn: "dummyuser", PasswordIn: "dummypass", }, - mockedStdout: "172.18.0.2:3260,1 iqn.2016-09.com.openebs.jiva:store1\n", - mockedExitStatus: 0, + mockedStdout: "172.18.0.2:3260,1 iqn.2016-09.com.openebs.jiva:store1\n", + mockedCmdError: nil, }, "ChapEntryFailure": { @@ -119,14 +120,14 @@ iscsiadm: Could not perform SendTargets discovery: encountered connection failur mockedStdout: `iscsiadm: Login failed to authenticate with target iscsiadm: discovery login to 172.18.0.2 rejected: initiator error (02/01), non-retryable, giving up iscsiadm: Could not perform SendTargets discovery.\n`, - mockedExitStatus: 4, - wantErr: true, + mockedCmdError: exec.Command("exit", "4").Run(), + wantErr: true, }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() + defer gostub.Stub(&execWithTimeout, makeFakeExecWithTimeout(false, []byte(tt.mockedStdout), tt.mockedCmdError)).Reset() err := Discoverydb(tt.tgtPortal, tt.iface, tt.discoverySecret, tt.chapDiscovery) if (err != nil) != tt.wantErr { t.Errorf("Discoverydb() error = %v, wantErr %v", err, tt.wantErr) @@ -138,14 +139,14 @@ iscsiadm: Could not perform SendTargets discovery.\n`, func TestCreateDBEntry(t *testing.T) { tests := map[string]struct { - tgtPortal string - tgtIQN string - iface string - discoverySecret Secrets - sessionSecret Secrets - wantErr bool - mockedStdout string - mockedExitStatus int + tgtPortal string + tgtIQN string + iface string + discoverySecret Secrets + sessionSecret Secrets + wantErr bool + mockedStdout string + mockedCmdError error }{ "CreateDBEntryWithChapDiscoverySuccess": { tgtPortal: "192.168.1.107:3260", @@ -161,22 +162,22 @@ func TestCreateDBEntry(t *testing.T) { PasswordIn: "dummypass", SecretsType: "chap", }, - mockedStdout: nodeDB, - mockedExitStatus: 0, + mockedStdout: nodeDB, + mockedCmdError: nil, }, "CreateDBEntryWithChapDiscoveryFailure": { - tgtPortal: "172.18.0.2:3260", - tgtIQN: "iqn.2016-09.com.openebs.jiva:store1", - iface: "default", - mockedStdout: "iscsiadm: No records found\n", - mockedExitStatus: 21, - wantErr: true, + tgtPortal: "172.18.0.2:3260", + tgtIQN: "iqn.2016-09.com.openebs.jiva:store1", + iface: "default", + mockedStdout: "iscsiadm: No records found\n", + mockedCmdError: exec.Command("exit", "21").Run(), + wantErr: true, }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { - defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() + defer gostub.Stub(&execWithTimeout, makeFakeExecWithTimeout(false, []byte(tt.mockedStdout), tt.mockedCmdError)).Reset() err := CreateDBEntry(tt.tgtIQN, tt.tgtPortal, tt.iface, tt.discoverySecret, tt.sessionSecret) if (err != nil) != tt.wantErr { t.Errorf("CreateDBEntry() error = %v, wantErr %v", err, tt.wantErr) @@ -189,41 +190,41 @@ func TestCreateDBEntry(t *testing.T) { func TestListInterfaces(t *testing.T) { tests := map[string]struct { - mockedStdout string - mockedExitStatus int - interfaces []string - wantErr bool + mockedStdout string + mockedCmdError error + interfaces []string + wantErr bool }{ "EmptyOutput": { - mockedStdout: "", - mockedExitStatus: 0, - interfaces: []string{""}, - wantErr: false, + mockedStdout: "", + mockedCmdError: nil, + interfaces: []string{""}, + wantErr: false, }, "DefaultInterface": { - mockedStdout: "default", - mockedExitStatus: 0, - interfaces: []string{"default"}, - wantErr: false, + mockedStdout: "default", + mockedCmdError: nil, + interfaces: []string{"default"}, + wantErr: false, }, "TwoInterface": { - mockedStdout: "default\ntest", - mockedExitStatus: 0, - interfaces: []string{"default", "test"}, - wantErr: false, + mockedStdout: "default\ntest", + mockedCmdError: nil, + interfaces: []string{"default", "test"}, + wantErr: false, }, "HasError": { - mockedStdout: "", - mockedExitStatus: 1, - interfaces: []string{}, - wantErr: true, + mockedStdout: "", + mockedCmdError: exec.Command("exit", "1").Run(), + interfaces: []string{}, + wantErr: true, }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) - defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() + defer gostub.Stub(&execWithTimeout, makeFakeExecWithTimeout(false, []byte(tt.mockedStdout), tt.mockedCmdError)).Reset() interfaces, err := ListInterfaces() if tt.wantErr { @@ -238,29 +239,29 @@ func TestListInterfaces(t *testing.T) { func TestShowInterface(t *testing.T) { tests := map[string]struct { - mockedStdout string - mockedExitStatus int - iFace string - wantErr bool + mockedStdout string + mockedCmdError error + iFace string + wantErr bool }{ "DefaultInterface": { - mockedStdout: defaultInterface, - mockedExitStatus: 0, - iFace: defaultInterface, - wantErr: false, + mockedStdout: defaultInterface, + mockedCmdError: nil, + iFace: defaultInterface, + wantErr: false, }, "HasError": { - mockedStdout: "", - mockedExitStatus: 1, - iFace: "", - wantErr: true, + mockedStdout: "", + mockedCmdError: exec.Command("exit", "1").Run(), + iFace: "", + wantErr: true, }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { assert := assert.New(t) - defer gostub.Stub(&execCommand, makeFakeExecCommand(tt.mockedExitStatus, tt.mockedStdout)).Reset() + defer gostub.Stub(&execWithTimeout, makeFakeExecWithTimeout(false, []byte(tt.mockedStdout), tt.mockedCmdError)).Reset() interfaces, err := ShowInterface("default") if tt.wantErr { From 75f4b8ed71a8654bd0bf021bcc7c16565da046b8 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Thu, 14 Oct 2021 17:37:26 +0200 Subject: [PATCH 18/19] backward-compatibility --- example/main.go | 15 +------- iscsi/iscsi.go | 94 +++++++++++++++++++++++++-------------------- iscsi/iscsi_test.go | 3 +- 3 files changed, 56 insertions(+), 56 deletions(-) diff --git a/example/main.go b/example/main.go index 327a19eb..bdc309a7 100644 --- a/example/main.go +++ b/example/main.go @@ -26,25 +26,14 @@ func main() { iscsi.EnableDebugLogging(os.Stdout) } - var targets []iscsi.TargetInfo - - for _, tgtp := range tgtps { - parts := strings.Split(tgtp, ":") - targets = append(targets, iscsi.TargetInfo{ - // Specify the target iqn we're dealing with - Iqn: *iqn, - Portal: parts[0], - Port: parts[1], - }) - } - // You can utilize the iscsiadm calls directly if you wish, but by creating a Connector // you can simplify interactions to simple calls like "Connect" and "Disconnect" c := &iscsi.Connector{ // Our example uses chap AuthType: "chap", // List of targets must be >= 1 (>1 signals multipath/mpio) - Targets: targets, + TargetIqn: *iqn, + TargetPortals: tgtps, // CHAP can be setup up for discovery as well as sessions, our example // device only uses CHAP security for sessions, for those that use Discovery // as well, we'd add a DiscoverySecrets entry the same way diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index d3cd12c2..be7891b6 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -39,13 +39,6 @@ type iscsiSession struct { Name string } -// TargetInfo contains connection information to connect to an iSCSI endpoint -type TargetInfo struct { - Iqn string `json:"iqn"` - Portal string `json:"portal"` - Port string `json:"port"` -} - type deviceInfo struct { BlockDevices []Device } @@ -69,13 +62,14 @@ type HCTL struct { // Connector provides a struct to hold all of the needed parameters to make our iSCSI connection type Connector struct { - VolumeName string `json:"volume_name"` - Targets []TargetInfo `json:"targets"` - Lun int32 `json:"lun"` - AuthType string `json:"auth_type"` - DiscoverySecrets Secrets `json:"discovery_secrets"` - SessionSecrets Secrets `json:"session_secrets"` - Interface string `json:"interface"` + VolumeName string `json:"volume_name"` + TargetIqn string `json:"target_iqn"` + TargetPortals []string `json:"target_portal"` + Lun int32 `json:"lun"` + AuthType string `json:"auth_type"` + DiscoverySecrets Secrets `json:"discovery_secrets"` + SessionSecrets Secrets `json:"session_secrets"` + Interface string `json:"interface"` MountTargetDevice *Device `json:"mount_target_device"` Devices []Device `json:"devices"` @@ -247,6 +241,11 @@ func getMultipathDevice(devices []Device) (*Device, error) { return multipathDevice, nil } +// Connect is for backward-compatiblity with c.Connect() +func Connect(c Connector) (string, error) { + return c.Connect() +} + // Connect attempts to connect a volume to this node using the provided Connector info func (c *Connector) Connect() (string, error) { if c.RetryCount == 0 { @@ -270,8 +269,8 @@ func (c *Connector) Connect() (string, error) { var lastErr error var devicePaths []string - for _, target := range c.Targets { - devicePath, err := c.connectTarget(&target, iFace, iscsiTransport) + for _, target := range c.TargetPortals { + devicePath, err := c.connectTarget(c.TargetIqn, target, iFace, iscsiTransport) if err != nil { lastErr = err } else { @@ -311,9 +310,15 @@ func (c *Connector) Connect() (string, error) { return c.MountTargetDevice.GetPath(), nil } -func (c *Connector) connectTarget(target *TargetInfo, iFace string, iscsiTransport string) (string, error) { - debug.Printf("Process targetIqn: %s, portal: %s\n", target.Iqn, target.Portal) - baseArgs := []string{"-m", "node", "-T", target.Iqn, "-p", target.Portal} +func (c *Connector) connectTarget(targetIqn string, target string, iFace string, iscsiTransport string) (string, error) { + debug.Printf("Process targetIqn: %s, portal: %s\n", targetIqn, target) + targetParts := strings.Split(target, ":") + targetPortal := targetParts[0] + targetPort := defaultPort + if len(targetParts) > 1 { + targetPort = targetParts[1] + } + baseArgs := []string{"-m", "node", "-T", targetIqn, "-p", targetPortal} // Rescan sessions to discover newly mapped LUNs. Do not specify the interface when rescanning // to avoid establishing additional sessions to the same target. if _, err := iscsiCmd(append(baseArgs, []string{"-R"}...)...); err != nil { @@ -329,18 +334,14 @@ func (c *Connector) connectTarget(target *TargetInfo, iFace string, iscsiTranspo } // create our devicePath that we'll be looking for based on the transport being used - port := defaultPort - if target.Port != "" { - port = target.Port - } // portal with port - portal := strings.Join([]string{target.Portal, port}, ":") - devicePath := strings.Join([]string{"/dev/disk/by-path/ip", portal, "iscsi", target.Iqn, "lun", fmt.Sprint(c.Lun)}, "-") + portal := strings.Join([]string{targetPortal, targetPort}, ":") + devicePath := strings.Join([]string{"/dev/disk/by-path/ip", portal, "iscsi", targetIqn, "lun", fmt.Sprint(c.Lun)}, "-") if iscsiTransport != "tcp" { - devicePath = strings.Join([]string{"/dev/disk/by-path/pci", "*", "ip", portal, "iscsi", target.Iqn, "lun", fmt.Sprint(c.Lun)}, "-") + devicePath = strings.Join([]string{"/dev/disk/by-path/pci", "*", "ip", portal, "iscsi", targetIqn, "lun", fmt.Sprint(c.Lun)}, "-") } - exists, _ := sessionExists(portal, target.Iqn) + exists, _ := sessionExists(portal, targetIqn) if exists { debug.Printf("Session already exists, checking if device path %q exists", devicePath) if err := waitForPathToExist(&devicePath, c.RetryCount, c.CheckInterval, iscsiTransport); err != nil { @@ -349,12 +350,12 @@ func (c *Connector) connectTarget(target *TargetInfo, iFace string, iscsiTranspo return devicePath, nil } - if err := c.discoverTarget(target, iFace, portal); err != nil { + if err := c.discoverTarget(targetIqn, iFace, portal); err != nil { return "", err } // perform the login - err := Login(target.Iqn, portal) + err := Login(targetIqn, portal) if err != nil { debug.Printf("Failed to login: %v", err) return "", err @@ -368,7 +369,7 @@ func (c *Connector) connectTarget(target *TargetInfo, iFace string, iscsiTranspo return devicePath, nil } -func (c *Connector) discoverTarget(target *TargetInfo, iFace string, portal string) error { +func (c *Connector) discoverTarget(targetIqn string, iFace string, portal string) error { if c.DoDiscovery { // build discoverydb and discover iscsi target if err := Discoverydb(portal, iFace, c.DiscoverySecrets, c.DoCHAPDiscovery); err != nil { @@ -379,7 +380,7 @@ func (c *Connector) discoverTarget(target *TargetInfo, iFace string, portal stri if c.DoCHAPDiscovery { // Make sure we don't log the secrets - err := CreateDBEntry(target.Iqn, portal, iFace, c.DiscoverySecrets, c.SessionSecrets) + err := CreateDBEntry(targetIqn, portal, iFace, c.DiscoverySecrets, c.SessionSecrets) if err != nil { debug.Printf("Error creating db entry: %s\n", err.Error()) return err @@ -389,21 +390,25 @@ func (c *Connector) discoverTarget(target *TargetInfo, iFace string, portal stri return nil } -// Disconnect performs a disconnect operation from an appliance. -// Be sure to disconnect all deivces properly before doing this as it can result in data loss. -func (c *Connector) Disconnect() { - for _, target := range c.Targets { - Logout(target.Iqn, target.Portal) +// Disconnect is for backward-compatibility with c.Disconnect() +func Disconnect(targetIqn string, targets []string) { + for _, target := range targets { + targetPortal := strings.Split(target, ":")[0] + Logout(targetIqn, targetPortal) } deleted := map[string]bool{} - for _, target := range c.Targets { - if _, ok := deleted[target.Iqn]; ok { - continue - } - deleted[target.Iqn] = true - DeleteDBEntry(target.Iqn) + if _, ok := deleted[targetIqn]; ok { + return } + deleted[targetIqn] = true + DeleteDBEntry(targetIqn) +} + +// Disconnect performs a disconnect operation from an appliance. +// Be sure to disconnect all deivces properly before doing this as it can result in data loss. +func (c *Connector) Disconnect() { + Disconnect(c.TargetIqn, c.TargetPortals) } // DisconnectVolume removes a volume from a Linux host. @@ -588,6 +593,11 @@ func RemoveSCSIDevices(devices ...Device) error { return nil } +// PersistConnector is for backward-compatibility with c.Persist() +func PersistConnector(c *Connector, filePath string) error { + return c.Persist(filePath) +} + // Persist persists the Connector to the specified file (ie /var/lib/pfile/myConnector.json) func (c *Connector) Persist(filePath string) error { //file := path.Join("mnt", c.VolumeName+".json") diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index aaaf1355..239a6b38 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -668,7 +668,8 @@ func TestConnectorPersistance(t *testing.T) { } c := Connector{ VolumeName: "fake volume name", - Targets: []TargetInfo{}, + TargetIqn: "fake target iqn", + TargetPortals: []string{}, Lun: 42, AuthType: "fake auth type", DiscoverySecrets: secret, From b0ce999d54f73c234a85e6ea074dd6154ea089f1 Mon Sep 17 00:00:00 2001 From: Paul Laffitte Date: Tue, 26 Oct 2021 17:57:49 +0200 Subject: [PATCH 19/19] use raw format in lsblk in order to support a wider range of versions --- iscsi/iscsi.go | 58 ++++++++++++++++++++++++++++++++++------- iscsi/iscsi_test.go | 63 +++++++++++++++++++++++++++------------------ 2 files changed, 87 insertions(+), 34 deletions(-) diff --git a/iscsi/iscsi.go b/iscsi/iscsi.go index be7891b6..e49ef2dc 100644 --- a/iscsi/iscsi.go +++ b/iscsi/iscsi.go @@ -39,9 +39,7 @@ type iscsiSession struct { Name string } -type deviceInfo struct { - BlockDevices []Device -} +type deviceInfo []Device // Device contains informations about a device type Device struct { @@ -483,7 +481,7 @@ func GetSCSIDevices(devicePaths []string, strict bool) ([]Device, error) { return nil, err } - return deviceInfo.BlockDevices, nil + return deviceInfo, nil } // GetISCSIDevices get iSCSI devices from device paths @@ -505,8 +503,8 @@ func GetISCSIDevices(devicePaths []string, strict bool) (devices []Device, err e } // lsblk execute the lsblk commands -func lsblk(devicePaths []string, strict bool) (*deviceInfo, error) { - flags := []string{"-J", "-o", "NAME,HCTL,TYPE,TRAN,SIZE"} +func lsblk(devicePaths []string, strict bool) (deviceInfo, error) { + flags := []string{"-rn", "-o", "NAME,KNAME,PKNAME,HCTL,TYPE,TRAN,SIZE"} command := execCommand("lsblk", append(flags, devicePaths...)...) debug.Println(command.String()) out, err := command.Output() @@ -522,12 +520,54 @@ func lsblk(devicePaths []string, strict bool) (*deviceInfo, error) { } } + var devices []*Device + devicesMap := make(map[string]*Device) + pkNames := []string{} + + // Parse devices + lines := strings.Split(strings.Trim(string(out), "\n"), "\n") + for _, line := range lines { + columns := strings.Split(line, " ") + if len(columns) < 5 { + return nil, fmt.Errorf("invalid output from lsblk: %s", line) + } + device := &Device{ + Name: columns[0], + Hctl: columns[3], + Type: columns[4], + Transport: columns[5], + Size: columns[6], + } + devices = append(devices, device) + pkNames = append(pkNames, columns[2]) + devicesMap[columns[1]] = device + } + + // Reconstruct devices tree + for i, pkName := range pkNames { + if pkName == "" { + continue + } + device := devices[i] + parent, ok := devicesMap[pkName] + if !ok { + return nil, fmt.Errorf("invalid output from lsblk: parent device %q not found", pkName) + } + if parent.Children == nil { + parent.Children = []Device{} + } + parent.Children = append(devicesMap[pkName].Children, *device) + } + + // Filter devices to keep only the roots of the tree var deviceInfo deviceInfo - if jsonErr := json.Unmarshal(out, &deviceInfo); jsonErr != nil { - return nil, jsonErr + for i, device := range devices { + if pkNames[i] == "" { + deviceInfo = append(deviceInfo, *device) + } } - return &deviceInfo, nil + return deviceInfo, nil } // writeInSCSIDeviceFile write into special devices files to change devices state diff --git a/iscsi/iscsi_test.go b/iscsi/iscsi_test.go index 239a6b38..3ef70d17 100644 --- a/iscsi/iscsi_test.go +++ b/iscsi/iscsi_test.go @@ -127,6 +127,20 @@ func makeFakeExecWithTimeout(withTimeout bool, output []byte, err error) func(st } } +func marshalDeviceInfo(d *deviceInfo) string { + var output string + pkNames := map[string]string{} + for _, device := range *d { + for _, child := range device.Children { + pkNames[child.Name] = device.Name + } + } + for _, device := range *d { + output += fmt.Sprintf("%s %s %s %s %s %s %s\n", device.Name, device.Name, pkNames[device.Name], device.Hctl, device.Type, device.Transport, device.Size) + } + return output +} + func TestExecCommandHelper(t *testing.T) { if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { return @@ -579,16 +593,15 @@ func Test_getMultipathDevice(t *testing.T) { } func Test_lsblk(t *testing.T) { - sda := Device{Name: "sda", Children: []Device{{}}} - - sdaOutput, err := json.Marshal(deviceInfo{BlockDevices: []Device{sda}}) - assert.New(t).Nil(err, "could not setup test") + sda1 := Device{Name: "sda1"} + sda := Device{Name: "sda", Children: []Device{sda1}} + sdaOutput := marshalDeviceInfo(&deviceInfo{sda, sda1}) tests := map[string]struct { devicePaths []string strict bool mockedStdout string - mockedDevices []Device + mockedDevices deviceInfo mockedExitStatus int wantErr bool }{ @@ -603,7 +616,7 @@ func Test_lsblk(t *testing.T) { mockedExitStatus: 32, wantErr: true, }, - "InvalidJson": { + "InvalidOutput": { mockedStdout: "{", mockedExitStatus: 0, wantErr: true, @@ -636,7 +649,7 @@ func Test_lsblk(t *testing.T) { assert.NotNil(err) } else { assert.NotNil(deviceInfo) - assert.Equal(tt.mockedDevices, deviceInfo.BlockDevices) + assert.Equal(tt.mockedDevices, deviceInfo) assert.Nil(err) } }) @@ -654,17 +667,17 @@ func TestConnectorPersistance(t *testing.T) { PasswordIn: "fake password in", } childDevice := Device{ - Name: "child name", - Hctl: "child hctl", - Type: "child type", - Transport: "child transport", + Name: "child-name", + Hctl: "child-hctl", + Type: "child-type", + Transport: "child-transport", } device := Device{ - Name: "device name", - Hctl: "device hctl", + Name: "device-name", + Hctl: "device-hctl", Children: []Device{childDevice}, - Type: "device type", - Transport: "device transport", + Type: "device-type", + Transport: "device-transport", } c := Connector{ VolumeName: "fake volume name", @@ -687,32 +700,32 @@ func TestConnectorPersistance(t *testing.T) { devicesByPath[device.GetPath()] = &device defer gostub.Stub(&execCommand, func(name string, arg ...string) *exec.Cmd { - blockDevices := []Device{} + blockDevices := deviceInfo{} for _, path := range arg[3:] { blockDevices = append(blockDevices, *devicesByPath[path]) } - out, err := json.Marshal(deviceInfo{BlockDevices: blockDevices}) - assert.Nil(err, "could not setup test") + out := marshalDeviceInfo(&blockDevices) return makeFakeExecCommand(0, string(out))(name, arg...) }).Reset() defer gostub.Stub(&execCommand, func(cmd string, args ...string) *exec.Cmd { - mockedDevice := device - if args[3] == "/dev/child name" { - mockedDevice = childDevice + devInfo := &deviceInfo{device, childDevice} + if args[3] == "/dev/child-name" { + devInfo = &deviceInfo{childDevice} } - mockedOutput, err := json.Marshal(deviceInfo{BlockDevices: []Device{mockedDevice}}) - assert.Nil(err, "could not setup test") - + mockedOutput := marshalDeviceInfo(devInfo) return makeFakeExecCommand(0, string(mockedOutput))(cmd, args...) }).Reset() c.Persist("/tmp/connector.json") c2, err := GetConnectorFromFile("/tmp/connector.json") assert.Nil(err) - assert.Equal(c, *c2) + assert.NotNil(c2) + if c2 != nil { + assert.Equal(c, *c2) + } err = c.Persist("/tmp") assert.NotNil(err)