Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing VolumesAreAttached and DisksAreAttached functions in vSphere #45569

Merged
merged 1 commit into from
May 11, 2017
Merged

Fixing VolumesAreAttached and DisksAreAttached functions in vSphere #45569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 19 additions & 14 deletions pkg/cloudprovider/providers/vsphere/vsphere.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,10 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName k8stypes.NodeName) (di
return "", "", err
}

attached, _ := checkDiskAttached(vmDiskPath, vmDevices, dc, vs.client)
attached, err := checkDiskAttached(vmDiskPath, vmDevices, dc, vs.client)
if err != nil {
return "", "", err
}
if attached {
diskID, _ = getVirtualDiskID(vmDiskPath, vmDevices, dc, vs.client)
diskUUID, _ = getVirtualDiskUUIDByPath(vmDiskPath, dc, vs.client)
Expand Down Expand Up @@ -1006,14 +1009,10 @@ func (vs *VSphere) DisksAreAttached(volPaths []string, nodeName k8stypes.NodeNam
defer cancel()

// Create vSphere client
attached := make(map[string]bool)
for _, volPath := range volPaths {
attached[volPath] = false
}
err := vSphereLogin(ctx, vs)
if err != nil {
glog.Errorf("Failed to login into vCenter, err: %v", err)
return attached, err
return nil, err
}

// Find VM to detach disk from
Expand All @@ -1029,14 +1028,14 @@ func (vs *VSphere) DisksAreAttached(volPaths []string, nodeName k8stypes.NodeNam

if err != nil {
glog.Errorf("Failed to check whether node exist. err: %s.", err)
return attached, 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 attached, fmt.Errorf("DisksAreAttached failed to determine whether disks %v are still attached: node %q does not exist",
return nil, fmt.Errorf("DisksAreAttached failed to determine whether disks %v are still attached: node %q does not exist",
volPaths,
vSphereInstance)
}
Expand All @@ -1045,17 +1044,23 @@ func (vs *VSphere) DisksAreAttached(volPaths []string, nodeName k8stypes.NodeNam
_, vmDevices, dc, err := getVirtualMachineDevices(ctx, vs.cfg, vs.client, vSphereInstance)
if err != nil {
glog.Errorf("Failed to get VM devices for VM %#q. err: %s", vSphereInstance, err)
return attached, err
return nil, err
}

attached := make(map[string]bool)
for _, volPath := range volPaths {
result, _ := checkDiskAttached(volPath, vmDevices, dc, vs.client)
if result {
attached[volPath] = true
result, err := checkDiskAttached(volPath, vmDevices, dc, vs.client)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have similar concern when checkDiskAttached is called inside "DiskIsAttached" function?
Seems if err is not nil, it will always return false. And according to the detach function here, even error is not nil, it will continue and mark this volume as already detached, which is also the same problem as resolved by this fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For DiskIsAttached, we are returning error, so should be ok.
https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/vsphere/vsphere.go#L1000

But in AttachDisk I missed to handle error. Added error check.

https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/vsphere/vsphere.go#L769

Also Verified attacher.go -> Detach() is properly handling returned err. When DiskIsAttached is retuning error with false, we log the error and retry detach operation. If retry fails, we return error.
Only when error is nil and returned value is true Kubernetes mark the detach as successful.

https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/vsphere_volume/attacher.go#L229

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I missed the err == nil check in Detach().
Looks good to me now :)

if err == nil {
if result {
attached[volPath] = true
} else {
attached[volPath] = false
}
} else {
return nil, err
}
}

return attached, err
return attached, nil
}

func checkDiskAttached(volPath string, vmdevices object.VirtualDeviceList, dc *object.Datacenter, client *govmomi.Client) (bool, error) {
Expand Down
11 changes: 6 additions & 5 deletions pkg/volume/vsphere_volume/attacher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,24 +94,25 @@ func (attacher *vsphereVMDKAttacher) VolumesAreAttached(specs []*volume.Spec, no
glog.Errorf("Error getting volume (%q) source : %v", spec.Name(), err)
continue
}

volumePathList = append(volumePathList, volumeSource.VolumePath)
volumesAttachedCheck[spec] = true
volumeSpecMap[volumeSource.VolumePath] = spec
}
attachedResult, err := attacher.vsphereVolumes.DisksAreAttached(volumePathList, nodeName)
if err != nil {
glog.Errorf(
"Error checking if volumes (%v) are attached to current node (%q). err=%v",
volumePathList, nodeName, err)
return volumesAttachedCheck, err
return nil, err
}

for volumePath, attached := range attachedResult {
spec := volumeSpecMap[volumePath]
if !attached {
spec := volumeSpecMap[volumePath]
volumesAttachedCheck[spec] = false
glog.V(2).Infof("VolumesAreAttached: check volume %q (specName: %q) is no longer attached", volumePath, spec.Name())
glog.V(2).Infof("VolumesAreAttached: volume %q (specName: %q) is no longer attached", volumePath, spec.Name())
} else {
volumesAttachedCheck[spec] = true
glog.V(2).Infof("VolumesAreAttached: volume %q (specName: %q) is attached", volumePath, spec.Name())
}
}
return volumesAttachedCheck, nil
Expand Down