diff --git a/pkg/csi/cinder/controllerserver.go b/pkg/csi/cinder/controllerserver.go index 1a1b9e2ff6..71f9325fd2 100644 --- a/pkg/csi/cinder/controllerserver.go +++ b/pkg/csi/cinder/controllerserver.go @@ -332,6 +332,9 @@ func (cs *controllerServer) ControllerPublishVolume(ctx context.Context, req *cs _, err = cloud.AttachVolume(ctx, instanceID, volumeID) if err != nil { klog.Errorf("Failed to AttachVolume: %v", err) + if s, ok := status.FromError(err); ok { + return nil, status.Error(s.Code(), fmt.Sprintf("[ControllerPublishVolume] Attach Volume failed with error %v", err)) + } return nil, status.Errorf(codes.Internal, "[ControllerPublishVolume] Attach Volume failed with error %v", err) } diff --git a/pkg/csi/cinder/controllerserver_test.go b/pkg/csi/cinder/controllerserver_test.go index df608d1b2c..771dfbc650 100644 --- a/pkg/csi/cinder/controllerserver_test.go +++ b/pkg/csi/cinder/controllerserver_test.go @@ -564,6 +564,28 @@ func TestControllerPublishVolume(t *testing.T) { assert.Equal(expectedRes, actualRes) } +func TestControllerPublishVolumePreservesFailedPrecondition(t *testing.T) { + fakeCs, osmock := fakeControllerServer() + + osmock.On("AttachVolume", FakeNodeID, FakeVolID).Return("", status.Error(codes.FailedPrecondition, "volume is already attached to another node")) + + fakeReq := &csi.ControllerPublishVolumeRequest{ + VolumeId: FakeVolID, + NodeId: FakeNodeID, + VolumeCapability: &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + }, + Readonly: false, + } + + actualRes, err := fakeCs.ControllerPublishVolume(FakeCtx, fakeReq) + + assert.Nil(t, actualRes) + assert.Equal(t, codes.FailedPrecondition, status.Code(err)) +} + // Test ControllerUnpublishVolume func TestControllerUnpublishVolume(t *testing.T) { fakeCs, osmock := fakeControllerServer() diff --git a/pkg/csi/cinder/openstack/openstack_volumes.go b/pkg/csi/cinder/openstack/openstack_volumes.go index 41499b77cf..7fb11a146d 100644 --- a/pkg/csi/cinder/openstack/openstack_volumes.go +++ b/pkg/csi/cinder/openstack/openstack_volumes.go @@ -218,6 +218,10 @@ func (os *OpenStack) AttachVolume(ctx context.Context, instanceID, volumeID stri } } + if len(volume.Attachments) > 0 && !volume.Multiattach { + return "", status.Errorf(codes.FailedPrecondition, "volume %s is already attached to node %s", volumeID, volume.Attachments[0].ServerID) + } + if volume.Multiattach { // For multiattach volumes, supported compute api version is 2.60 // Init a local thread safe copy of the compute ServiceClient diff --git a/pkg/csi/cinder/openstack/openstack_volumes_test.go b/pkg/csi/cinder/openstack/openstack_volumes_test.go new file mode 100644 index 0000000000..4cd18221b1 --- /dev/null +++ b/pkg/csi/cinder/openstack/openstack_volumes_test.go @@ -0,0 +1,83 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package openstack + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gophercloud/gophercloud/v2" + "github.com/stretchr/testify/assert" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func TestAttachVolumeRejectsNonMultiattachVolumeAttachedToAnotherInstance(t *testing.T) { + const ( + volumeID = "volume-id" + oldInstanceID = "old-instance-id" + newInstanceID = "new-instance-id" + attachmentID = "attachment-id" + volumeEndpoint = "/volumes/" + volumeID + ) + + computeAttachCalled := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, volumeEndpoint): + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{ + "volume": { + "id": "` + volumeID + `", + "status": "in-use", + "multiattach": false, + "attachments": [{ + "attachment_id": "` + attachmentID + `", + "id": "` + volumeID + `", + "server_id": "` + oldInstanceID + `", + "volume_id": "` + volumeID + `" + }] + } + }`)) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/os-volume_attachments"): + computeAttachCalled = true + w.WriteHeader(http.StatusAccepted) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + })) + defer server.Close() + + client := &gophercloud.ServiceClient{ + ProviderClient: &gophercloud.ProviderClient{}, + Endpoint: server.URL + "/", + } + os := &OpenStack{ + blockstorage: client, + compute: client, + } + + _, err := os.AttachVolume(context.Background(), newInstanceID, volumeID) + + assert.Equal(t, codes.FailedPrecondition, status.Code(err)) + assert.Contains(t, err.Error(), oldInstanceID) + assert.False(t, computeAttachCalled) +}