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 a397ef108..c79d94a97 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" @@ -393,17 +393,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 {