diff --git a/changelogs/unreleased/5388-blackpiglet b/changelogs/unreleased/5388-blackpiglet new file mode 100644 index 000000000..2bd9f7375 --- /dev/null +++ b/changelogs/unreleased/5388-blackpiglet @@ -0,0 +1 @@ +Add some corner cases checking for CSI snapshot in backup controller. \ No newline at end of file diff --git a/pkg/builder/volume_snapshot_builder.go b/pkg/builder/volume_snapshot_builder.go new file mode 100644 index 000000000..19815c0f0 --- /dev/null +++ b/pkg/builder/volume_snapshot_builder.go @@ -0,0 +1,69 @@ +/* +Copyright the Velero contributors. + +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 builder + +import ( + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// VolumeSnapshotBuilder builds VolumeSnapshot objects. +type VolumeSnapshotBuilder struct { + object *snapshotv1api.VolumeSnapshot +} + +// ForVolumeSnapshot is the constructor for VolumeSnapshotBuilder. +func ForVolumeSnapshot(ns, name string) *VolumeSnapshotBuilder { + return &VolumeSnapshotBuilder{ + object: &snapshotv1api.VolumeSnapshot{ + TypeMeta: metav1.TypeMeta{ + APIVersion: snapshotv1api.SchemeGroupVersion.String(), + Kind: "VolumeSnapshot", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + }, + }, + } +} + +// ObjectMeta applies functional options to the VolumeSnapshot's ObjectMeta. +func (v *VolumeSnapshotBuilder) ObjectMeta(opts ...ObjectMetaOpt) *VolumeSnapshotBuilder { + for _, opt := range opts { + opt(v.object) + } + + return v +} + +// Result return the built VolumeSnapshot. +func (v *VolumeSnapshotBuilder) Result() *snapshotv1api.VolumeSnapshot { + return v.object +} + +// Status init the built VolumeSnapshot's status. +func (v *VolumeSnapshotBuilder) Status() *VolumeSnapshotBuilder { + v.object.Status = &snapshotv1api.VolumeSnapshotStatus{} + return v +} + +// BoundVolumeSnapshotContentName set built VolumeSnapshot's status BoundVolumeSnapshotContentName field. +func (v *VolumeSnapshotBuilder) BoundVolumeSnapshotContentName(vscName string) *VolumeSnapshotBuilder { + v.object.Status.BoundVolumeSnapshotContentName = &vscName + return v +} diff --git a/pkg/builder/volume_snapshot_content_builder.go b/pkg/builder/volume_snapshot_content_builder.go new file mode 100644 index 000000000..936eb74c5 --- /dev/null +++ b/pkg/builder/volume_snapshot_content_builder.go @@ -0,0 +1,70 @@ +/* +Copyright the Velero contributors. + +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 builder + +import ( + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// VolumeSnapshotContentBuilder builds VolumeSnapshotContent object. +type VolumeSnapshotContentBuilder struct { + object *snapshotv1api.VolumeSnapshotContent +} + +// ForVolumeSnapshotContent is the constructor of VolumeSnapshotContentBuilder. +func ForVolumeSnapshotContent(name string) *VolumeSnapshotContentBuilder { + return &VolumeSnapshotContentBuilder{ + object: &snapshotv1api.VolumeSnapshotContent{ + TypeMeta: metav1.TypeMeta{ + APIVersion: snapshotv1api.SchemeGroupVersion.String(), + Kind: "VolumeSnapshotContent", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + }, + } +} + +// Result returns the built VolumeSnapshotContent. +func (v *VolumeSnapshotContentBuilder) Result() *snapshotv1api.VolumeSnapshotContent { + return v.object +} + +// Status initiates VolumeSnapshotContent's status. +func (v *VolumeSnapshotContentBuilder) Status() *VolumeSnapshotContentBuilder { + v.object.Status = &snapshotv1api.VolumeSnapshotContentStatus{} + return v +} + +// DeletionPolicy sets built VolumeSnapshotContent's spec.DeletionPolicy value. +func (v *VolumeSnapshotContentBuilder) DeletionPolicy(policy snapshotv1api.DeletionPolicy) *VolumeSnapshotContentBuilder { + v.object.Spec.DeletionPolicy = policy + return v +} + +func (v *VolumeSnapshotContentBuilder) VolumeSnapshotRef(namespace, name string) *VolumeSnapshotContentBuilder { + v.object.Spec.VolumeSnapshotRef = v1.ObjectReference{ + APIVersion: "snapshot.storage.k8s.io/v1", + Kind: "VolumeSnapshot", + Namespace: namespace, + Name: name, + } + return v +} diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 5b3235e8a..177b4f975 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -95,7 +95,7 @@ type backupController struct { backupStoreGetter persistence.ObjectBackupStoreGetter formatFlag logging.Format volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister - volumeSnapshotClient *snapshotterClientSet.Clientset + volumeSnapshotClient snapshotterClientSet.Interface credentialFileStore credentials.FileStore } @@ -119,7 +119,7 @@ func NewBackupController( backupStoreGetter persistence.ObjectBackupStoreGetter, formatFlag logging.Format, volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister, - volumeSnapshotClient *snapshotterClientSet.Clientset, + volumeSnapshotClient snapshotterClientSet.Interface, credentialStore credentials.FileStore, ) Interface { c := &backupController{ @@ -674,10 +674,11 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { } } - err = c.checkVolumeSnapshotReadyToUse(context.Background(), volumeSnapshots, backup.Spec.CSISnapshotTimeout.Duration) + volumeSnapshots, err = c.waitVolumeSnapshotReadyToUse(context.Background(), backup.Spec.CSISnapshotTimeout.Duration, backup.Name) if err != nil { backupLog.Errorf("fail to wait VolumeSnapshot change to Ready: %s", err.Error()) } + backup.CSISnapshots = volumeSnapshots err = c.kbClient.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector}) @@ -911,18 +912,35 @@ func encodeToJSONGzip(data interface{}, desc string) (*bytes.Buffer, []error) { return buf, nil } -// Waiting for VolumeSnapshot ReadyTosue to true is time consuming. Try to make the process parallel by +// waitVolumeSnapshotReadyToUse is used to wait VolumeSnapshot turned to ReadyToUse. +// Waiting for VolumeSnapshot ReadyToUse to true is time consuming. Try to make the process parallel by // using goroutine here instead of waiting in CSI plugin, because it's not easy to make BackupItemAction // parallel by now. After BackupItemAction parallel is implemented, this logic should be moved to CSI plugin // as https://github.com/vmware-tanzu/velero-plugin-for-csi/pull/100 -func (c *backupController) checkVolumeSnapshotReadyToUse(ctx context.Context, volumesnapshots []snapshotv1api.VolumeSnapshot, - csiSnapshotTimeout time.Duration) error { +func (c *backupController) waitVolumeSnapshotReadyToUse(ctx context.Context, + csiSnapshotTimeout time.Duration, backupName string) ([]snapshotv1api.VolumeSnapshot, error) { eg, _ := errgroup.WithContext(ctx) timeout := csiSnapshotTimeout interval := 5 * time.Second + volumeSnapshots := make([]snapshotv1api.VolumeSnapshot, 0) - for _, vs := range volumesnapshots { - volumeSnapshot := vs + if c.volumeSnapshotLister != nil { + tmpVSs, err := c.volumeSnapshotLister.List(label.NewSelectorForBackup(backupName)) + if err != nil { + c.logger.Error(err) + return volumeSnapshots, err + } + + for _, vs := range tmpVSs { + volumeSnapshots = append(volumeSnapshots, *vs) + } + } + + vsChannel := make(chan snapshotv1api.VolumeSnapshot, len(volumeSnapshots)) + defer close(vsChannel) + + for index := range volumeSnapshots { + volumeSnapshot := volumeSnapshots[index] eg.Go(func() error { err := wait.PollImmediate(interval, timeout, func() (bool, error) { tmpVS, err := c.volumeSnapshotClient.SnapshotV1().VolumeSnapshots(volumeSnapshot.Namespace).Get(ctx, volumeSnapshot.Name, metav1.GetOptions{}) @@ -934,6 +952,9 @@ func (c *backupController) checkVolumeSnapshotReadyToUse(ctx context.Context, vo return false, nil } + c.logger.Debugf("VolumeSnapshot %s/%s turned into ReadyToUse.", volumeSnapshot.Namespace, volumeSnapshot.Name) + // Put the ReadyToUse VolumeSnapshot element in the result channel. + vsChannel <- *tmpVS return true, nil }) if err == wait.ErrWaitTimeout { @@ -942,7 +963,16 @@ func (c *backupController) checkVolumeSnapshotReadyToUse(ctx context.Context, vo return err }) } - return eg.Wait() + + err := eg.Wait() + + result := make([]snapshotv1api.VolumeSnapshot, 0) + length := len(vsChannel) + for index := 0; index < length; index++ { + result = append(result, <-vsChannel) + } + + return result, err } // deleteVolumeSnapshot delete VolumeSnapshot created during backup. @@ -965,7 +995,8 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api. defer wg.Done() var vsc snapshotv1api.VolumeSnapshotContent modifyVSCFlag := false - if vs.Status.BoundVolumeSnapshotContentName != nil && + if vs.Status != nil && + vs.Status.BoundVolumeSnapshotContentName != nil && len(*vs.Status.BoundVolumeSnapshotContentName) > 0 { var found bool if vsc, found = vscMap[*vs.Status.BoundVolumeSnapshotContentName]; !found { @@ -976,6 +1007,8 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api. if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete { modifyVSCFlag = true } + } else { + logger.Errorf("VolumeSnapshot %s/%s is not ready. This is not expected.", vs.Namespace, vs.Name) } // Change VolumeSnapshotContent's DeletionPolicy to Retain before deleting VolumeSnapshot, @@ -1001,7 +1034,7 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api. } // Delete VolumeSnapshot from cluster - logger.Debugf("Deleting VolumeSnapshotContent %s", vsc.Name) + logger.Debugf("Deleting VolumeSnapshot %s/%s", vs.Namespace, vs.Name) err := c.volumeSnapshotClient.SnapshotV1().VolumeSnapshots(vs.Namespace).Delete(context.TODO(), vs.Name, metav1.DeleteOptions{}) if err != nil { logger.Errorf("fail to delete VolumeSnapshot %s/%s: %s", vs.Namespace, vs.Name, err.Error()) diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 461797568..696397b72 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -27,6 +27,9 @@ import ( "testing" "time" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + snapshotfake "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake" + snapshotinformers "github.com/kubernetes-csi/external-snapshotter/client/v4/informers/externalversions" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -1393,3 +1396,90 @@ func Test_getLastSuccessBySchedule(t *testing.T) { }) } } + +func TestDeleteVolumeSnapshot(t *testing.T) { + tests := []struct { + name string + vsArray []snapshotv1api.VolumeSnapshot + vscArray []snapshotv1api.VolumeSnapshotContent + expectedVSArray []snapshotv1api.VolumeSnapshot + expectedVSCArray []snapshotv1api.VolumeSnapshotContent + }{ + { + name: "VS is ReadyToUse, and VS has corresponding VSC. VS should be deleted.", + vsArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").ObjectMeta(builder.WithLabels("testing-vs", "vs1")).Status().BoundVolumeSnapshotContentName("vsc1").Result(), + }, + vscArray: []snapshotv1api.VolumeSnapshotContent{ + *builder.ForVolumeSnapshotContent("vsc1").DeletionPolicy(snapshotv1api.VolumeSnapshotContentDelete).Status().Result(), + }, + expectedVSArray: []snapshotv1api.VolumeSnapshot{}, + expectedVSCArray: []snapshotv1api.VolumeSnapshotContent{ + *builder.ForVolumeSnapshotContent("vsc1").DeletionPolicy(snapshotv1api.VolumeSnapshotContentRetain).VolumeSnapshotRef("ns-", "name-").Status().Result(), + }, + }, + { + name: "Corresponding VSC not found for VS. VS is not deleted.", + vsArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").ObjectMeta(builder.WithLabels("testing-vs", "vs1")).Status().BoundVolumeSnapshotContentName("vsc1").Result(), + }, + vscArray: []snapshotv1api.VolumeSnapshotContent{}, + expectedVSArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").Status().BoundVolumeSnapshotContentName("vsc1").Result(), + }, + expectedVSCArray: []snapshotv1api.VolumeSnapshotContent{}, + }, + { + name: "VS status is nil. VSC should not be modified.", + vsArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").ObjectMeta(builder.WithLabels("testing-vs", "vs1")).Result(), + }, + vscArray: []snapshotv1api.VolumeSnapshotContent{ + *builder.ForVolumeSnapshotContent("vsc1").DeletionPolicy(snapshotv1api.VolumeSnapshotContentDelete).Status().Result(), + }, + expectedVSArray: []snapshotv1api.VolumeSnapshot{}, + expectedVSCArray: []snapshotv1api.VolumeSnapshotContent{ + *builder.ForVolumeSnapshotContent("vsc1").DeletionPolicy(snapshotv1api.VolumeSnapshotContentDelete).Status().Result(), + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fakeClient := velerotest.NewFakeControllerRuntimeClientBuilder(t).WithLists( + &snapshotv1api.VolumeSnapshotContentList{Items: tc.vscArray}, + ).Build() + + vsClient := snapshotfake.NewSimpleClientset(&tc.vsArray[0]) + sharedInformers := snapshotinformers.NewSharedInformerFactory(vsClient, 0) + + for _, vs := range tc.vsArray { + sharedInformers.Snapshot().V1().VolumeSnapshots().Informer().GetStore().Add(vs) + } + + logger := logging.DefaultLogger(logrus.DebugLevel, logging.FormatText) + c := &backupController{ + kbClient: fakeClient, + volumeSnapshotClient: vsClient, + volumeSnapshotLister: sharedInformers.Snapshot().V1().VolumeSnapshots().Lister(), + } + + c.deleteVolumeSnapshot(tc.vsArray, tc.vscArray, logger) + + vsList, err := c.volumeSnapshotClient.SnapshotV1().VolumeSnapshots("velero").List(context.TODO(), metav1.ListOptions{}) + require.NoError(t, err) + assert.Equal(t, len(tc.expectedVSArray), len(vsList.Items)) + for index := range tc.expectedVSArray { + assert.Equal(t, tc.expectedVSArray[index].Status, vsList.Items[index].Status) + assert.Equal(t, tc.expectedVSArray[index].Spec, vsList.Items[index].Spec) + } + + vscList := &snapshotv1api.VolumeSnapshotContentList{} + require.NoError(t, c.kbClient.List(context.Background(), vscList)) + assert.Equal(t, len(tc.expectedVSCArray), len(vscList.Items)) + for index := range tc.expectedVSCArray { + assert.Equal(t, tc.expectedVSCArray[index].Spec, vscList.Items[index].Spec) + } + }) + } +} diff --git a/pkg/test/fake_controller_runtime_client.go b/pkg/test/fake_controller_runtime_client.go index d1c1b6106..0be391bd9 100644 --- a/pkg/test/fake_controller_runtime_client.go +++ b/pkg/test/fake_controller_runtime_client.go @@ -19,6 +19,7 @@ package test import ( "testing" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/stretchr/testify/require" corev1api "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -34,6 +35,8 @@ func NewFakeControllerRuntimeClientBuilder(t *testing.T) *k8sfake.ClientBuilder require.NoError(t, err) err = corev1api.AddToScheme(scheme) require.NoError(t, err) + err = snapshotv1api.AddToScheme(scheme) + require.NoError(t, err) return k8sfake.NewClientBuilder().WithScheme(scheme) } @@ -43,5 +46,7 @@ func NewFakeControllerRuntimeClient(t *testing.T, initObjs ...runtime.Object) cl require.NoError(t, err) err = corev1api.AddToScheme(scheme) require.NoError(t, err) + err = snapshotv1api.AddToScheme(scheme) + require.NoError(t, err) return k8sfake.NewFakeClientWithScheme(scheme, initObjs...) }