Skip to content
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.

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

[cinder-csi-plugin] define availability zone for snapshot backup #2569

Merged
merged 3 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
15 changes: 11 additions & 4 deletions pkg/csi/cinder/controllerserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
// In case a snapshot is not found
// check if a Backup with the same ID exists
if backupsAreEnabled && cpoerrors.IsNotFound(err) {
back, err := cloud.GetBackupByID(snapshotID)
var back *backups.Backup
back, err = cloud.GetBackupByID(snapshotID)
zetaab marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
//If there is an error getting the backup as well, fail.
return nil, status.Errorf(codes.NotFound, "VolumeContentSource Snapshot or Backup with ID %s not found", snapshotID)
Expand All @@ -154,7 +155,6 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
if cpoerrors.IsNotFound(err) && snapshotID == "" {
return nil, err
}

}

if content != nil && content.GetVolume() != nil {
Expand Down Expand Up @@ -420,10 +420,17 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
}

if len(backups) == 1 {
backup = &backups[0]
// since backup.VolumeID is not part of ListBackups response
// we need fetch single backup to get the full object.
backup, err = cs.Cloud.GetBackupByID(backups[0].ID)
Copy link
Member Author

Choose a reason for hiding this comment

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

ListBackups filters by Name and the response is https://docs.openstack.org/api-ref/block-storage/v3/index.html#list-backups-for-project

There is another list backups api that will response all of the details https://docs.openstack.org/api-ref/block-storage/v3/index.html#list-backups-with-detail However, that will not support filter by Name. So I think fetching this single backup by id is enough for now.

if err != nil {
klog.Errorf("Failed to get backup by ID %s: %v", backup.ID, err)
return nil, status.Error(codes.Internal, "Failed to get backup by ID")
}

// Verify the existing backup has the same VolumeID, otherwise it belongs to another volume
if backup.VolumeID != volumeID {
klog.Errorf("found existing backup for volumeID (%s) but different source volume ID (%s)", volumeID, backup.VolumeID)
return nil, status.Error(codes.AlreadyExists, "Backup with given name already exists, with different source volume ID")
}

Expand Down Expand Up @@ -503,7 +510,7 @@ func (cs *controllerServer) CreateSnapshot(ctx context.Context, req *csi.CreateS
return nil, status.Error(codes.Internal, fmt.Sprintf("GetBackupByID failed with error %v", err))
}

err = cs.Cloud.DeleteSnapshot(snap.ID)
err = cs.Cloud.DeleteSnapshot(backup.SnapshotID)
if err != nil && !cpoerrors.IsNotFound(err) {
klog.Errorf("Failed to DeleteSnapshot: %v", err)
return nil, status.Error(codes.Internal, fmt.Sprintf("DeleteSnapshot failed with error %v", err))
Expand Down
21 changes: 8 additions & 13 deletions pkg/csi/cinder/openstack/openstack_backups.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,23 +63,18 @@ func (os *OpenStack) CreateBackup(name, volID, snapshotID, availabilityZone stri
}

opts := &backups.CreateOpts{
VolumeID: volID,
SnapshotID: snapshotID,
Name: name,
Force: force,
Description: backupDescription,
VolumeID: volID,
SnapshotID: snapshotID,
Name: name,
Force: force,
Description: backupDescription,
AvailabilityZone: availabilityZone,
}

if tags != nil {
// Set openstack microversion to 3.43 to send metadata along with the backup
blockstorageServiceClient.Microversion = "3.43"
opts.Metadata = tags
}

if availabilityZone != "" {
// 3.51 is minimum if az is defined for the request
// Set openstack microversion to 3.51 to send metadata along with the backup
blockstorageServiceClient.Microversion = "3.51"
opts.AvailabilityZone = availabilityZone
opts.Metadata = tags
}

// TODO: Do some check before really call openstack API on the input
Expand Down
4 changes: 2 additions & 2 deletions pkg/csi/cinder/openstack/openstack_volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ func (os *OpenStack) CreateVolume(name string, size int, vtype, availability str
return nil, err
}

// creating volumes from backups is available since 3.47 microversion
// creating volumes from backups and backups cross-az is available since 3.51 microversion
// https://docs.openstack.org/cinder/latest/contributor/api_microversion_history.html#id47
if !os.bsOpts.IgnoreVolumeMicroversion && sourceBackupID != "" {
blockstorageClient.Microversion = "3.47"
blockstorageClient.Microversion = "3.51"
zetaab marked this conversation as resolved.
Show resolved Hide resolved
}

mc := metrics.NewMetricContext("volume", "create")
Expand Down