Skip to content

[cinder-csi-plugin] Validate volume status before attach #3129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hemna wants to merge 1 commit into
base: master
Choose a base branch
from
+505 −9
Open

[cinder-csi-plugin] Validate volume status before attach #3129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 40 additions & 9 deletions pkg/csi/cinder/openstack/openstack_volumes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,42 +202,73 @@ 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"`

@kayrus kayrus Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that migration_statusmakes sense here since the CSI plugin typically runs with project admin (not cloud admin) privileges, and this field might not be accessible. If this is needed, please first add MigrationStatus *string to gophercloud's Volume struct via a PR upstream, then we can use it here without defining a private volume struct.

}

// 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.
// Per OpenStack docs, "starting", "migrating", and "completing" all indicate
// a migration is in progress.
if volResult.MigrationStatus != nil {
switch *volResult.MigrationStatus {
case "starting", "migrating", "completing":

@kayrus kayrus Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a slice of desired statuses into a global var along with slices.Contains

return "", fmt.Errorf("volume %s has migration_status %q, volume must not be migrating before attach", volumeID, *volResult.MigrationStatus)
}
}

if volResult.Multiattach {

@yahor-kurachkin yahor-kurachkin Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor observation. if volResult.Multiattach here and then a few lines further down there's a second if volResult.Multiattach. Would it make sense to merge them into a single if/else block?

@hemna hemna Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Merged both if volResult.Multiattach blocks into a single if/else that handles status validation and microversion setup together.

if volResult.Status != VolumeAvailableStatus && volResult.Status != VolumeInUseStatus {

@kayrus kayrus Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use slices.Contains here too

return "", fmt.Errorf("volume %s is in %s status, volume must be available or in-use for multi-attach capable volumes", volumeID, volResult.Status)
}
// 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)
if err != nil {
return "", err
}
computeServiceClient.Microversion = "2.60"
} else {
if volResult.Status != VolumeAvailableStatus {
return "", fmt.Errorf("volume %s is in %s status, volume must be available", volumeID, volResult.Status)
}
}
Comment on lines +256 to 260

@kayrus kayrus Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rework this part with a possible early return on err to avoid complex if/else/if structure


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
Expand Down
Loading