mark it but don't detach it yet.

Cannot comment on line 216. Consider something like the following

for volumeID, attached := range attachedResult {
 		if !attached {
 			spec := volumeSpecMap[volumeID]
 			volumesAttachedCheck[spec] = false
 			glog.V(2).Infof("VolumesAreAttached: check volume %q (specName: %q) is no longer attached", volumeID, spec.Name())
 		} else 
               // volume attached, check if the instance is alive here
 	}

Why we need check if the instance is alive when volume is attached?
Can you tell me more detail message about it? Can you give me a example? Thank you.

Checking the instance is alive is not sufficient: the cinder volume could be moved already to other nodes or in a different state.

We need both attachment info and node info to ensure that the cinder volume is not moved elsewhere and the node the volume is attached is dead, then we can safely detach the volume.

Agree with @rootfs

Oh, I see. Thank you very much @rootfs.
I update it soon, please take a look.
And I think the change should be inside of openstach_volume.go DiskIsAttached function too. @jingxu97

print both nodeName and instanceID for better diagnostics.

@jingxu97 should raise severity to glog.Warningf too?

@rootfs if instannce is deleted, openstack cloud provider can't get its instanceID, so attacher.nodeInstanceID() return ("", err). Printing nodename is enough.
Yeah, it should use glog.Warningf, thank you.

I don't think the logic here is right. The function is checking whether the volume is attached to the instanced passed in as parameters. Here is the instanceId is empty, it means that the instance is deleted from the cloudprovider. If the volume.AttachedServerId is not empty, that means the volume is a attached to a different instance. But the function should return false because the volume is not attached to the original instance.

For most of the volume plugins, if the node no longer exist in cloud provider, we can safely assume the volume is detached from the node. But for cinder, could you please double check the behavior if you have the environment? Thanks!

@jingxu97 IMO, if instanceID is empty, the DiskIsAttached() is checking whether the volume is attached. If the volume is attached, returning true means that the disk is attached, or returning false means that the disk is not attached.

But the function should return false because the volume is not attached to the original instance.

We can't get the instance id for the original instance which does not exist. If DiskIsAttached() return true when instance does not exist, that means the volume is attached to another instance and we should not mark the volume detached(as rootfs says). If DiskIsAttached() return false when instance does not exist, that means the volume is available, we can mark the volume detached. The code from line 207 already do it. And that also is my double check, I have tested it.

But for cinder, could you please double check the behavior if you have the environment?

I am sorry, I can't understand the meaning of your ‘double check’, can you tell more detail messages? thank you again.

"We can't get the instance id for the original instance which does not exist. If DiskIsAttached() return true when instance does not exist that means the volume is attached to another instance and we should not mark the volume detached"

Your description is not correct.
DiskIsAttached does not just check disk is attached to ANY node, it checks whether it is attached to the Node that you passed in that function. Suppose disk is currently attached to node B, the function check of DiskIsAttached (disk, nodeA) should return false because it is not attached to nodeA, it is attached to node B. Please let me know if this is clear.

Double check means that I want to confirm that node not found means volume is detached automatically. Seems like this is true for most of cloud provider. But I don't have the cinder volume setup to test it.

If DiskIsAttached() without my codes, DiskIsAttached() just check whether disk is attached to the instanceID that you passed in that function, and the instanceID can't be empty, right?
Now I add the codes into DiskIsAttached(), DiskIsAttached() can check whether disk is attached to the instanceID(not empty) that you passed in that function, also can check whether disk is attached when instanceID is empty.

Yes, volume will be detached when instance is not found in openstack cinder.

Is that confused? Is it need to add another func to check whether disk is attached?

If we are sure that if node not found in openstack cinder indicates that volume is already detached. Then I suggest we can the following changes
In cinder/attacher.go

if err == cloudprovider.InstanceNotFound {
    //return false with nil error
   ...
}

You might have this change in the very first version. Sorry for the confusion.
@rootfs mentioned that cinder might have a more complicated status, and node not exist might not represent volume is already detached successfully from the node. If this is true, we might not return false directly. But I could not think a good way to check it. Passing an empty string for node instance seems not right because our goal is to check whether the volume is still attached to that specified node (not any other node). What does it mean if node id is empty for this function?

@jingxu97 @rootfs Yeah, volume will be detached when instance is not found in openstack cinder. I am sorry for that I can't find the doc from openstack, but find the codes:
https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L899
nova will detach all the device from instance when user delete instance.
https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L983
detach volume:
https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L1387
If instance does not exist, nova will detach the volume too:
https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L1389

So we can do: If the node instance is no longer exist (return cloudprovider.InstanceNotFound error), we can safely assume that volume is no longer attached to the node. So DiskIsAttached() return disk is not attached without error.