Return success if instance or volume are not found

[bertinatto]

Is this a bug fix or adding new feature?

/kind bug

What is this PR about? / Why do we need it?

From the CSI specs;

If the volume corresponding to the volume_id or the node corresponding to node_id cannot be found by the Plugin and the volume can be safely regarded as ControllerUnpublished from the node, the plugin SHOULD return 0 OK.

Fixes #330

[bertinatto]

/retest

[coveralls]

Pull Request Test Coverage Report for Build 949

• 12 of 20 (60.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.06%) to 70.622%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/cloud/cloud.go	9	17	52.94%

Totals
Change from base Build 947:	-0.06%
Covered Lines:	1226
Relevant Lines:	1736

---

💛 - Coveralls

[leakingtapan]

Is the ErrNotFound returned from c.getInstance(ctx, nodeID)enough to cover the issue reported by @jsafrane ? Why do we still need this check?

Also, regarding driver is returning internal error, should we check for IncorrectState when detaching an volume that is already detached, and return success? It will get following error in this case:

An error occurred (IncorrectState) when calling the DetachVolume operation: Volume 'vol-0c1cd2b52306edf24'is in the 'available' state.

[bertinatto]

Is the ErrNotFound returned from c.getInstance(ctx, nodeID)enough to cover the issue reported by @jsafrane ? Why do we still need this check?

The ErrNotfound returned by getInstance() alone is not enough because it covers only cases where the instance has been deleted.

We also need to cover cases where the volume has been deleted, which is why I added this check.

Also, regarding driver is returning internal error, should we check for IncorrectState when detaching an volume that is already detached, and return success? It will get following error in this case:

An error occurred (IncorrectState) when calling the DetachVolume operation: Volume 'vol-0c1cd2b52306edf24'is in the 'available' state.

That's a good point. Are you sure we get a IncorrectState in this case? Looking at the docs, it seems like we get a InvalidAttachment.NotFound instead.

[leakingtapan]

That's a good point. Are you sure we get a IncorrectState in this case? Looking at the docs, it seems like we get a InvalidAttachment.NotFound instead.

I tested using awscli as follows:

aws ec2 detach-volume --volume-id vol-0d63967ab4999f4b6 --region us-east-1 --instanc
e-id i-03376857fb7981add

An error occurred (IncorrectState) when calling the DetachVolume operation: Volume 'vol-0d63967ab4999f4b6'is in the 'available' state.

And similar result with a simple go program:

func main() {
	c, _ := cloud.NewCloud("us-east-1")
	err := c.DetachDisk(context.Background(), "vol-0d63967ab4999f4b6", "i-03376857fb7981add")

	fmt.Println(err)
}

go run main.go
could not detach volume "vol-0d63967ab4999f4b6" from node "i-03376857fb7981add": IncorrectState: Volume 'vol-0d63967ab4999f4b6'is in the 'available' state.
        status code: 400, request id: bbf6560f-a44f-470b-a7c8-fbd0d1684201

Although I don't know when InvalidAttachment.NotFound could happen, how about we check for both?

[leakingtapan]

@jsafrane how is the check done in cloud provider now?

[bertinatto]

Although I don't know when InvalidAttachment.NotFound could happen, how about we check for both?

I think that makes sense. I changed the code to reflect that.

From the docs:

InvalidAttachment.NotFound | Indicates an attempt to detach a volume from an instance to which it is not attached.

Perhaps InvalidAttachment.NotFound is returned when the volume is attached to another instance?

[bertinatto]

@jsafrane how is the check done in cloud provider now?

Apparently it doesn't special-case any error; it just returns it:

https://github.com/kubernetes/kubernetes/blob/5bac42bff9bfb9dfe0f2ea40f1c80cac47fc12b2/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go#L2392-L2395

That makes sense because the in-tree plugin doesn't any requirement to be idempotent.

[leakingtapan]

Perhaps InvalidAttachment.NotFound is returned when the volume is attached to another instance?

Yep, just tested and confirmed you are right 👍

[leakingtapan]

@jsafrane how is the check done in cloud provider now?

Apparently it doesn't special-case any error; it just returns it:

https://github.com/kubernetes/kubernetes/blob/5bac42bff9bfb9dfe0f2ea40f1c80cac47fc12b2/staging/src/k8s.io/legacy-cloud-providers/aws/aws.go#L2392-L2395

That makes sense because the in-tree plugin doesn't any requirement to be idempotent.

I'm thinking how about we move the logic of:

if isAWSErrorIncorrectState(err) ||
			isAWSErrorInvalidAttachmentNotFound(err) ||
			isAWSErrorVolumeNotFound(err) {
			return ErrNotFound
		}

To controller.go ControllerUnpublishVolume ?
This has several advantages that: 1) keep the cloud provider simpler and cleaner 2) align with current cloud provider implementation so that we can unify the two cloud providers (legacy cloud provider and the one used in CSI driver) easier See @jsafrane 's suggestion here 3) keep the CSI specific error handling in CSI controller

[bertinatto]

I'm thinking how about we move the logic of:

if isAWSErrorIncorrectState(err) ||
			isAWSErrorInvalidAttachmentNotFound(err) ||
			isAWSErrorVolumeNotFound(err) {
			return ErrNotFound
		}

To controller.go ControllerUnpublishVolume ?
This has several advantages that: 1) keep the cloud provider simpler and cleaner 2) align with current cloud provider implementation so that we can unify the two cloud providers (legacy cloud provider and the one used in CSI driver) easier See @jsafrane 's suggestion here 3) keep the CSI specific error handling in CSI controller

A drawback would be that the controller will have to understand AWS-specific error codes.

As an alternative, we could implement a helper function in the cloud package (be it the legacy one or this one) to evaluate whether the error means "OK" in an idempotent context. This method would then be used in the CSI driver only, not in the in-tree plugin.

In any case, whatever the solution we choose, we should implement it equally across all the other methods in the cloud package. Unfortunately I don't have the cycles for that at the moment.

[leakingtapan]

k. I can leave with the current change for now. We will reconsider this when unifying legacy cloud provider here

[AtkinsChang]

@leakingtapan
any progress on this pr?
This may address the volume state bug when node failure.
If aws instant is terminated, the volume state will transit to available causing following error:

rpc error: code = Internal desc = Could not detach volume "vol-xxxxxxxxxxxxxxxxx" from node "i-xxxxxxxxxxxxxxxxx": could not detach volume "vol-xxxxxxxxxxxxxxxxx" from node "i-xxxxxxxxxxxxxxxxx": IncorrectState: Volume 'vol-xxxxxxxxxxxxxxxxx ' is in the 'available' state.
status code: 400, request id: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx

[leakingtapan]

@bertinatto could you look at my comment about this fix?

[leakingtapan]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, leakingtapan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [bertinatto,leakingtapan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[leakingtapan]

/test all