From f8d4aa4721b7adf47c8e0cfcab1429408ff47486 Mon Sep 17 00:00:00 2001 From: Walter Boring Date: Mon, 15 Jun 2026 13:06:32 -0400 Subject: [PATCH] cinder csi: validate volume status and migration_status before attach Check that a volume is in an acceptable state before calling the Nova attach API: - For regular (single-attach) volumes: status must be 'available' - For multi-attach capable volumes: status must be 'available' or 'in-use' - migration_status must not be 'migrating' Previously the driver would blindly call volumeattach.Create regardless of volume status, relying on Nova/Cinder to reject the request. This produced opaque errors that made troubleshooting difficult and generated unnecessary API calls for volumes in transitional states (creating, downloading, detaching, etc.). The migration_status check prevents attempting to attach a volume that is actively being migrated, which would fail with an opaque error from Cinder. A custom volumeForAttach struct is used to extract migration_status from the Cinder API response, since gophercloud's Volume struct does not map this field. With this change the driver returns immediate, descriptive errors when the volume is not attachable, allowing the CO (external-attacher) to retry with exponential backoff via standard CSI error handling. --- pkg/csi/cinder/openstack/openstack_volumes.go | 47 +- .../openstack/openstack_volumes_test.go | 445 ++++++++++++++++++ 2 files changed, 483 insertions(+), 9 deletions(-) create mode 100644 pkg/csi/cinder/openstack/openstack_volumes_test.go diff --git a/pkg/csi/cinder/openstack/openstack_volumes.go b/pkg/csi/cinder/openstack/openstack_volumes.go index 41499b77cf..cd99a6ec98 100644 --- a/pkg/csi/cinder/openstack/openstack_volumes.go +++ b/pkg/csi/cinder/openstack/openstack_volumes.go @@ -202,23 +202,52 @@ func (os *OpenStack) GetVolume(ctx context.Context, volumeID string) (*volumes.V return vol, nil } +// volumeForAttach contains the volume fields needed for attach validation, +// including migration_status which gophercloud's Volume struct does not map. +type volumeForAttach struct { + ID string `json:"id"` + Status string `json:"status"` + Multiattach bool `json:"multiattach"` + Attachments []volumes.Attachment `json:"attachments"` + MigrationStatus *string `json:"migration_status"` +} + // AttachVolume attaches given cinder volume to the compute func (os *OpenStack) AttachVolume(ctx context.Context, instanceID, volumeID string) (string, error) { computeServiceClient := os.compute - volume, err := os.GetVolume(ctx, volumeID) - if err != nil { + // Fetch the volume with migration_status using a single API call. + // We use a custom struct because gophercloud's Volume does not include migration_status. + mc := metrics.NewMetricContext("volume", "get") + var volResult volumeForAttach + err := volumes.Get(ctx, os.blockstorage, volumeID).ExtractInto(&volResult) + if mc.ObserveRequest(err) != nil { return "", err } - for _, att := range volume.Attachments { + for _, att := range volResult.Attachments { if instanceID == att.ServerID { klog.V(4).Infof("Disk %s is already attached to instance %s", volumeID, instanceID) - return volume.ID, nil + return volResult.ID, nil } } - if volume.Multiattach { + // Check migration_status: if the volume is migrating, it cannot be attached. + if volResult.MigrationStatus != nil && *volResult.MigrationStatus == "migrating" { + return "", fmt.Errorf("volume %s has migration_status %q, volume must not be migrating before attach", volumeID, *volResult.MigrationStatus) + } + + if volResult.Multiattach { + if volResult.Status != VolumeAvailableStatus && volResult.Status != VolumeInUseStatus { + return "", fmt.Errorf("volume %s is in %s status, volume must be available or in-use for multi-attach capable volumes", volumeID, volResult.Status) + } + } else { + if volResult.Status != VolumeAvailableStatus { + return "", fmt.Errorf("volume %s is in %s status, volume must be available", volumeID, volResult.Status) + } + } + + if volResult.Multiattach { // For multiattach volumes, supported compute api version is 2.60 // Init a local thread safe copy of the compute ServiceClient computeServiceClient, err = openstack.NewComputeV2(os.compute.ProviderClient, os.epOpts) @@ -228,16 +257,16 @@ func (os *OpenStack) AttachVolume(ctx context.Context, instanceID, volumeID stri computeServiceClient.Microversion = "2.60" } - mc := metrics.NewMetricContext("volume", "attach") + mcAttach := metrics.NewMetricContext("volume", "attach") _, err = volumeattach.Create(ctx, computeServiceClient, instanceID, &volumeattach.CreateOpts{ - VolumeID: volume.ID, + VolumeID: volResult.ID, }).Extract() - if mc.ObserveRequest(err) != nil { + if mcAttach.ObserveRequest(err) != nil { return "", fmt.Errorf("failed to attach %s volume to %s compute: %v", volumeID, instanceID, err) } - return volume.ID, nil + return volResult.ID, nil } // WaitDiskAttached waits for attached 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..d52db698f9 --- /dev/null +++ b/pkg/csi/cinder/openstack/openstack_volumes_test.go @@ -0,0 +1,445 @@ +/* +Copyright 2024 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" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gophercloud/gophercloud/v2" + "github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes" +) + +// fakeVolumeResponse returns a JSON response body for a Cinder volume GET. +func fakeVolumeResponse(id, status string, multiattach bool, attachments []volumes.Attachment) string { + return fakeVolumeResponseWithMigration(id, status, multiattach, attachments, "") +} + +// fakeVolumeResponseWithMigration returns a JSON response body for a Cinder volume GET +// including the migration_status field. +func fakeVolumeResponseWithMigration(id, status string, multiattach bool, attachments []volumes.Attachment, migrationStatus string) string { + atts := "[]" + if len(attachments) > 0 { + b, _ := json.Marshal(attachments) + atts = string(b) + } + migField := "null" + if migrationStatus != "" { + migField = fmt.Sprintf("%q", migrationStatus) + } + return fmt.Sprintf(`{ + "volume": { + "id": %q, + "status": %q, + "multiattach": %t, + "attachments": %s, + "migration_status": %s, + "size": 1, + "availability_zone": "nova" + } + }`, id, status, multiattach, atts, migField) +} + +// fakeAttachResponse returns a JSON response for a successful Nova volume attach. +func fakeAttachResponse(serverID, volumeID, device string) string { + return fmt.Sprintf(`{ + "volumeAttachment": { + "serverId": %q, + "volumeId": %q, + "device": %q + } + }`, serverID, volumeID, device) +} + +// newFakeOpenStack creates an OpenStack instance backed by httptest servers. +// cinderHandler handles /volumes/{id} requests. +// novaHandler handles /servers/{id}/os-volume_attachments requests. +func newFakeOpenStack(cinderHandler http.HandlerFunc, novaHandler http.HandlerFunc) (*OpenStack, *httptest.Server, *httptest.Server) { + cinderServer := httptest.NewServer(cinderHandler) + novaServer := httptest.NewServer(novaHandler) + + novaEndpoint := novaServer.URL + "/" + + providerClient := &gophercloud.ProviderClient{ + // EndpointLocator is required for openstack.NewComputeV2 used in multiattach path + EndpointLocator: func(opts gophercloud.EndpointOpts) (string, error) { + return novaEndpoint, nil + }, + } + + blockstorageClient := &gophercloud.ServiceClient{ + ProviderClient: &gophercloud.ProviderClient{}, + Endpoint: cinderServer.URL + "/", + } + + computeClient := &gophercloud.ServiceClient{ + ProviderClient: providerClient, + Endpoint: novaEndpoint, + } + + os := &OpenStack{ + compute: computeClient, + blockstorage: blockstorageClient, + epOpts: gophercloud.EndpointOpts{}, + } + + return os, cinderServer, novaServer +} + +func TestAttachVolume_AvailableStatus(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeVolumeResponse(volumeID, VolumeAvailableStatus, false, nil)) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeAttachResponse(instanceID, volumeID, "/dev/vdb")) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + result, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err != nil { + t.Fatalf("expected no error for available volume, got: %v", err) + } + if result != volumeID { + t.Errorf("expected volume ID %q, got %q", volumeID, result) + } +} + +func TestAttachVolume_NonAvailableStatus_SingleAttach(t *testing.T) { + tests := []struct { + name string + status string + }{ + {"creating", "creating"}, + {"error", "error"}, + {"in-use", VolumeInUseStatus}, + {"detaching", VolumeDetachingStatus}, + {"attaching", "attaching"}, + {"downloading", "downloading"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeVolumeResponse(volumeID, tc.status, false, nil)) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("Nova attach should not be called for non-available volume") + w.WriteHeader(http.StatusInternalServerError) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + _, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err == nil { + t.Fatalf("expected error for volume in %q status, got nil", tc.status) + } + + expectedMsg := fmt.Sprintf("volume %s is in %s status, volume must be available", volumeID, tc.status) + if err.Error() != expectedMsg { + t.Errorf("expected error message %q, got %q", expectedMsg, err.Error()) + } + }) + } +} + +func TestAttachVolume_MultiAttach_AvailableStatus(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeVolumeResponse(volumeID, VolumeAvailableStatus, true, nil)) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeAttachResponse(instanceID, volumeID, "/dev/vdb")) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + result, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err != nil { + t.Fatalf("expected no error for multiattach available volume, got: %v", err) + } + if result != volumeID { + t.Errorf("expected volume ID %q, got %q", volumeID, result) + } +} + +func TestAttachVolume_MultiAttach_InUseStatus(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + otherInstanceID := "instance-789" + + // Volume is already attached to another instance + attachments := []volumes.Attachment{ + {ServerID: otherInstanceID, VolumeID: volumeID}, + } + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeVolumeResponse(volumeID, VolumeInUseStatus, true, attachments)) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeAttachResponse(instanceID, volumeID, "/dev/vdc")) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + result, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err != nil { + t.Fatalf("expected no error for multiattach in-use volume, got: %v", err) + } + if result != volumeID { + t.Errorf("expected volume ID %q, got %q", volumeID, result) + } +} + +func TestAttachVolume_MultiAttach_InvalidStatus(t *testing.T) { + tests := []struct { + name string + status string + }{ + {"creating", "creating"}, + {"error", "error"}, + {"detaching", VolumeDetachingStatus}, + {"attaching", "attaching"}, + {"downloading", "downloading"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeVolumeResponse(volumeID, tc.status, true, nil)) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("Nova attach should not be called for volume in invalid status") + w.WriteHeader(http.StatusInternalServerError) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + _, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err == nil { + t.Fatalf("expected error for multiattach volume in %q status, got nil", tc.status) + } + + expectedMsg := fmt.Sprintf("volume %s is in %s status, volume must be available or in-use for multi-attach capable volumes", volumeID, tc.status) + if err.Error() != expectedMsg { + t.Errorf("expected error message %q, got %q", expectedMsg, err.Error()) + } + }) + } +} + +func TestAttachVolume_AlreadyAttachedToSameInstance(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + // Volume is already attached to the same instance we're trying to attach to + attachments := []volumes.Attachment{ + {ServerID: instanceID, VolumeID: volumeID}, + } + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + // Status is in-use because it's already attached + fmt.Fprint(w, fakeVolumeResponse(volumeID, VolumeInUseStatus, false, attachments)) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("Nova attach should not be called when already attached to same instance") + w.WriteHeader(http.StatusInternalServerError) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + // Should return early with the volume ID (idempotent) + result, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err != nil { + t.Fatalf("expected no error for already-attached volume, got: %v", err) + } + if result != volumeID { + t.Errorf("expected volume ID %q, got %q", volumeID, result) + } +} + +func TestAttachVolume_MigrationStatus_Migrating(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + // Volume is available but migration_status is "migrating" + fmt.Fprint(w, fakeVolumeResponseWithMigration(volumeID, VolumeAvailableStatus, false, nil, "migrating")) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("Nova attach should not be called for migrating volume") + w.WriteHeader(http.StatusInternalServerError) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + _, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err == nil { + t.Fatal("expected error for migrating volume, got nil") + } + + expectedMsg := fmt.Sprintf("volume %s has migration_status %q, volume must not be migrating before attach", volumeID, "migrating") + if err.Error() != expectedMsg { + t.Errorf("expected error message %q, got %q", expectedMsg, err.Error()) + } +} + +func TestAttachVolume_MigrationStatus_Null(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + // migration_status is null (normal case) + fmt.Fprint(w, fakeVolumeResponseWithMigration(volumeID, VolumeAvailableStatus, false, nil, "")) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeAttachResponse(instanceID, volumeID, "/dev/vdb")) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + result, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err != nil { + t.Fatalf("expected no error for volume with null migration_status, got: %v", err) + } + if result != volumeID { + t.Errorf("expected volume ID %q, got %q", volumeID, result) + } +} + +func TestAttachVolume_MigrationStatus_Success(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + // migration_status is "success" — migration complete, attach should proceed + fmt.Fprint(w, fakeVolumeResponseWithMigration(volumeID, VolumeAvailableStatus, false, nil, "success")) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeAttachResponse(instanceID, volumeID, "/dev/vdb")) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + result, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err != nil { + t.Fatalf("expected no error for volume with success migration_status, got: %v", err) + } + if result != volumeID { + t.Errorf("expected volume ID %q, got %q", volumeID, result) + } +} + +func TestAttachVolume_MigrationStatus_Error(t *testing.T) { + volumeID := "vol-123" + instanceID := "instance-456" + + cinderHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + // migration_status is "error" — migration failed, but volume is available + fmt.Fprint(w, fakeVolumeResponseWithMigration(volumeID, VolumeAvailableStatus, false, nil, "error")) + }) + + novaHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, fakeAttachResponse(instanceID, volumeID, "/dev/vdb")) + }) + + os, cinderSrv, novaSrv := newFakeOpenStack(cinderHandler, novaHandler) + defer cinderSrv.Close() + defer novaSrv.Close() + + result, err := os.AttachVolume(context.Background(), instanceID, volumeID) + if err != nil { + t.Fatalf("expected no error for volume with error migration_status, got: %v", err) + } + if result != volumeID { + t.Errorf("expected volume ID %q, got %q", volumeID, result) + } +}