modify error message when request volume is in use with other node #698

AndyXiangLi · 2021-01-14T17:34:38Z

Is this a bug fix or adding new feature?
Fixes #515
What is this PR about? / Why do we need it?
According to https://github.com/container-storage-interface/spec/blob/master/spec.md#controllerpublishvolume-errors
We are returning wrong error code and error message when volume already assigned to different node
TODO: add logic to handle ALREADY_EXISTS scenario
What testing is done?
Sanity test passed
Added unit tests

k8s-ci-robot · 2021-01-14T17:34:46Z

Hi @AndyXiangLi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

coveralls · 2021-01-14T18:16:14Z

Pull Request Test Coverage Report for Build 1561

13 of 14 (92.86%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.09%) to 81.286%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/cloud/cloud.go	9	10	90.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/driver/controller.go	1	90.49%

Totals
Change from base Build 1558:	0.09%
Covered Lines:	1707
Relevant Lines:	2100

💛 - Coveralls

ayberk · 2021-01-15T18:51:55Z

/ok-to-test

ayberk · 2021-01-15T19:07:25Z

pkg/cloud/cloud.go

@@ -115,6 +115,9 @@ var (
 	// ErrAlreadyExists is returned when a resource is already existent.
 	ErrAlreadyExists = errors.New("Resource already exists")

+	// ErrAlreadyExists is returned when a resource is already existent.


Hehe a copy-paste artifact here I see :)

ayberk · 2021-01-15T19:08:31Z

pkg/cloud/cloud.go

@@ -115,6 +115,9 @@ var (
 	// ErrAlreadyExists is returned when a resource is already existent.
 	ErrAlreadyExists = errors.New("Resource already exists")


Do we not use this anymore?

Ok I see the TODO below.

ayberk · 2021-01-15T19:11:29Z

pkg/cloud/cloud.go

+
+func getVolumeAttachmentsList(volume *ec2.Volume) []string {
+	var volumeAttachmentList []string
+	if len(volume.Attachments) > 0 {


Do you need this check?

You are right, we actually not need this.

ayberk · 2021-01-15T19:21:48Z

pkg/driver/controller.go

-		if err == cloud.ErrAlreadyExists {
-			return nil, status.Error(codes.AlreadyExists, err.Error())
+		if err == cloud.ErrVolumeInUse {
+			return nil, status.Error(codes.FailedPrecondition, strings.Join(disk.Attachments, ","))


I don't think this is what we want. Looking at the API documentation[1][2], this list will have information about previous attachments as well, but the status will be "detached". So I think what we want to do is filter for "attached".

Yeah, this is a good point, will update

ayberk · 2021-01-15T19:23:08Z

pkg/driver/controller_test.go

@@ -2697,7 +2697,7 @@ func TestControllerPublishVolume(t *testing.T) {
 					if !ok {
 						t.Fatalf("Could not get error status code from error: %v", srvErr)
 					}
-					if srvErr.Code() != codes.AlreadyExists {
+					if srvErr.Code() != codes.FailedPrecondition {


Can we also check if we're returning the correct instance id in the error message? It's a requirement from the spec.

ayberk · 2021-01-28T02:07:57Z

pkg/cloud/cloud.go

+func getVolumeAttachmentsList(volume *ec2.Volume) []string {
+	var volumeAttachmentList []string
+	for _, attachment := range volume.Attachments {
+		if attachment.State != nil && aws.StringValue(attachment.State) == "attached" {


Can we lowercase aws.StringValue(attachment.State) just to be safe?

ayberk · 2021-02-06T00:29:17Z

/approve
/lgtm

k8s-ci-robot · 2021-02-06T00:29:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndyXiangLi, ayberk

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~~ [ayberk]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 14, 2021

k8s-ci-robot requested review from ddebroy and jsafrane January 14, 2021 17:34

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 14, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 15, 2021

ayberk reviewed Jan 15, 2021

View reviewed changes

AndyXiangLi force-pushed the attach-volume-error-code branch from 4805335 to 8f68b67 Compare January 15, 2021 23:10

ayberk reviewed Jan 28, 2021

View reviewed changes

AndyXiangLi force-pushed the attach-volume-error-code branch from 8f68b67 to 22d1383 Compare February 5, 2021 21:25

modify error message when request volume is in use with other node

81f4bb1

AndyXiangLi force-pushed the attach-volume-error-code branch from 22d1383 to 81f4bb1 Compare February 5, 2021 23:54

k8s-ci-robot assigned ayberk Feb 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2021

k8s-ci-robot merged commit 5af60ea into kubernetes-sigs:master Feb 6, 2021

AndyXiangLi mentioned this pull request Mar 2, 2021

REQUEST: New membership for AndyXiangLi kubernetes/org#2534

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modify error message when request volume is in use with other node #698

modify error message when request volume is in use with other node #698

AndyXiangLi commented Jan 14, 2021

k8s-ci-robot commented Jan 14, 2021

coveralls commented Jan 14, 2021 •

edited

Loading

ayberk commented Jan 15, 2021

ayberk Jan 15, 2021

ayberk Jan 15, 2021

ayberk Jan 15, 2021

ayberk Jan 15, 2021

AndyXiangLi Jan 15, 2021

ayberk Jan 15, 2021

AndyXiangLi Jan 15, 2021

ayberk Jan 15, 2021

ayberk Jan 28, 2021

ayberk commented Feb 6, 2021

k8s-ci-robot commented Feb 6, 2021

		@@ -115,6 +115,9 @@ var (
		// ErrAlreadyExists is returned when a resource is already existent.
		ErrAlreadyExists = errors.New("Resource already exists")

modify error message when request volume is in use with other node #698

modify error message when request volume is in use with other node #698

Conversation

AndyXiangLi commented Jan 14, 2021

k8s-ci-robot commented Jan 14, 2021

coveralls commented Jan 14, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1561

💛 - Coveralls

ayberk commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayberk commented Feb 6, 2021

k8s-ci-robot commented Feb 6, 2021

coveralls commented Jan 14, 2021 •

edited

Loading