From 6a8cec1c5c9f19b2762f6c77b4c7ab11f1113de6 Mon Sep 17 00:00:00 2001 From: Abitha Palaniappan Date: Thu, 26 May 2016 15:08:47 -0700 Subject: [PATCH] Fix vSphere Volume plugin bugs - replaces probeVolume with scsiHostRescan to scan hot attached disks - fixes substring match of UUID returned from AttachDisk - changes DetachDisk to take volumePath argument instead of diskID - fixes delayed failure at mount rather than attach disk - removes cloning of virtual disk in AttachDisk --- .../providers/vsphere/vsphere.go | 46 ++++++++++++++----- .../providers/vsphere/vsphere_test.go | 8 ++-- pkg/volume/vsphere_volume/vsphere_volume.go | 9 +++- .../vsphere_volume/vsphere_volume_util.go | 36 +++++++-------- 4 files changed, 63 insertions(+), 36 deletions(-) diff --git a/pkg/cloudprovider/providers/vsphere/vsphere.go b/pkg/cloudprovider/providers/vsphere/vsphere.go index 7724149e591..87577351c6b 100644 --- a/pkg/cloudprovider/providers/vsphere/vsphere.go +++ b/pkg/cloudprovider/providers/vsphere/vsphere.go @@ -44,7 +44,8 @@ const ActivePowerState = "poweredOn" const DefaultDiskController = "scsi" const DefaultSCSIControllerType = "lsilogic" -var ErrNoDiskUUIDFound = errors.New("no disk UUID found") +var ErrNoDiskUUIDFound = errors.New("No disk UUID found") +var ErrNoDiskIDFound = errors.New("No vSphere disk ID found") var ErrNoDevicesFound = errors.New("No devices found") // VSphere is an implementation of cloud provider Interface for VSphere. @@ -273,7 +274,7 @@ func (i *Instances) List(filter string) ([]string, error) { return nil, err } - glog.V(3).Infof("Found %s instances matching %s: %s", + glog.V(3).Infof("found %s instances matching %s: %s", len(vmList), filter, vmList) return vmList, nil @@ -435,9 +436,9 @@ func (vs *VSphere) GetZone() (cloudprovider.Zone, error) { return cloudprovider.Zone{Region: vs.cfg.Global.Datacenter}, nil } -// Routes returns an implementation of Routes for vSphere. +// Routes returns a false since the interface is not supported for vSphere. func (vs *VSphere) Routes() (cloudprovider.Routes, bool) { - return nil, true + return nil, false } // ScrubDNS filters DNS settings for pods. @@ -525,7 +526,7 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName string) (diskID string // create a scsi controller if there is not one newSCSIController, err := vmDevices.CreateSCSIController(vs.cfg.Disk.SCSIControllerType) if err != nil { - glog.V(3).Infof("Cannot create new SCSI controller - %v", err) + glog.V(3).Infof("cannot create new SCSI controller - %v", err) return "", "", err } configNewSCSIController := newSCSIController.(types.BaseVirtualSCSIController).GetVirtualSCSIController() @@ -536,7 +537,7 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName string) (diskID string // add the scsi controller to virtual machine err = vm.AddDevice(context.TODO(), newSCSIController) if err != nil { - glog.V(3).Infof("Cannot add SCSI controller to vm - %v", err) + glog.V(3).Infof("cannot add SCSI controller to vm - %v", err) // attempt clean up of scsi controller if vmDevices, err := vm.Device(ctx); err == nil { cleanUpController(newSCSIController, vmDevices, vm, ctx) @@ -551,7 +552,7 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName string) (diskID string return "", "", err } if diskController, err = vmDevices.FindDiskController(vs.cfg.Disk.DiskController); err != nil { - glog.V(3).Infof("Cannot find disk controller - %v", err) + glog.V(3).Infof("cannot find disk controller - %v", err) // attempt clean up of scsi controller cleanUpController(newSCSIController, vmDevices, vm, ctx) return "", "", err @@ -562,12 +563,11 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName string) (diskID string disk := vmDevices.CreateDisk(diskController, ds.Reference(), vmDiskPath) backing := disk.Backing.(*types.VirtualDiskFlatVer2BackingInfo) backing.DiskMode = string(types.VirtualDiskModeIndependent_persistent) - disk = vmDevices.ChildDisk(disk) // Attach disk to the VM err = vm.AddDevice(ctx, disk) if err != nil { - glog.V(3).Infof("Cannot add disk to the vm - %v", err) + glog.V(3).Infof("cannot attach disk to the vm - %v", err) if newSCSICreated { cleanUpController(newSCSIController, vmDevices, vm, ctx) } @@ -611,13 +611,29 @@ func getVirtualDiskUUID(newDevice types.BaseVirtualDevice) (string, error) { if b, ok := vd.Backing.(*types.VirtualDiskFlatVer2BackingInfo); ok { uuidWithNoHypens := strings.Replace(b.Uuid, "-", "", -1) - return uuidWithNoHypens, nil + return strings.ToLower(uuidWithNoHypens), nil } return "", ErrNoDiskUUIDFound } +func getVirtualDiskID(volPath string, vmDevices object.VirtualDeviceList) (string, error) { + // filter vm devices to retrieve disk ID for the given vmdk file + for _, device := range vmDevices { + if vmDevices.TypeName(device) == "VirtualDisk" { + d := device.GetVirtualDevice() + if b, ok := d.Backing.(types.BaseVirtualDeviceFileBackingInfo); ok { + fileName := b.GetVirtualDeviceFileBackingInfo().FileName + if fileName == volPath { + return vmDevices.Name(device), nil + } + } + } + } + return "", ErrNoDiskIDFound +} + // Detaches given virtual disk volume from the compute running kubelet. -func (vs *VSphere) DetachDisk(diskID string, nodeName string) error { +func (vs *VSphere) DetachDisk(volPath string, nodeName string) error { // Create context ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -642,13 +658,19 @@ func (vs *VSphere) DetachDisk(diskID string, nodeName string) error { return err } + diskID, err := getVirtualDiskID(volPath, vmDevices) + if err != nil { + glog.Warningf("disk ID not found for %v ", volPath) + return err + } + // Remove disk from VM device := vmDevices.Find(diskID) if device == nil { return fmt.Errorf("device '%s' not found", diskID) } - err = vm.RemoveDevice(ctx, false, device) + err = vm.RemoveDevice(ctx, true, device) if err != nil { return err } diff --git a/pkg/cloudprovider/providers/vsphere/vsphere_test.go b/pkg/cloudprovider/providers/vsphere/vsphere_test.go index 6d5f82738f9..2f991db8b68 100644 --- a/pkg/cloudprovider/providers/vsphere/vsphere_test.go +++ b/pkg/cloudprovider/providers/vsphere/vsphere_test.go @@ -224,19 +224,19 @@ func TestVolumes(t *testing.T) { t.Fatalf("Cannot create a new VMDK volume: %v", err) } - diskID, _, err := vs.AttachDisk(volPath, "") + _, _, err = vs.AttachDisk(volPath, "") if err != nil { t.Fatalf("Cannot attach volume(%s) to VM(%s): %v", volPath, srvs[0], err) } - err = vs.DetachDisk(diskID, "") + err = vs.DetachDisk(volPath, "") if err != nil { - t.Fatalf("Cannot detach disk(%s) from VM(%s): %v", diskID, srvs[0], err) + t.Fatalf("Cannot detach disk(%s) from VM(%s): %v", volPath, srvs[0], err) } // todo: Deleting a volume after detach currently not working through API or UI (vSphere) // err = vs.DeleteVolume(volPath) // if err != nil { - // t.Fatalf("Cannot delete VMDK volume %s: %v", volPath, err) + // t.Fatalf("Cannot delete VMDK volume %s: %v", volPath, err) // } } diff --git a/pkg/volume/vsphere_volume/vsphere_volume.go b/pkg/volume/vsphere_volume/vsphere_volume.go index 9de3fc33f95..fd686cfa379 100644 --- a/pkg/volume/vsphere_volume/vsphere_volume.go +++ b/pkg/volume/vsphere_volume/vsphere_volume.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "path" + "strings" "github.com/golang/glog" "k8s.io/kubernetes/pkg/api" @@ -197,7 +198,7 @@ func (b *vsphereVolumeMounter) SetUpAt(dir string, fsGroup *int64) error { } globalPDPath := makeGlobalPDPath(b.plugin.host, b.volPath) if err := b.manager.AttachDisk(b, globalPDPath); err != nil { - glog.V(4).Infof("AttachDisk failed: %v", err) + glog.V(3).Infof("AttachDisk failed: %v", err) return err } glog.V(3).Infof("vSphere volume %s attached", b.volPath) @@ -280,7 +281,11 @@ func (v *vsphereVolumeUnmounter) TearDownAt(dir string) error { return fmt.Errorf("directory %s is not mounted", dir) } - v.volPath = path.Base(refs[0]) + // space between datastore and vmdk name in volumePath is encoded as '\040' when returned by GetMountRefs(). + // volumePath eg: "[local] xxx.vmdk" provided to attach/mount + // replacing \040 with space to match the actual volumePath + mountPath := strings.Replace(path.Base(refs[0]), "\\040", " ", -1) + v.volPath = mountPath glog.V(4).Infof("Found volume %s mounted to %s", v.volPath, dir) // Reload list of references, there might be SetUpAt finished in the meantime diff --git a/pkg/volume/vsphere_volume/vsphere_volume_util.go b/pkg/volume/vsphere_volume/vsphere_volume_util.go index f2e20a60839..cfc8bf4cce9 100644 --- a/pkg/volume/vsphere_volume/vsphere_volume_util.go +++ b/pkg/volume/vsphere_volume/vsphere_volume_util.go @@ -25,7 +25,6 @@ import ( "time" "github.com/golang/glog" - "k8s.io/kubernetes/pkg/util/exec" "k8s.io/kubernetes/pkg/util/keymutex" "k8s.io/kubernetes/pkg/volume" ) @@ -34,6 +33,8 @@ const ( maxRetries = 10 ) +var ErrProbeVolume = errors.New("Error scanning attached volumes") + // Singleton key mutex for keeping attach/detach operations for the same PD atomic var attachDetachMutex = keymutex.NewKeyMutex() @@ -55,7 +56,7 @@ func (util *VsphereDiskUtil) AttachDisk(vm *vsphereVolumeMounter, globalPDPath s diskID, diskUUID, attachError := cloud.AttachDisk(vm.volPath, "") if attachError != nil { - return err + return attachError } else if diskUUID == "" { return errors.New("Disk UUID has no value") } @@ -68,7 +69,7 @@ func (util *VsphereDiskUtil) AttachDisk(vm *vsphereVolumeMounter, globalPDPath s for { devicePath = verifyDevicePath(diskUUID) // probe the attached vol so that symlink in /dev/disk/by-id is created - probeAttachedVolume() + scsiHostRescan() _, err := os.Stat(devicePath) if err == nil { @@ -106,13 +107,25 @@ func (util *VsphereDiskUtil) AttachDisk(vm *vsphereVolumeMounter, globalPDPath s return nil } +// rescan scsi bus +func scsiHostRescan() { + scsi_path := "/sys/class/scsi_host/" + if dirs, err := ioutil.ReadDir(scsi_path); err == nil { + for _, f := range dirs { + name := scsi_path + f.Name() + "/scan" + data := []byte("- - -") + ioutil.WriteFile(name, data, 0666) + } + } +} + func verifyDevicePath(diskUUID string) string { files, _ := ioutil.ReadDir("/dev/disk/by-id/") for _, f := range files { // TODO: should support other controllers if strings.Contains(f.Name(), "scsi-") { devID := f.Name()[len("scsi-"):len(f.Name())] - if strings.Contains(diskUUID, devID) { + if strings.Contains(devID, diskUUID) { glog.V(4).Infof("Found disk attached as %q; full devicepath: %s\n", f.Name(), path.Join("/dev/disk/by-id/", f.Name())) return path.Join("/dev/disk/by-id/", f.Name()) } @@ -122,19 +135,6 @@ func verifyDevicePath(diskUUID string) string { return "" } -func probeAttachedVolume() error { - executor := exec.New() - args := []string{"trigger"} - cmd := executor.Command("/usr/bin/udevadm", args...) - _, err := cmd.CombinedOutput() - if err != nil { - glog.Errorf("error running udevadm trigger %v\n", err) - return err - } - glog.V(4).Infof("Successfully probed all attachments") - return nil -} - // Unmounts the device and detaches the disk from the kubelet's host machine. func (util *VsphereDiskUtil) DetachDisk(vu *vsphereVolumeUnmounter) error { @@ -156,7 +156,7 @@ func (util *VsphereDiskUtil) DetachDisk(vu *vsphereVolumeUnmounter) error { return err } - if err = cloud.DetachDisk(vu.diskID, ""); err != nil { + if err = cloud.DetachDisk(vu.volPath, ""); err != nil { return err } glog.V(2).Infof("Successfully detached vSphere volume %s", vu.volPath)