From 714a8e7f9c662eb273e4a4bbec420a249a71c1df 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: Tue, 15 Oct 2024 13:46:11 +0800 Subject: [PATCH] Fix backup post hook issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix backup post hook issue: hook executes before the backup is finished Signed-off-by: Wenkai Yin(尹文开) --- changelogs/unreleased/8490-ywk253100 | 1 + internal/hook/handler.go | 159 +++ internal/hook/handler_test.go | 189 +++ internal/hook/hook.go | 64 + internal/hook/hook_test.go | 21 + internal/hook/item_hook_handler.go | 204 ---- internal/hook/item_hook_handler_test.go | 1030 ----------------- internal/hook/parser.go | 186 +++ internal/hook/parser_test.go | 251 ++++ internal/hook/pod_backup_post_hook_handler.go | 58 + .../hook/pod_backup_post_hook_handler_test.go | 82 ++ internal/hook/pod_backup_pre_hook_handler.go | 47 + .../hook/pod_backup_pre_hook_handler_test.go | 71 ++ internal/hook/resource_hook_handler.go | 16 + pkg/backup/backup.go | 131 +-- pkg/backup/backup_test.go | 76 +- pkg/backup/item_backupper.go | 3 - pkg/backup/request.go | 2 - pkg/podvolume/backupper.go | 38 +- pkg/podvolume/backupper_test.go | 31 +- pkg/podvolume/mocks/backupper.go | 174 +++ pkg/test/mock_pod_command_executor.go | 11 + 22 files changed, 1442 insertions(+), 1403 deletions(-) create mode 100644 changelogs/unreleased/8490-ywk253100 create mode 100644 internal/hook/handler.go create mode 100644 internal/hook/handler_test.go create mode 100644 internal/hook/hook.go create mode 100644 internal/hook/hook_test.go create mode 100644 internal/hook/parser.go create mode 100644 internal/hook/parser_test.go create mode 100644 internal/hook/pod_backup_post_hook_handler.go create mode 100644 internal/hook/pod_backup_post_hook_handler_test.go create mode 100644 internal/hook/pod_backup_pre_hook_handler.go create mode 100644 internal/hook/pod_backup_pre_hook_handler_test.go create mode 100644 internal/hook/resource_hook_handler.go create mode 100644 pkg/podvolume/mocks/backupper.go diff --git a/changelogs/unreleased/8490-ywk253100 b/changelogs/unreleased/8490-ywk253100 new file mode 100644 index 0000000000..d675afbabc --- /dev/null +++ b/changelogs/unreleased/8490-ywk253100 @@ -0,0 +1 @@ +Fix backup post hook issue \ No newline at end of file diff --git a/internal/hook/handler.go b/internal/hook/handler.go new file mode 100644 index 0000000000..5a7e969685 --- /dev/null +++ b/internal/hook/handler.go @@ -0,0 +1,159 @@ +package hook + +import ( + "context" + "sync" + "time" + + "github.com/pkg/errors" + "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/vmware-tanzu/velero/pkg/podexec" + "github.com/vmware-tanzu/velero/pkg/podvolume" +) + +// Handler handles all the hooks of one backup or restore. +// +// The pod's backup post hooks cannot execute until the PVBs are processed, Handler leverages the podvolume.Backupper to check this, +// and the podvolume.Backupper is per backup/restore, so one instance of Handler can only handle the hooks for one backup or restore. +// +// Handler only handles the hooks of pods for now, but it can be extended +// to other resources or even the hook defined for backup/restore if needed +type Handler interface { + // HandleResourceHooks handles a group of same type hooks for a specific resource, e.g. handles all backup pre hooks for one pod. + // Because whether to execute the hook may depend on the execution result of previous hooks (e.g. hooks will not execute + // if the previous hook is failed and marked as not continue), this function accepts a hook list as a group to handle. + // + // This function blocks until the hook completed, use "AsyncHandleResourceHooks()" instead if you want to handle the hooks asynchronously. + // + // The execution results are returned and also tracked inside the handler, calling the "WaitAllResourceHooksCompleted()" returns the results. + // + // This function only handles the hooks of pod for now, but it can be extended to other resources easily + HandleResourceHooks(ctx context.Context, log logrus.FieldLogger, resource *unstructured.Unstructured, hooks []*ResourceHook) []*ResourceHookResult + // AsyncHandleResourceHooks is the asynchronous version of "HandleResourceHooks()". + // + // Call "WaitAllHooksCompleted()" to wait all hooks completed and get the results. + AsyncHandleResourceHooks(ctx context.Context, log logrus.FieldLogger, resource *unstructured.Unstructured, hooks []*ResourceHook) + // WaitAllResourceHooksCompleted waits resource hooks completed and returns the execution results + WaitAllResourceHooksCompleted(ctx context.Context, log logrus.FieldLogger) *ResourceHookResults +} + +// make sure "handler" implements "Handler" interface +var _ Handler = &handler{} + +func NewHandler(podVolumeBackupper podvolume.Backupper, podCommandExecutor podexec.PodCommandExecutor) Handler { + return &handler{ + WaitGroup: &sync.WaitGroup{}, + results: &ResourceHookResults{ + RWMutex: &sync.RWMutex{}, + Results: []*ResourceHookResult{}, + }, + podVolumeBackupper: podVolumeBackupper, + podCommandExecutor: podCommandExecutor, + } +} + +type handler struct { + *sync.WaitGroup + results *ResourceHookResults + podVolumeBackupper podvolume.Backupper + podCommandExecutor podexec.PodCommandExecutor +} + +func (h *handler) HandleResourceHooks(ctx context.Context, log logrus.FieldLogger, resource *unstructured.Unstructured, hooks []*ResourceHook) []*ResourceHookResult { + if len(hooks) == 0 { + return nil + } + + var results []*ResourceHookResult + // make sure the results are tracked inside the handler + defer func() { + for _, result := range results { + h.results.AddResult(result) + } + }() + + markHooksFailed := func(hooks []*ResourceHook, err error) []*ResourceHookResult { + now := time.Now() + for _, hook := range hooks { + results = append(results, &ResourceHookResult{ + Hook: hook, + Status: StatusFailed, + StartTime: now, + EndTime: now, + Error: err, + }) + } + return results + } + + resourceHookHandler, err := h.getResourceHookHandler(hooks[0].Type) + if err != nil { + return markHooksFailed(hooks, errors.Wrapf(err, "failed to get the resource hook handler for type %q", hooks[0].Type)) + } + + if err = resourceHookHandler.WaitUntilReadyToExec(ctx, log, resource); err != nil { + return markHooksFailed(hooks, errors.Wrap(err, "failed to wait ready to execute hook")) + } + + for i, hook := range hooks { + now := time.Now() + result := &ResourceHookResult{ + Hook: hook, + StartTime: now, + } + + // execution failed + if err = resourceHookHandler.Exec(ctx, log, resource, hook); err != nil { + result.Status = StatusFailed + result.EndTime = time.Now() + result.Error = err + results = append(results, result) + // skip + if !hook.ContinueOnError { + markHooksFailed(hooks[i+1:], errors.New("skip because the previous hook execution failed")) + break + } + continue + } + + // execution completed + result.Status = StatusCompleted + result.EndTime = time.Now() + results = append(results, result) + } + + return results +} + +func (h *handler) AsyncHandleResourceHooks(ctx context.Context, log logrus.FieldLogger, resource *unstructured.Unstructured, hooks []*ResourceHook) { + n := len(hooks) + h.WaitGroup.Add(n) + go func() { + defer func() { + for i := 0; i < n; i++ { + h.Done() + } + }() + + results := h.HandleResourceHooks(ctx, log, resource, hooks) + _ = results + }() +} + +func (h *handler) WaitAllResourceHooksCompleted(ctx context.Context, log logrus.FieldLogger) *ResourceHookResults { + h.Wait() + return h.results +} + +func (h *handler) getResourceHookHandler(hookType string) (ResourceHookHandler, error) { + switch hookType { + case TypePodBackupPreHook: + return NewPodBackupPreHookHandler(h.podCommandExecutor), nil + case TypePodBackupPostHook: + return NewPodBackupPostHookHandler(h.podVolumeBackupper, h.podCommandExecutor), nil + default: + return nil, errors.Errorf("unknown hook type %q", hookType) + } +} diff --git a/internal/hook/handler_test.go b/internal/hook/handler_test.go new file mode 100644 index 0000000000..7c541ba2cb --- /dev/null +++ b/internal/hook/handler_test.go @@ -0,0 +1,189 @@ +package hook + +import ( + "context" + "errors" + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + mock_podvolume "github.com/vmware-tanzu/velero/pkg/podvolume/mocks" + "github.com/vmware-tanzu/velero/pkg/test" +) + +const ( + pod = `{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": { + "name": "nginx", + "namespace": "nginx", + "labels": { + "app": "nginx" + } + }, + "spec": { + "containers": [ + { + "name": "nginx", + "image": "nginx:1.14.2", + "ports": [ + { + "containerPort": 80 + } + ] + } + ] + } +} + ` +) + +func TestHandleResourceHooks(t *testing.T) { + podvolumeBackupper := mock_podvolume.NewMockBackupper(t) + podCMDExecutor := &test.MockPodCommandExecutor{} + handler := NewHandler(podvolumeBackupper, podCMDExecutor) + ctx := context.Background() + log := logrus.New() + res := &unstructured.Unstructured{} + err := res.UnmarshalJSON([]byte(pod)) + require.NoError(t, err) + var hooks []*ResourceHook + + // empty hooks list + results := handler.HandleResourceHooks(ctx, log, res, hooks) + require.Empty(t, results) + + // unknown hooks + hooks = []*ResourceHook{ + { + Name: "hook01", + Type: "unknown", + }, + { + Name: "hook02", + Type: "unknown", + }, + } + results = handler.HandleResourceHooks(ctx, log, res, hooks) + require.Len(t, results, 2) + assert.Equal(t, StatusFailed, results[0].Status) + assert.Equal(t, StatusFailed, results[1].Status) + + // skip other hooks if the former one failed and marked as not continue + podCMDExecutor.On("ExecutePodCommand", mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(errors.New("failed to exec command")) + hooks = []*ResourceHook{ + { + Name: "hook01", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + Resource: res, + ContinueOnError: true, + }, + { + Name: "hook02", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + Resource: res, + ContinueOnError: false, + }, + { + Name: "hook03", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + Resource: res, + ContinueOnError: false, + }, + } + results = handler.HandleResourceHooks(ctx, log, res, hooks) + require.Len(t, results, 3) + assert.Equal(t, StatusFailed, results[0].Status) + assert.Equal(t, StatusFailed, results[1].Status) + assert.Equal(t, StatusFailed, results[2].Status) + + // all completed + podCMDExecutor.On("ExecutePodCommand").Unset() + podCMDExecutor.On("ExecutePodCommand", mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(nil) + hooks = []*ResourceHook{ + { + Name: "hook01", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + Resource: res, + ContinueOnError: true, + }, + { + Name: "hook02", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + Resource: res, + ContinueOnError: false, + }, + } + results = handler.HandleResourceHooks(ctx, log, res, hooks) + require.Len(t, results, 2) + assert.Equal(t, StatusCompleted, results[0].Status) + assert.Equal(t, StatusCompleted, results[1].Status) +} + +func TestAsyncHandleResourceHooksAndWaitAllResourceHooksCompleted(t *testing.T) { + podvolumeBackupper := mock_podvolume.NewMockBackupper(t) + podCMDExecutor := &test.MockPodCommandExecutor{} + handler := NewHandler(podvolumeBackupper, podCMDExecutor) + ctx := context.Background() + log := logrus.New() + res := &unstructured.Unstructured{} + err := res.UnmarshalJSON([]byte(pod)) + require.NoError(t, err) + + podCMDExecutor.On("ExecutePodCommand", mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(nil) + hooks := []*ResourceHook{ + { + Name: "hook01", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + Resource: res, + ContinueOnError: true, + }, + { + Name: "hook02", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + Resource: res, + ContinueOnError: false, + }, + } + handler.AsyncHandleResourceHooks(ctx, log, res, hooks) + results := handler.WaitAllResourceHooksCompleted(ctx, log) + require.NotNil(t, results) + require.Equal(t, 2, results.Total) + require.Equal(t, 2, results.Completed) + assert.Equal(t, StatusCompleted, results.Results[0].Status) + assert.Equal(t, StatusCompleted, results.Results[1].Status) +} + +func Test_getResourceHookHandler(t *testing.T) { + handler := &handler{} + + // pod backup pre hook + resourceHookHandler, err := handler.getResourceHookHandler(TypePodBackupPreHook) + require.NoError(t, err) + assert.NotNil(t, resourceHookHandler) + + // pod backup post hook + resourceHookHandler, err = handler.getResourceHookHandler(TypePodBackupPostHook) + require.NoError(t, err) + assert.NotNil(t, resourceHookHandler) + + // unknown hook + _, err = handler.getResourceHookHandler("unknown") + require.Error(t, err) +} diff --git a/internal/hook/hook.go b/internal/hook/hook.go new file mode 100644 index 0000000000..79585d91ea --- /dev/null +++ b/internal/hook/hook.go @@ -0,0 +1,64 @@ +package hook + +import ( + "sync" + "time" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +const ( + TypePodBackupPreHook string = "POD_BACKUP_PRE_HOOk" + TypePodBackupPostHook string = "POD_BACKUP_POST_HOOk" + TypePodRestoreInitHook string = "POD_RESTORE_INIT_HOOk" + TypePodRestoreExecHook string = "POD_RESTORE_EXEC_HOOk" + + StatusCompleted string = "COMPLETED" // hook execution completed + StatusSkipped string = "SKIPPED" // hook execution skipped because of the last hook execution failed and the hook is marked as not continue + StatusFailed string = "FAILED" // hook execution failed +) + +// ResourceHook is the general resource hook definition handled by the Handler +type ResourceHook struct { + Name string + Type string + // multiple hooks may be defined under the same name backup/restore hook spec, the index here + // indicate the location of the hook in the list. This is helpful to locate the hook when needed + Index int + Spec interface{} + ContinueOnError bool + Resource *unstructured.Unstructured +} + +// the execution result for a specific resource hook +type ResourceHookResult struct { + Hook *ResourceHook + Status string + StartTime time.Time + EndTime time.Time + Error error +} + +// ResourceHookResults hold the execution results for all resource hooks of a specific backup/restore +type ResourceHookResults struct { + *sync.RWMutex + Total int + Completed int + Failed int + Results []*ResourceHookResult +} + +func (r *ResourceHookResults) AddResult(result *ResourceHookResult) { + r.Lock() + defer r.Unlock() + + r.Total++ + switch result.Status { + case StatusCompleted: + r.Completed++ + case StatusFailed: + r.Failed++ + } + + r.Results = append(r.Results, result) +} diff --git a/internal/hook/hook_test.go b/internal/hook/hook_test.go new file mode 100644 index 0000000000..5d68283095 --- /dev/null +++ b/internal/hook/hook_test.go @@ -0,0 +1,21 @@ +package hook + +import ( + "sync" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAddResult(t *testing.T) { + results := &ResourceHookResults{ + RWMutex: &sync.RWMutex{}, + Results: []*ResourceHookResult{}, + } + + results.AddResult(&ResourceHookResult{Status: StatusCompleted}) + results.AddResult(&ResourceHookResult{Status: StatusFailed}) + assert.Equal(t, 2, results.Total) + assert.Equal(t, 1, results.Completed) + assert.Equal(t, 1, results.Failed) +} diff --git a/internal/hook/item_hook_handler.go b/internal/hook/item_hook_handler.go index af8e84a043..bff7d94f4e 100644 --- a/internal/hook/item_hook_handler.go +++ b/internal/hook/item_hook_handler.go @@ -36,7 +36,6 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/kuberesource" - "github.com/vmware-tanzu/velero/pkg/podexec" "github.com/vmware-tanzu/velero/pkg/restorehelper" "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/collections" @@ -51,12 +50,6 @@ const ( ) const ( - // Backup hook annotations - podBackupHookContainerAnnotationKey = "hook.backup.velero.io/container" - podBackupHookCommandAnnotationKey = "hook.backup.velero.io/command" - podBackupHookOnErrorAnnotationKey = "hook.backup.velero.io/on-error" - podBackupHookTimeoutAnnotationKey = "hook.backup.velero.io/timeout" - // Restore hook annotations podRestoreHookContainerAnnotationKey = "post.hook.restore.velero.io/container" podRestoreHookCommandAnnotationKey = "post.hook.restore.velero.io/command" @@ -70,22 +63,6 @@ const ( podRestoreHookInitContainerTimeoutAnnotationKey = "init.hook.restore.velero.io/timeout" ) -// ItemHookHandler invokes hooks for an item. -type ItemHookHandler interface { - // HandleHooks invokes hooks for an item. If the item is a pod and the appropriate annotations exist - // to specify a hook, that is executed. Otherwise, this looks at the backup context's Backup to - // determine if there are any hooks relevant to the item, taking into account the hook spec's - // namespaces, resources, and label selector. - HandleHooks( - log logrus.FieldLogger, - groupResource schema.GroupResource, - obj runtime.Unstructured, - resourceHooks []ResourceHook, - phase HookPhase, - hookTracker *HookTracker, - ) error -} - // ItemRestoreHookHandler invokes restore hooks for an item type ItemRestoreHookHandler interface { HandleRestoreHooks( @@ -190,179 +167,6 @@ func (i *InitContainerRestoreHookHandler) HandleRestoreHooks( return &unstructured.Unstructured{Object: podMap}, nil } -// DefaultItemHookHandler is the default itemHookHandler. -type DefaultItemHookHandler struct { - PodCommandExecutor podexec.PodCommandExecutor -} - -func (h *DefaultItemHookHandler) HandleHooks( - log logrus.FieldLogger, - groupResource schema.GroupResource, - obj runtime.Unstructured, - resourceHooks []ResourceHook, - phase HookPhase, - hookTracker *HookTracker, -) error { - // We only support hooks on pods right now - if groupResource != kuberesource.Pods { - return nil - } - - metadata, err := meta.Accessor(obj) - if err != nil { - return errors.Wrap(err, "unable to get a metadata accessor") - } - - namespace := metadata.GetNamespace() - name := metadata.GetName() - - // If the pod has the hook specified via annotations, that takes priority. - hookFromAnnotations := getPodExecHookFromAnnotations(metadata.GetAnnotations(), phase, log) - if phase == PhasePre && hookFromAnnotations == nil { - // See if the pod has the legacy hook annotation keys (i.e. without a phase specified) - hookFromAnnotations = getPodExecHookFromAnnotations(metadata.GetAnnotations(), "", log) - } - if hookFromAnnotations != nil { - hookTracker.Add(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase) - - hookLog := log.WithFields( - logrus.Fields{ - "hookSource": HookSourceAnnotation, - "hookType": "exec", - "hookPhase": phase, - }, - ) - - hookFailed := false - var errExec error - if errExec = h.PodCommandExecutor.ExecutePodCommand(hookLog, obj.UnstructuredContent(), namespace, name, "", hookFromAnnotations); errExec != nil { - hookLog.WithError(errExec).Error("Error executing hook") - hookFailed = true - } - errTracker := hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, hookFailed, errExec) - if errTracker != nil { - hookLog.WithError(errTracker).Warn("Error recording the hook in hook tracker") - } - - if errExec != nil && hookFromAnnotations.OnError == velerov1api.HookErrorModeFail { - return errExec - } - - return nil - } - - labels := labels.Set(metadata.GetLabels()) - // Otherwise, check for hooks defined in the backup spec. - // modeFailError records the error from the hook with "Fail" error mode - var modeFailError error - for _, resourceHook := range resourceHooks { - if !resourceHook.Selector.applicableTo(groupResource, namespace, labels) { - continue - } - - var hooks []velerov1api.BackupResourceHook - if phase == PhasePre { - hooks = resourceHook.Pre - } else { - hooks = resourceHook.Post - } - - for _, hook := range hooks { - if groupResource == kuberesource.Pods { - if hook.Exec != nil { - hookTracker.Add(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase) - // The remaining hooks will only be executed if modeFailError is nil. - // Otherwise, execution will stop and only hook collection will occur. - if modeFailError == nil { - hookLog := log.WithFields( - logrus.Fields{ - "hookSource": HookSourceSpec, - "hookType": "exec", - "hookPhase": phase, - }, - ) - - hookFailed := false - err := h.PodCommandExecutor.ExecutePodCommand(hookLog, obj.UnstructuredContent(), namespace, name, resourceHook.Name, hook.Exec) - if err != nil { - hookLog.WithError(err).Error("Error executing hook") - hookFailed = true - if hook.Exec.OnError == velerov1api.HookErrorModeFail { - modeFailError = err - } - } - errTracker := hookTracker.Record(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, hookFailed, err) - if errTracker != nil { - hookLog.WithError(errTracker).Warn("Error recording the hook in hook tracker") - } - } - } - } - } - } - - return modeFailError -} - -// NoOpItemHookHandler is the an itemHookHandler for the Finalize controller where hooks don't run -type NoOpItemHookHandler struct{} - -func (h *NoOpItemHookHandler) HandleHooks( - log logrus.FieldLogger, - groupResource schema.GroupResource, - obj runtime.Unstructured, - resourceHooks []ResourceHook, - phase HookPhase, - hookTracker *HookTracker, -) error { - return nil -} - -func phasedKey(phase HookPhase, key string) string { - if phase != "" { - return fmt.Sprintf("%v.%v", phase, key) - } - return key -} - -func getHookAnnotation(annotations map[string]string, key string, phase HookPhase) string { - return annotations[phasedKey(phase, key)] -} - -// getPodExecHookFromAnnotations returns an ExecHook based on the annotations, as long as the -// 'command' annotation is present. If it is absent, this returns nil. -// If there is an error in parsing a supplied timeout, it is logged. -func getPodExecHookFromAnnotations(annotations map[string]string, phase HookPhase, log logrus.FieldLogger) *velerov1api.ExecHook { - commandValue := getHookAnnotation(annotations, podBackupHookCommandAnnotationKey, phase) - if commandValue == "" { - return nil - } - - container := getHookAnnotation(annotations, podBackupHookContainerAnnotationKey, phase) - - onError := velerov1api.HookErrorMode(getHookAnnotation(annotations, podBackupHookOnErrorAnnotationKey, phase)) - if onError != velerov1api.HookErrorModeContinue && onError != velerov1api.HookErrorModeFail { - onError = "" - } - - var timeout time.Duration - timeoutString := getHookAnnotation(annotations, podBackupHookTimeoutAnnotationKey, phase) - if timeoutString != "" { - if temp, err := time.ParseDuration(timeoutString); err == nil { - timeout = temp - } else { - log.Warn(errors.Wrapf(err, "Unable to parse provided timeout %s, using default", timeoutString)) - } - } - - return &velerov1api.ExecHook{ - Container: container, - Command: parseStringToCommand(commandValue), - OnError: onError, - Timeout: metav1.Duration{Duration: timeout}, - } -} - func parseStringToCommand(commandValue string) []string { var command []string // check for json array @@ -382,14 +186,6 @@ type ResourceHookSelector struct { LabelSelector labels.Selector } -// ResourceHook is a hook for a given resource. -type ResourceHook struct { - Name string - Selector ResourceHookSelector - Pre []velerov1api.BackupResourceHook - Post []velerov1api.BackupResourceHook -} - func (r ResourceHookSelector) applicableTo(groupResource schema.GroupResource, namespace string, labels labels.Set) bool { if r.Namespaces != nil && !r.Namespaces.ShouldInclude(namespace) { return false diff --git a/internal/hook/item_hook_handler_test.go b/internal/hook/item_hook_handler_test.go index 3ff9eee0cd..51d8633c0c 100644 --- a/internal/hook/item_hook_handler_test.go +++ b/internal/hook/item_hook_handler_test.go @@ -21,16 +21,12 @@ import ( "testing" "time" - "github.com/pkg/errors" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" corev1api "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" @@ -40,661 +36,6 @@ import ( "github.com/vmware-tanzu/velero/pkg/util/collections" ) -func TestHandleHooksSkips(t *testing.T) { - tests := []struct { - name string - groupResource string - item runtime.Unstructured - hooks []ResourceHook - }{ - { - name: "not a pod", - groupResource: "widget.group", - }, - { - name: "pod without annotation / no spec hooks", - item: velerotest.UnstructuredOrDie( - ` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "foo" - } - } - `, - ), - }, - { - name: "spec hooks not applicable", - groupResource: "pods", - item: velerotest.UnstructuredOrDie( - ` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "foo", - "labels": { - "color": "blue" - } - } - } - `, - ), - hooks: []ResourceHook{ - { - Name: "ns exclude", - Selector: ResourceHookSelector{Namespaces: collections.NewIncludesExcludes().Excludes("ns")}, - }, - { - Name: "resource exclude", - Selector: ResourceHookSelector{Resources: collections.NewIncludesExcludes().Includes("widgets.group")}, - }, - { - Name: "label selector mismatch", - Selector: ResourceHookSelector{LabelSelector: parseLabelSelectorOrDie("color=green")}, - }, - { - Name: "missing exec hook", - Pre: []velerov1api.BackupResourceHook{ - {}, - {}, - }, - }, - }, - }, - } - - hookTracker := NewHookTracker() - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - podCommandExecutor := &velerotest.MockPodCommandExecutor{} - defer podCommandExecutor.AssertExpectations(t) - - h := &DefaultItemHookHandler{ - PodCommandExecutor: podCommandExecutor, - } - - groupResource := schema.ParseGroupResource(test.groupResource) - err := h.HandleHooks(velerotest.NewLogger(), groupResource, test.item, test.hooks, PhasePre, hookTracker) - assert.NoError(t, err) - }) - } -} - -func TestHandleHooks(t *testing.T) { - tests := []struct { - name string - phase HookPhase - groupResource string - item runtime.Unstructured - hooks []ResourceHook - hookErrorsByContainer map[string]error - expectedError error - expectedPodHook *velerov1api.ExecHook - expectedPodHookError error - }{ - { - name: "pod, no annotation, spec (multiple pre hooks) = run spec", - phase: PhasePre, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name" - } - }`), - hooks: []ResourceHook{ - { - Name: "hook1", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"pre-1a"}, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "1b", - Command: []string{"pre-1b"}, - }, - }, - }, - }, - { - Name: "hook2", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "2a", - Command: []string{"2a"}, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "2b", - Command: []string{"2b"}, - }, - }, - }, - }, - }, - }, - { - name: "pod, no annotation, spec (multiple post hooks) = run spec", - phase: PhasePost, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name" - } - }`), - hooks: []ResourceHook{ - { - Name: "hook1", - Post: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"pre-1a"}, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "1b", - Command: []string{"pre-1b"}, - }, - }, - }, - }, - { - Name: "hook2", - Post: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "2a", - Command: []string{"2a"}, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "2b", - Command: []string{"2b"}, - }, - }, - }, - }, - }, - }, - { - name: "pod, annotation (legacy), no spec = run annotation", - phase: PhasePre, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "hook.backup.velero.io/container": "c", - "hook.backup.velero.io/command": "/bin/ls" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - }, - }, - { - name: "pod, annotation (pre), no spec = run annotation", - phase: PhasePre, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "pre.hook.backup.velero.io/container": "c", - "pre.hook.backup.velero.io/command": "/bin/ls" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - }, - }, - { - name: "pod, annotation (post), no spec = run annotation", - phase: PhasePost, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "post.hook.backup.velero.io/container": "c", - "post.hook.backup.velero.io/command": "/bin/ls" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - }, - }, - { - name: "pod, annotation & spec = run annotation", - phase: PhasePre, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "hook.backup.velero.io/container": "c", - "hook.backup.velero.io/command": "/bin/ls" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - }, - hooks: []ResourceHook{ - { - Name: "hook1", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"1a"}, - }, - }, - }, - }, - }, - }, - { - name: "pod, annotation, onError=fail = return error", - phase: PhasePre, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "hook.backup.velero.io/container": "c", - "hook.backup.velero.io/command": "/bin/ls", - "hook.backup.velero.io/on-error": "Fail" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - OnError: velerov1api.HookErrorModeFail, - }, - expectedPodHookError: errors.New("pod hook error"), - expectedError: errors.New("pod hook error"), - }, - { - name: "pod, annotation, onError=continue = return nil", - phase: PhasePre, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "hook.backup.velero.io/container": "c", - "hook.backup.velero.io/command": "/bin/ls", - "hook.backup.velero.io/on-error": "Continue" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - OnError: velerov1api.HookErrorModeContinue, - }, - expectedPodHookError: errors.New("pod hook error"), - expectedError: nil, - }, - { - name: "pod, spec, onError=fail = don't run other hooks", - phase: PhasePre, - groupResource: "pods", - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name" - } - }`), - hooks: []ResourceHook{ - { - Name: "hook1", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"1a"}, - OnError: velerov1api.HookErrorModeContinue, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "1b", - Command: []string{"1b"}, - }, - }, - }, - }, - { - Name: "hook2", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "2", - Command: []string{"2"}, - OnError: velerov1api.HookErrorModeFail, - }, - }, - }, - }, - { - Name: "hook3", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "3", - Command: []string{"3"}, - }, - }, - }, - }, - }, - hookErrorsByContainer: map[string]error{ - "1a": errors.New("1a error, but continue"), - "2": errors.New("2 error, fail"), - }, - expectedError: errors.New("2 error, fail"), - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - podCommandExecutor := &velerotest.MockPodCommandExecutor{} - defer podCommandExecutor.AssertExpectations(t) - - h := &DefaultItemHookHandler{ - PodCommandExecutor: podCommandExecutor, - } - - if test.expectedPodHook != nil { - podCommandExecutor.On("ExecutePodCommand", mock.Anything, test.item.UnstructuredContent(), "ns", "name", "", test.expectedPodHook).Return(test.expectedPodHookError) - } else { - hookLoop: - for _, resourceHook := range test.hooks { - for _, hook := range resourceHook.Pre { - hookError := test.hookErrorsByContainer[hook.Exec.Container] - podCommandExecutor.On("ExecutePodCommand", mock.Anything, test.item.UnstructuredContent(), "ns", "name", resourceHook.Name, hook.Exec).Return(hookError) - if hookError != nil && hook.Exec.OnError == velerov1api.HookErrorModeFail { - break hookLoop - } - } - for _, hook := range resourceHook.Post { - hookError := test.hookErrorsByContainer[hook.Exec.Container] - podCommandExecutor.On("ExecutePodCommand", mock.Anything, test.item.UnstructuredContent(), "ns", "name", resourceHook.Name, hook.Exec).Return(hookError) - if hookError != nil && hook.Exec.OnError == velerov1api.HookErrorModeFail { - break hookLoop - } - } - } - } - - groupResource := schema.ParseGroupResource(test.groupResource) - hookTracker := NewHookTracker() - err := h.HandleHooks(velerotest.NewLogger(), groupResource, test.item, test.hooks, test.phase, hookTracker) - - if test.expectedError != nil { - assert.EqualError(t, err, test.expectedError.Error()) - return - } - - require.NoError(t, err) - }) - } -} - -func TestGetPodExecHookFromAnnotations(t *testing.T) { - phases := []HookPhase{"", PhasePre, PhasePost} - for _, phase := range phases { - tests := []struct { - name string - annotations map[string]string - expectedHook *velerov1api.ExecHook - }{ - { - name: "missing command annotation", - expectedHook: nil, - }, - { - name: "malformed command json array", - annotations: map[string]string{ - phasedKey(phase, podBackupHookCommandAnnotationKey): "[blarg", - }, - expectedHook: &velerov1api.ExecHook{ - Command: []string{"[blarg"}, - }, - }, - { - name: "valid command json array", - annotations: map[string]string{ - phasedKey(phase, podBackupHookCommandAnnotationKey): `["a","b","c"]`, - }, - expectedHook: &velerov1api.ExecHook{ - Command: []string{"a", "b", "c"}, - }, - }, - { - name: "command as a string", - annotations: map[string]string{ - phasedKey(phase, podBackupHookCommandAnnotationKey): "/usr/bin/foo", - }, - expectedHook: &velerov1api.ExecHook{ - Command: []string{"/usr/bin/foo"}, - }, - }, - { - name: "hook mode set to continue", - annotations: map[string]string{ - phasedKey(phase, podBackupHookCommandAnnotationKey): "/usr/bin/foo", - phasedKey(phase, podBackupHookOnErrorAnnotationKey): string(velerov1api.HookErrorModeContinue), - }, - expectedHook: &velerov1api.ExecHook{ - Command: []string{"/usr/bin/foo"}, - OnError: velerov1api.HookErrorModeContinue, - }, - }, - { - name: "hook mode set to fail", - annotations: map[string]string{ - phasedKey(phase, podBackupHookCommandAnnotationKey): "/usr/bin/foo", - phasedKey(phase, podBackupHookOnErrorAnnotationKey): string(velerov1api.HookErrorModeFail), - }, - expectedHook: &velerov1api.ExecHook{ - Command: []string{"/usr/bin/foo"}, - OnError: velerov1api.HookErrorModeFail, - }, - }, - { - name: "use the specified timeout", - annotations: map[string]string{ - phasedKey(phase, podBackupHookCommandAnnotationKey): "/usr/bin/foo", - phasedKey(phase, podBackupHookTimeoutAnnotationKey): "5m3s", - }, - expectedHook: &velerov1api.ExecHook{ - Command: []string{"/usr/bin/foo"}, - Timeout: metav1.Duration{Duration: 5*time.Minute + 3*time.Second}, - }, - }, - { - name: "invalid timeout is logged", - annotations: map[string]string{ - phasedKey(phase, podBackupHookCommandAnnotationKey): "/usr/bin/foo", - phasedKey(phase, podBackupHookTimeoutAnnotationKey): "invalid", - }, - expectedHook: &velerov1api.ExecHook{ - Command: []string{"/usr/bin/foo"}, - }, - }, - { - name: "use the specified container", - annotations: map[string]string{ - phasedKey(phase, podBackupHookContainerAnnotationKey): "some-container", - phasedKey(phase, podBackupHookCommandAnnotationKey): "/usr/bin/foo", - }, - expectedHook: &velerov1api.ExecHook{ - Container: "some-container", - Command: []string{"/usr/bin/foo"}, - }, - }, - } - - for _, test := range tests { - t.Run(fmt.Sprintf("%s (phase=%q)", test.name, phase), func(t *testing.T) { - l := velerotest.NewLogger() - hook := getPodExecHookFromAnnotations(test.annotations, phase, l) - assert.Equal(t, test.expectedHook, hook) - }) - } - } -} - -func TestResourceHookApplicableTo(t *testing.T) { - tests := []struct { - name string - includedNamespaces []string - excludedNamespaces []string - includedResources []string - excludedResources []string - labelSelector string - namespace string - resource schema.GroupResource - labels labels.Set - expected bool - }{ - { - name: "allow anything", - namespace: "foo", - resource: schema.GroupResource{Group: "foo", Resource: "bar"}, - expected: true, - }, - { - name: "namespace in included list", - includedNamespaces: []string{"a", "b"}, - excludedNamespaces: []string{"c", "d"}, - namespace: "b", - expected: true, - }, - { - name: "namespace not in included list", - includedNamespaces: []string{"a", "b"}, - namespace: "c", - expected: false, - }, - { - name: "namespace excluded", - excludedNamespaces: []string{"a", "b"}, - namespace: "a", - expected: false, - }, - { - name: "resource in included list", - includedResources: []string{"foo.a", "bar.b"}, - excludedResources: []string{"baz.c"}, - resource: schema.GroupResource{Group: "a", Resource: "foo"}, - expected: true, - }, - { - name: "resource not in included list", - includedResources: []string{"foo.a", "bar.b"}, - resource: schema.GroupResource{Group: "c", Resource: "baz"}, - expected: false, - }, - { - name: "resource excluded", - excludedResources: []string{"foo.a", "bar.b"}, - resource: schema.GroupResource{Group: "b", Resource: "bar"}, - expected: false, - }, - { - name: "label selector matches", - labelSelector: "a=b", - labels: labels.Set{"a": "b"}, - expected: true, - }, - { - name: "label selector doesn't match", - labelSelector: "a=b", - labels: labels.Set{"a": "c"}, - expected: false, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - h := ResourceHook{ - Selector: ResourceHookSelector{ - Namespaces: collections.NewIncludesExcludes().Includes(test.includedNamespaces...).Excludes(test.excludedNamespaces...), - Resources: collections.NewIncludesExcludes().Includes(test.includedResources...).Excludes(test.excludedResources...), - }, - } - if test.labelSelector != "" { - selector, err := labels.Parse(test.labelSelector) - require.NoError(t, err) - h.Selector.LabelSelector = selector - } - - result := h.Selector.applicableTo(test.resource, test.namespace, test.labels) - assert.Equal(t, test.expected, result) - }) - } -} - func parseLabelSelectorOrDie(s string) labels.Selector { ret, err := labels.Parse(s) if err != nil { @@ -1988,377 +1329,6 @@ func TestValidateContainer(t *testing.T) { assert.Equal(t, expectedError, ValidateContainer([]byte(noCommand))) } -func TestBackupHookTracker(t *testing.T) { - type podWithHook struct { - item runtime.Unstructured - hooks []ResourceHook - hookErrorsByContainer map[string]error - expectedPodHook *velerov1api.ExecHook - expectedPodHookError error - expectedError error - } - test1 := []struct { - name string - phase HookPhase - groupResource string - pods []podWithHook - hookTracker *HookTracker - expectedHookAttempted int - expectedHookFailed int - }{ - { - name: "a pod with spec hooks, no error", - phase: PhasePre, - groupResource: "pods", - hookTracker: NewHookTracker(), - expectedHookAttempted: 2, - expectedHookFailed: 0, - pods: []podWithHook{ - { - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name" - } - }`), - hooks: []ResourceHook{ - { - Name: "hook1", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"pre-1a"}, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "1b", - Command: []string{"pre-1b"}, - }, - }, - }, - }, - }, - }, - }, - }, - { - name: "a pod with spec hooks and same container under different hook name, no error", - phase: PhasePre, - groupResource: "pods", - hookTracker: NewHookTracker(), - expectedHookAttempted: 4, - expectedHookFailed: 0, - pods: []podWithHook{ - { - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name" - } - }`), - hooks: []ResourceHook{ - { - Name: "hook1", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"pre-1a"}, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "1b", - Command: []string{"pre-1b"}, - }, - }, - }, - }, - { - Name: "hook2", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"2a"}, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "2b", - Command: []string{"2b"}, - }, - }, - }, - }, - }, - }, - }, - }, - { - name: "a pod with spec hooks, on error=fail", - phase: PhasePre, - groupResource: "pods", - hookTracker: NewHookTracker(), - expectedHookAttempted: 4, - expectedHookFailed: 2, - pods: []podWithHook{ - { - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name" - } - }`), - hooks: []ResourceHook{ - { - Name: "hook1", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"1a"}, - OnError: velerov1api.HookErrorModeContinue, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "1b", - Command: []string{"1b"}, - }, - }, - }, - }, - { - Name: "hook2", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "2", - Command: []string{"2"}, - OnError: velerov1api.HookErrorModeFail, - }, - }, - }, - }, - { - Name: "hook3", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "3", - Command: []string{"3"}, - }, - }, - }, - }, - }, - hookErrorsByContainer: map[string]error{ - "1a": errors.New("1a error, but continue"), - "2": errors.New("2 error, fail"), - }, - }, - }, - }, - { - name: "a pod with annotation and spec hooks", - phase: PhasePre, - groupResource: "pods", - hookTracker: NewHookTracker(), - expectedHookAttempted: 1, - expectedHookFailed: 0, - pods: []podWithHook{ - { - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "hook.backup.velero.io/container": "c", - "hook.backup.velero.io/command": "/bin/ls" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - }, - hooks: []ResourceHook{ - { - Name: "hook1", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"1a"}, - OnError: velerov1api.HookErrorModeContinue, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "1b", - Command: []string{"1b"}, - }, - }, - }, - }, - }, - }, - }, - }, - { - name: "a pod with annotation, on error=fail", - phase: PhasePre, - groupResource: "pods", - hookTracker: NewHookTracker(), - expectedHookAttempted: 1, - expectedHookFailed: 1, - pods: []podWithHook{ - { - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "hook.backup.velero.io/container": "c", - "hook.backup.velero.io/command": "/bin/ls", - "hook.backup.velero.io/on-error": "Fail" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - OnError: velerov1api.HookErrorModeFail, - }, - expectedPodHookError: errors.New("pod hook error"), - }, - }, - }, - { - name: "two pods, one with annotation, the other with spec", - phase: PhasePre, - groupResource: "pods", - hookTracker: NewHookTracker(), - expectedHookAttempted: 3, - expectedHookFailed: 1, - pods: []podWithHook{ - { - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name", - "annotations": { - "hook.backup.velero.io/container": "c", - "hook.backup.velero.io/command": "/bin/ls", - "hook.backup.velero.io/on-error": "Fail" - } - } - }`), - expectedPodHook: &velerov1api.ExecHook{ - Container: "c", - Command: []string{"/bin/ls"}, - OnError: velerov1api.HookErrorModeFail, - }, - expectedPodHookError: errors.New("pod hook error"), - }, - { - item: velerotest.UnstructuredOrDie(` - { - "apiVersion": "v1", - "kind": "Pod", - "metadata": { - "namespace": "ns", - "name": "name" - } - }`), - hooks: []ResourceHook{ - { - Name: "hook1", - Pre: []velerov1api.BackupResourceHook{ - { - Exec: &velerov1api.ExecHook{ - Container: "1a", - Command: []string{"pre-1a"}, - }, - }, - { - Exec: &velerov1api.ExecHook{ - Container: "1b", - Command: []string{"pre-1b"}, - }, - }, - }, - }, - }, - }, - }, - }, - } - - for _, test := range test1 { - t.Run(test.name, func(t *testing.T) { - podCommandExecutor := &velerotest.MockPodCommandExecutor{} - defer podCommandExecutor.AssertExpectations(t) - - h := &DefaultItemHookHandler{ - PodCommandExecutor: podCommandExecutor, - } - - groupResource := schema.ParseGroupResource(test.groupResource) - hookTracker := test.hookTracker - - for _, pod := range test.pods { - if pod.expectedPodHook != nil { - podCommandExecutor.On("ExecutePodCommand", mock.Anything, pod.item.UnstructuredContent(), "ns", "name", "", pod.expectedPodHook).Return(pod.expectedPodHookError) - } else { - hookLoop: - for _, resourceHook := range pod.hooks { - for _, hook := range resourceHook.Pre { - hookError := pod.hookErrorsByContainer[hook.Exec.Container] - podCommandExecutor.On("ExecutePodCommand", mock.Anything, pod.item.UnstructuredContent(), "ns", "name", resourceHook.Name, hook.Exec).Return(hookError) - if hookError != nil && hook.Exec.OnError == velerov1api.HookErrorModeFail { - break hookLoop - } - } - for _, hook := range resourceHook.Post { - hookError := pod.hookErrorsByContainer[hook.Exec.Container] - podCommandExecutor.On("ExecutePodCommand", mock.Anything, pod.item.UnstructuredContent(), "ns", "name", resourceHook.Name, hook.Exec).Return(hookError) - if hookError != nil && hook.Exec.OnError == velerov1api.HookErrorModeFail { - break hookLoop - } - } - } - } - h.HandleHooks(velerotest.NewLogger(), groupResource, pod.item, pod.hooks, test.phase, hookTracker) - } - actualAtemptted, actualFailed := hookTracker.Stat() - assert.Equal(t, test.expectedHookAttempted, actualAtemptted) - assert.Equal(t, test.expectedHookFailed, actualFailed) - }) - } -} - func TestRestoreHookTrackerAdd(t *testing.T) { testCases := []struct { name string diff --git a/internal/hook/parser.go b/internal/hook/parser.go new file mode 100644 index 0000000000..2105e31e69 --- /dev/null +++ b/internal/hook/parser.go @@ -0,0 +1,186 @@ +package hook + +import ( + "time" + + "github.com/pkg/errors" + "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" + + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/vmware-tanzu/velero/pkg/discovery" + "github.com/vmware-tanzu/velero/pkg/kuberesource" + "github.com/vmware-tanzu/velero/pkg/util/collections" +) + +const ( + nameFromAnnotation = "" + + annotationKeyPreBackupHookContainer = "pre.hook.backup.velero.io/container" + annotationKeyPreBackupHookCommand = "pre.hook.backup.velero.io/command" + annotationKeyPreBackupHookOnError = "pre.hook.backup.velero.io/on-error" + annotationKeyPreBackupHookTimeout = "pre.hook.backup.velero.io/timeout" + annotationKeyPostBackupHookContainer = "post.hook.backup.velero.io/container" + annotationKeyPostBackupHookCommand = "post.hook.backup.velero.io/command" + annotationKeyPostBackupHookOnError = "post.hook.backup.velero.io/on-error" + annotationKeyPostBackupHookTimeout = "post.hook.backup.velero.io/timeout" + + defaultExecTimeout = 30 * time.Second +) + +// Parser parses the different hook definitions into the general "ResourceHook" that the Handler can handle +type Parser interface { + // ListApplicableResourcePreBackupHooks returns the pre backup "ResourceHook" list applicable to the provided resource + ListApplicableResourcePreBackupHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hooks []velerov1.BackupResourceHookSpec) ([]*ResourceHook, error) + // ListApplicableResourcePostBackupHooks returns the post backup "ResourceHook" list applicable to the provided resource + ListApplicableResourcePostBackupHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hooks []velerov1.BackupResourceHookSpec) ([]*ResourceHook, error) + // ListApplicableResourcePreRestoreHooks returns the pre restore "ResourceHook" list applicable to the provided resource + ListApplicableResourcePreRestoreHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hooks []velerov1.RestoreResourceHookSpec) ([]*ResourceHook, error) + // ListApplicableResourcePostRestoreHooks returns the post restore "ResourceHook" list applicable to the provided resource + ListApplicableResourcePostRestoreHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hooks []velerov1.RestoreResourceHookSpec) ([]*ResourceHook, error) +} + +var _ Parser = &parser{} + +func NewParser(discoveryHelper discovery.Helper) Parser { + return &parser{ + discoveryHelper: discoveryHelper, + } +} + +type parser struct { + discoveryHelper discovery.Helper +} + +func (p *parser) ListApplicableResourcePreBackupHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hookSpecs []velerov1.BackupResourceHookSpec) ([]*ResourceHook, error) { + return p.listApplicableResourceBackupHooks(log, res, gr, hookSpecs, true) +} + +func (p *parser) ListApplicableResourcePostBackupHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hookSpecs []velerov1.BackupResourceHookSpec) ([]*ResourceHook, error) { + return p.listApplicableResourceBackupHooks(log, res, gr, hookSpecs, false) +} + +func (p *parser) ListApplicableResourcePreRestoreHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hooks []velerov1.RestoreResourceHookSpec) ([]*ResourceHook, error) { + return nil, errors.New("not implemented") +} + +func (p *parser) ListApplicableResourcePostRestoreHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hooks []velerov1.RestoreResourceHookSpec) ([]*ResourceHook, error) { + return nil, errors.New("not implemented") +} + +func (p *parser) listApplicableResourceBackupHooks(log logrus.FieldLogger, res *unstructured.Unstructured, gr schema.GroupResource, hookSpecs []velerov1.BackupResourceHookSpec, pre bool) ([]*ResourceHook, error) { + // only support hooks on pods right now + if gr != kuberesource.Pods { + return nil, nil + } + + annotationKeyContainer := annotationKeyPreBackupHookContainer + annotationKeyCommand := annotationKeyPreBackupHookCommand + annotationKeyTimeout := annotationKeyPreBackupHookTimeout + annotationKeyOnError := annotationKeyPreBackupHookOnError + hookType := TypePodBackupPreHook + + if !pre { + annotationKeyContainer = annotationKeyPostBackupHookContainer + annotationKeyCommand = annotationKeyPostBackupHookCommand + annotationKeyTimeout = annotationKeyPostBackupHookTimeout + annotationKeyOnError = annotationKeyPostBackupHookOnError + hookType = TypePodBackupPostHook + } + + var hooks []*ResourceHook + + // check the hook defined in the annotation + annotations := res.GetAnnotations() + if len(annotations) > 0 && len(annotations[annotationKeyCommand]) > 0 { + execHook := assembleExecHook(log, annotations[annotationKeyContainer], + annotations[annotationKeyCommand], annotations[annotationKeyTimeout], + annotations[annotationKeyOnError]) + if execHook != nil { + hooks = append(hooks, &ResourceHook{ + Name: nameFromAnnotation, + Type: hookType, + Spec: execHook, + ContinueOnError: execHook.OnError == velerov1.HookErrorModeContinue, + Resource: res, + }) + return hooks, nil + } + } + + // check the hooks defined in the backup spec + for _, hookSpec := range hookSpecs { + matches, err := matchesResource(res, gr, hookSpec.IncludedNamespaces, hookSpec.ExcludedNamespaces, hookSpec.IncludedResources, hookSpec.ExcludedResources, hookSpec.LabelSelector, p.discoveryHelper) + if err != nil { + return nil, err + } + if !matches { + continue + } + + hks := hookSpec.PreHooks + if !pre { + hks = hookSpec.PostHooks + } + for i, preHook := range hks { + hooks = append(hooks, &ResourceHook{ + Name: hookSpec.Name, + Type: hookType, + Index: i, + Spec: preHook.Exec, + ContinueOnError: preHook.Exec.OnError == velerov1.HookErrorModeContinue, + Resource: res, + }) + } + } + + return hooks, nil +} + +func assembleExecHook(log logrus.FieldLogger, container, command, timeout, onError string) *velerov1.ExecHook { + hook := &velerov1.ExecHook{ + Container: container, + Command: parseStringToCommand(command), + Timeout: metav1.Duration{Duration: defaultExecTimeout}, + } + + if len(timeout) > 0 { + if duration, err := time.ParseDuration(timeout); err == nil { + hook.Timeout = metav1.Duration{Duration: duration} + } else { + log.Warn(errors.Wrapf(err, "failed to parse the provided timeout %q, use the default value", timeout)) + } + } + + if velerov1.HookErrorMode(onError) == velerov1.HookErrorModeContinue || + velerov1.HookErrorMode(onError) == velerov1.HookErrorModeFail { + hook.OnError = velerov1.HookErrorMode(onError) + } + + return hook +} + +func matchesResource(res *unstructured.Unstructured, gr schema.GroupResource, includedNamespaces, excludedNamespaces, includedResources, excludedResources []string, + labelSelector *metav1.LabelSelector, discoveryHelper discovery.Helper) (bool, error) { + namespaceIncludesExcludes := collections.NewIncludesExcludes().Includes(includedNamespaces...).Excludes(excludedNamespaces...) + if !namespaceIncludesExcludes.ShouldInclude(res.GetNamespace()) { + return false, nil + } + + resourceIncludesExcludes := collections.GetResourceIncludesExcludes(discoveryHelper, includedResources, excludedResources) + if !resourceIncludesExcludes.ShouldInclude(gr.String()) { + return false, nil + } + + if labelSelector == nil { + return true, nil + } + selector, err := metav1.LabelSelectorAsSelector(labelSelector) + if err != nil { + return false, errors.Wrap(err, "failed to convert to label selector") + } + return selector.Matches(labels.Set(res.GetLabels())), nil +} diff --git a/internal/hook/parser_test.go b/internal/hook/parser_test.go new file mode 100644 index 0000000000..4b57119f61 --- /dev/null +++ b/internal/hook/parser_test.go @@ -0,0 +1,251 @@ +package hook + +import ( + "testing" + "time" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + discovery_mocks "github.com/vmware-tanzu/velero/pkg/discovery/mocks" +) + +func Test_listApplicableResourceBackupHooks(t *testing.T) { + discoveryHelper := discovery_mocks.NewHelper(t) + parser := &parser{ + discoveryHelper: discoveryHelper, + } + log := logrus.New() + res := &unstructured.Unstructured{} + err := res.UnmarshalJSON([]byte(pod)) + require.NoError(t, err) + annotations := map[string]string{ + "pre.hook.backup.velero.io/container": "nginx", + "pre.hook.backup.velero.io/command": "command-for-pre", + "pre.hook.backup.velero.io/on-error": "Continue", + "pre.hook.backup.velero.io/timeout": "60s", + "post.hook.backup.velero.io/container": "nginx", + "post.hook.backup.velero.io/command": "command-for-post", + "post.hook.backup.velero.io/on-error": "Fail", + "post.hook.backup.velero.io/timeout": "90s", + } + res.SetAnnotations(annotations) + + // not pod + gr := schema.GroupResource{ + Group: "", + Resource: "services", + } + hookSpecs := []velerov1.BackupResourceHookSpec{} + pre := true + hooks, err := parser.listApplicableResourceBackupHooks(log, res, gr, hookSpecs, pre) + require.NoError(t, err) + assert.Empty(t, hooks) + + // pre hook defined in resource annotation + gr = schema.GroupResource{ + Group: "", + Resource: "pods", + } + hookSpecs = []velerov1.BackupResourceHookSpec{} + pre = true + hooks, err = parser.listApplicableResourceBackupHooks(log, res, gr, hookSpecs, pre) + require.NoError(t, err) + require.Len(t, hooks, 1) + assert.Equal(t, "", hooks[0].Name) + assert.Equal(t, TypePodBackupPreHook, hooks[0].Type) + assert.Equal(t, 0, hooks[0].Index) + assert.True(t, hooks[0].ContinueOnError) + + // post hook defined in resource annotation + gr = schema.GroupResource{ + Group: "", + Resource: "pods", + } + hookSpecs = []velerov1.BackupResourceHookSpec{} + pre = false + hooks, err = parser.listApplicableResourceBackupHooks(log, res, gr, hookSpecs, pre) + require.NoError(t, err) + require.Len(t, hooks, 1) + assert.Equal(t, "", hooks[0].Name) + assert.Equal(t, TypePodBackupPostHook, hooks[0].Type) + assert.Equal(t, 0, hooks[0].Index) + assert.False(t, hooks[0].ContinueOnError) + + // hook defined in backup spec + res.SetAnnotations(nil) // remove the hooks defined in annotations + gr = schema.GroupResource{ + Group: "", + Resource: "pods", + } + hookSpecs = []velerov1.BackupResourceHookSpec{ + { + Name: "hook01", + PreHooks: []velerov1.BackupResourceHook{ + { + Exec: &velerov1api.ExecHook{ + Container: "nginx", + Command: []string{"command01"}, + OnError: velerov1.HookErrorModeContinue, + Timeout: metav1.Duration{ + Duration: 60 * time.Second, + }, + }, + }, + { + Exec: &velerov1api.ExecHook{ + Container: "nginx", + Command: []string{"command02"}, + OnError: velerov1.HookErrorModeContinue, + Timeout: metav1.Duration{ + Duration: 60 * time.Second, + }, + }, + }, + }, + }, + { + Name: "hook02", + PreHooks: []velerov1.BackupResourceHook{ + { + Exec: &velerov1api.ExecHook{ + Container: "nginx", + Command: []string{"command03"}, + OnError: velerov1.HookErrorModeFail, + Timeout: metav1.Duration{ + Duration: 90 * time.Second, + }, + }, + }, + }, + }, + } + pre = true + hooks, err = parser.listApplicableResourceBackupHooks(log, res, gr, hookSpecs, pre) + require.NoError(t, err) + require.Len(t, hooks, 3) + assert.Equal(t, "hook01", hooks[0].Name) + assert.Equal(t, TypePodBackupPreHook, hooks[0].Type) + assert.Equal(t, 0, hooks[0].Index) + assert.True(t, hooks[0].ContinueOnError) + assert.Equal(t, "hook01", hooks[1].Name) + assert.Equal(t, TypePodBackupPreHook, hooks[1].Type) + assert.Equal(t, 1, hooks[1].Index) + assert.True(t, hooks[1].ContinueOnError) + assert.Equal(t, "hook02", hooks[2].Name) + assert.Equal(t, TypePodBackupPreHook, hooks[2].Type) + assert.Equal(t, 0, hooks[2].Index) + assert.False(t, hooks[2].ContinueOnError) +} + +func Test_assembleExecHook(t *testing.T) { + log := logrus.New() + container := "container01" + command := "cmd" + + // invalid "timeout" and "onError" + timeout := "invalid" + onError := "invalid" + hook := assembleExecHook(log, container, command, timeout, onError) + assert.Equal(t, container, hook.Container) + assert.Equal(t, []string{"cmd"}, hook.Command) + assert.Equal(t, defaultExecTimeout, hook.Timeout.Duration) + assert.Equal(t, "", string(hook.OnError)) + + // valid "timeout" and "onError" + timeout = "60s" + onError = "Continue" + hook = assembleExecHook(log, container, command, timeout, onError) + assert.Equal(t, container, hook.Container) + assert.Equal(t, []string{"cmd"}, hook.Command) + assert.Equal(t, 60*time.Second, hook.Timeout.Duration) + assert.Equal(t, velerov1api.HookErrorModeContinue, hook.OnError) +} + +func Test_matchesResource(t *testing.T) { + res := &unstructured.Unstructured{} + err := res.UnmarshalJSON([]byte(pod)) + require.NoError(t, err) + + discoveryHelper := discovery_mocks.NewHelper(t) + + gr := schema.GroupResource{ + Group: "", + Resource: "pods", + } + + // check namespace + includedNamespaces := []string{"not-matched-namespace"} + excludedNamespaces := []string{} + includedResources := []string{} + excludedResources := []string{} + labelSelector := &metav1.LabelSelector{} + match, err := matchesResource(res, gr, includedNamespaces, excludedNamespaces, includedResources, excludedResources, labelSelector, discoveryHelper) + require.NoError(t, err) + assert.False(t, match) + + // check resource + includedNamespaces = []string{"nginx"} + excludedNamespaces = []string{} + includedResources = []string{"services"} + excludedResources = []string{} + labelSelector = &metav1.LabelSelector{} + discoveryHelper.On("ResourceFor", mock.Anything).Return(schema.GroupVersionResource{Version: "v1", Resource: "services"}, metav1.APIResource{}, nil) + match, err = matchesResource(res, gr, includedNamespaces, excludedNamespaces, includedResources, excludedResources, labelSelector, discoveryHelper) + require.NoError(t, err) + assert.False(t, match) + + // check label selector: invalid label selector + includedNamespaces = []string{"nginx"} + excludedNamespaces = []string{} + includedResources = []string{"pods"} + excludedResources = []string{} + labelSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "not-matched-label", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: "nonexistent-operator", + Values: []string{"not-matched-label"}, + }, + }, + } + discoveryHelper.On("ResourceFor").Unset() + discoveryHelper.On("ResourceFor", mock.Anything).Return(schema.GroupVersionResource{Version: "v1", Resource: "pods"}, metav1.APIResource{}, nil) + _, err = matchesResource(res, gr, includedNamespaces, excludedNamespaces, includedResources, excludedResources, labelSelector, discoveryHelper) + require.Error(t, err) + + // check label selector: valid label selector + includedNamespaces = []string{"nginx"} + excludedNamespaces = []string{} + includedResources = []string{"pods"} + excludedResources = []string{} + labelSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "not-matched-label", + }, + } + discoveryHelper.On("ResourceFor").Unset() + discoveryHelper.On("ResourceFor", mock.Anything).Return(schema.GroupVersionResource{Version: "v1", Resource: "pods"}, metav1.APIResource{}, nil) + match, err = matchesResource(res, gr, includedNamespaces, excludedNamespaces, includedResources, excludedResources, labelSelector, discoveryHelper) + require.NoError(t, err) + assert.False(t, match) + + labelSelector = &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "nginx", + }, + } + match, err = matchesResource(res, gr, includedNamespaces, excludedNamespaces, includedResources, excludedResources, labelSelector, discoveryHelper) + require.NoError(t, err) + assert.True(t, match) +} diff --git a/internal/hook/pod_backup_post_hook_handler.go b/internal/hook/pod_backup_post_hook_handler.go new file mode 100644 index 0000000000..8ac120afc6 --- /dev/null +++ b/internal/hook/pod_backup_post_hook_handler.go @@ -0,0 +1,58 @@ +package hook + +import ( + "context" + + "github.com/pkg/errors" + "github.com/sirupsen/logrus" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/vmware-tanzu/velero/pkg/podexec" + "github.com/vmware-tanzu/velero/pkg/podvolume" +) + +var _ ResourceHookHandler = &podBackupPostHookHandler{} + +func NewPodBackupPostHookHandler(backupper podvolume.Backupper, executor podexec.PodCommandExecutor) ResourceHookHandler { + return &podBackupPostHookHandler{ + podVolumeBackupper: backupper, + podCommandExecutor: executor, + } +} + +// podBackupPostHookHandler handles the post backup hooks for pods +type podBackupPostHookHandler struct { + podVolumeBackupper podvolume.Backupper + podCommandExecutor podexec.PodCommandExecutor +} + +// Don't need to wait when using CSI snapshot, because: +// +// PVCs are backed up before Pods (Pod's BIA returns PVCs as additional resources). +// PVC's BIA creates VS and returns it as additional resource. +// VS's BIA waits VSC's handle ready: https://github.com/vmware-tanzu/velero/blob/v1.14.1/pkg/backup/actions/csi/volumesnapshot_action.go#L118 +// +// So when Pods are backed up (when hooks execute), the snapshot is taken already +func (p *podBackupPostHookHandler) WaitUntilReadyToExec(ctx context.Context, log logrus.FieldLogger, res *unstructured.Unstructured) error { + // wait all PVBs processed + _, err := p.podVolumeBackupper.WaitAllPodVolumesProcessed(log) + return err +} + +func (p *podBackupPostHookHandler) Exec(ctx context.Context, log logrus.FieldLogger, res *unstructured.Unstructured, hook *ResourceHook) error { + execHook, ok := hook.Spec.(*velerov1.ExecHook) + if !ok { + return errors.New("failed to convert to ExecHook") + } + pod := &corev1.Pod{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(res.UnstructuredContent(), pod); err != nil { + return errors.Wrap(err, "failed to convert Unstructured to pod") + } + if err := p.podCommandExecutor.ExecutePodCommand(log, res.UnstructuredContent(), pod.Namespace, pod.Name, hook.Name, execHook); err != nil { + return errors.Wrap(err, "failed to execute pod command") + } + return nil +} diff --git a/internal/hook/pod_backup_post_hook_handler_test.go b/internal/hook/pod_backup_post_hook_handler_test.go new file mode 100644 index 0000000000..9340ee958f --- /dev/null +++ b/internal/hook/pod_backup_post_hook_handler_test.go @@ -0,0 +1,82 @@ +package hook + +import ( + "context" + "errors" + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + mock_podvolume "github.com/vmware-tanzu/velero/pkg/podvolume/mocks" + "github.com/vmware-tanzu/velero/pkg/test" +) + +func TestWaitUntilReadyToExecOf_podBackupPostHookHandler(t *testing.T) { + podvolumeBackupper := mock_podvolume.NewMockBackupper(t) + podCMDExecutor := &test.MockPodCommandExecutor{} + handler := NewPodBackupPostHookHandler(podvolumeBackupper, podCMDExecutor) + ctx := context.Background() + log := logrus.New() + res := &unstructured.Unstructured{} + err := res.UnmarshalJSON([]byte(pod)) + require.NoError(t, err) + + // failed to wait all pod volumes processed + podvolumeBackupper.On("WaitAllPodVolumesProcessed", mock.Anything).Return(nil, errors.New("timeout")) + err = handler.WaitUntilReadyToExec(ctx, log, res) + assert.Error(t, err) + + // succeed to wait all pod volumes processed + podvolumeBackupper.On("WaitAllPodVolumesProcessed").Unset() + podvolumeBackupper.On("WaitAllPodVolumesProcessed", mock.Anything).Return(nil, nil) + err = handler.WaitUntilReadyToExec(ctx, log, res) + assert.NoError(t, err) +} + +func TestExecOf_podBackupPostHookHandler(t *testing.T) { + podvolumeBackupper := mock_podvolume.NewMockBackupper(t) + podCMDExecutor := &test.MockPodCommandExecutor{} + handler := NewPodBackupPostHookHandler(podvolumeBackupper, podCMDExecutor) + ctx := context.Background() + log := logrus.New() + res := &unstructured.Unstructured{} + err := res.UnmarshalJSON([]byte(pod)) + require.NoError(t, err) + + // not velerov1.ExecHook + hook := &ResourceHook{ + Name: "hook01", + Type: TypePodBackupPostHook, + Spec: "invalid", + } + err = handler.Exec(ctx, log, res, hook) + assert.Error(t, err) + + // command exec failed + hook = &ResourceHook{ + Name: "hook01", + Type: TypePodBackupPostHook, + Spec: &velerov1.ExecHook{}, + } + podCMDExecutor.On("ExecutePodCommand", mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(errors.New("error")) + err = handler.Exec(ctx, log, res, hook) + assert.Error(t, err) + + // command exec succeed + hook = &ResourceHook{ + Name: "hook01", + Type: TypePodBackupPostHook, + Spec: &velerov1.ExecHook{}, + } + podCMDExecutor.On("ExecutePodCommand").Unset() + podCMDExecutor.On("ExecutePodCommand", mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(nil) + err = handler.Exec(ctx, log, res, hook) + assert.NoError(t, err) +} diff --git a/internal/hook/pod_backup_pre_hook_handler.go b/internal/hook/pod_backup_pre_hook_handler.go new file mode 100644 index 0000000000..f158070a96 --- /dev/null +++ b/internal/hook/pod_backup_pre_hook_handler.go @@ -0,0 +1,47 @@ +package hook + +import ( + "context" + + "github.com/pkg/errors" + "github.com/sirupsen/logrus" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/vmware-tanzu/velero/pkg/podexec" +) + +var _ ResourceHookHandler = &podBackupPreHookHandler{} + +func NewPodBackupPreHookHandler(executor podexec.PodCommandExecutor) ResourceHookHandler { + return &podBackupPreHookHandler{ + podCommandExecutor: executor, + } +} + +// podBackupPreHookHandler handles pre backup hooks for pods +type podBackupPreHookHandler struct { + podCommandExecutor podexec.PodCommandExecutor +} + +func (p *podBackupPreHookHandler) WaitUntilReadyToExec(ctx context.Context, log logrus.FieldLogger, res *unstructured.Unstructured) error { + // no-op + return nil +} + +func (p *podBackupPreHookHandler) Exec(ctx context.Context, log logrus.FieldLogger, res *unstructured.Unstructured, hook *ResourceHook) error { + execHook, ok := hook.Spec.(*velerov1.ExecHook) + if !ok { + return errors.New("failed to convert to ExecHook") + } + pod := &corev1.Pod{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(res.UnstructuredContent(), pod); err != nil { + return errors.Wrap(err, "failed to convert Unstructured to pod") + } + if err := p.podCommandExecutor.ExecutePodCommand(log, res.UnstructuredContent(), pod.Namespace, pod.Name, hook.Name, execHook); err != nil { + return errors.Wrap(err, "failed to execute pod command") + } + return nil +} diff --git a/internal/hook/pod_backup_pre_hook_handler_test.go b/internal/hook/pod_backup_pre_hook_handler_test.go new file mode 100644 index 0000000000..8fe3632137 --- /dev/null +++ b/internal/hook/pod_backup_pre_hook_handler_test.go @@ -0,0 +1,71 @@ +package hook + +import ( + "context" + "errors" + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "github.com/vmware-tanzu/velero/pkg/test" +) + +func TestWaitUntilReadyToExecOf_podBackupPreHookHandler(t *testing.T) { + podCMDExecutor := &test.MockPodCommandExecutor{} + handler := NewPodBackupPreHookHandler(podCMDExecutor) + ctx := context.Background() + log := logrus.New() + res := &unstructured.Unstructured{} + err := res.UnmarshalJSON([]byte(pod)) + require.NoError(t, err) + + err = handler.WaitUntilReadyToExec(ctx, log, res) + assert.NoError(t, err) +} + +func TestExecOf_podBackupPreHookHandler(t *testing.T) { + podCMDExecutor := &test.MockPodCommandExecutor{} + handler := NewPodBackupPreHookHandler(podCMDExecutor) + ctx := context.Background() + log := logrus.New() + res := &unstructured.Unstructured{} + err := res.UnmarshalJSON([]byte(pod)) + require.NoError(t, err) + + // not velerov1.ExecHook + hook := &ResourceHook{ + Name: "hook01", + Type: TypePodBackupPreHook, + Spec: "invalid", + } + err = handler.Exec(ctx, log, res, hook) + assert.Error(t, err) + + // command exec failed + hook = &ResourceHook{ + Name: "hook01", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + } + podCMDExecutor.On("ExecutePodCommand", mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(errors.New("error")) + err = handler.Exec(ctx, log, res, hook) + assert.Error(t, err) + + // command exec succeed + hook = &ResourceHook{ + Name: "hook01", + Type: TypePodBackupPreHook, + Spec: &velerov1.ExecHook{}, + } + podCMDExecutor.On("ExecutePodCommand").Unset() + podCMDExecutor.On("ExecutePodCommand", mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything).Return(nil) + err = handler.Exec(ctx, log, res, hook) + assert.NoError(t, err) +} diff --git a/internal/hook/resource_hook_handler.go b/internal/hook/resource_hook_handler.go new file mode 100644 index 0000000000..64787d93b6 --- /dev/null +++ b/internal/hook/resource_hook_handler.go @@ -0,0 +1,16 @@ +package hook + +import ( + "context" + + "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// ResourceHookHandler executes one hook for a specific Kubernetes resource +type ResourceHookHandler interface { + // WaitUntilReadyToExec waits the provided resource to be ready for hooks to execute + WaitUntilReadyToExec(ctx context.Context, log logrus.FieldLogger, resource *unstructured.Unstructured) error + // Exec executes the hook + Exec(ctx context.Context, log logrus.FieldLogger, resource *unstructured.Unstructured, hook *ResourceHook) error +} diff --git a/pkg/backup/backup.go b/pkg/backup/backup.go index 465f0753f4..7e531ff6f8 100644 --- a/pkg/backup/backup.go +++ b/pkg/backup/backup.go @@ -121,6 +121,7 @@ type kubernetesBackupper struct { uploaderType string pluginManager func(logrus.FieldLogger) clientmgmt.Manager backupStoreGetter persistence.ObjectBackupStoreGetter + hookParser hook.Parser } func (i *itemKey) String() string { @@ -163,6 +164,7 @@ func NewKubernetesBackupper( uploaderType: uploaderType, pluginManager: pluginManager, backupStoreGetter: backupStoreGetter, + hookParser: hook.NewParser(discoveryHelper), }, nil } @@ -172,43 +174,6 @@ func getNamespaceIncludesExcludes(backup *velerov1api.Backup) *collections.Inclu return collections.NewIncludesExcludes().Includes(backup.Spec.IncludedNamespaces...).Excludes(backup.Spec.ExcludedNamespaces...) } -func getResourceHooks(hookSpecs []velerov1api.BackupResourceHookSpec, discoveryHelper discovery.Helper) ([]hook.ResourceHook, error) { - resourceHooks := make([]hook.ResourceHook, 0, len(hookSpecs)) - - for _, s := range hookSpecs { - h, err := getResourceHook(s, discoveryHelper) - if err != nil { - return []hook.ResourceHook{}, err - } - - resourceHooks = append(resourceHooks, h) - } - - return resourceHooks, nil -} - -func getResourceHook(hookSpec velerov1api.BackupResourceHookSpec, discoveryHelper discovery.Helper) (hook.ResourceHook, error) { - h := hook.ResourceHook{ - Name: hookSpec.Name, - Selector: hook.ResourceHookSelector{ - Namespaces: collections.NewIncludesExcludes().Includes(hookSpec.IncludedNamespaces...).Excludes(hookSpec.ExcludedNamespaces...), - Resources: collections.GetResourceIncludesExcludes(discoveryHelper, hookSpec.IncludedResources, hookSpec.ExcludedResources), - }, - Pre: hookSpec.PreHooks, - Post: hookSpec.PostHooks, - } - - if hookSpec.LabelSelector != nil { - labelSelector, err := metav1.LabelSelectorAsSelector(hookSpec.LabelSelector) - if err != nil { - return hook.ResourceHook{}, errors.WithStack(err) - } - h.Selector.LabelSelector = labelSelector - } - - return h, nil -} - type VolumeSnapshotterGetter interface { GetVolumeSnapshotter(name string) (vsv1.VolumeSnapshotter, error) } @@ -279,11 +244,6 @@ func (kb *kubernetesBackupper) BackupWithResolvers( log.Infof("Backing up all volumes using pod volume backup: %t", boolptr.IsSetToTrue(backupRequest.Backup.Spec.DefaultVolumesToFsBackup)) var err error - backupRequest.ResourceHooks, err = getResourceHooks(backupRequest.Spec.Hooks.Resources, kb.discoveryHelper) - if err != nil { - log.WithError(errors.WithStack(err)).Debugf("Error from getResourceHooks") - return err - } backupRequest.ResolvedActions, err = backupItemActionResolver.ResolveActions(kb.discoveryHelper, log) if err != nil { @@ -365,10 +325,6 @@ func (kb *kubernetesBackupper) BackupWithResolvers( podVolumeBackupper: podVolumeBackupper, podVolumeSnapshotTracker: podvolume.NewTracker(), volumeSnapshotterGetter: volumeSnapshotterGetter, - itemHookHandler: &hook.DefaultItemHookHandler{ - PodCommandExecutor: kb.podCommandExecutor, - }, - hookTracker: hook.NewHookTracker(), volumeHelperImpl: volumehelper.NewVolumeHelperImpl( resourcePolicy, backupRequest.Spec.SnapshotVolumes, @@ -442,6 +398,8 @@ func (kb *kubernetesBackupper) BackupWithResolvers( itemsMap[key] = append(itemsMap[key], items[i]) } + hookHandler := hook.NewHandler(podVolumeBackupper, kb.podCommandExecutor) + var itemBlock *BackupItemBlock for i := range items { @@ -488,7 +446,7 @@ func (kb *kubernetesBackupper) BackupWithResolvers( addNextToBlock := i < len(items)-1 && items[i].orderedResource && items[i+1].orderedResource && items[i].groupResource == items[i+1].groupResource if itemBlock != nil && len(itemBlock.Items) > 0 && !addNextToBlock { log.Infof("Backing Up Item Block including %s %s/%s (%v items in block)", items[i].groupResource.String(), items[i].namespace, items[i].name, len(itemBlock.Items)) - backedUpGRs := kb.backupItemBlock(*itemBlock) + backedUpGRs := kb.backupItemBlock(hookHandler, *itemBlock) for _, backedUpGR := range backedUpGRs { backedUpGroupResources[backedUpGR] = true } @@ -528,9 +486,6 @@ func (kb *kubernetesBackupper) BackupWithResolvers( } } - processedPVBs := itemBackupper.podVolumeBackupper.WaitAllPodVolumesProcessed(log) - backupRequest.PodVolumeBackups = append(backupRequest.PodVolumeBackups, processedPVBs...) - // do a final update on progress since we may have just added some CRDs and may not have updated // for the last few processed items. updated = backupRequest.Backup.DeepCopy() @@ -541,13 +496,22 @@ func (kb *kubernetesBackupper) BackupWithResolvers( updated.Status.Progress.TotalItems = backedUpItems updated.Status.Progress.ItemsBackedUp = backedUpItems + results := hookHandler.WaitAllResourceHooksCompleted(ctx, log) + for _, result := range results.Results { + if result.Status == hook.StatusFailed { + log.WithError(result.Error).WithField("name", result.Hook.Resource.GetName()).Errorf("Error running %s hooks for pod", result.Hook.Type) + } + } // update the hooks execution status if updated.Status.HookStatus == nil { updated.Status.HookStatus = &velerov1api.HookStatus{} } - updated.Status.HookStatus.HooksAttempted, updated.Status.HookStatus.HooksFailed = itemBackupper.hookTracker.Stat() + updated.Status.HookStatus.HooksAttempted, updated.Status.HookStatus.HooksFailed = results.Total, results.Failed log.Debugf("hookAttempted: %d, hookFailed: %d", updated.Status.HookStatus.HooksAttempted, updated.Status.HookStatus.HooksFailed) + processedPVBs, _ := itemBackupper.podVolumeBackupper.WaitAllPodVolumesProcessed(log) + backupRequest.PodVolumeBackups = append(backupRequest.PodVolumeBackups, processedPVBs...) + if err := kube.PatchResource(backupRequest.Backup, updated, kb.kbClient); err != nil { log.WithError(errors.WithStack((err))).Warn("Got error trying to update backup's status.progress and hook status") } @@ -649,7 +613,7 @@ func (kb *kubernetesBackupper) executeItemBlockActions( } } -func (kb *kubernetesBackupper) backupItemBlock(itemBlock BackupItemBlock) []schema.GroupResource { +func (kb *kubernetesBackupper) backupItemBlock(hookHandler hook.Handler, itemBlock BackupItemBlock) []schema.GroupResource { // find pods in ItemBlock // filter pods based on whether they still need to be backed up // this list will be used to run pre/post hooks @@ -672,9 +636,35 @@ func (kb *kubernetesBackupper) backupItemBlock(itemBlock BackupItemBlock) []sche } } } - postHookPods, failedPods, errs := kb.handleItemBlockHooks(itemBlock, preHookPods, hook.PhasePre) - for i, pod := range failedPods { - itemBlock.Log.WithError(errs[i]).WithField("name", pod.Item.GetName()).Error("Error running pre hooks for pod") + + var successPods []itemblock.ItemBlockItem + var failedPods []itemblock.ItemBlockItem + + for _, pod := range preHookPods { + hooks, err := kb.hookParser.ListApplicableResourcePreBackupHooks(itemBlock.Log, pod.Item, pod.Gr, itemBlock.itemBackupper.backupRequest.Spec.Hooks.Resources) + if err != nil { + itemBlock.Log.WithError(errors.Wrap(err, "failed to list applicable resource pre backup hooks")) + failedPods = append(failedPods, pod) + continue + } + if len(hooks) == 0 { + successPods = append(successPods, pod) + continue + } + results := hookHandler.HandleResourceHooks(context.Background(), itemBlock.Log, pod.Item, hooks) + failed := false + for _, result := range results { + if result.Status == hook.StatusFailed { + failed = true + } + } + if failed { + failedPods = append(failedPods, pod) + } else { + successPods = append(successPods, pod) + } + } + for _, pod := range failedPods { // if pre hook fails, flag pod as backed-up and move on _, key, err := kb.itemMetadataAndKey(pod) if err != nil { @@ -693,9 +683,16 @@ func (kb *kubernetesBackupper) backupItemBlock(itemBlock BackupItemBlock) []sche } itemBlock.Log.Debug("Executing post hooks") - _, failedPods, errs = kb.handleItemBlockHooks(itemBlock, postHookPods, hook.PhasePost) - for i, pod := range failedPods { - itemBlock.Log.WithError(errs[i]).WithField("name", pod.Item.GetName()).Error("Error running post hooks for pod") + for _, pod := range successPods { + hooks, err := kb.hookParser.ListApplicableResourcePostBackupHooks(itemBlock.Log, pod.Item, pod.Gr, itemBlock.itemBackupper.backupRequest.Spec.Hooks.Resources) + if err != nil { + itemBlock.Log.WithError(errors.Wrap(err, "failed to list applicable resource pre backup hooks")) + continue + } + if len(hooks) == 0 { + continue + } + hookHandler.AsyncHandleResourceHooks(context.Background(), itemBlock.Log, pod.Item, hooks) } return grList @@ -714,22 +711,6 @@ func (kb *kubernetesBackupper) itemMetadataAndKey(item itemblock.ItemBlockItem) return metadata, key, nil } -func (kb *kubernetesBackupper) handleItemBlockHooks(itemBlock BackupItemBlock, hookPods []itemblock.ItemBlockItem, phase hook.HookPhase) ([]itemblock.ItemBlockItem, []itemblock.ItemBlockItem, []error) { - var successPods []itemblock.ItemBlockItem - var failedPods []itemblock.ItemBlockItem - var errs []error - for _, pod := range hookPods { - err := itemBlock.itemBackupper.itemHookHandler.HandleHooks(itemBlock.Log, pod.Gr, pod.Item, itemBlock.itemBackupper.backupRequest.ResourceHooks, phase, itemBlock.itemBackupper.hookTracker) - if err == nil { - successPods = append(successPods, pod) - } else { - failedPods = append(failedPods, pod) - errs = append(errs, err) - } - } - return successPods, failedPods, errs -} - func (kb *kubernetesBackupper) backupItem(log logrus.FieldLogger, gr schema.GroupResource, itemBackupper *itemBackupper, unstructured *unstructured.Unstructured, preferredGVR schema.GroupVersionResource, itemBlock *BackupItemBlock) bool { backedUpItem, _, err := itemBackupper.backupItem(log, unstructured, gr, preferredGVR, false, false, itemBlock) if aggregate, ok := err.(kubeerrs.Aggregate); ok { @@ -895,9 +876,7 @@ func (kb *kubernetesBackupper) FinalizeBackup( dynamicFactory: kb.dynamicFactory, kbClient: kb.kbClient, discoveryHelper: kb.discoveryHelper, - itemHookHandler: &hook.NoOpItemHookHandler{}, podVolumeSnapshotTracker: podvolume.NewTracker(), - hookTracker: hook.NewHookTracker(), } updateFiles := make(map[string]FileForArchive) backedUpGroupResources := map[schema.GroupResource]bool{} diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 3745b691ac..a1e2ce6fd5 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -41,6 +41,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "github.com/vmware-tanzu/velero/internal/hook" "github.com/vmware-tanzu/velero/internal/resourcepolicies" "github.com/vmware-tanzu/velero/internal/volume" "github.com/vmware-tanzu/velero/pkg/apis/velero/shared" @@ -3383,65 +3384,6 @@ func TestBackupWithAsyncOperations(t *testing.T) { } } -// TestBackupWithInvalidHooks runs backups with invalid hook specifications and verifies -// that an error is returned. -func TestBackupWithInvalidHooks(t *testing.T) { - tests := []struct { - name string - backup *velerov1.Backup - apiResources []*test.APIResource - want error - }{ - { - name: "hook with invalid label selector causes backup to fail", - backup: defaultBackup(). - Hooks(velerov1.BackupHooks{ - Resources: []velerov1.BackupResourceHookSpec{ - { - Name: "hook-with-invalid-label-selector", - LabelSelector: &metav1.LabelSelector{ - MatchExpressions: []metav1.LabelSelectorRequirement{ - { - Key: "foo", - Operator: metav1.LabelSelectorOperator("nonexistent-operator"), - Values: []string{"bar"}, - }, - }, - }, - }, - }, - }). - Result(), - apiResources: []*test.APIResource{ - test.Pods( - builder.ForPod("foo", "bar").Result(), - ), - }, - want: errors.New("\"nonexistent-operator\" is not a valid label selector operator"), - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - var ( - h = newHarness(t) - req = &Request{ - Backup: tc.backup, - SkippedPVTracker: NewSkipPVTracker(), - BackedUpItems: NewBackedUpItemsMap(), - } - backupFile = bytes.NewBuffer([]byte{}) - ) - - for _, resource := range tc.apiResources { - h.addItems(t, resource) - } - - assert.EqualError(t, h.backupper.Backup(h.log, req, backupFile, nil, nil, nil), tc.want.Error()) - }) - } -} - // TestBackupWithHooks runs backups with valid hook specifications and verifies that the // hooks are run. It uses a MockPodCommandExecutor since hooks can't actually be executed // in running pods during the unit test. Verification is done by asserting expected method @@ -3926,7 +3868,15 @@ func TestBackupWithHooks(t *testing.T) { require.NoError(t, h.backupper.Backup(h.log, req, backupFile, nil, tc.actions, nil)) if tc.wantHookExecutionLog != nil { - assert.Equal(t, tc.wantHookExecutionLog, podCommandExecutor.HookExecutionLog) + assert.Equal(t, len(tc.wantHookExecutionLog), len(podCommandExecutor.HookExecutionLog)) + m := map[string]struct{}{} + for _, entry := range podCommandExecutor.HookExecutionLog { + m[entry.String()] = struct{}{} + } + for _, expectedEntry := range tc.wantHookExecutionLog { + _, exist := m[expectedEntry.String()] + assert.True(t, exist) + } } assertTarballContents(t, backupFile, append(tc.wantBackedUp, "metadata/version")...) }) @@ -3964,8 +3914,8 @@ func (b *fakePodVolumeBackupper) BackupPodVolumes(backup *velerov1.Backup, pod * return res, pvcSummary, nil } -func (b *fakePodVolumeBackupper) WaitAllPodVolumesProcessed(log logrus.FieldLogger) []*velerov1.PodVolumeBackup { - return b.pvbs +func (b *fakePodVolumeBackupper) WaitAllPodVolumesProcessed(log logrus.FieldLogger) ([]*velerov1.PodVolumeBackup, error) { + return b.pvbs, nil } // TestBackupWithPodVolume runs backups of pods that are annotated for PodVolume backup, @@ -4233,6 +4183,8 @@ func newHarness(t *testing.T) *harness { podCommandExecutor: nil, podVolumeBackupperFactory: new(fakePodVolumeBackupperFactory), podVolumeTimeout: 0, + + hookParser: hook.NewParser(discoveryHelper), }, log: log, } diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index 9e5caef0ed..d7486a22e0 100644 --- a/pkg/backup/item_backupper.go +++ b/pkg/backup/item_backupper.go @@ -37,7 +37,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" kbClient "sigs.k8s.io/controller-runtime/pkg/client" - "github.com/vmware-tanzu/velero/internal/hook" "github.com/vmware-tanzu/velero/internal/resourcepolicies" "github.com/vmware-tanzu/velero/internal/volume" "github.com/vmware-tanzu/velero/internal/volumehelper" @@ -72,9 +71,7 @@ type itemBackupper struct { podVolumeSnapshotTracker *podvolume.Tracker volumeSnapshotterGetter VolumeSnapshotterGetter - itemHookHandler hook.ItemHookHandler snapshotLocationVolumeSnapshotters map[string]vsv1.VolumeSnapshotter - hookTracker *hook.HookTracker volumeHelperImpl volumehelper.VolumeHelper } diff --git a/pkg/backup/request.go b/pkg/backup/request.go index f89510933d..064a874a31 100644 --- a/pkg/backup/request.go +++ b/pkg/backup/request.go @@ -17,7 +17,6 @@ limitations under the License. package backup import ( - "github.com/vmware-tanzu/velero/internal/hook" "github.com/vmware-tanzu/velero/internal/resourcepolicies" "github.com/vmware-tanzu/velero/internal/volume" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -41,7 +40,6 @@ type Request struct { SnapshotLocations []*velerov1api.VolumeSnapshotLocation NamespaceIncludesExcludes *collections.IncludesExcludes ResourceIncludesExcludes collections.IncludesExcludesInterface - ResourceHooks []hook.ResourceHook ResolvedActions []framework.BackupItemResolvedActionV2 ResolvedItemBlockActions []framework.ItemBlockResolvedAction VolumeSnapshots []*volume.Snapshot diff --git a/pkg/podvolume/backupper.go b/pkg/podvolume/backupper.go index 0a0c63eff1..a1a5fe1f03 100644 --- a/pkg/podvolume/backupper.go +++ b/pkg/podvolume/backupper.go @@ -47,19 +47,21 @@ import ( type Backupper interface { // BackupPodVolumes backs up all specified volumes in a pod. BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api.Pod, volumesToBackup []string, resPolicies *resourcepolicies.Policies, log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, *PVCBackupSummary, []error) - WaitAllPodVolumesProcessed(log logrus.FieldLogger) []*velerov1api.PodVolumeBackup + WaitAllPodVolumesProcessed(log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, error) } type backupper struct { - ctx context.Context - repoLocker *repository.RepoLocker - repoEnsurer *repository.Ensurer - crClient ctrlclient.Client - uploaderType string - pvbInformer ctrlcache.Informer - handlerRegistration cache.ResourceEventHandlerRegistration - wg sync.WaitGroup - result []*velerov1api.PodVolumeBackup + *sync.Mutex + ctx context.Context + repoLocker *repository.RepoLocker + repoEnsurer *repository.Ensurer + crClient ctrlclient.Client + uploaderType string + pvbInformer ctrlcache.Informer + handlerRegistration cache.ResourceEventHandlerRegistration + wg sync.WaitGroup + result []*velerov1api.PodVolumeBackup + allPodVolumesProcessed bool } type skippedPVC struct { @@ -119,6 +121,7 @@ func newBackupper( pvbInformer: pvbInformer, wg: sync.WaitGroup{}, result: []*velerov1api.PodVolumeBackup{}, + Mutex: &sync.Mutex{}, } b.handlerRegistration, _ = pvbInformer.AddEventHandler( @@ -319,7 +322,13 @@ func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api. return podVolumeBackups, pvcSummary, errs } -func (b *backupper) WaitAllPodVolumesProcessed(log logrus.FieldLogger) []*velerov1api.PodVolumeBackup { +func (b *backupper) WaitAllPodVolumesProcessed(log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, error) { + b.Lock() + defer b.Unlock() + if b.allPodVolumesProcessed { + return b.result, nil + } + defer func() { if err := b.pvbInformer.RemoveEventHandler(b.handlerRegistration); err != nil { log.Debugf("failed to remove the event handler for PVB: %v", err) @@ -330,12 +339,15 @@ func (b *backupper) WaitAllPodVolumesProcessed(log logrus.FieldLogger) []*velero go func() { defer close(done) b.wg.Wait() + b.allPodVolumesProcessed = true }() var podVolumeBackups []*velerov1api.PodVolumeBackup select { case <-b.ctx.Done(): - log.Error("timed out waiting for all PodVolumeBackups to complete") + err := fmt.Errorf("timed out waiting for all PodVolumeBackups to complete") + log.Error(err) + return nil, err case <-done: for _, pvb := range b.result { podVolumeBackups = append(podVolumeBackups, pvb) @@ -344,7 +356,7 @@ func (b *backupper) WaitAllPodVolumesProcessed(log logrus.FieldLogger) []*velero } } } - return podVolumeBackups + return podVolumeBackups, nil } func skipAllPodVolumes(pod *corev1api.Pod, volumesToBackup []string, err error, pvcSummary *PVCBackupSummary, log logrus.FieldLogger) { diff --git a/pkg/podvolume/backupper_test.go b/pkg/podvolume/backupper_test.go index fe50f9e30e..92371a3e90 100644 --- a/pkg/podvolume/backupper_test.go +++ b/pkg/podvolume/backupper_test.go @@ -599,16 +599,18 @@ func TestWaitAllPodVolumesProcessed(t *testing.T) { cancelFunc() }() cases := []struct { - name string - ctx context.Context - statusToBeUpdated *velerov1api.PodVolumeBackupStatus - expectedErr string - expectedPVBPhase velerov1api.PodVolumeBackupPhase + name string + ctx context.Context + statusToBeUpdated *velerov1api.PodVolumeBackupStatus + expectedError bool + expectedErrorMessage string + expectedPVBPhase velerov1api.PodVolumeBackupPhase }{ { - name: "context canceled", - ctx: timeoutCtx, - expectedErr: "timed out waiting for all PodVolumeBackups to complete", + name: "context canceled", + expectedError: true, + ctx: timeoutCtx, + expectedErrorMessage: "timed out waiting for all PodVolumeBackups to complete", }, { name: "failed pvbs", @@ -617,8 +619,8 @@ func TestWaitAllPodVolumesProcessed(t *testing.T) { Phase: velerov1api.PodVolumeBackupPhaseFailed, Message: "failed", }, - expectedPVBPhase: velerov1api.PodVolumeBackupPhaseFailed, - expectedErr: "pod volume backup failed: failed", + expectedPVBPhase: velerov1api.PodVolumeBackupPhaseFailed, + expectedErrorMessage: "pod volume backup failed: failed", }, { name: "completed pvbs", @@ -666,10 +668,13 @@ func TestWaitAllPodVolumesProcessed(t *testing.T) { require.NoError(t, err) } - pvbs := backuper.WaitAllPodVolumesProcessed(logger) + pvbs, err := backuper.WaitAllPodVolumesProcessed(logger) + if c.expectedError { + require.Error(t, err) + } - if c.expectedErr != "" { - assert.Equal(t, c.expectedErr, logHook.entry.Message) + if c.expectedErrorMessage != "" { + assert.Equal(t, c.expectedErrorMessage, logHook.entry.Message) } if c.expectedPVBPhase != "" { diff --git a/pkg/podvolume/mocks/backupper.go b/pkg/podvolume/mocks/backupper.go new file mode 100644 index 0000000000..8e0e92deb9 --- /dev/null +++ b/pkg/podvolume/mocks/backupper.go @@ -0,0 +1,174 @@ +// Code generated by mockery v2.49.1. DO NOT EDIT. + +package mocks + +import ( + logrus "github.com/sirupsen/logrus" + corev1 "k8s.io/api/core/v1" + + mock "github.com/stretchr/testify/mock" + + podvolume "github.com/vmware-tanzu/velero/pkg/podvolume" + + resourcepolicies "github.com/vmware-tanzu/velero/internal/resourcepolicies" + + v1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" +) + +// MockBackupper is an autogenerated mock type for the Backupper type +type MockBackupper struct { + mock.Mock +} + +type MockBackupper_Expecter struct { + mock *mock.Mock +} + +func (_m *MockBackupper) EXPECT() *MockBackupper_Expecter { + return &MockBackupper_Expecter{mock: &_m.Mock} +} + +// BackupPodVolumes provides a mock function with given fields: backup, pod, volumesToBackup, resPolicies, log +func (_m *MockBackupper) BackupPodVolumes(backup *v1.Backup, pod *corev1.Pod, volumesToBackup []string, resPolicies *resourcepolicies.Policies, log logrus.FieldLogger) ([]*v1.PodVolumeBackup, *podvolume.PVCBackupSummary, []error) { + ret := _m.Called(backup, pod, volumesToBackup, resPolicies, log) + + if len(ret) == 0 { + panic("no return value specified for BackupPodVolumes") + } + + var r0 []*v1.PodVolumeBackup + var r1 *podvolume.PVCBackupSummary + var r2 []error + if rf, ok := ret.Get(0).(func(*v1.Backup, *corev1.Pod, []string, *resourcepolicies.Policies, logrus.FieldLogger) ([]*v1.PodVolumeBackup, *podvolume.PVCBackupSummary, []error)); ok { + return rf(backup, pod, volumesToBackup, resPolicies, log) + } + if rf, ok := ret.Get(0).(func(*v1.Backup, *corev1.Pod, []string, *resourcepolicies.Policies, logrus.FieldLogger) []*v1.PodVolumeBackup); ok { + r0 = rf(backup, pod, volumesToBackup, resPolicies, log) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*v1.PodVolumeBackup) + } + } + + if rf, ok := ret.Get(1).(func(*v1.Backup, *corev1.Pod, []string, *resourcepolicies.Policies, logrus.FieldLogger) *podvolume.PVCBackupSummary); ok { + r1 = rf(backup, pod, volumesToBackup, resPolicies, log) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*podvolume.PVCBackupSummary) + } + } + + if rf, ok := ret.Get(2).(func(*v1.Backup, *corev1.Pod, []string, *resourcepolicies.Policies, logrus.FieldLogger) []error); ok { + r2 = rf(backup, pod, volumesToBackup, resPolicies, log) + } else { + if ret.Get(2) != nil { + r2 = ret.Get(2).([]error) + } + } + + return r0, r1, r2 +} + +// MockBackupper_BackupPodVolumes_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'BackupPodVolumes' +type MockBackupper_BackupPodVolumes_Call struct { + *mock.Call +} + +// BackupPodVolumes is a helper method to define mock.On call +// - backup *v1.Backup +// - pod *corev1.Pod +// - volumesToBackup []string +// - resPolicies *resourcepolicies.Policies +// - log logrus.FieldLogger +func (_e *MockBackupper_Expecter) BackupPodVolumes(backup interface{}, pod interface{}, volumesToBackup interface{}, resPolicies interface{}, log interface{}) *MockBackupper_BackupPodVolumes_Call { + return &MockBackupper_BackupPodVolumes_Call{Call: _e.mock.On("BackupPodVolumes", backup, pod, volumesToBackup, resPolicies, log)} +} + +func (_c *MockBackupper_BackupPodVolumes_Call) Run(run func(backup *v1.Backup, pod *corev1.Pod, volumesToBackup []string, resPolicies *resourcepolicies.Policies, log logrus.FieldLogger)) *MockBackupper_BackupPodVolumes_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(*v1.Backup), args[1].(*corev1.Pod), args[2].([]string), args[3].(*resourcepolicies.Policies), args[4].(logrus.FieldLogger)) + }) + return _c +} + +func (_c *MockBackupper_BackupPodVolumes_Call) Return(_a0 []*v1.PodVolumeBackup, _a1 *podvolume.PVCBackupSummary, _a2 []error) *MockBackupper_BackupPodVolumes_Call { + _c.Call.Return(_a0, _a1, _a2) + return _c +} + +func (_c *MockBackupper_BackupPodVolumes_Call) RunAndReturn(run func(*v1.Backup, *corev1.Pod, []string, *resourcepolicies.Policies, logrus.FieldLogger) ([]*v1.PodVolumeBackup, *podvolume.PVCBackupSummary, []error)) *MockBackupper_BackupPodVolumes_Call { + _c.Call.Return(run) + return _c +} + +// WaitAllPodVolumesProcessed provides a mock function with given fields: log +func (_m *MockBackupper) WaitAllPodVolumesProcessed(log logrus.FieldLogger) ([]*v1.PodVolumeBackup, error) { + ret := _m.Called(log) + + if len(ret) == 0 { + panic("no return value specified for WaitAllPodVolumesProcessed") + } + + var r0 []*v1.PodVolumeBackup + var r1 error + if rf, ok := ret.Get(0).(func(logrus.FieldLogger) ([]*v1.PodVolumeBackup, error)); ok { + return rf(log) + } + if rf, ok := ret.Get(0).(func(logrus.FieldLogger) []*v1.PodVolumeBackup); ok { + r0 = rf(log) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*v1.PodVolumeBackup) + } + } + + if rf, ok := ret.Get(1).(func(logrus.FieldLogger) error); ok { + r1 = rf(log) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockBackupper_WaitAllPodVolumesProcessed_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'WaitAllPodVolumesProcessed' +type MockBackupper_WaitAllPodVolumesProcessed_Call struct { + *mock.Call +} + +// WaitAllPodVolumesProcessed is a helper method to define mock.On call +// - log logrus.FieldLogger +func (_e *MockBackupper_Expecter) WaitAllPodVolumesProcessed(log interface{}) *MockBackupper_WaitAllPodVolumesProcessed_Call { + return &MockBackupper_WaitAllPodVolumesProcessed_Call{Call: _e.mock.On("WaitAllPodVolumesProcessed", log)} +} + +func (_c *MockBackupper_WaitAllPodVolumesProcessed_Call) Run(run func(log logrus.FieldLogger)) *MockBackupper_WaitAllPodVolumesProcessed_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(logrus.FieldLogger)) + }) + return _c +} + +func (_c *MockBackupper_WaitAllPodVolumesProcessed_Call) Return(_a0 []*v1.PodVolumeBackup, _a1 error) *MockBackupper_WaitAllPodVolumesProcessed_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockBackupper_WaitAllPodVolumesProcessed_Call) RunAndReturn(run func(logrus.FieldLogger) ([]*v1.PodVolumeBackup, error)) *MockBackupper_WaitAllPodVolumesProcessed_Call { + _c.Call.Return(run) + return _c +} + +// NewMockBackupper creates a new instance of MockBackupper. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMockBackupper(t interface { + mock.TestingT + Cleanup(func()) +}) *MockBackupper { + mock := &MockBackupper{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/test/mock_pod_command_executor.go b/pkg/test/mock_pod_command_executor.go index 414ae30860..6b7a612e51 100644 --- a/pkg/test/mock_pod_command_executor.go +++ b/pkg/test/mock_pod_command_executor.go @@ -16,6 +16,10 @@ limitations under the License. package test import ( + "fmt" + "strings" + "sync" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/mock" @@ -24,6 +28,7 @@ import ( type MockPodCommandExecutor struct { mock.Mock + sync.Mutex // hook execution order HookExecutionLog []HookExecutionEntry } @@ -33,7 +38,13 @@ type HookExecutionEntry struct { HookCommand []string } +func (h *HookExecutionEntry) String() string { + return fmt.Sprintf("%s.%s.%s.%s", h.Namespace, h.Name, h.HookName, strings.Join(h.HookCommand, ".")) +} + func (e *MockPodCommandExecutor) ExecutePodCommand(log logrus.FieldLogger, item map[string]interface{}, namespace, name, hookName string, hook *v1.ExecHook) error { + e.Lock() + defer e.Unlock() e.HookExecutionLog = append(e.HookExecutionLog, HookExecutionEntry{ Namespace: namespace, Name: name,