From 72c56e2b1592b22e719ae60d8ad0f9e7c0a6efd6 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Mon, 12 Aug 2019 12:26:43 +0200 Subject: [PATCH 1/2] Fixed ControllerUnpublish error handling Any error from ControllerUnpublish can mean that a volume could be still attached (or being detached). --- pkg/attacher/attacher.go | 16 ++---- pkg/attacher/attacher_test.go | 91 +++++++++++++++--------------- pkg/controller/csi_handler.go | 12 ++-- pkg/controller/csi_handler_test.go | 36 +++++------- pkg/controller/framework_test.go | 8 +-- 5 files changed, 73 insertions(+), 90 deletions(-) diff --git a/pkg/attacher/attacher.go b/pkg/attacher/attacher.go index 555fe41e8..bdf3f4394 100644 --- a/pkg/attacher/attacher.go +++ b/pkg/attacher/attacher.go @@ -37,11 +37,8 @@ type Attacher interface { // status. Attach(ctx context.Context, volumeID string, readOnly bool, nodeID string, caps *csi.VolumeCapability, attributes, secrets map[string]string) (metadata map[string]string, detached bool, err error) - // Detach given volume from given node. Note that "detached" is returned on - // error and means that the volume is for sure detached from the node. - // "false" means that the volume may or may not be detached and caller - // should retry. - Detach(ctx context.Context, volumeID string, nodeID string, secrets map[string]string) (detached bool, err error) + // Detach given volume from given node. + Detach(ctx context.Context, volumeID string, nodeID string, secrets map[string]string) error } type attacher struct { @@ -79,7 +76,7 @@ func (a *attacher) Attach(ctx context.Context, volumeID string, readOnly bool, n return rsp.PublishContext, false, nil } -func (a *attacher) Detach(ctx context.Context, volumeID string, nodeID string, secrets map[string]string) (detached bool, err error) { +func (a *attacher) Detach(ctx context.Context, volumeID string, nodeID string, secrets map[string]string) error { client := csi.NewControllerClient(a.conn) req := csi.ControllerUnpublishVolumeRequest{ @@ -88,11 +85,8 @@ func (a *attacher) Detach(ctx context.Context, volumeID string, nodeID string, s Secrets: secrets, } - _, err = client.ControllerUnpublishVolume(ctx, &req) - if err != nil { - return isFinalError(err), err - } - return true, nil + _, err := client.ControllerUnpublishVolume(ctx, &req) + return err } func logGRPC(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { diff --git a/pkg/attacher/attacher_test.go b/pkg/attacher/attacher_test.go index 06d6eb14e..7c014a3a6 100644 --- a/pkg/attacher/attacher_test.go +++ b/pkg/attacher/attacher_test.go @@ -291,54 +291,60 @@ func TestDetachAttach(t *testing.T) { } tests := []struct { - name string - volumeID string - nodeID string - secrets map[string]string - input *csi.ControllerUnpublishVolumeRequest - output *csi.ControllerUnpublishVolumeResponse - injectError codes.Code - expectError bool - expectDetached bool + name string + volumeID string + nodeID string + secrets map[string]string + input *csi.ControllerUnpublishVolumeRequest + output *csi.ControllerUnpublishVolumeResponse + injectError codes.Code + expectError bool }{ { - name: "success", - volumeID: defaultVolumeID, - nodeID: defaultNodeID, - input: defaultRequest, - output: &csi.ControllerUnpublishVolumeResponse{}, - expectError: false, - expectDetached: true, + name: "success", + volumeID: defaultVolumeID, + nodeID: defaultNodeID, + input: defaultRequest, + output: &csi.ControllerUnpublishVolumeResponse{}, + expectError: false, }, { - name: "secrets", - volumeID: defaultVolumeID, - nodeID: defaultNodeID, - secrets: map[string]string{"foo": "bar"}, - input: secretsRequest, - output: &csi.ControllerUnpublishVolumeResponse{}, - expectError: false, - expectDetached: true, + name: "secrets", + volumeID: defaultVolumeID, + nodeID: defaultNodeID, + secrets: map[string]string{"foo": "bar"}, + input: secretsRequest, + output: &csi.ControllerUnpublishVolumeResponse{}, + expectError: false, }, { - name: "gRPC final error", - volumeID: defaultVolumeID, - nodeID: defaultNodeID, - input: defaultRequest, - output: nil, - injectError: codes.NotFound, - expectError: true, - expectDetached: true, + name: "gRPC final error", + volumeID: defaultVolumeID, + nodeID: defaultNodeID, + input: defaultRequest, + output: nil, + injectError: codes.PermissionDenied, + expectError: true, }, { - name: "gRPC transient error", - volumeID: defaultVolumeID, - nodeID: defaultNodeID, - input: defaultRequest, - output: nil, - injectError: codes.DeadlineExceeded, - expectError: true, - expectDetached: false, + name: "gRPC transient error", + volumeID: defaultVolumeID, + nodeID: defaultNodeID, + input: defaultRequest, + output: nil, + injectError: codes.DeadlineExceeded, + expectError: true, + }, + { + // Explicitly test NotFound, it's handled as any other error. + // https://github.com/kubernetes-csi/external-attacher/pull/165 + name: "gRPC NotFound error", + volumeID: defaultVolumeID, + nodeID: defaultNodeID, + input: defaultRequest, + output: nil, + injectError: codes.NotFound, + expectError: true, }, } @@ -365,15 +371,12 @@ func TestDetachAttach(t *testing.T) { } a := NewAttacher(csiConn) - detached, err := a.Detach(context.Background(), test.volumeID, test.nodeID, test.secrets) + err := a.Detach(context.Background(), test.volumeID, test.nodeID, test.secrets) if test.expectError && err == nil { t.Errorf("test %q: Expected error, got none", test.name) } if !test.expectError && err != nil { t.Errorf("test %q: got error: %v", test.name, err) } - if detached != test.expectDetached { - t.Errorf("test %q: expected detached=%v, got %v", test.name, test.expectDetached, detached) - } } } diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go index 21840f393..dec18782c 100644 --- a/pkg/controller/csi_handler.go +++ b/pkg/controller/csi_handler.go @@ -25,7 +25,7 @@ import ( "k8s.io/klog" "github.com/kubernetes-csi/external-attacher/pkg/attacher" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -384,17 +384,13 @@ func (h *csiHandler) csiDetach(va *storage.VolumeAttachment) (*storage.VolumeAtt ctx, cancel := context.WithTimeout(context.Background(), h.timeout) defer cancel() - detached, err := h.attacher.Detach(ctx, volumeHandle, nodeID, secrets) - if err != nil && !detached { + err = h.attacher.Detach(ctx, volumeHandle, nodeID, secrets) + if err != nil { // The volume may not be fully detached. Save the error and try again // after backoff. return va, err } - if err != nil { - klog.V(2).Infof("Detached %q with error %s", va.Name, err.Error()) - } else { - klog.V(2).Infof("Detached %q", va.Name) - } + klog.V(2).Infof("Detached %q", va.Name) if va, err := markAsDetached(h.client, va); err != nil { return va, fmt.Errorf("could not mark as detached: %s", err) diff --git a/pkg/controller/csi_handler_test.go b/pkg/controller/csi_handler_test.go index 98da47868..8ca2fa34f 100644 --- a/pkg/controller/csi_handler_test.go +++ b/pkg/controller/csi_handler_test.go @@ -24,7 +24,7 @@ import ( "github.com/kubernetes-csi/external-attacher/pkg/attacher" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" storage "k8s.io/api/storage/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -266,6 +266,7 @@ func TestCSIHandler(t *testing.T) { var success error var readWrite = false var readOnly = true + var ignored = false // the value is irrelevant for given call tests := []testCase{ // @@ -737,7 +738,7 @@ func TestCSIHandler(t *testing.T) { core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(va(false /*attached*/, "", ann))), }, expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, detached, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, ignored, noMetadata, 0}, }, }, { @@ -748,7 +749,7 @@ func TestCSIHandler(t *testing.T) { core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(vaWithInlineSpec(va(false /*attached*/, "", ann)))), }, expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, detached, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, ignored, noMetadata, 0}, }, }, { @@ -760,7 +761,7 @@ func TestCSIHandler(t *testing.T) { core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(va(false /*attached*/, "", ann))), }, expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{"foo": "bar"}, readWrite, success, detached, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{"foo": "bar"}, readWrite, success, ignored, noMetadata, 0}, }, }, { @@ -772,7 +773,7 @@ func TestCSIHandler(t *testing.T) { core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(false /*attached*/, "", ann)), "secret"))), }, expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{"foo": "bar"}, readWrite, success, detached, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{"foo": "bar"}, readWrite, success, ignored, noMetadata, 0}, }, }, { @@ -784,7 +785,7 @@ func TestCSIHandler(t *testing.T) { core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(va(false /*attached*/, "", ann))), }, expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{}, readWrite, success, detached, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{}, readWrite, success, ignored, noMetadata, 0}, }, }, { @@ -796,7 +797,7 @@ func TestCSIHandler(t *testing.T) { core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(vaInlineSpecWithSecret(vaWithInlineSpec(va(false /*attached*/, "", ann)), "emptySecret"))), }, expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{}, readWrite, success, detached, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, map[string]string{}, readWrite, success, ignored, noMetadata, 0}, }, }, { @@ -810,7 +811,7 @@ func TestCSIHandler(t *testing.T) { expectedCSICalls: []csiCall{}, }, { - name: "CSI detach fails with transient error -> controller retries", + name: "CSI detach fails with an error -> controller retries", initialObjects: []runtime.Object{pvWithFinalizer(), node()}, addedVA: deleted(va(true, fin, ann)), expectedActions: []core.Action{ @@ -818,8 +819,8 @@ func TestCSIHandler(t *testing.T) { core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(va(false /*attached*/, "", ann))), }, expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, fmt.Errorf("mock error"), notDetached, noMetadata, 0}, - {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, detached, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, fmt.Errorf("mock error"), ignored, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, ignored, noMetadata, 0}, }, }, { @@ -830,19 +831,8 @@ func TestCSIHandler(t *testing.T) { core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(va(false /*attached*/, "", ann))), }, expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, detached, noMetadata, 500 * time.Millisecond}, - {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, detached, noMetadata, time.Duration(0)}, - }, - }, - { - name: "CSI detach fails with final error -> controller does not retry", - initialObjects: []runtime.Object{pvWithFinalizer(), node()}, - addedVA: deleted(va(true, fin, ann)), - expectedActions: []core.Action{ - core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, deleted(va(false /*attached*/, "", ann))), - }, - expectedCSICalls: []csiCall{ - {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, fmt.Errorf("mock error"), detached, noMetadata, 0}, + {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, ignored, noMetadata, 500 * time.Millisecond}, + {"detach", testVolumeHandle, testNodeID, noAttrs, noSecrets, readWrite, success, ignored, noMetadata, time.Duration(0)}, }, }, { diff --git a/pkg/controller/framework_test.go b/pkg/controller/framework_test.go index ffba6c375..3294b6d7a 100644 --- a/pkg/controller/framework_test.go +++ b/pkg/controller/framework_test.go @@ -423,10 +423,10 @@ func (f *fakeCSIConnection) Attach(ctx context.Context, volumeID string, readOnl return call.metadata, call.detached, call.err } -func (f *fakeCSIConnection) Detach(ctx context.Context, volumeID string, nodeID string, secrets map[string]string) (bool, error) { +func (f *fakeCSIConnection) Detach(ctx context.Context, volumeID string, nodeID string, secrets map[string]string) error { if f.index >= len(f.calls) { f.t.Errorf("Unexpected CSI Detach call: volume=%s, node=%s, index: %d, calls: %+v", volumeID, nodeID, f.index, f.calls) - return true, fmt.Errorf("unexpected call") + return fmt.Errorf("unexpected call") } call := f.calls[f.index] f.index++ @@ -457,9 +457,9 @@ func (f *fakeCSIConnection) Detach(ctx context.Context, volumeID string, nodeID } if err != nil { - return true, err + return err } - return call.detached, call.err + return call.err } func (f *fakeCSIConnection) Close() error { From 4441c582c35246fc0863181cb2c2f767169aa724 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Mon, 12 Aug 2019 12:40:17 +0200 Subject: [PATCH 2/2] Ignore NotFound errors form ControllerUnpublish The attacher should retry ControllerUnpublish after error. At the same time, we do not want to change behavior of NotFound handling in stable branches, so treat it as no error. --- pkg/attacher/attacher.go | 15 +++++++++++++++ pkg/attacher/attacher_test.go | 5 ++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/pkg/attacher/attacher.go b/pkg/attacher/attacher.go index bdf3f4394..6eb00e22d 100644 --- a/pkg/attacher/attacher.go +++ b/pkg/attacher/attacher.go @@ -86,6 +86,12 @@ func (a *attacher) Detach(ctx context.Context, volumeID string, nodeID string, s } _, err := client.ControllerUnpublishVolume(ctx, &req) + if err != nil && isNotFound(err) { + // Do not change behavior of NotFound in stable branches. + // See https://github.com/kubernetes-csi/external-attacher/pull/165 and + // https://github.com/container-storage-interface/spec/pull/375 + return nil + } return err } @@ -125,3 +131,12 @@ func isFinalError(err error) bool { // even start or failed. It is for sure not in progress. return true } + +func isNotFound(err error) bool { + st, ok := status.FromError(err) + if !ok { + // This is not gRPC error. + return false + } + return st.Code() == codes.NotFound +} diff --git a/pkg/attacher/attacher_test.go b/pkg/attacher/attacher_test.go index 7c014a3a6..c453abe94 100644 --- a/pkg/attacher/attacher_test.go +++ b/pkg/attacher/attacher_test.go @@ -336,15 +336,14 @@ func TestDetachAttach(t *testing.T) { expectError: true, }, { - // Explicitly test NotFound, it's handled as any other error. - // https://github.com/kubernetes-csi/external-attacher/pull/165 + // Explicitly test NotFound, it should be ignored. name: "gRPC NotFound error", volumeID: defaultVolumeID, nodeID: defaultNodeID, input: defaultRequest, output: nil, injectError: codes.NotFound, - expectError: true, + expectError: false, }, }