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

mark volume detach as success when Node vm is deleted from vcenter #1879

Merged
merged 1 commit into from
Jul 22, 2022
Merged

mark volume detach as success when Node vm is deleted from vcenter #1879

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pkg/csi/service/vanilla/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1247,8 +1247,14 @@ func (c *controller) ControllerUnpublishVolume(ctx context.Context, req *csi.Con
node, err = c.nodeMgr.GetNodeByName(ctx, req.NodeId)
}
if err != nil {
return nil, csifault.CSIInternalFault, logger.LogNewErrorCodef(log, codes.Internal,
"failed to find VirtualMachine for node:%q. Error: %v", req.NodeId, err)
if err == cnsvsphere.ErrVMNotFound {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we can infer that the disk is detached when we get ErrVmNotFound. A better way to be sure is to check if the volume is attached to any VM. FCD has an API for that but that API is currently not exposed by CNS. Can CNS expose this API? Once available, CSI can invoke that to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we can infer that the disk is detached when we get ErrVmNotFound.

@SandeepPissay so are you saying the searchIndex.FindByUuid API call can not determine if VM is deleted from VC Inventory.
if VM is not found in the VC inventory then what is holding us from letting k8s know that this volume can be marked as detached from the requested node. Why do we need to check if FCD is attached to any other VM? Here we are making a change in the ControllerUnpublishVolume call.

svm, err := searchIndex.FindByUuid(ctx, dc.Datacenter, uuid, true, &instanceUUID)
	if err != nil {
		log.Errorf("failed to find VM given uuid %s with err: %v", uuid, err)
		return nil, err
	} else if svm == nil {
		log.Errorf("Couldn't find VM given uuid %s", uuid)
		return nil, ErrVMNotFound
	}

Copy link
Member Author

Choose a reason for hiding this comment

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

Also for Supervisor Pod VM, we have relied on this API.
Refer to https://github.com/kubernetes-sigs/vsphere-csi-driver/pull/1702/files
then why we can not use this API for Vanilla Node VM?

Copy link
Contributor

Choose a reason for hiding this comment

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

searchIndex.FindByUuid returns from the VC inventory and it could return an incorrect result when it is stale. IIRC the VC inventory can be stale in a few cases (VC restored to backup, host sync issues, etc). We should confirm this with the VC team. Instead of VM existence in VC inventory, its better to have a reliable way to know whether the disk is really attached to any VM or not. And this operation should actually check at the ESX host layer. If such an API does not exist today, we should file a request. As far as this PR goes, we can commit with the above caveats.

log.Infof("Virtual Machine for Node ID: %v is not present in the VC Inventory. "+
"Marking ControllerUnpublishVolume for Volume: %q as successful.", req.NodeId, req.VolumeId)
return &csi.ControllerUnpublishVolumeResponse{}, "", nil
} else {
return nil, csifault.CSIInternalFault, logger.LogNewErrorCodef(log, codes.Internal,
"failed to find VirtualMachine for node:%q. Error: %v", req.NodeId, err)
}
}
faultType, err = common.DetachVolumeUtil(ctx, c.manager, node, req.VolumeId)
if err != nil {
Expand Down