Skip to content

Commit

Permalink
Wait for results of restore exec hook executions in Finalizing phase …
Browse files Browse the repository at this point in the history 
…instead of InProgress phase

Signed-off-by: allenxu404 <qix2@vmware.com>
  • Loading branch information
@allenxu404
allenxu404 committed Apr 3, 2024
1 parent c2d267d commit a22542f
Show file tree
Hide file tree
Showing 12 changed files with 372 additions and 140 deletions.
1 change: 1 addition & 0 deletions changelogs/unreleased/7619-allenxu404
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Wait for results of restore exec hook executions in Finalizing phase instead of InProgress phase
94 changes: 68 additions & 26 deletions internal/hook/hook_tracker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ const (
HookSourceSpec = "spec"
)

// hookTrackerKey identifies a backup/restore hook
type hookTrackerKey struct {
// hookKey identifies a backup/restore hook
type hookKey struct {
// PodNamespace indicates the namespace of pod where hooks are executed.
// For hooks specified in the backup/restore spec, this field is the namespace of an applicable pod.
// For hooks specified in pod annotation, this field is the namespace of pod where hooks are annotated.
Expand All @@ -48,37 +48,42 @@ type hookTrackerKey struct {
container string
}

// hookTrackerVal records the execution status of a specific hook.
// hookTrackerVal is extensible to accommodate additional fields as needs develop.
type hookTrackerVal struct {
// hookStatus records the execution status of a specific hook.
// hookStatus is extensible to accommodate additional fields as needs develop.
type hookStatus struct {
// HookFailed indicates if hook failed to execute.
hookFailed bool
// hookExecuted indicates if hook already execute.
hookExecuted bool
// hookErr records error if hook execution fails
hookErr error
}

// HookTracker tracks all hooks' execution status
type HookTracker struct {
lock *sync.RWMutex
tracker map[hookTrackerKey]hookTrackerVal
tracker map[string]map[hookKey]hookStatus
}

// NewHookTracker creates a hookTracker.
// HookTracker is a map that uses the backup/restore name as the key
// and stores another map representing a group of hooks associated with a specific backup/restore.
func NewHookTracker() *HookTracker {
return &HookTracker{
lock: &sync.RWMutex{},
tracker: make(map[hookTrackerKey]hookTrackerVal),
tracker: make(map[string]map[hookKey]hookStatus),
}
}

// 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) {
// Currently, the backup hookTracker only tracks a single individual, allowing the name to be set to empty string for simplicity."
func (ht *HookTracker) Add(name, podNamespace, podName, container, source, hookName string, hookPhase hookPhase) {
ht.lock.Lock()
defer ht.lock.Unlock()

key := hookTrackerKey{
key := hookKey{
podNamespace: podNamespace,
podName: podName,
hookSource: source,
Expand All @@ -87,8 +92,12 @@ func (ht *HookTracker) Add(podNamespace, podName, container, source, hookName st
hookName: hookName,
}

if _, ok := ht.tracker[key]; !ok {
ht.tracker[key] = hookTrackerVal{
if _, ok := ht.tracker[name]; !ok {
ht.tracker[name] = make(map[hookKey]hookStatus)
}

if _, ok := ht.tracker[name][key]; !ok {
ht.tracker[name][key] = hookStatus{
hookFailed: false,
hookExecuted: false,
}
Expand All @@ -98,11 +107,11 @@ func (ht *HookTracker) Add(podNamespace, podName, container, source, hookName st
// Record records the hook's execution status
// 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 {
func (ht *HookTracker) Record(name, podNamespace, podName, container, source, hookName string, hookPhase hookPhase, hookFailed bool, hookErr error) error {
ht.lock.Lock()
defer ht.lock.Unlock()

key := hookTrackerKey{
key := hookKey{
podNamespace: podNamespace,
podName: podName,
hookSource: source,
Expand All @@ -112,37 +121,70 @@ func (ht *HookTracker) Record(podNamespace, podName, container, source, hookName
}

var err error
if _, ok := ht.tracker[key]; ok {
ht.tracker[key] = hookTrackerVal{
if _, ok := ht.tracker[name][key]; ok {
ht.tracker[name][key] = hookStatus{
hookFailed: hookFailed,
hookExecuted: true,
hookErr: hookErr,
}
} else {
err = fmt.Errorf("hook not exist in hooks tracker, hook key: %v", key)
err = fmt.Errorf("hook not exist in hooks tracker, hook: %s, %+v", name, key)
}
return err
}

// Stat calculates the number of attempted hooks and failed hooks
func (ht *HookTracker) Stat() (hookAttemptedCnt int, hookFailed int) {
ht.lock.RLock()
defer ht.lock.RUnlock()
// Stat calculates the number of attempted hooks and failed hooks for a specific backup/restore
func (ht *HookTracker) Stat(name string) (hookAttemptedCnt int, hookFailed int) {
hooksData := ht.GetHookData(name)

for _, hookInfo := range ht.tracker {
if hookInfo.hookExecuted {
for _, hs := range hooksData {
if hs.hookExecuted {
hookAttemptedCnt++
if hookInfo.hookFailed {
if hs.hookFailed {
hookFailed++
}
}
}
return
}

// GetTracker gets the tracker inside HookTracker
func (ht *HookTracker) GetTracker() map[hookTrackerKey]hookTrackerVal {
// Delete removes the hook data under a specific backup/restore
func (ht *HookTracker) Delete(name string) {
ht.lock.Lock()
defer ht.lock.Unlock()

delete(ht.tracker, name)
}

// GetTracker gets the tracker for a specific backup/restore
func (ht *HookTracker) GetHookData(name string) map[hookKey]hookStatus {
ht.lock.RLock()
defer ht.lock.RUnlock()

return ht.tracker
return ht.tracker[name]
}

// IsComplete returns if all hook executions under a specific backup/restore are complete
func (ht *HookTracker) IsComplete(name string) bool {
hooksData := ht.GetHookData(name)
for _, status := range hooksData {
if !status.hookExecuted {
return false
}
}

return true
}

// HooksErr returns hook execution errors under a specific backup/restore
func (ht *HookTracker) HookErrs(name string) []HookErrInfo {
hooksData := ht.GetHookData(name)
errInfo := make([]HookErrInfo, 0)
for key, status := range hooksData {
if status.hookFailed && status.hookErr != nil {
errInfo = append(errInfo, HookErrInfo{Namespace: key.podNamespace, Err: status.hookErr})
}
}

return errInfo
}
62 changes: 45 additions & 17 deletions internal/hook/hook_tracker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package hook

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -32,61 +33,88 @@ func TestNewHookTracker(t *testing.T) {
func TestHookTracker_Add(t *testing.T) {
tracker := NewHookTracker()

tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre)
tracker.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")

key := hookTrackerKey{
key := hookKey{
podNamespace: "ns1",
podName: "pod1",
container: "container1",
hookPhase: PhasePre,
hookPhase: "",
hookSource: HookSourceAnnotation,
hookName: "h1",
}

_, ok := tracker.tracker[key]
_, ok := tracker.tracker["restore1"][key]
assert.True(t, ok)
}

func TestHookTracker_Record(t *testing.T) {
tracker := NewHookTracker()
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre)
err := tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, true)
tracker.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
err := tracker.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))

key := hookTrackerKey{
key := hookKey{
podNamespace: "ns1",
podName: "pod1",
container: "container1",
hookPhase: PhasePre,
hookPhase: "",
hookSource: HookSourceAnnotation,
hookName: "h1",
}

info := tracker.tracker[key]
info := tracker.tracker["restore1"][key]
assert.True(t, info.hookFailed)
assert.True(t, info.hookExecuted)
assert.Nil(t, err)

err = tracker.Record("ns2", "pod2", "container1", HookSourceAnnotation, "h1", PhasePre, true)
err = tracker.Record("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
assert.NotNil(t, err)
}

func TestHookTracker_Stat(t *testing.T) {
tracker := NewHookTracker()

tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre)
tracker.Add("ns2", "pod2", "container1", HookSourceAnnotation, "h2", PhasePre)
tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, true)
tracker.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
tracker.Add("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h2", "")
tracker.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))

attempted, failed := tracker.Stat()
attempted, failed := tracker.Stat("restore1")
assert.Equal(t, 1, attempted)
assert.Equal(t, 1, failed)
}

func TestHookTracker_Get(t *testing.T) {
tracker := NewHookTracker()
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre)
tracker.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")

tr := tracker.GetTracker()
tr := tracker.GetHookData("restore1")
assert.NotNil(t, tr)
}

func TestHookTracker_Delete(t *testing.T) {
tracker := NewHookTracker()
tracker.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
tracker.Delete("restore1")

_, ok := tracker.tracker["restore1"]
assert.False(t, ok)
}

func TestHookTracker_IsComplete(t *testing.T) {
tracker := NewHookTracker()
tracker.Add("", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre)
tracker.Record("", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, true, fmt.Errorf("err"))
assert.True(t, tracker.IsComplete("backup1"))

tracker.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
assert.False(t, tracker.IsComplete("restore1"))
}

func TestHookTracker_HookErrs(t *testing.T) {
tracker := NewHookTracker()
tracker.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
tracker.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))

t.Logf("tracker :%+v", tr)
hookErrs := tracker.HookErrs("restore1")
assert.Len(t, hookErrs, 1)
}
13 changes: 7 additions & 6 deletions internal/hook/item_hook_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
hookFromAnnotations = getPodExecHookFromAnnotations(metadata.GetAnnotations(), "", log)
}
if hookFromAnnotations != nil {
hookTracker.Add(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase)
hookTracker.Add("", namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase)

hookLog := log.WithFields(
logrus.Fields{
Expand All @@ -239,7 +239,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
hookLog.WithError(errExec).Error("Error executing hook")
hookFailed = true
}
errTracker := hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, hookFailed)
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")
}
Expand Down Expand Up @@ -270,7 +270,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
for _, hook := range hooks {
if groupResource == kuberesource.Pods {
if hook.Exec != nil {
hookTracker.Add(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase)
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 {
Expand All @@ -291,7 +291,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
modeFailError = err
}
}
errTracker := hookTracker.Record(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, hookFailed)
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")
}
Expand Down Expand Up @@ -540,6 +540,7 @@ type PodExecRestoreHook struct {
// container name. If an exec hook is defined in annotation that is used, else applicable exec
// hooks from the restore resource are accumulated.
func GroupRestoreExecHooks(
restoreName string,
resourceRestoreHooks []ResourceRestoreHook,
pod *corev1api.Pod,
log logrus.FieldLogger,
Expand All @@ -560,7 +561,7 @@ func GroupRestoreExecHooks(
if hookFromAnnotation.Container == "" {
hookFromAnnotation.Container = pod.Spec.Containers[0].Name
}
hookTrack.Add(metadata.GetNamespace(), metadata.GetName(), hookFromAnnotation.Container, HookSourceAnnotation, "<from-annotation>", hookPhase(""))
hookTrack.Add(restoreName, metadata.GetNamespace(), metadata.GetName(), hookFromAnnotation.Container, HookSourceAnnotation, "<from-annotation>", hookPhase(""))
byContainer[hookFromAnnotation.Container] = []PodExecRestoreHook{
{
HookName: "<from-annotation>",
Expand Down Expand Up @@ -595,7 +596,7 @@ func GroupRestoreExecHooks(
if named.Hook.Container == "" {
named.Hook.Container = pod.Spec.Containers[0].Name
}
hookTrack.Add(metadata.GetNamespace(), metadata.GetName(), named.Hook.Container, HookSourceSpec, rrh.Name, hookPhase(""))
hookTrack.Add(restoreName, metadata.GetNamespace(), metadata.GetName(), named.Hook.Container, HookSourceSpec, rrh.Name, hookPhase(""))
byContainer[named.Hook.Container] = append(byContainer[named.Hook.Container], named)
}
}
Expand Down
8 changes: 4 additions & 4 deletions internal/hook/item_hook_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1198,7 +1198,7 @@ func TestGroupRestoreExecHooks(t *testing.T) {
hookTracker := NewHookTracker()
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actual, err := GroupRestoreExecHooks(tc.resourceRestoreHooks, tc.pod, velerotest.NewLogger(), hookTracker)
actual, err := GroupRestoreExecHooks("restore1", tc.resourceRestoreHooks, tc.pod, velerotest.NewLogger(), hookTracker)
assert.Nil(t, err)
assert.Equal(t, tc.expected, actual)
})
Expand Down Expand Up @@ -2352,7 +2352,7 @@ func TestBackupHookTracker(t *testing.T) {
}
h.HandleHooks(velerotest.NewLogger(), groupResource, pod.item, pod.hooks, test.phase, hookTracker)
}
actualAtemptted, actualFailed := hookTracker.Stat()
actualAtemptted, actualFailed := hookTracker.Stat("")
assert.Equal(t, test.expectedHookAttempted, actualAtemptted)
assert.Equal(t, test.expectedHookFailed, actualFailed)
})
Expand Down Expand Up @@ -2470,8 +2470,8 @@ func TestRestoreHookTrackerAdd(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
_, _ = GroupRestoreExecHooks(tc.resourceRestoreHooks, tc.pod, velerotest.NewLogger(), tc.hookTracker)
tracker := tc.hookTracker.GetTracker()
_, _ = GroupRestoreExecHooks("restore1", tc.resourceRestoreHooks, tc.pod, velerotest.NewLogger(), tc.hookTracker)
tracker := tc.hookTracker.GetHookData("restore1")
assert.Len(t, tracker, tc.expectedCnt)
})
}
Expand Down
Loading

0 comments on commit a22542f

Please sign in to comment.