Merge pull request #7153 from allenxu404/hooktracker-update

Enhance hooks tracker by adding an returned error to record function

changelogs/unreleased/7153-allenxu404

@@ -0,0 +1 @@
+Enhance hooks tracker by adding a returned error to record function

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

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) {

internal/hook/item_hook_handler.go

@@ -233,14 +233,19 @@ func (h *DefaultItemHookHandler) HandleHooks(
 			},
 		)
 
-		hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, false)
-		if err := h.PodCommandExecutor.ExecutePodCommand(hookLog, obj.UnstructuredContent(), namespace, name, "<from-annotation>", hookFromAnnotations); err != nil {
-			hookLog.WithError(err).Error("Error executing hook")
-			hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, true)
+		hookFailed := false
+		var errExec error
+		if errExec = h.PodCommandExecutor.ExecutePodCommand(hookLog, obj.UnstructuredContent(), namespace, name, "<from-annotation>", hookFromAnnotations); errExec != nil {
+			hookLog.WithError(errExec).Error("Error executing hook")
+			hookFailed = true
+		}
+		errTracker := hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, hookFailed)
+		if errTracker != nil {
+			hookLog.WithError(errTracker).Warn("Error recording the hook in hook tracker")
+		}
 
-			if hookFromAnnotations.OnError == velerov1api.HookErrorModeFail {
-				return err
-			}
+		if errExec != nil && hookFromAnnotations.OnError == velerov1api.HookErrorModeFail {
+			return errExec
 		}
 
 		return nil
@@ -277,15 +282,19 @@ 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
 							}
 						}
+						errTracker := hookTracker.Record(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, hookFailed)
+						if errTracker != nil {
+							hookLog.WithError(errTracker).Warn("Error recording the hook in hook tracker")
+						}
 					}
 				}
 			}

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)
+
+					errTracker := hookTracker.Record(newPod.Namespace, newPod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), true)
+					if errTracker != nil {
+						hookLog.WithError(errTracker).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
+				}
+
+				errTracker := hookTracker.Record(newPod.Namespace, newPod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), hookFailed)
+				if errTracker != nil {
+					hookLog.WithError(errTracker).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)
+
+			errTracker := hookTracker.Record(pod.Namespace, pod.Name, hook.Hook.Container, hook.HookSource, hook.HookName, hookPhase(""), true)
+			if errTracker != nil {
+				hookLog.WithError(errTracker).Warn("Error recording the hook in hook tracker")
+			}
+
 			hookLog.Error(err)
 			errors = append(errors, err)
 		}

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 {