From 027cba1886e1059ab334d200d725ab262ddf76e3 Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Tue, 6 Sep 2022 16:13:27 +0800 Subject: [PATCH] Add more nil pointer check for CSI related code in backup controller. Signed-off-by: Xun Jiang Add some corner cases checking for CSI snapshot in backup controller. Signed-off-by: Xun Jiang --- changelogs/unreleased/5388-blackpiglet | 1 + pkg/builder/volume_snapshot_builder.go | 60 +++++++++++++++ .../volume_snapshot_content_builder.go | 70 +++++++++++++++++ pkg/controller/backup_controller.go | 21 ++++- pkg/controller/backup_controller_test.go | 77 +++++++++++++++++++ pkg/test/fake_controller_runtime_client.go | 5 ++ 6 files changed, 230 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/5388-blackpiglet create mode 100644 pkg/builder/volume_snapshot_builder.go create mode 100644 pkg/builder/volume_snapshot_content_builder.go diff --git a/changelogs/unreleased/5388-blackpiglet b/changelogs/unreleased/5388-blackpiglet new file mode 100644 index 0000000000..2bd9f73752 --- /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 0000000000..0df9c9eb27 --- /dev/null +++ b/pkg/builder/volume_snapshot_builder.go @@ -0,0 +1,60 @@ +/* +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, + }, + }, + } +} + +// 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 0000000000..936eb74c53 --- /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 39444459e4..49af33a83d 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -670,6 +670,16 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { if err != nil { backupLog.Errorf("fail to wait VolumeSnapshot change to Ready: %s", err.Error()) } + + // checkVolumeSnapshotReadyToUse could run for a long time. Update VolumeSnapshot array after it finishes. + err = c.kbClient.List(context.Background(), vsList, &kbclient.ListOptions{LabelSelector: selector}) + if err != nil { + backupLog.Error(err) + } + if len(vsList.Items) >= 0 { + volumeSnapshots = vsList.Items + } + backup.CSISnapshots = volumeSnapshots err = c.kbClient.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector}) @@ -699,7 +709,7 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { } // Delete the VolumeSnapshots created in the backup, when CSI feature is enabled. - c.deleteVolumeSnapshot(volumeSnapshots, volumeSnapshotContents, *backup, backupLog) + c.deleteVolumeSnapshot(volumeSnapshots, volumeSnapshotContents, backupLog) } @@ -944,8 +954,7 @@ func (c *backupController) checkVolumeSnapshotReadyToUse(ctx context.Context, vo // If DeletionPolicy is Retain, just delete it. If DeletionPolicy is Delete, need to // change DeletionPolicy to Retain before deleting VS, then change DeletionPolicy back to Delete. func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api.VolumeSnapshot, - volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent, - backup pkgbackup.Request, logger logrus.FieldLogger) { + volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent, logger logrus.FieldLogger) { var wg sync.WaitGroup vscMap := make(map[string]snapshotv1api.VolumeSnapshotContent) for _, vsc := range volumeSnapshotContents { @@ -958,7 +967,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 { @@ -969,6 +979,9 @@ func (c *backupController) deleteVolumeSnapshot(volumeSnapshots []snapshotv1api. if vsc.Spec.DeletionPolicy == snapshotv1api.VolumeSnapshotContentDelete { modifyVSCFlag = true } + } else { + logger.Errorf("VS %s/%s is not ready. This is not expected.", vs.Namespace, vs.Name) + return } // Change VolumeSnapshotContent's DeletionPolicy to Retain before deleting VolumeSnapshot, diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 84aafd7647..9761e8a764 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -27,6 +27,7 @@ import ( "testing" "time" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -1281,3 +1282,79 @@ 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").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").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. VS should not be modified.", + vsArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").Result(), + }, + vscArray: []snapshotv1api.VolumeSnapshotContent{}, + expectedVSArray: []snapshotv1api.VolumeSnapshot{ + *builder.ForVolumeSnapshot("velero", "vs1").Result(), + }, + expectedVSCArray: []snapshotv1api.VolumeSnapshotContent{}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fakeClient := velerotest.NewFakeControllerRuntimeClientBuilder(t).WithLists( + &snapshotv1api.VolumeSnapshotList{Items: tc.vsArray}, + &snapshotv1api.VolumeSnapshotContentList{Items: tc.vscArray}, + ).Build() + logger := logging.DefaultLogger(logrus.DebugLevel, logging.FormatText) + c := &backupController{ + kbClient: fakeClient, + } + + c.deleteVolumeSnapshot(tc.vsArray, tc.vscArray, logger) + + vsList := &snapshotv1api.VolumeSnapshotList{} + require.NoError(t, c.kbClient.List(context.Background(), vsList)) + 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 d1c1b6106f..0be391bd9f 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...) }