diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index 95531cb6b5..4750cb0edb 100644 --- a/pkg/controller/data_download_controller.go +++ b/pkg/controller/data_download_controller.go @@ -431,7 +431,9 @@ func prepareDataDownload(ssb *velerov2alpha1api.DataDownload) { } func (r *DataDownloadReconciler) errorOut(ctx context.Context, dd *velerov2alpha1api.DataDownload, err error, msg string, log logrus.FieldLogger) (ctrl.Result, error) { - r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(dd)) + if r.restoreExposer != nil { + r.restoreExposer.CleanUp(ctx, getDataDownloadOwnerObject(dd)) + } return ctrl.Result{}, r.updateStatusToFailed(ctx, dd, err, msg, log) } diff --git a/pkg/controller/data_download_controller_test.go b/pkg/controller/data_download_controller_test.go index 7ccf20e4e6..4317ef76d1 100644 --- a/pkg/controller/data_download_controller_test.go +++ b/pkg/controller/data_download_controller_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "testing" + "time" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -27,23 +28,25 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clientgofake "k8s.io/client-go/kubernetes/fake" ctrl "sigs.k8s.io/controller-runtime" kbclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/vmware-tanzu/velero/internal/credentials" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" velerov2alpha1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1" "github.com/vmware-tanzu/velero/pkg/builder" "github.com/vmware-tanzu/velero/pkg/datapath" + datapathmockes "github.com/vmware-tanzu/velero/pkg/datapath/mocks" "github.com/vmware-tanzu/velero/pkg/exposer" velerotest "github.com/vmware-tanzu/velero/pkg/test" - + "github.com/vmware-tanzu/velero/pkg/uploader" "sigs.k8s.io/controller-runtime/pkg/client/fake" - datapathmockes "github.com/vmware-tanzu/velero/pkg/datapath/mocks" exposermockes "github.com/vmware-tanzu/velero/pkg/exposer/mocks" ) @@ -85,10 +88,15 @@ func initDataDownloadReconciler(objects []runtime.Object, needError ...bool) (*D fakeClient.updateError = needError[2] fakeClient.patchError = needError[3] } + var fakeKubeClient *clientgofake.Clientset + if len(objects) != 0 { + fakeKubeClient = clientgofake.NewSimpleClientset(objects...) + } else { + fakeKubeClient = clientgofake.NewSimpleClientset() + } - fakeKubeClient := clientgofake.NewSimpleClientset(objects...) fakeFS := velerotest.NewFakeFileSystem() - pathGlob := fmt.Sprintf("/host_pods/%s/volumes/*/%s", "", dataDownloadName) + pathGlob := fmt.Sprintf("/host_pods/%s/volumes/*/%s", "test-uid", "test-pvc") _, err = fakeFS.Create(pathGlob) if err != nil { return nil, err @@ -113,10 +121,104 @@ func TestDataDownloadReconcile(t *testing.T) { targetPVC *corev1.PersistentVolumeClaim dataMgr *datapath.Manager needErrs []bool + needCreateFSBR bool isExposeErr bool isGetExposeErr bool + isNilExposer bool + isFSBRInitErr bool + isFSBRRestoreErr bool + notNilExpose bool + notMockCleanUp bool + mockCancel bool expectedStatusMsg string + expectedResult *ctrl.Result }{ + { + name: "Unknown data download status", + dd: dataDownloadBuilder().Phase("Unknown").Cancel(true).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + }, + { + name: "Cancel data downloand in progress and patch data download error", + dd: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseInProgress).Cancel(true).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + needErrs: []bool{false, false, false, true}, + needCreateFSBR: true, + expectedStatusMsg: "Patch error", + }, + { + name: "Cancel data downloand in progress with empty FSBR", + dd: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseInProgress).Cancel(true).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + mockCancel: true, + }, + { + name: "Cancel data downloand in progress", + dd: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseInProgress).Cancel(true).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + needCreateFSBR: true, + mockCancel: true, + }, + { + name: "Error in data path is concurrent limited", + dd: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhasePrepared).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + dataMgr: datapath.NewManager(0), + notNilExpose: true, + notMockCleanUp: true, + expectedResult: &ctrl.Result{Requeue: true, RequeueAfter: time.Minute}, + }, + { + name: "Error getting volume directory name for pvc in pod", + dd: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhasePrepared).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + notNilExpose: true, + expectedStatusMsg: "error identifying unique volume path on host", + }, + { + name: "Unable to update status to in progress for data download", + dd: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhasePrepared).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + needErrs: []bool{false, false, false, true}, + notNilExpose: true, + notMockCleanUp: true, + expectedStatusMsg: "Patch error", + }, + { + name: "accept DataDownload error", + dd: dataDownloadBuilder().Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + needErrs: []bool{false, false, true, false}, + expectedStatusMsg: "Update error", + }, + { + name: "Not create target pvc", + dd: dataDownloadBuilder().Result(), + }, + { + name: "Uninitialized dataDownload", + dd: dataDownloadBuilder().Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + isNilExposer: true, + expectedStatusMsg: "uninitialized generic exposer", + }, + { + name: "DataDownload not created in velero default namespace", + dd: builder.ForDataDownload("test-ns", dataDownloadName).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + }, + { + name: "Failed to get dataDownload", + dd: builder.ForDataDownload("test-ns", dataDownloadName).Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + needErrs: []bool{true, false, false, false}, + expectedStatusMsg: "Create error", + }, + { + name: "Unsupported dataDownload type", + dd: dataDownloadBuilder().DataMover("Unsuppoorted type").Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + }, { name: "Restore is exposed", dd: dataDownloadBuilder().Result(), @@ -134,11 +236,22 @@ func TestDataDownloadReconcile(t *testing.T) { expectedStatusMsg: "Error to get restore exposer", isGetExposeErr: true, }, + { + name: "Error to start restore expose", + dd: dataDownloadBuilder().Result(), + targetPVC: builder.ForPersistentVolumeClaim("test-ns", "test-pvc").Result(), + expectedStatusMsg: "Error to expose restore exposer", + isExposeErr: true, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - r, err := initDataDownloadReconciler([]runtime.Object{test.targetPVC}, test.needErrs...) + var objs []runtime.Object + if test.targetPVC != nil { + objs = []runtime.Object{test.targetPVC} + } + r, err := initDataDownloadReconciler(objs, test.needErrs...) require.NoError(t, err) defer func() { r.client.Delete(ctx, test.dd, &kbclient.DeleteOptions{}) @@ -160,31 +273,44 @@ func TestDataDownloadReconcile(t *testing.T) { } datapath.FSBRCreator = func(string, string, kbclient.Client, string, datapath.Callbacks, logrus.FieldLogger) datapath.AsyncBR { - return datapathmockes.NewAsyncBR(t) + fsBR := datapathmockes.NewAsyncBR(t) + if test.mockCancel { + fsBR.On("Cancel").Return() + } + return fsBR } - if test.isExposeErr || test.isGetExposeErr { - r.restoreExposer = func() exposer.GenericRestoreExposer { - ep := exposermockes.NewGenericRestoreExposer(t) - if test.isExposeErr { - ep.On("Expose", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errors.New("Error to expose restore exposer")) - } - - if test.isGetExposeErr { - ep.On("GetExposed", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("Error to get restore exposer")) - } + if test.isExposeErr || test.isGetExposeErr || test.isNilExposer || test.notNilExpose { + if test.isNilExposer { + r.restoreExposer = nil + } else { + r.restoreExposer = func() exposer.GenericRestoreExposer { + ep := exposermockes.NewGenericRestoreExposer(t) + if test.isExposeErr { + ep.On("Expose", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errors.New("Error to expose restore exposer")) + } else if test.notNilExpose { + hostingPod := builder.ForPod("test-ns", "test-name").Volumes(&corev1.Volume{Name: "test-pvc"}).Result() + hostingPod.ObjectMeta.SetUID("test-uid") + ep.On("GetExposed", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(&exposer.ExposeResult{ByPod: exposer.ExposeByPod{HostingPod: hostingPod, PVC: "test-pvc"}}, nil) + } else if test.isGetExposeErr { + ep.On("GetExposed", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("Error to get restore exposer")) + } - ep.On("CleanUp", mock.Anything, mock.Anything).Return() - return ep - }() + if !test.notMockCleanUp { + ep.On("CleanUp", mock.Anything, mock.Anything).Return() + } + return ep + }() + } } - if test.dd.Status.Phase == velerov2alpha1api.DataDownloadPhaseInProgress { + if test.needCreateFSBR { if fsBR := r.dataPathMgr.GetAsyncBR(test.dd.Name); fsBR == nil { _, err := r.dataPathMgr.CreateFileSystemBR(test.dd.Name, pVBRRequestor, ctx, r.client, velerov1api.DefaultNamespace, datapath.Callbacks{OnCancelled: r.OnDataDownloadCancelled}, velerotest.NewLogger()) require.NoError(t, err) } } + actualResult, err := r.Reconcile(ctx, ctrl.Request{ NamespacedName: types.NamespacedName{ Namespace: velerov1api.DefaultNamespace, @@ -192,7 +318,7 @@ func TestDataDownloadReconcile(t *testing.T) { }, }) - if test.isGetExposeErr { + if test.expectedStatusMsg != "" { assert.Contains(t, err.Error(), test.expectedStatusMsg) } else { require.Nil(t, err) @@ -200,6 +326,11 @@ func TestDataDownloadReconcile(t *testing.T) { require.NotNil(t, actualResult) + if test.expectedResult != nil { + assert.Equal(t, test.expectedResult.Requeue, test.expectedResult.Requeue) + assert.Equal(t, test.expectedResult.RequeueAfter, test.expectedResult.RequeueAfter) + } + dd := velerov2alpha1api.DataDownload{} err = r.client.Get(ctx, kbclient.ObjectKey{ Name: test.dd.Name, @@ -209,8 +340,235 @@ func TestDataDownloadReconcile(t *testing.T) { if test.isGetExposeErr { assert.Contains(t, dd.Status.Message, test.expectedStatusMsg) } - require.Nil(t, err) + if test.dd.Namespace == velerov1api.DefaultNamespace { + require.Nil(t, err) + } else { + assert.True(t, true, apierrors.IsNotFound(err)) + } + t.Logf("%s: \n %v \n", test.name, dd) }) } } + +func TestOnDataDownloadFailed(t *testing.T) { + for _, getErr := range []bool{true, false} { + ctx := context.TODO() + needErrs := []bool{getErr, false, false, false} + r, err := initDataDownloadReconciler(nil, needErrs...) + require.NoError(t, err) + + dd := dataDownloadBuilder().Result() + namespace := dd.Namespace + ddName := dd.Name + // Add the DataDownload object to the fake client + assert.NoError(t, r.client.Create(ctx, dd)) + r.OnDataDownloadFailed(ctx, namespace, ddName, fmt.Errorf("Failed to handle %v", ddName)) + updatedDD := &velerov2alpha1api.DataDownload{} + if getErr { + assert.Error(t, r.client.Get(ctx, types.NamespacedName{Name: ddName, Namespace: namespace}, updatedDD)) + assert.NotEqual(t, velerov2alpha1api.DataDownloadPhaseFailed, updatedDD.Status.Phase) + assert.Equal(t, updatedDD.Status.StartTimestamp.IsZero(), true) + } else { + assert.NoError(t, r.client.Get(ctx, types.NamespacedName{Name: ddName, Namespace: namespace}, updatedDD)) + assert.Equal(t, velerov2alpha1api.DataDownloadPhaseFailed, updatedDD.Status.Phase) + assert.Equal(t, updatedDD.Status.StartTimestamp.IsZero(), true) + } + } +} + +func TestOnDataDownloadCancelled(t *testing.T) { + for _, getErr := range []bool{true, false} { + ctx := context.TODO() + needErrs := []bool{getErr, false, false, false} + r, err := initDataDownloadReconciler(nil, needErrs...) + require.NoError(t, err) + + dd := dataDownloadBuilder().Result() + namespace := dd.Namespace + ddName := dd.Name + // Add the DataDownload object to the fake client + assert.NoError(t, r.client.Create(ctx, dd)) + r.OnDataDownloadCancelled(ctx, namespace, ddName) + updatedDD := &velerov2alpha1api.DataDownload{} + if getErr { + assert.Error(t, r.client.Get(ctx, types.NamespacedName{Name: ddName, Namespace: namespace}, updatedDD)) + assert.NotEqual(t, velerov2alpha1api.DataDownloadPhaseFailed, updatedDD.Status.Phase) + assert.Equal(t, updatedDD.Status.StartTimestamp.IsZero(), true) + } else { + assert.NoError(t, r.client.Get(ctx, types.NamespacedName{Name: ddName, Namespace: namespace}, updatedDD)) + assert.Equal(t, velerov2alpha1api.DataDownloadPhaseCanceled, updatedDD.Status.Phase) + assert.Equal(t, updatedDD.Status.StartTimestamp.IsZero(), false) + assert.Equal(t, updatedDD.Status.CompletionTimestamp.IsZero(), false) + } + } +} + +func TestOnDataDownloadCompleted(t *testing.T) { + tests := []struct { + name string + emptyFSBR bool + isGetErr bool + rebindVolumeErr bool + }{ + { + name: "Data download complete", + emptyFSBR: false, + isGetErr: false, + rebindVolumeErr: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.TODO() + needErrs := []bool{test.isGetErr, false, false, false} + r, err := initDataDownloadReconciler(nil, needErrs...) + r.restoreExposer = func() exposer.GenericRestoreExposer { + ep := exposermockes.NewGenericRestoreExposer(t) + if test.rebindVolumeErr { + ep.On("RebindVolume", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errors.New("Error to rebind volume")) + + } else { + ep.On("RebindVolume", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + } + ep.On("CleanUp", mock.Anything, mock.Anything).Return() + return ep + }() + + require.NoError(t, err) + dd := dataDownloadBuilder().Result() + namespace := dd.Namespace + ddName := dd.Name + // Add the DataDownload object to the fake client + assert.NoError(t, r.client.Create(ctx, dd)) + r.OnDataDownloadCompleted(ctx, namespace, ddName, datapath.Result{}) + updatedDD := &velerov2alpha1api.DataDownload{} + if test.isGetErr { + assert.Error(t, r.client.Get(ctx, types.NamespacedName{Name: ddName, Namespace: namespace}, updatedDD)) + assert.Equal(t, velerov2alpha1api.DataDownloadPhase(""), updatedDD.Status.Phase) + assert.Equal(t, updatedDD.Status.CompletionTimestamp.IsZero(), true) + } else { + assert.NoError(t, r.client.Get(ctx, types.NamespacedName{Name: ddName, Namespace: namespace}, updatedDD)) + assert.Equal(t, velerov2alpha1api.DataDownloadPhaseCompleted, updatedDD.Status.Phase) + assert.Equal(t, updatedDD.Status.CompletionTimestamp.IsZero(), false) + } + }) + } +} + +func TestOnDataDownloadProgress(t *testing.T) { + totalBytes := int64(1024) + bytesDone := int64(512) + tests := []struct { + name string + dd *velerov2alpha1api.DataDownload + progress uploader.Progress + needErrs []bool + }{ + { + name: "patch in progress phase success", + dd: dataDownloadBuilder().Result(), + progress: uploader.Progress{ + TotalBytes: totalBytes, + BytesDone: bytesDone, + }, + }, + { + name: "failed to get datadownload", + dd: dataDownloadBuilder().Result(), + needErrs: []bool{true, false, false, false}, + }, + { + name: "failed to patch datadownload", + dd: dataDownloadBuilder().Result(), + needErrs: []bool{false, false, false, true}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := context.TODO() + + r, err := initDataDownloadReconciler(nil, test.needErrs...) + require.NoError(t, err) + defer func() { + r.client.Delete(ctx, test.dd, &kbclient.DeleteOptions{}) + }() + // Create a DataDownload object + dd := dataDownloadBuilder().Result() + namespace := dd.Namespace + duName := dd.Name + // Add the DataDownload object to the fake client + assert.NoError(t, r.client.Create(context.Background(), dd)) + + // Create a Progress object + progress := &uploader.Progress{ + TotalBytes: totalBytes, + BytesDone: bytesDone, + } + + // Call the OnDataDownloadProgress function + r.OnDataDownloadProgress(ctx, namespace, duName, progress) + if len(test.needErrs) != 0 && !test.needErrs[0] { + // Get the updated DataDownload object from the fake client + updatedDu := &velerov2alpha1api.DataDownload{} + assert.NoError(t, r.client.Get(ctx, types.NamespacedName{Name: duName, Namespace: namespace}, updatedDu)) + // Assert that the DataDownload object has been updated with the progress + assert.Equal(t, test.progress.TotalBytes, updatedDu.Status.Progress.TotalBytes) + assert.Equal(t, test.progress.BytesDone, updatedDu.Status.Progress.BytesDone) + } + }) + } +} + +func TestFindDataDownloadForPod(t *testing.T) { + needErrs := []bool{false, false, false, false} + r, err := initDataDownloadReconciler(nil, needErrs...) + require.NoError(t, err) + tests := []struct { + name string + du *velerov2alpha1api.DataDownload + pod *corev1.Pod + checkFunc func(*velerov2alpha1api.DataDownload, []reconcile.Request) + }{ + { + name: "find dataDownload for pod", + du: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseAccepted).Result(), + pod: builder.ForPod(velerov1api.DefaultNamespace, dataDownloadName).Labels(map[string]string{velerov1api.DataDownloadLabel: dataDownloadName}).Result(), + checkFunc: func(du *velerov2alpha1api.DataDownload, requests []reconcile.Request) { + // Assert that the function returns a single request + assert.Len(t, requests, 1) + // Assert that the request contains the correct namespaced name + assert.Equal(t, du.Namespace, requests[0].Namespace) + assert.Equal(t, du.Name, requests[0].Name) + }, + }, { + name: "no matched pod", + du: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseAccepted).Result(), + pod: builder.ForPod(velerov1api.DefaultNamespace, dataDownloadName).Labels(map[string]string{velerov1api.DataDownloadLabel: "non-existing-datadownload"}).Result(), + checkFunc: func(du *velerov2alpha1api.DataDownload, requests []reconcile.Request) { + assert.Empty(t, requests) + }, + }, + { + name: "dataDownload not accepte", + du: dataDownloadBuilder().Phase(velerov2alpha1api.DataDownloadPhaseInProgress).Result(), + pod: builder.ForPod(velerov1api.DefaultNamespace, dataDownloadName).Labels(map[string]string{velerov1api.DataDownloadLabel: dataDownloadName}).Result(), + checkFunc: func(du *velerov2alpha1api.DataDownload, requests []reconcile.Request) { + assert.Empty(t, requests) + }, + }, + } + for _, test := range tests { + ctx := context.Background() + assert.NoError(t, r.client.Create(ctx, test.pod)) + assert.NoError(t, r.client.Create(ctx, test.du)) + // Call the findDataDownloadForPod function + requests := r.findSnapshotRestoreForPod(test.pod) + test.checkFunc(test.du, requests) + r.client.Delete(ctx, test.du, &kbclient.DeleteOptions{}) + if test.pod != nil { + r.client.Delete(ctx, test.pod, &kbclient.DeleteOptions{}) + } + } +}