Skip to content

Commit 061e4ce

Browse files
committed
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.
1 parent d1cc4cb commit 061e4ce

2 files changed

Lines changed: 505 additions & 9 deletions

File tree

pkg/csi/cinder/openstack/openstack_volumes.go

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -202,42 +202,73 @@ func (os *OpenStack) GetVolume(ctx context.Context, volumeID string) (*volumes.V
202202
return vol, nil
203203
}
204204

205+
// volumeForAttach contains the volume fields needed for attach validation,
206+
// including migration_status which gophercloud's Volume struct does not map.
207+
type volumeForAttach struct {
208+
ID string `json:"id"`
209+
Status string `json:"status"`
210+
Multiattach bool `json:"multiattach"`
211+
Attachments []volumes.Attachment `json:"attachments"`
212+
MigrationStatus *string `json:"migration_status"`
213+
}
214+
205215
// AttachVolume attaches given cinder volume to the compute
206216
func (os *OpenStack) AttachVolume(ctx context.Context, instanceID, volumeID string) (string, error) {
207217
computeServiceClient := os.compute
208218

209-
volume, err := os.GetVolume(ctx, volumeID)
210-
if err != nil {
219+
// Fetch the volume with migration_status using a single API call.
220+
// We use a custom struct because gophercloud's Volume does not include migration_status.
221+
mc := metrics.NewMetricContext("volume", "get")
222+
var volResult volumeForAttach
223+
err := volumes.Get(ctx, os.blockstorage, volumeID).ExtractInto(&volResult)
224+
if mc.ObserveRequest(err) != nil {
211225
return "", err
212226
}
213227

214-
for _, att := range volume.Attachments {
228+
for _, att := range volResult.Attachments {
215229
if instanceID == att.ServerID {
216230
klog.V(4).Infof("Disk %s is already attached to instance %s", volumeID, instanceID)
217-
return volume.ID, nil
231+
return volResult.ID, nil
218232
}
219233
}
220234

221-
if volume.Multiattach {
235+
// Check migration_status: if the volume is migrating, it cannot be attached.
236+
// Per OpenStack docs, "starting", "migrating", and "completing" all indicate
237+
// a migration is in progress.
238+
if volResult.MigrationStatus != nil {
239+
switch *volResult.MigrationStatus {
240+
case "starting", "migrating", "completing":
241+
return "", fmt.Errorf("volume %s has migration_status %q, volume must not be migrating before attach", volumeID, *volResult.MigrationStatus)
242+
}
243+
}
244+
245+
if volResult.Multiattach {
246+
if volResult.Status != VolumeAvailableStatus && volResult.Status != VolumeInUseStatus {
247+
return "", fmt.Errorf("volume %s is in %s status, volume must be available or in-use for multi-attach capable volumes", volumeID, volResult.Status)
248+
}
222249
// For multiattach volumes, supported compute api version is 2.60
223250
// Init a local thread safe copy of the compute ServiceClient
224251
computeServiceClient, err = openstack.NewComputeV2(os.compute.ProviderClient, os.epOpts)
225252
if err != nil {
226253
return "", err
227254
}
228255
computeServiceClient.Microversion = "2.60"
256+
} else {
257+
if volResult.Status != VolumeAvailableStatus {
258+
return "", fmt.Errorf("volume %s is in %s status, volume must be available", volumeID, volResult.Status)
259+
}
229260
}
230261

231-
mc := metrics.NewMetricContext("volume", "attach")
262+
mcAttach := metrics.NewMetricContext("volume", "attach")
232263
_, err = volumeattach.Create(ctx, computeServiceClient, instanceID, &volumeattach.CreateOpts{
233-
VolumeID: volume.ID,
264+
VolumeID: volResult.ID,
234265
}).Extract()
235266

236-
if mc.ObserveRequest(err) != nil {
267+
if mcAttach.ObserveRequest(err) != nil {
237268
return "", fmt.Errorf("failed to attach %s volume to %s compute: %v", volumeID, instanceID, err)
238269
}
239270

240-
return volume.ID, nil
271+
return volResult.ID, nil
241272
}
242273

243274
// WaitDiskAttached waits for attached

0 commit comments

Comments
 (0)