diff --git a/changelogs/unreleased/7153-allenxu404 b/changelogs/unreleased/7153-allenxu404 new file mode 100644 index 00000000000..a8faaf99c9a --- /dev/null +++ b/changelogs/unreleased/7153-allenxu404 @@ -0,0 +1 @@ +Enhance hooks tracker by adding a returned error to record function \ No newline at end of file diff --git a/internal/hook/hook_tracker.go b/internal/hook/hook_tracker.go index f4e2bb817ee..a0300d8f634 100644 --- a/internal/hook/hook_tracker.go +++ b/internal/hook/hook_tracker.go @@ -16,7 +16,10 @@ limitations under the License. package hook -import "sync" +import ( + "fmt" + "sync" +) const ( HookSourceAnnotation = "annotation" @@ -69,6 +72,8 @@ func NewHookTracker() *HookTracker { } // Add adds a hook to the tracker +// Add must precede the Record for each individual hook. +// In other words, a hook must be added to the tracker before its execution result is recorded. func (ht *HookTracker) Add(podNamespace, podName, container, source, hookName string, hookPhase hookPhase) { ht.lock.Lock() defer ht.lock.Unlock() @@ -91,7 +96,9 @@ func (ht *HookTracker) Add(podNamespace, podName, container, source, hookName st } // Record records the hook's execution status -func (ht *HookTracker) Record(podNamespace, podName, container, source, hookName string, hookPhase hookPhase, hookFailed bool) { +// Add must precede the Record for each individual hook. +// In other words, a hook must be added to the tracker before its execution result is recorded. +func (ht *HookTracker) Record(podNamespace, podName, container, source, hookName string, hookPhase hookPhase, hookFailed bool) error { ht.lock.Lock() defer ht.lock.Unlock() @@ -104,12 +111,16 @@ func (ht *HookTracker) Record(podNamespace, podName, container, source, hookName hookName: hookName, } + var err error if _, ok := ht.tracker[key]; ok { ht.tracker[key] = hookTrackerVal{ hookFailed: hookFailed, hookExecuted: true, } + } else { + err = fmt.Errorf("hook not exist in hooks tracker, hook key: %v", key) } + return err } // Stat calculates the number of attempted hooks and failed hooks diff --git a/internal/hook/hook_tracker_test.go b/internal/hook/hook_tracker_test.go index d104cc91d94..9e6ca95d335 100644 --- a/internal/hook/hook_tracker_test.go +++ b/internal/hook/hook_tracker_test.go @@ -50,7 +50,7 @@ func TestHookTracker_Add(t *testing.T) { func TestHookTracker_Record(t *testing.T) { tracker := NewHookTracker() tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre) - tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, true) + err := tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, true) key := hookTrackerKey{ podNamespace: "ns1", @@ -63,6 +63,11 @@ func TestHookTracker_Record(t *testing.T) { info := tracker.tracker[key] assert.True(t, info.hookFailed) + assert.Nil(t, err) + + err = tracker.Record("ns2", "pod2", "container1", HookSourceAnnotation, "h1", PhasePre, true) + assert.NotNil(t, err) + } func TestHookTracker_Stat(t *testing.T) { diff --git a/internal/hook/item_hook_handler.go b/internal/hook/item_hook_handler.go index 9075bc50fad..e3fa62e34c4 100644 --- a/internal/hook/item_hook_handler.go +++ b/internal/hook/item_hook_handler.go @@ -233,14 +233,14 @@ func (h *DefaultItemHookHandler) HandleHooks( }, ) - hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, false) - if err := h.PodCommandExecutor.ExecutePodCommand(hookLog, obj.UnstructuredContent(), namespace, name, "", hookFromAnnotations); err != nil { + hookFailed := false + if err = h.PodCommandExecutor.ExecutePodCommand(hookLog, obj.UnstructuredContent(), namespace, name, "", hookFromAnnotations); err != nil { hookLog.WithError(err).Error("Error executing hook") - hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, true) - - if hookFromAnnotations.OnError == velerov1api.HookErrorModeFail { - return err - } + hookFailed = true + } + hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, hookFailed) + if err != nil && hookFromAnnotations.OnError == velerov1api.HookErrorModeFail { + return err } return nil @@ -277,15 +277,16 @@ func (h *DefaultItemHookHandler) HandleHooks( }, ) - hookTracker.Record(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, false) + 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") - hookTracker.Record(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, true) + hookFailed = true if hook.Exec.OnError == velerov1api.HookErrorModeFail { modeFailError = err } } + hookTracker.Record(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, hookFailed) } } } diff --git a/internal/hook/wait_exec_hook_handler.go b/internal/hook/wait_exec_hook_handler.go index 452b8c421c2..9a5b1c53cc5 100644 --- a/internal/hook/wait_exec_hook_handler.go +++ b/internal/hook/wait_exec_hook_handler.go @@ -166,7 +166,11 @@ func (e *DefaultWaitExecHookHandler) HandleHooks( err := fmt.Errorf("hook %s in container %s expired before executing", hook.HookName, hook.Hook.Container) hookLog.Error(err) errors = append(errors, err) - hookTracker.Record(newPod.Namespace, newPod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), true) + + trackerErr := hookTracker.Record(newPod.Namespace, newPod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), true) + if trackerErr != nil { + hookLog.WithError(trackerErr).Warn("Error recording the hook in hook tracker") + } if hook.Hook.OnError == velerov1api.HookErrorModeFail { cancel() @@ -179,17 +183,24 @@ func (e *DefaultWaitExecHookHandler) HandleHooks( OnError: hook.Hook.OnError, Timeout: hook.Hook.ExecTimeout, } - hookTracker.Record(newPod.Namespace, newPod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), false) - if err := e.PodCommandExecutor.ExecutePodCommand(hookLog, podMap, pod.Namespace, pod.Name, hook.HookName, eh); err != nil { - hookLog.WithError(err).Error("Error executing hook") - err = fmt.Errorf("hook %s in container %s failed to execute, err: %v", hook.HookName, hook.Hook.Container, err) - errors = append(errors, err) - hookTracker.Record(newPod.Namespace, newPod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), true) - if hook.Hook.OnError == velerov1api.HookErrorModeFail { - cancel() - return - } + hookFailed := false + var hookErr error + if hookErr = e.PodCommandExecutor.ExecutePodCommand(hookLog, podMap, pod.Namespace, pod.Name, hook.HookName, eh); hookErr != nil { + hookLog.WithError(hookErr).Error("Error executing hook") + hookErr = fmt.Errorf("hook %s in container %s failed to execute, err: %v", hook.HookName, hook.Hook.Container, hookErr) + errors = append(errors, hookErr) + hookFailed = true + } + + trackerErr := hookTracker.Record(newPod.Namespace, newPod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), hookFailed) + if trackerErr != nil { + hookLog.WithError(trackerErr).Warn("Error recording the hook in hook tracker") + } + + if hookErr != nil && hook.Hook.OnError == velerov1api.HookErrorModeFail { + cancel() + return } } delete(byContainer, containerName) @@ -233,7 +244,12 @@ func (e *DefaultWaitExecHookHandler) HandleHooks( "hookPhase": "post", }, ) - hookTracker.Record(pod.Namespace, pod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), true) + + trackerErr := hookTracker.Record(pod.Namespace, pod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), true) + if trackerErr != nil { + hookLog.WithError(trackerErr).Warn("Error recording the hook in hook tracker") + } + hookLog.Error(err) errors = append(errors, err) } diff --git a/internal/hook/wait_exec_hook_handler_test.go b/internal/hook/wait_exec_hook_handler_test.go index fe632d11382..3f23542744c 100644 --- a/internal/hook/wait_exec_hook_handler_test.go +++ b/internal/hook/wait_exec_hook_handler_test.go @@ -1216,6 +1216,37 @@ func TestRestoreHookTrackerUpdate(t *testing.T) { hookTracker: hookTracker3, expectedFailed: 2, }, + { + name: "a hook was recorded before added to tracker", + groupResource: "pods", + initialPod: builder.ForPod("default", "my-pod"). + Containers(&v1.Container{ + Name: "container1", + }). + ContainerStatuses(&v1.ContainerStatus{ + Name: "container1", + State: v1.ContainerState{ + Waiting: &v1.ContainerStateWaiting{}, + }, + }). + Result(), + byContainer: map[string][]PodExecRestoreHook{ + "container1": { + { + HookName: "my-hook-1", + HookSource: HookSourceSpec, + Hook: velerov1api.ExecRestoreHook{ + Container: "container1", + Command: []string{"/usr/bin/foo"}, + OnError: velerov1api.HookErrorModeContinue, + WaitTimeout: metav1.Duration{Duration: time.Millisecond}, + }, + }, + }, + }, + hookTracker: NewHookTracker(), + expectedFailed: 0, + }, } for _, test := range tests1 {