From 7e35fd32612f11beabfbaa39cbdffd83bdcaa9f7 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 15:53:47 -0400 Subject: [PATCH 1/6] restore: Use warning when Create IsAlreadyExist and Get error (#7004) Signed-off-by: Tiger Kaovilai --- changelogs/unreleased/7054-kaovilai | 1 + pkg/restore/restore.go | 11 +++++------ 2 files changed, 6 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/7054-kaovilai diff --git a/changelogs/unreleased/7054-kaovilai b/changelogs/unreleased/7054-kaovilai new file mode 100644 index 000000000..3b85df196 --- /dev/null +++ b/changelogs/unreleased/7054-kaovilai @@ -0,0 +1 @@ +restore: Use warning when Create IsAlreadyExist and Get error \ No newline at end of file diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 835553330..e18e4d3e3 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1524,18 +1524,17 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } if restoreErr != nil { - // check for the existence of the object in cluster, if no error then it implies that object exists - // and if err then we want to judge whether there is an existing error in the previous creation. - // if so, we will return the 'get' error. - // otherwise, we will return the original creation error. + // check for the existence of the object that failed creation due to alreadyExist in cluster, if no error then it implies that object exists. + // and if err then itemExists remains false as we were not able to confirm the existence of the object via Get call or creation call. + // We return the get error as a warning to notify the user that the object could exist in cluster and we were not able to confirm it. if !ctx.disableInformerCache { fromCluster, err = ctx.getResource(groupResource, obj, namespace, name) } else { fromCluster, err = resourceClient.Get(name, metav1.GetOptions{}) } if err != nil && isAlreadyExistsError { - ctx.log.Errorf("Error retrieving in-cluster version of %s: %v", kube.NamespaceAndName(obj), err) - errs.Add(namespace, err) + ctx.log.Warnf("Unable to retrieve in-cluster version of %s: %v, object won't be restored by velero or have restore labels, and existing resource policy is not applied", kube.NamespaceAndName(obj), err) + warnings.Add(namespace, err) return warnings, errs, itemExists } } From bb4f9094fd92fc93d2feb36709fd2aed07273571 Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Tue, 31 Oct 2023 12:08:48 +0800 Subject: [PATCH 2/6] issue 7027: backup exposer -- don't assume first volume as the backup volume Signed-off-by: Lyndon-Li --- changelogs/unreleased/7060-Lyndon-Li | 1 + pkg/exposer/csi_snapshot.go | 16 ++- pkg/exposer/csi_snapshot_test.go | 179 +++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/7060-Lyndon-Li diff --git a/changelogs/unreleased/7060-Lyndon-Li b/changelogs/unreleased/7060-Lyndon-Li new file mode 100644 index 000000000..f2a3a9ae2 --- /dev/null +++ b/changelogs/unreleased/7060-Lyndon-Li @@ -0,0 +1 @@ +Fix issue #7027, data mover backup exposer should not assume the first volume as the backup volume in backup pod \ No newline at end of file diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 85f511524..8b501e648 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -190,6 +190,7 @@ func (e *csiSnapshotExposer) GetExposed(ctx context.Context, ownerObject corev1. backupPodName := ownerObject.Name backupPVCName := ownerObject.Name + volumeName := string(ownerObject.UID) curLog := e.log.WithFields(logrus.Fields{ "owner": ownerObject.Name, @@ -218,7 +219,20 @@ func (e *csiSnapshotExposer) GetExposed(ctx context.Context, ownerObject corev1. curLog.WithField("backup pvc", backupPVCName).Info("Backup PVC is bound") - return &ExposeResult{ByPod: ExposeByPod{HostingPod: pod, VolumeName: pod.Spec.Volumes[0].Name}}, nil + i := 0 + for i = 0; i < len(pod.Spec.Volumes); i++ { + if pod.Spec.Volumes[i].Name == volumeName { + break + } + } + + if i == len(pod.Spec.Volumes) { + return nil, errors.Errorf("backup pod %s doesn't have the expected backup volume", pod.Name) + } + + curLog.WithField("pod", pod.Name).Infof("Backup volume is found in pod at index %v", i) + + return &ExposeResult{ByPod: ExposeByPod{HostingPod: pod, VolumeName: volumeName}}, nil } func (e *csiSnapshotExposer) CleanUp(ctx context.Context, ownerObject corev1.ObjectReference, vsName string, sourceNamespace string) { diff --git a/pkg/exposer/csi_snapshot_test.go b/pkg/exposer/csi_snapshot_test.go index 7ea6d5bf0..d57d78ed4 100644 --- a/pkg/exposer/csi_snapshot_test.go +++ b/pkg/exposer/csi_snapshot_test.go @@ -37,6 +37,8 @@ import ( velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" velerotest "github.com/vmware-tanzu/velero/pkg/test" "github.com/vmware-tanzu/velero/pkg/util/boolptr" + + clientFake "sigs.k8s.io/controller-runtime/pkg/client/fake" ) type reactor struct { @@ -384,3 +386,180 @@ func TestExpose(t *testing.T) { }) } } + +func TestGetExpose(t *testing.T) { + backup := &velerov1.Backup{ + TypeMeta: metav1.TypeMeta{ + APIVersion: velerov1.SchemeGroupVersion.String(), + Kind: "Backup", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1.DefaultNamespace, + Name: "fake-backup", + UID: "fake-uid", + }, + } + + backupPod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: backup.Name, + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + Name: "fake-volume", + }, + { + Name: "fake-volume-2", + }, + { + Name: string(backup.UID), + }, + }, + }, + } + + backupPodWithoutVolume := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: backup.Name, + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + Name: "fake-volume-1", + }, + { + Name: "fake-volume-2", + }, + }, + }, + } + + backupPVC := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: backup.Namespace, + Name: backup.Name, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + VolumeName: "fake-pv-name", + }, + } + + backupPV := &corev1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-pv-name", + }, + } + + scheme := runtime.NewScheme() + corev1.AddToScheme(scheme) + + tests := []struct { + name string + kubeClientObj []runtime.Object + ownerBackup *velerov1.Backup + exposeWaitParam CSISnapshotExposeWaitParam + Timeout time.Duration + err string + expectedResult *ExposeResult + }{ + { + name: "backup pod is not found", + ownerBackup: backup, + exposeWaitParam: CSISnapshotExposeWaitParam{ + NodeName: "fake-node", + }, + }, + { + name: "wait pvc bound fail", + ownerBackup: backup, + exposeWaitParam: CSISnapshotExposeWaitParam{ + NodeName: "fake-node", + }, + kubeClientObj: []runtime.Object{ + backupPod, + }, + Timeout: time.Second, + err: "error to wait backup PVC bound, fake-backup: error to wait for rediness of PVC: error to get pvc velero/fake-backup: persistentvolumeclaims \"fake-backup\" not found", + }, + { + name: "backup volume not found in pod", + ownerBackup: backup, + exposeWaitParam: CSISnapshotExposeWaitParam{ + NodeName: "fake-node", + }, + kubeClientObj: []runtime.Object{ + backupPodWithoutVolume, + backupPVC, + backupPV, + }, + Timeout: time.Second, + err: "backup pod fake-backup doesn't have the expected backup volume", + }, + { + name: "succeed", + ownerBackup: backup, + exposeWaitParam: CSISnapshotExposeWaitParam{ + NodeName: "fake-node", + }, + kubeClientObj: []runtime.Object{ + backupPod, + backupPVC, + backupPV, + }, + Timeout: time.Second, + expectedResult: &ExposeResult{ + ByPod: ExposeByPod{ + HostingPod: backupPod, + VolumeName: string(backup.UID), + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fakeKubeClient := fake.NewSimpleClientset(test.kubeClientObj...) + + fakeClientBuilder := clientFake.NewClientBuilder() + fakeClientBuilder = fakeClientBuilder.WithScheme(scheme) + + fakeClient := fakeClientBuilder.WithRuntimeObjects(test.kubeClientObj...).Build() + + exposer := csiSnapshotExposer{ + kubeClient: fakeKubeClient, + log: velerotest.NewLogger(), + } + + var ownerObject corev1.ObjectReference + if test.ownerBackup != nil { + ownerObject = corev1.ObjectReference{ + Kind: test.ownerBackup.Kind, + Namespace: test.ownerBackup.Namespace, + Name: test.ownerBackup.Name, + UID: test.ownerBackup.UID, + APIVersion: test.ownerBackup.APIVersion, + } + } + + test.exposeWaitParam.NodeClient = fakeClient + + result, err := exposer.GetExposed(context.Background(), ownerObject, test.Timeout, &test.exposeWaitParam) + if test.err == "" { + assert.NoError(t, err) + + if test.expectedResult == nil { + assert.Nil(t, result) + } else { + assert.NoError(t, err) + assert.Equal(t, test.expectedResult.ByPod.VolumeName, result.ByPod.VolumeName) + assert.Equal(t, test.expectedResult.ByPod.HostingPod.Name, result.ByPod.HostingPod.Name) + } + } else { + assert.EqualError(t, err, test.err) + } + }) + } +} From 6438fc9a694a9e03bac2cb6c2075aa29e1301476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wenkai=20Yin=28=E5=B0=B9=E6=96=87=E5=BC=80=29?= Date: Mon, 6 Nov 2023 16:16:38 +0800 Subject: [PATCH 3/6] Truncate the credential file to avoid the change of secret content messing it up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Truncate the credential file to avoid the change of secret content messing it up Signed-off-by: Wenkai Yin(尹文开) --- changelogs/unreleased/7058-ywk253100 | 1 + internal/credentials/file_store.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/7058-ywk253100 diff --git a/changelogs/unreleased/7058-ywk253100 b/changelogs/unreleased/7058-ywk253100 new file mode 100644 index 000000000..2a6faffe3 --- /dev/null +++ b/changelogs/unreleased/7058-ywk253100 @@ -0,0 +1 @@ +Truncate the credential file to avoid the change of secret content messing it up \ No newline at end of file diff --git a/internal/credentials/file_store.go b/internal/credentials/file_store.go index 1332d4f8d..4b5d25664 100644 --- a/internal/credentials/file_store.go +++ b/internal/credentials/file_store.go @@ -71,7 +71,7 @@ func (n *namespacedFileStore) Path(selector *corev1api.SecretKeySelector) (strin keyFilePath := filepath.Join(n.fsRoot, fmt.Sprintf("%s-%s", selector.Name, selector.Key)) - file, err := n.fs.OpenFile(keyFilePath, os.O_RDWR|os.O_CREATE, 0644) + file, err := n.fs.OpenFile(keyFilePath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644) if err != nil { return "", errors.Wrap(err, "unable to open credentials file for writing") } From 5923046471a1b3e9e3ac011cae70c0da023fe614 Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Mon, 6 Nov 2023 20:32:45 +0800 Subject: [PATCH 4/6] Add DataUpload Result and CSI VolumeSnapshot check for restore PV. Signed-off-by: Xun Jiang --- changelogs/unreleased/7087-blackpiglet | 1 + pkg/builder/volume_snapshot_builder.go | 5 + pkg/controller/restore_controller.go | 6 + pkg/controller/restore_controller_test.go | 2 + pkg/restore/request.go | 2 + pkg/restore/restore.go | 94 +++++++- pkg/restore/restore_test.go | 262 +++++++++++++++++++++- pkg/test/resources.go | 11 + 8 files changed, 375 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/7087-blackpiglet diff --git a/changelogs/unreleased/7087-blackpiglet b/changelogs/unreleased/7087-blackpiglet new file mode 100644 index 000000000..ac965ed13 --- /dev/null +++ b/changelogs/unreleased/7087-blackpiglet @@ -0,0 +1 @@ +Add DataUpload Result and CSI VolumeSnapshot check for restore PV. \ No newline at end of file diff --git a/pkg/builder/volume_snapshot_builder.go b/pkg/builder/volume_snapshot_builder.go index 19815c0f0..bbaedd16e 100644 --- a/pkg/builder/volume_snapshot_builder.go +++ b/pkg/builder/volume_snapshot_builder.go @@ -67,3 +67,8 @@ func (v *VolumeSnapshotBuilder) BoundVolumeSnapshotContentName(vscName string) * v.object.Status.BoundVolumeSnapshotContentName = &vscName return v } + +func (v *VolumeSnapshotBuilder) SourcePVC(name string) *VolumeSnapshotBuilder { + v.object.Spec.Source.PersistentVolumeClaimName = &name + return v +} diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 5d6ed505e..f6b9b39d9 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -515,6 +515,11 @@ func (r *restoreReconciler) runValidatedRestore(restore *api.Restore, info backu return errors.Wrap(err, "error fetching volume snapshots metadata") } + csiVolumeSnapshots, err := backupStore.GetCSIVolumeSnapshots(restore.Spec.BackupName) + if err != nil { + return errors.Wrap(err, "fail to fetch CSI VolumeSnapshots metadata") + } + restoreLog.Info("starting restore") var podVolumeBackups []*api.PodVolumeBackup @@ -531,6 +536,7 @@ func (r *restoreReconciler) runValidatedRestore(restore *api.Restore, info backu BackupReader: backupFile, ResourceModifiers: resourceModifiers, DisableInformerCache: r.disableInformerCache, + CSIVolumeSnapshots: csiVolumeSnapshots, } restoreWarnings, restoreErrors := r.restorer.RestoreWithResolvers(restoreReq, actionsResolver, pluginManager) diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index b7f6fbd25..9437f1d1c 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -23,6 +23,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" @@ -471,6 +472,7 @@ func TestRestoreReconcile(t *testing.T) { } if test.expectedRestorerCall != nil { backupStore.On("GetBackupContents", test.backup.Name).Return(io.NopCloser(bytes.NewReader([]byte("hello world"))), nil) + backupStore.On("GetCSIVolumeSnapshots", test.backup.Name).Return([]*snapshotv1api.VolumeSnapshot{}, nil) restorer.On("RestoreWithResolvers", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(warnings, errors) diff --git a/pkg/restore/request.go b/pkg/restore/request.go index dcc2ef3d6..2a267a5ff 100644 --- a/pkg/restore/request.go +++ b/pkg/restore/request.go @@ -21,6 +21,7 @@ import ( "io" "sort" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/runtime" @@ -60,6 +61,7 @@ type Request struct { itemOperationsList *[]*itemoperation.RestoreOperation ResourceModifiers *resourcemodifiers.ResourceModifiers DisableInformerCache bool + CSIVolumeSnapshots []*snapshotv1api.VolumeSnapshot } type restoredItemStatus struct { diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index e18e4d3e3..48af7b70d 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -30,6 +30,7 @@ import ( "time" "github.com/google/uuid" + snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" @@ -298,6 +299,7 @@ func (kr *kubernetesRestorer) RestoreWithResolvers( pvsToProvision: sets.NewString(), pvRestorer: pvRestorer, volumeSnapshots: req.VolumeSnapshots, + csiVolumeSnapshots: req.CSIVolumeSnapshots, podVolumeBackups: req.PodVolumeBackups, resourceTerminatingTimeout: kr.resourceTerminatingTimeout, resourceTimeout: kr.resourceTimeout, @@ -347,6 +349,7 @@ type restoreContext struct { pvsToProvision sets.String pvRestorer PVRestorer volumeSnapshots []*volume.Snapshot + csiVolumeSnapshots []*snapshotv1api.VolumeSnapshot podVolumeBackups []*velerov1api.PodVolumeBackup resourceTerminatingTimeout time.Duration resourceTimeout time.Duration @@ -1287,7 +1290,35 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } case hasPodVolumeBackup(obj, ctx): - ctx.log.Infof("Dynamically re-provisioning persistent volume because it has a pod volume backup to be restored.") + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Dynamically re-provisioning persistent volume because it has a pod volume backup to be restored.") + ctx.pvsToProvision.Insert(name) + + // Return early because we don't want to restore the PV itself, we + // want to dynamically re-provision it. + return warnings, errs, itemExists + + case hasCSIVolumeSnapshot(ctx, obj): + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Dynamically re-provisioning persistent volume because it has a related CSI VolumeSnapshot.") + ctx.pvsToProvision.Insert(name) + + // Return early because we don't want to restore the PV itself, we + // want to dynamically re-provision it. + return warnings, errs, itemExists + + case hasSnapshotDataUpload(ctx, obj): + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Dynamically re-provisioning persistent volume because it has a related snapshot DataUpload.") ctx.pvsToProvision.Insert(name) // Return early because we don't want to restore the PV itself, we @@ -1295,7 +1326,11 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs, itemExists case hasDeleteReclaimPolicy(obj.Object): - ctx.log.Infof("Dynamically re-provisioning persistent volume because it doesn't have a snapshot and its reclaim policy is Delete.") + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Dynamically re-provisioning persistent volume because it doesn't have a snapshot and its reclaim policy is Delete.") ctx.pvsToProvision.Insert(name) // Return early because we don't want to restore the PV itself, we @@ -1303,7 +1338,11 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs, itemExists default: - ctx.log.Infof("Restoring persistent volume as-is because it doesn't have a snapshot and its reclaim policy is not Delete.") + ctx.log.WithFields(logrus.Fields{ + "namespace": obj.GetNamespace(), + "name": obj.GetName(), + "groupResource": groupResource.String(), + }).Infof("Restoring persistent volume as-is because it doesn't have a snapshot and its reclaim policy is not Delete.") // Check to see if the claimRef.namespace field needs to be remapped, and do so if necessary. _, err = remapClaimRefNS(ctx, obj) @@ -1929,6 +1968,55 @@ func hasSnapshot(pvName string, snapshots []*volume.Snapshot) bool { return false } +func hasCSIVolumeSnapshot(ctx *restoreContext, unstructuredPV *unstructured.Unstructured) bool { + pv := new(v1.PersistentVolume) + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredPV.Object, pv); err != nil { + ctx.log.WithError(err).Warnf("Unable to convert PV from unstructured to structured") + return false + } + + for _, vs := range ctx.csiVolumeSnapshots { + if pv.Spec.ClaimRef.Name == *vs.Spec.Source.PersistentVolumeClaimName && + pv.Spec.ClaimRef.Namespace == vs.Namespace { + return true + } + } + return false +} + +func hasSnapshotDataUpload(ctx *restoreContext, unstructuredPV *unstructured.Unstructured) bool { + pv := new(v1.PersistentVolume) + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredPV.Object, pv); err != nil { + ctx.log.WithError(err).Warnf("Unable to convert PV from unstructured to structured") + return false + } + + if pv.Spec.ClaimRef == nil { + return false + } + + dataUploadResultList := new(v1.ConfigMapList) + err := ctx.kbClient.List(go_context.TODO(), dataUploadResultList, &crclient.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + velerov1api.RestoreUIDLabel: label.GetValidName(string(ctx.restore.GetUID())), + velerov1api.PVCNamespaceNameLabel: label.GetValidName(pv.Spec.ClaimRef.Namespace + "." + pv.Spec.ClaimRef.Name), + velerov1api.ResourceUsageLabel: label.GetValidName(string(velerov1api.VeleroResourceUsageDataUploadResult)), + }), + }) + if err != nil { + ctx.log.WithError(err).Warnf("Fail to list DataUpload result CM.") + return false + } + + if len(dataUploadResultList.Items) != 1 { + ctx.log.WithError(fmt.Errorf("dataupload result number is not expected")). + Warnf("Got %d DataUpload result. Expect one.", len(dataUploadResultList.Items)) + return false + } + + return true +} + func hasPodVolumeBackup(unstructuredPV *unstructured.Unstructured, ctx *restoreContext) bool { if len(ctx.podVolumeBackups) == 0 { return false diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 4ffd76257..643d001af 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -25,6 +25,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" @@ -2256,6 +2257,7 @@ func (*volumeSnapshotter) DeleteSnapshot(snapshotID string) error { // Verification is done by looking at the contents of the API and the metadata/spec/status of // the items in the API. func TestRestorePersistentVolumes(t *testing.T) { + testPVCName := "testPVC" tests := []struct { name string restore *velerov1api.Restore @@ -2265,6 +2267,8 @@ func TestRestorePersistentVolumes(t *testing.T) { volumeSnapshots []*volume.Snapshot volumeSnapshotLocations []*velerov1api.VolumeSnapshotLocation volumeSnapshotterGetter volumeSnapshotterGetter + csiVolumeSnapshots []*snapshotv1api.VolumeSnapshot + dataUploadResult *corev1api.ConfigMap want []*test.APIResource wantError bool wantWarning bool @@ -2923,6 +2927,77 @@ func TestRestorePersistentVolumes(t *testing.T) { ), }, }, + { + name: "when a PV with a reclaim policy of retain has a CSI VolumeSnapshot and does not exist in-cluster, the PV is not restored", + restore: defaultRestore().Result(), + backup: defaultBackup().Result(), + tarball: test.NewTarWriter(t). + AddItems("persistentvolumes", + builder.ForPersistentVolume("pv-1"). + ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain). + ClaimRef("velero", testPVCName). + Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.PVs(), + test.PVCs(), + }, + csiVolumeSnapshots: []*snapshotv1api.VolumeSnapshot{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "velero", + Name: "test", + }, + Spec: snapshotv1api.VolumeSnapshotSpec{ + Source: snapshotv1api.VolumeSnapshotSource{ + PersistentVolumeClaimName: &testPVCName, + }, + }, + }, + }, + volumeSnapshotLocations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "default").Provider("provider-1").Result(), + }, + volumeSnapshotterGetter: map[string]vsv1.VolumeSnapshotter{ + "provider-1": &volumeSnapshotter{ + snapshotVolumes: map[string]string{"snapshot-1": "new-volume"}, + }, + }, + want: []*test.APIResource{}, + }, + { + name: "when a PV with a reclaim policy of retain has a DataUpload result CM and does not exist in-cluster, the PV is not restored", + restore: defaultRestore().ObjectMeta(builder.WithUID("fakeUID")).Result(), + backup: defaultBackup().Result(), + tarball: test.NewTarWriter(t). + AddItems("persistentvolumes", + builder.ForPersistentVolume("pv-1"). + ReclaimPolicy(corev1api.PersistentVolumeReclaimRetain). + ClaimRef("velero", testPVCName). + Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.PVs(), + test.PVCs(), + test.ConfigMaps(), + }, + volumeSnapshotLocations: []*velerov1api.VolumeSnapshotLocation{ + builder.ForVolumeSnapshotLocation(velerov1api.DefaultNamespace, "default").Provider("provider-1").Result(), + }, + volumeSnapshotterGetter: map[string]vsv1.VolumeSnapshotter{ + "provider-1": &volumeSnapshotter{ + snapshotVolumes: map[string]string{"snapshot-1": "new-volume"}, + }, + }, + dataUploadResult: builder.ForConfigMap("velero", "test").ObjectMeta(builder.WithLabelsMap(map[string]string{ + velerov1api.RestoreUIDLabel: "fakeUID", + velerov1api.PVCNamespaceNameLabel: "velero/testPVC", + velerov1api.ResourceUsageLabel: string(velerov1api.VeleroResourceUsageDataUploadResult), + })).Result(), + want: []*test.APIResource{}, + }, } for _, tc := range tests { @@ -2939,6 +3014,10 @@ func TestRestorePersistentVolumes(t *testing.T) { require.NoError(t, h.restorer.kbClient.Create(context.Background(), vsl)) } + if tc.dataUploadResult != nil { + require.NoError(t, h.restorer.kbClient.Create(context.TODO(), tc.dataUploadResult)) + } + for _, r := range tc.apiResources { h.AddItems(t, r) } @@ -2955,11 +3034,12 @@ func TestRestorePersistentVolumes(t *testing.T) { } data := &Request{ - Log: h.log, - Restore: tc.restore, - Backup: tc.backup, - VolumeSnapshots: tc.volumeSnapshots, - BackupReader: tc.tarball, + Log: h.log, + Restore: tc.restore, + Backup: tc.backup, + VolumeSnapshots: tc.volumeSnapshots, + BackupReader: tc.tarball, + CSIVolumeSnapshots: tc.csiVolumeSnapshots, } warnings, errs := h.restorer.Restore( data, @@ -3652,3 +3732,175 @@ func TestIsAlreadyExistsError(t *testing.T) { }) } } + +func TestHasCSIVolumeSnapshot(t *testing.T) { + tests := []struct { + name string + vs *snapshotv1api.VolumeSnapshot + obj *unstructured.Unstructured + expectedResult bool + }{ + { + name: "Invalid PV, expect false.", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": 1, + }, + }, + expectedResult: false, + }, + { + name: "Cannot find VS, expect false", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "test", + }, + }, + }, + expectedResult: false, + }, + { + name: "Find VS, expect true.", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "velero", + "name": "test", + }, + "spec": map[string]interface{}{ + "claimRef": map[string]interface{}{ + "namespace": "velero", + "name": "test", + }, + }, + }, + }, + vs: builder.ForVolumeSnapshot("velero", "test").SourcePVC("test").Result(), + expectedResult: true, + }, + } + + for _, tc := range tests { + h := newHarness(t) + + ctx := &restoreContext{ + log: h.log, + } + + if tc.vs != nil { + ctx.csiVolumeSnapshots = []*snapshotv1api.VolumeSnapshot{tc.vs} + } + + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expectedResult, hasCSIVolumeSnapshot(ctx, tc.obj)) + }) + } +} + +func TestHasSnapshotDataUpload(t *testing.T) { + tests := []struct { + name string + duResult *corev1api.ConfigMap + obj *unstructured.Unstructured + expectedResult bool + restore *velerov1api.Restore + }{ + { + name: "Invalid PV, expect false.", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": 1, + }, + }, + expectedResult: false, + }, + { + name: "PV without ClaimRef, expect false", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "test", + }, + }, + }, + duResult: builder.ForConfigMap("velero", "test").Result(), + restore: builder.ForRestore("velero", "test").ObjectMeta(builder.WithUID("fakeUID")).Result(), + expectedResult: false, + }, + { + name: "Cannot find DataUploadResult CM, expect false", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "test", + }, + "spec": map[string]interface{}{ + "claimRef": map[string]interface{}{ + "namespace": "velero", + "name": "testPVC", + }, + }, + }, + }, + duResult: builder.ForConfigMap("velero", "test").Result(), + restore: builder.ForRestore("velero", "test").ObjectMeta(builder.WithUID("fakeUID")).Result(), + expectedResult: false, + }, + { + name: "Find DataUploadResult CM, expect true", + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "PersistentVolume", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "namespace": "default", + "name": "test", + }, + "spec": map[string]interface{}{ + "claimRef": map[string]interface{}{ + "namespace": "velero", + "name": "testPVC", + }, + }, + }, + }, + duResult: builder.ForConfigMap("velero", "test").ObjectMeta(builder.WithLabelsMap(map[string]string{ + velerov1api.RestoreUIDLabel: "fakeUID", + velerov1api.PVCNamespaceNameLabel: "velero/testPVC", + velerov1api.ResourceUsageLabel: string(velerov1api.VeleroResourceUsageDataUploadResult), + })).Result(), + restore: builder.ForRestore("velero", "test").ObjectMeta(builder.WithUID("fakeUID")).Result(), + expectedResult: false, + }, + } + + for _, tc := range tests { + h := newHarness(t) + + ctx := &restoreContext{ + log: h.log, + kbClient: h.restorer.kbClient, + restore: tc.restore, + } + + if tc.duResult != nil { + require.NoError(t, ctx.kbClient.Create(context.TODO(), tc.duResult)) + } + + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expectedResult, hasSnapshotDataUpload(ctx, tc.obj)) + }) + } +} diff --git a/pkg/test/resources.go b/pkg/test/resources.go index 7c2fa17f6..709497fca 100644 --- a/pkg/test/resources.go +++ b/pkg/test/resources.go @@ -142,6 +142,17 @@ func ServiceAccounts(items ...metav1.Object) *APIResource { } } +func ConfigMaps(items ...metav1.Object) *APIResource { + return &APIResource{ + Group: "", + Version: "v1", + Name: "configmaps", + ShortName: "cm", + Namespaced: true, + Items: items, + } +} + func CRDs(items ...metav1.Object) *APIResource { return &APIResource{ Group: "apiextensions.k8s.io", From 84d8bbda247a7c7841ca74a1b2911fceeeed00da Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Tue, 14 Nov 2023 10:40:20 +0800 Subject: [PATCH 5/6] issue 7094: fallback to full backup if previous snapshot is not found Signed-off-by: Lyndon-Li --- changelogs/unreleased/7097-Lyndon-Li | 1 + pkg/uploader/kopia/snapshot.go | 6 +++--- pkg/uploader/kopia/snapshot_test.go | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/7097-Lyndon-Li diff --git a/changelogs/unreleased/7097-Lyndon-Li b/changelogs/unreleased/7097-Lyndon-Li new file mode 100644 index 000000000..7ce331248 --- /dev/null +++ b/changelogs/unreleased/7097-Lyndon-Li @@ -0,0 +1 @@ +Fix issue #7094, fallback to full backup if previous snapshot is not found \ No newline at end of file diff --git a/pkg/uploader/kopia/snapshot.go b/pkg/uploader/kopia/snapshot.go index 27ac84253..d87404a36 100644 --- a/pkg/uploader/kopia/snapshot.go +++ b/pkg/uploader/kopia/snapshot.go @@ -236,10 +236,10 @@ func SnapshotSource( mani, err := loadSnapshotFunc(ctx, rep, manifest.ID(parentSnapshot)) if err != nil { - return "", 0, errors.Wrapf(err, "Failed to load previous snapshot %v from kopia", parentSnapshot) + log.WithError(err).Warnf("Failed to load previous snapshot %v from kopia, fallback to full backup", parentSnapshot) + } else { + previous = append(previous, mani) } - - previous = append(previous, mani) } else { log.Infof("Searching for parent snapshot") diff --git a/pkg/uploader/kopia/snapshot_test.go b/pkg/uploader/kopia/snapshot_test.go index 65ec22136..34bc530b2 100644 --- a/pkg/uploader/kopia/snapshot_test.go +++ b/pkg/uploader/kopia/snapshot_test.go @@ -112,7 +112,7 @@ func TestSnapshotSource(t *testing.T) { notError: true, }, { - name: "failed to load snapshot", + name: "failed to load snapshot, should fallback to full backup and not error", args: []mockArgs{ {methodName: "LoadSnapshot", returns: []interface{}{manifest, errors.New("failed to load snapshot")}}, {methodName: "SaveSnapshot", returns: []interface{}{manifest.ID, nil}}, @@ -122,7 +122,7 @@ func TestSnapshotSource(t *testing.T) { {methodName: "Upload", returns: []interface{}{manifest, nil}}, {methodName: "Flush", returns: []interface{}{nil}}, }, - notError: false, + notError: true, }, { name: "failed to save snapshot", From 06e3773b22e6f5cfe72091c7fe8678e49827bdd5 Mon Sep 17 00:00:00 2001 From: yanggang Date: Thu, 7 Sep 2023 11:04:49 +0800 Subject: [PATCH 6/6] Fix node-agent missing metrics-addr parms to define the server start. Signed-off-by: yanggang --- changelogs/unreleased/7098-yanggangtony | 1 + pkg/cmd/cli/nodeagent/server.go | 18 ++++++++++-------- pkg/install/daemonset.go | 1 + 3 files changed, 12 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/7098-yanggangtony diff --git a/changelogs/unreleased/7098-yanggangtony b/changelogs/unreleased/7098-yanggangtony new file mode 100644 index 000000000..8d63d94a1 --- /dev/null +++ b/changelogs/unreleased/7098-yanggangtony @@ -0,0 +1 @@ +Fix the node-agent missing metrics-address defines. \ No newline at end of file diff --git a/pkg/cmd/cli/nodeagent/server.go b/pkg/cmd/cli/nodeagent/server.go index 10df06f89..105e37052 100644 --- a/pkg/cmd/cli/nodeagent/server.go +++ b/pkg/cmd/cli/nodeagent/server.go @@ -114,6 +114,7 @@ func NewServerCommand(f client.Factory) *cobra.Command { command.Flags().Var(formatFlag, "log-format", fmt.Sprintf("The format for log output. Valid values are %s.", strings.Join(formatFlag.AllowedValues(), ", "))) command.Flags().DurationVar(&config.resourceTimeout, "resource-timeout", config.resourceTimeout, "How long to wait for resource processes which are not covered by other specific timeout parameters. Default is 10 minutes.") command.Flags().DurationVar(&config.dataMoverPrepareTimeout, "data-mover-prepare-timeout", config.dataMoverPrepareTimeout, "How long to wait for preparing a DataUpload/DataDownload. Default is 30 minutes.") + command.Flags().StringVar(&config.metricsAddress, "metrics-address", config.metricsAddress, "The address to expose prometheus metrics") return command } @@ -193,14 +194,15 @@ func newNodeAgentServer(logger logrus.FieldLogger, factory client.Factory, confi } s := &nodeAgentServer{ - logger: logger, - ctx: ctx, - cancelFunc: cancelFunc, - fileSystem: filesystem.NewFileSystem(), - mgr: mgr, - config: config, - namespace: factory.Namespace(), - nodeName: nodeName, + logger: logger, + ctx: ctx, + cancelFunc: cancelFunc, + fileSystem: filesystem.NewFileSystem(), + mgr: mgr, + config: config, + namespace: factory.Namespace(), + nodeName: nodeName, + metricsAddress: config.metricsAddress, } // the cache isn't initialized yet when "validatePodVolumesHostPath" is called, the client returned by the manager cannot diff --git a/pkg/install/daemonset.go b/pkg/install/daemonset.go index 8e74e16da..17580f05d 100644 --- a/pkg/install/daemonset.go +++ b/pkg/install/daemonset.go @@ -105,6 +105,7 @@ func DaemonSet(namespace string, opts ...podTemplateOption) *appsv1.DaemonSet { { Name: "node-agent", Image: c.image, + Ports: containerPorts(), ImagePullPolicy: pullPolicy, Command: []string{ "/velero",