From b343c1ba6d3a4cd9aa1fa8b68a5b1fada90ca6cd Mon Sep 17 00:00:00 2001 From: Jesse Haka Date: Wed, 24 Apr 2024 14:52:54 +0300 Subject: [PATCH] [cinder-csi-plugin] define availability zone for snapshot backup (#2569) * define availability zone for snapshot backup * fix to volume backup & restore creation * add doc --- .../using-cinder-csi-plugin.md | 1 + pkg/csi/cinder/controllerserver.go | 17 ++++++++++++----- pkg/csi/cinder/openstack/openstack.go | 2 +- pkg/csi/cinder/openstack/openstack_backups.go | 17 +++++++++-------- pkg/csi/cinder/openstack/openstack_mock.go | 12 ++++++------ pkg/csi/cinder/openstack/openstack_snapshots.go | 7 ++++--- pkg/csi/cinder/openstack/openstack_volumes.go | 4 ++-- tests/sanity/cinder/fakecloud.go | 15 ++++++++------- 8 files changed, 43 insertions(+), 32 deletions(-) diff --git a/docs/cinder-csi-plugin/using-cinder-csi-plugin.md b/docs/cinder-csi-plugin/using-cinder-csi-plugin.md index 20638ea8a0..210d26a2c4 100644 --- a/docs/cinder-csi-plugin/using-cinder-csi-plugin.md +++ b/docs/cinder-csi-plugin/using-cinder-csi-plugin.md @@ -269,6 +269,7 @@ helm install --namespace kube-system --name cinder-csi ./charts/cinder-csi-plugi | VolumeSnapshotClass `parameters` | `force-create` | `false` | Enable to support creating snapshot for a volume in in-use status | | VolumeSnapshotClass `parameters` | `type` | Empty String | `snapshot` creates a VolumeSnapshot object linked to a Cinder volume snapshot. `backup` creates a VolumeSnapshot object linked to a cinder volume backup. Defaults to `snapshot` if not defined | | VolumeSnapshotClass `parameters` | `backup-max-duration-seconds-per-gb` | `20` | Defines the amount of time to wait for a backup to complete in seconds per GB of volume size | +| VolumeSnapshotClass `parameters` | `availability` | Same as volume | String. Backup Availability Zone | | Inline Volume `volumeAttributes` | `capacity` | `1Gi` | volume size for creating inline volumes| | Inline Volume `VolumeAttributes` | `type` | Empty String | Name/ID of Volume type. Corresponding volume type should exist in cinder | diff --git a/pkg/csi/cinder/controllerserver.go b/pkg/csi/cinder/controllerserver.go index 4768705ab8..fcf7aacbd7 100644 --- a/pkg/csi/cinder/controllerserver.go +++ b/pkg/csi/cinder/controllerserver.go @@ -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) 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) @@ -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 { @@ -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) + 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") } @@ -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)) @@ -593,7 +600,7 @@ func (cs *controllerServer) createBackup(name string, volumeID string, snap *sna } } - backup, err := cs.Cloud.CreateBackup(name, volumeID, snap.ID, properties) + backup, err := cs.Cloud.CreateBackup(name, volumeID, snap.ID, parameters[openstack.SnapshotAvailabilityZone], properties) if err != nil { klog.Errorf("Failed to Create backup: %v", err) return nil, status.Error(codes.Internal, fmt.Sprintf("CreateBackup failed with error %v", err)) diff --git a/pkg/csi/cinder/openstack/openstack.go b/pkg/csi/cinder/openstack/openstack.go index 4782e43545..ee603bae74 100644 --- a/pkg/csi/cinder/openstack/openstack.go +++ b/pkg/csi/cinder/openstack/openstack.go @@ -61,7 +61,7 @@ type IOpenStack interface { DeleteSnapshot(snapID string) error GetSnapshotByID(snapshotID string) (*snapshots.Snapshot, error) WaitSnapshotReady(snapshotID string) (string, error) - CreateBackup(name, volID string, snapshotID string, tags map[string]string) (*backups.Backup, error) + CreateBackup(name, volID, snapshotID, availabilityZone string, tags map[string]string) (*backups.Backup, error) ListBackups(filters map[string]string) ([]backups.Backup, error) DeleteBackup(backupID string) error GetBackupByID(backupID string) (*backups.Backup, error) diff --git a/pkg/csi/cinder/openstack/openstack_backups.go b/pkg/csi/cinder/openstack/openstack_backups.go index 8ddc05cff0..ddc98cc624 100644 --- a/pkg/csi/cinder/openstack/openstack_backups.go +++ b/pkg/csi/cinder/openstack/openstack_backups.go @@ -44,7 +44,7 @@ const ( // CreateBackup issues a request to create a Backup from the specified Snapshot with the corresponding ID and // returns the resultant gophercloud Backup Item upon success. -func (os *OpenStack) CreateBackup(name, volID string, snapshotID string, tags map[string]string) (*backups.Backup, error) { +func (os *OpenStack) CreateBackup(name, volID, snapshotID, availabilityZone string, tags map[string]string) (*backups.Backup, error) { blockstorageServiceClient, err := openstack.NewBlockStorageV3(os.blockstorage.ProviderClient, os.epOpts) if err != nil { return &backups.Backup{}, err @@ -63,16 +63,17 @@ func (os *OpenStack) CreateBackup(name, volID string, snapshotID string, tags ma } 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" + // Set openstack microversion to 3.51 to send metadata along with the backup + blockstorageServiceClient.Microversion = "3.51" opts.Metadata = tags } diff --git a/pkg/csi/cinder/openstack/openstack_mock.go b/pkg/csi/cinder/openstack/openstack_mock.go index 481e0157fc..14fa4bba09 100644 --- a/pkg/csi/cinder/openstack/openstack_mock.go +++ b/pkg/csi/cinder/openstack/openstack_mock.go @@ -315,12 +315,12 @@ func (_m *OpenStackMock) ListBackups(filters map[string]string) ([]backups.Backu return r0, r1 } -func (_m *OpenStackMock) CreateBackup(name, volID string, snapshotID string, tags map[string]string) (*backups.Backup, error) { - ret := _m.Called(name, volID, snapshotID, tags) +func (_m *OpenStackMock) CreateBackup(name, volID, snapshotID, availabilityZone string, tags map[string]string) (*backups.Backup, error) { + ret := _m.Called(name, volID, snapshotID, availabilityZone, tags) var r0 *backups.Backup - if rf, ok := ret.Get(0).(func(string, string, string, map[string]string) *backups.Backup); ok { - r0 = rf(name, volID, snapshotID, tags) + if rf, ok := ret.Get(0).(func(string, string, string, string, map[string]string) *backups.Backup); ok { + r0 = rf(name, volID, snapshotID, availabilityZone, tags) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*backups.Backup) @@ -328,8 +328,8 @@ func (_m *OpenStackMock) CreateBackup(name, volID string, snapshotID string, tag } var r1 error - if rf, ok := ret.Get(1).(func(string, string, string, map[string]string) error); ok { - r1 = rf(name, volID, snapshotID, tags) + if rf, ok := ret.Get(1).(func(string, string, string, string, map[string]string) error); ok { + r1 = rf(name, volID, snapshotID, availabilityZone, tags) } else { r1 = ret.Error(1) } diff --git a/pkg/csi/cinder/openstack/openstack_snapshots.go b/pkg/csi/cinder/openstack/openstack_snapshots.go index bd2f94365f..c3405de1eb 100644 --- a/pkg/csi/cinder/openstack/openstack_snapshots.go +++ b/pkg/csi/cinder/openstack/openstack_snapshots.go @@ -37,9 +37,10 @@ const ( snapReadyFactor = 1.2 snapReadySteps = 10 - snapshotDescription = "Created by OpenStack Cinder CSI driver" - SnapshotForceCreate = "force-create" - SnapshotType = "type" + snapshotDescription = "Created by OpenStack Cinder CSI driver" + SnapshotForceCreate = "force-create" + SnapshotType = "type" + SnapshotAvailabilityZone = "availability" ) // CreateSnapshot issues a request to take a Snapshot of the specified Volume with the corresponding ID and diff --git a/pkg/csi/cinder/openstack/openstack_volumes.go b/pkg/csi/cinder/openstack/openstack_volumes.go index 96550975d3..3da3de550d 100644 --- a/pkg/csi/cinder/openstack/openstack_volumes.go +++ b/pkg/csi/cinder/openstack/openstack_volumes.go @@ -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" } mc := metrics.NewMetricContext("volume", "create") diff --git a/tests/sanity/cinder/fakecloud.go b/tests/sanity/cinder/fakecloud.go index f04f21fc67..cff3a1bc60 100644 --- a/tests/sanity/cinder/fakecloud.go +++ b/tests/sanity/cinder/fakecloud.go @@ -227,15 +227,16 @@ func (cloud *cloud) WaitSnapshotReady(snapshotID string) (string, error) { return "available", nil } -func (cloud *cloud) CreateBackup(name, volID string, snapshotID string, tags map[string]string) (*backups.Backup, error) { +func (cloud *cloud) CreateBackup(name, volID, snapshotID, availabilityZone string, tags map[string]string) (*backups.Backup, error) { backup := &backups.Backup{ - ID: randString(10), - Name: name, - Status: "available", - VolumeID: volID, - SnapshotID: snapshotID, - CreatedAt: time.Now(), + ID: randString(10), + Name: name, + Status: "available", + VolumeID: volID, + SnapshotID: snapshotID, + AvailabilityZone: &availabilityZone, + CreatedAt: time.Now(), } cloud.backups[backup.ID] = backup