From b85743b868ef8ce9e7a4b015904ddc7afdfdd572 Mon Sep 17 00:00:00 2001 From: FengyunPan Date: Mon, 14 Aug 2017 10:09:50 +0800 Subject: [PATCH 1/2] Mark volume as detached when node does not exist for vsphere If node does not exist, node's volumes will be detached automatically and become available. So mark them detached and return false without error. Fix #50266 --- .../providers/vsphere/vclib/utils.go | 15 ++++++ .../providers/vsphere/vclib/virtualmachine.go | 12 ++--- .../providers/vsphere/vsphere.go | 54 +++++++++---------- 3 files changed, 43 insertions(+), 38 deletions(-) diff --git a/pkg/cloudprovider/providers/vsphere/vclib/utils.go b/pkg/cloudprovider/providers/vsphere/vclib/utils.go index 8b80b4a4482..3ae00e8c660 100644 --- a/pkg/cloudprovider/providers/vsphere/vclib/utils.go +++ b/pkg/cloudprovider/providers/vsphere/vclib/utils.go @@ -27,6 +27,21 @@ import ( "github.com/vmware/govmomi/vim25/types" ) +// IsNotFound return true if err is NotFoundError or DefaultNotFoundError +func IsNotFound(err error) bool { + _, ok := err.(*find.NotFoundError) + if ok { + return true + } + + _, ok = err.(*find.DefaultNotFoundError) + if ok { + return true + } + + return false +} + func getFinder(dc *Datacenter) *find.Finder { finder := find.NewFinder(dc.Client(), true) finder.SetDatacenter(dc.Datacenter) diff --git a/pkg/cloudprovider/providers/vsphere/vclib/virtualmachine.go b/pkg/cloudprovider/providers/vsphere/vclib/virtualmachine.go index aed47f3c566..91d70535b8f 100644 --- a/pkg/cloudprovider/providers/vsphere/vclib/virtualmachine.go +++ b/pkg/cloudprovider/providers/vsphere/vclib/virtualmachine.go @@ -188,9 +188,9 @@ func (vm *VirtualMachine) GetResourcePool(ctx context.Context) (*object.Resource return object.NewResourcePool(vm.Client(), vmMoList[0].ResourcePool.Reference()), nil } -// Exists checks if the VM exists. -// Returns false if VM doesn't exist or VM is in powerOff state. -func (vm *VirtualMachine) Exists(ctx context.Context) (bool, error) { +// IsActive checks if the VM is active. +// Returns true if VM is in poweredOn state. +func (vm *VirtualMachine) IsActive(ctx context.Context) (bool, error) { vmMoList, err := vm.Datacenter.GetVMMoList(ctx, []*VirtualMachine{vm}, []string{"summary"}) if err != nil { glog.Errorf("Failed to get VM Managed object with property summary. err: +%v", err) @@ -199,11 +199,7 @@ func (vm *VirtualMachine) Exists(ctx context.Context) (bool, error) { if vmMoList[0].Summary.Runtime.PowerState == ActivePowerState { return true, nil } - if vmMoList[0].Summary.Config.Template == false { - glog.Warningf("VM is not in %s state", ActivePowerState) - } else { - glog.Warningf("VM is a template") - } + return false, nil } diff --git a/pkg/cloudprovider/providers/vsphere/vsphere.go b/pkg/cloudprovider/providers/vsphere/vsphere.go index 60086d52297..d362666eba0 100644 --- a/pkg/cloudprovider/providers/vsphere/vsphere.go +++ b/pkg/cloudprovider/providers/vsphere/vsphere.go @@ -391,18 +391,22 @@ func (vs *VSphere) InstanceID(nodeName k8stypes.NodeName) (string, error) { } vm, err := vs.getVMByName(ctx, nodeName) if err != nil { + if vclib.IsNotFound(err) { + return "", cloudprovider.InstanceNotFound + } glog.Errorf("Failed to get VM object for node: %q. err: +%v", nodeNameToVMName(nodeName), err) return "", err } - nodeExist, err := vm.Exists(ctx) + isActive, err := vm.IsActive(ctx) if err != nil { - glog.Errorf("Failed to check whether node %q exist. err: %+v.", nodeNameToVMName(nodeName), err) + glog.Errorf("Failed to check whether node %q is active. err: %+v.", nodeNameToVMName(nodeName), err) return "", err } - if nodeExist { + if isActive { return "/" + vm.InventoryPath, nil } - return "", cloudprovider.InstanceNotFound + + return "", fmt.Errorf("The node %q is not active", nodeNameToVMName(nodeName)) } // InstanceTypeByProviderID returns the cloudprovider instance type of the node with the specified unique providerID @@ -530,22 +534,15 @@ func (vs *VSphere) DiskIsAttached(volPath string, nodeName k8stypes.NodeName) (b } vm, err := vs.getVMByName(ctx, nodeName) if err != nil { + if vclib.IsNotFound(err) { + glog.Warningf("Node %q does not exist, vsphere CP will assume disk %v is not attached to it.", nodeName, volPath) + // make the disk as detached and return false without error. + return false, nil + } glog.Errorf("Failed to get VM object for node: %q. err: +%v", vSphereInstance, err) return false, err } - nodeExist, err := vm.Exists(ctx) - if err != nil { - glog.Errorf("Failed to check whether node %q exist. err: %+v", vSphereInstance, err) - return false, err - } - if !nodeExist { - glog.Errorf("DiskIsAttached failed to determine whether disk %q is still attached: node %q is powered off", - volPath, - vSphereInstance) - return false, fmt.Errorf("DiskIsAttached failed to determine whether disk %q is still attached: node %q is powered off", - volPath, - vSphereInstance) - } + attached, err := vm.IsDiskAttached(ctx, volPath) if err != nil { glog.Errorf("DiskIsAttached failed to determine whether disk %q is still attached on node %q", @@ -584,22 +581,19 @@ func (vs *VSphere) DisksAreAttached(volPaths []string, nodeName k8stypes.NodeNam } vm, err := vs.getVMByName(ctx, nodeName) if err != nil { + if vclib.IsNotFound(err) { + glog.Warningf("Node %q does not exist, vsphere CP will assume all disks %v are not attached to it.", nodeName, volPaths) + // make all the disks as detached and return false without error. + attached := make(map[string]bool) + for _, volPath := range volPaths { + attached[volPath] = false + } + return attached, nil + } glog.Errorf("Failed to get VM object for node: %q. err: +%v", vSphereInstance, err) return nil, err } - nodeExist, err := vm.Exists(ctx) - if err != nil { - glog.Errorf("Failed to check whether node %q exist. err: %+v", vSphereInstance, err) - return nil, err - } - if !nodeExist { - glog.Errorf("DisksAreAttached failed to determine whether disks %v are still attached: node %q does not exist", - volPaths, - vSphereInstance) - return nil, fmt.Errorf("DisksAreAttached failed to determine whether disks %v are still attached: node %q does not exist", - volPaths, - vSphereInstance) - } + for _, volPath := range volPaths { result, err := vm.IsDiskAttached(ctx, volPath) if err == nil { From ea32f06d20b9f6d547f52b419333b8620aba9368 Mon Sep 17 00:00:00 2001 From: FengyunPan Date: Mon, 14 Aug 2017 10:12:46 +0800 Subject: [PATCH 2/2] [VSphere] Don't return err when node doesn't exist in DetachDisk() --- pkg/cloudprovider/providers/vsphere/vsphere.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/cloudprovider/providers/vsphere/vsphere.go b/pkg/cloudprovider/providers/vsphere/vsphere.go index d362666eba0..2105edcd6e9 100644 --- a/pkg/cloudprovider/providers/vsphere/vsphere.go +++ b/pkg/cloudprovider/providers/vsphere/vsphere.go @@ -498,6 +498,12 @@ func (vs *VSphere) DetachDisk(volPath string, nodeName k8stypes.NodeName) error } vm, err := vs.getVMByName(ctx, nodeName) if err != nil { + // If node doesn't exist, disk is already detached from node. + if vclib.IsNotFound(err) { + glog.Infof("Node %q does not exist, disk %s is already detached from node.", nodeNameToVMName(nodeName), volPath) + return nil + } + glog.Errorf("Failed to get VM object for node: %q. err: +%v", nodeNameToVMName(nodeName), err) return err }