Skip to content

Commit 1017d7a

Browse files
ywk253100ywk253100
authored
Merge pull request velero-io#9060 from sseago/multiple-hook-tracking-1.16
[release-1.16] Allow for proper tracking of multiple hooks per container
2 parents 3415f39 + 6709a8a commit 1017d7a

8 files changed

Lines changed: 79 additions & 57 deletions

File tree

‎changelogs/unreleased/9060-sseago‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Allow for proper tracking of multiple hooks per container

‎internal/hook/hook_tracker.go‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,9 @@ type hookKey struct {
4646
// Container indicates the container hooks use.
4747
// For hooks specified in the backup/restore spec, the container might be the same under different hookName.
4848
container string
49+
// hookIndex contains the slice index for the specific hook, in order to track multiple hooks
50+
// for the same container
51+
hookIndex int
4952
}
5053

5154
// hookStatus records the execution status of a specific hook.
@@ -83,7 +86,7 @@ func NewHookTracker() *HookTracker {
8386
// Add adds a hook to the hook tracker
8487
// Add must precede the Record for each individual hook.
8588
// In other words, a hook must be added to the tracker before its execution result is recorded.
86-
func (ht *HookTracker) Add(podNamespace, podName, container, source, hookName string, hookPhase HookPhase) {
89+
func (ht *HookTracker) Add(podNamespace, podName, container, source, hookName string, hookPhase HookPhase, hookIndex int) {
8790
ht.lock.Lock()
8891
defer ht.lock.Unlock()
8992

@@ -94,6 +97,7 @@ func (ht *HookTracker) Add(podNamespace, podName, container, source, hookName st
9497
container: container,
9598
hookPhase: hookPhase,
9699
hookName: hookName,
100+
hookIndex: hookIndex,
97101
}
98102

99103
if _, ok := ht.tracker[key]; !ok {
@@ -108,7 +112,7 @@ func (ht *HookTracker) Add(podNamespace, podName, container, source, hookName st
108112
// Record records the hook's execution status
109113
// Add must precede the Record for each individual hook.
110114
// In other words, a hook must be added to the tracker before its execution result is recorded.
111-
func (ht *HookTracker) Record(podNamespace, podName, container, source, hookName string, hookPhase HookPhase, hookFailed bool, hookErr error) error {
115+
func (ht *HookTracker) Record(podNamespace, podName, container, source, hookName string, hookPhase HookPhase, hookIndex int, hookFailed bool, hookErr error) error {
112116
ht.lock.Lock()
113117
defer ht.lock.Unlock()
114118

@@ -119,6 +123,7 @@ func (ht *HookTracker) Record(podNamespace, podName, container, source, hookName
119123
container: container,
120124
hookPhase: hookPhase,
121125
hookName: hookName,
126+
hookIndex: hookIndex,
122127
}
123128

124129
if _, ok := ht.tracker[key]; !ok {
@@ -179,24 +184,24 @@ func NewMultiHookTracker() *MultiHookTracker {
179184
}
180185

181186
// Add adds a backup/restore hook to the tracker
182-
func (mht *MultiHookTracker) Add(name, podNamespace, podName, container, source, hookName string, hookPhase HookPhase) {
187+
func (mht *MultiHookTracker) Add(name, podNamespace, podName, container, source, hookName string, hookPhase HookPhase, hookIndex int) {
183188
mht.lock.Lock()
184189
defer mht.lock.Unlock()
185190

186191
if _, ok := mht.trackers[name]; !ok {
187192
mht.trackers[name] = NewHookTracker()
188193
}
189-
mht.trackers[name].Add(podNamespace, podName, container, source, hookName, hookPhase)
194+
mht.trackers[name].Add(podNamespace, podName, container, source, hookName, hookPhase, hookIndex)
190195
}
191196

192197
// Record records a backup/restore hook execution status
193-
func (mht *MultiHookTracker) Record(name, podNamespace, podName, container, source, hookName string, hookPhase HookPhase, hookFailed bool, hookErr error) error {
198+
func (mht *MultiHookTracker) Record(name, podNamespace, podName, container, source, hookName string, hookPhase HookPhase, hookIndex int, hookFailed bool, hookErr error) error {
194199
mht.lock.RLock()
195200
defer mht.lock.RUnlock()
196201

197202
var err error
198203
if _, ok := mht.trackers[name]; ok {
199-
err = mht.trackers[name].Record(podNamespace, podName, container, source, hookName, hookPhase, hookFailed, hookErr)
204+
err = mht.trackers[name].Record(podNamespace, podName, container, source, hookName, hookPhase, hookIndex, hookFailed, hookErr)
200205
} else {
201206
err = fmt.Errorf("the backup/restore not exist in hook tracker, backup/restore name: %s", name)
202207
}

‎internal/hook/hook_tracker_test.go‎

Lines changed: 35 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func TestNewHookTracker(t *testing.T) {
3333
func TestHookTracker_Add(t *testing.T) {
3434
tracker := NewHookTracker()
3535

36-
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
36+
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
3737

3838
key := hookKey{
3939
podNamespace: "ns1",
@@ -50,8 +50,8 @@ func TestHookTracker_Add(t *testing.T) {
5050

5151
func TestHookTracker_Record(t *testing.T) {
5252
tracker := NewHookTracker()
53-
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
54-
err := tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
53+
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
54+
err := tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
5555

5656
key := hookKey{
5757
podNamespace: "ns1",
@@ -67,40 +67,41 @@ func TestHookTracker_Record(t *testing.T) {
6767
assert.True(t, info.hookExecuted)
6868
assert.NoError(t, err)
6969

70-
err = tracker.Record("ns2", "pod2", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
70+
err = tracker.Record("ns2", "pod2", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
7171
assert.Error(t, err)
7272

73-
err = tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", false, nil)
73+
err = tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0, false, nil)
7474
assert.NoError(t, err)
7575
assert.True(t, info.hookFailed)
7676
}
7777

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

81-
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
82-
tracker.Add("ns2", "pod2", "container1", HookSourceAnnotation, "h2", "")
83-
tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
81+
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
82+
tracker.Add("ns2", "pod2", "container1", HookSourceAnnotation, "h2", "", 0)
83+
tracker.Add("ns2", "pod2", "container1", HookSourceAnnotation, "h2", "", 1)
84+
tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
8485

8586
attempted, failed := tracker.Stat()
86-
assert.Equal(t, 2, attempted)
87+
assert.Equal(t, 3, attempted)
8788
assert.Equal(t, 1, failed)
8889
}
8990

9091
func TestHookTracker_IsComplete(t *testing.T) {
9192
tracker := NewHookTracker()
92-
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre)
93-
tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, true, fmt.Errorf("err"))
93+
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, 0)
94+
tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, 0, true, fmt.Errorf("err"))
9495
assert.True(t, tracker.IsComplete())
9596

96-
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
97+
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
9798
assert.False(t, tracker.IsComplete())
9899
}
99100

100101
func TestHookTracker_HookErrs(t *testing.T) {
101102
tracker := NewHookTracker()
102-
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
103-
tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
103+
tracker.Add("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
104+
tracker.Record("ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
104105

105106
hookErrs := tracker.HookErrs()
106107
assert.Len(t, hookErrs, 1)
@@ -109,7 +110,7 @@ func TestHookTracker_HookErrs(t *testing.T) {
109110
func TestMultiHookTracker_Add(t *testing.T) {
110111
mht := NewMultiHookTracker()
111112

112-
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
113+
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
113114

114115
key := hookKey{
115116
podNamespace: "ns1",
@@ -118,6 +119,7 @@ func TestMultiHookTracker_Add(t *testing.T) {
118119
hookPhase: "",
119120
hookSource: HookSourceAnnotation,
120121
hookName: "h1",
122+
hookIndex: 0,
121123
}
122124

123125
_, ok := mht.trackers["restore1"].tracker[key]
@@ -126,8 +128,8 @@ func TestMultiHookTracker_Add(t *testing.T) {
126128

127129
func TestMultiHookTracker_Record(t *testing.T) {
128130
mht := NewMultiHookTracker()
129-
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
130-
err := mht.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
131+
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
132+
err := mht.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
131133

132134
key := hookKey{
133135
podNamespace: "ns1",
@@ -136,36 +138,39 @@ func TestMultiHookTracker_Record(t *testing.T) {
136138
hookPhase: "",
137139
hookSource: HookSourceAnnotation,
138140
hookName: "h1",
141+
hookIndex: 0,
139142
}
140143

141144
info := mht.trackers["restore1"].tracker[key]
142145
assert.True(t, info.hookFailed)
143146
assert.True(t, info.hookExecuted)
144147
assert.NoError(t, err)
145148

146-
err = mht.Record("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
149+
err = mht.Record("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
147150
assert.Error(t, err)
148151

149-
err = mht.Record("restore2", "ns2", "pod2", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
152+
err = mht.Record("restore2", "ns2", "pod2", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
150153
assert.Error(t, err)
151154
}
152155

153156
func TestMultiHookTracker_Stat(t *testing.T) {
154157
mht := NewMultiHookTracker()
155158

156-
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
157-
mht.Add("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h2", "")
158-
mht.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
159-
mht.Record("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h2", "", false, nil)
159+
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
160+
mht.Add("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h2", "", 0)
161+
mht.Add("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h2", "", 1)
162+
mht.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
163+
mht.Record("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h2", "", 0, false, nil)
164+
mht.Record("restore1", "ns2", "pod2", "container1", HookSourceAnnotation, "h2", "", 1, false, nil)
160165

161166
attempted, failed := mht.Stat("restore1")
162-
assert.Equal(t, 2, attempted)
167+
assert.Equal(t, 3, attempted)
163168
assert.Equal(t, 1, failed)
164169
}
165170

166171
func TestMultiHookTracker_Delete(t *testing.T) {
167172
mht := NewMultiHookTracker()
168-
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
173+
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
169174
mht.Delete("restore1")
170175

171176
_, ok := mht.trackers["restore1"]
@@ -174,20 +179,20 @@ func TestMultiHookTracker_Delete(t *testing.T) {
174179

175180
func TestMultiHookTracker_IsComplete(t *testing.T) {
176181
mht := NewMultiHookTracker()
177-
mht.Add("backup1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre)
178-
mht.Record("backup1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, true, fmt.Errorf("err"))
182+
mht.Add("backup1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, 0)
183+
mht.Record("backup1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", PhasePre, 0, true, fmt.Errorf("err"))
179184
assert.True(t, mht.IsComplete("backup1"))
180185

181-
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
186+
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
182187
assert.False(t, mht.IsComplete("restore1"))
183188

184189
assert.True(t, mht.IsComplete("restore2"))
185190
}
186191

187192
func TestMultiHookTracker_HookErrs(t *testing.T) {
188193
mht := NewMultiHookTracker()
189-
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "")
190-
mht.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", true, fmt.Errorf("err"))
194+
mht.Add("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0)
195+
mht.Record("restore1", "ns1", "pod1", "container1", HookSourceAnnotation, "h1", "", 0, true, fmt.Errorf("err"))
191196

192197
hookErrs := mht.HookErrs("restore1")
193198
assert.Len(t, hookErrs, 1)

‎internal/hook/item_hook_handler.go‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
223223
hookFromAnnotations = getPodExecHookFromAnnotations(metadata.GetAnnotations(), "", log)
224224
}
225225
if hookFromAnnotations != nil {
226-
hookTracker.Add(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase)
226+
hookTracker.Add(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, 0)
227227

228228
hookLog := log.WithFields(
229229
logrus.Fields{
@@ -239,7 +239,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
239239
hookLog.WithError(errExec).Error("Error executing hook")
240240
hookFailed = true
241241
}
242-
errTracker := hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, hookFailed, errExec)
242+
errTracker := hookTracker.Record(namespace, name, hookFromAnnotations.Container, HookSourceAnnotation, "", phase, 0, hookFailed, errExec)
243243
if errTracker != nil {
244244
hookLog.WithError(errTracker).Warn("Error recording the hook in hook tracker")
245245
}
@@ -267,10 +267,10 @@ func (h *DefaultItemHookHandler) HandleHooks(
267267
hooks = resourceHook.Post
268268
}
269269

270-
for _, hook := range hooks {
270+
for i, hook := range hooks {
271271
if groupResource == kuberesource.Pods {
272272
if hook.Exec != nil {
273-
hookTracker.Add(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase)
273+
hookTracker.Add(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, i)
274274
// The remaining hooks will only be executed if modeFailError is nil.
275275
// Otherwise, execution will stop and only hook collection will occur.
276276
if modeFailError == nil {
@@ -291,7 +291,7 @@ func (h *DefaultItemHookHandler) HandleHooks(
291291
modeFailError = err
292292
}
293293
}
294-
errTracker := hookTracker.Record(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, hookFailed, err)
294+
errTracker := hookTracker.Record(namespace, name, hook.Exec.Container, HookSourceSpec, resourceHook.Name, phase, i, hookFailed, err)
295295
if errTracker != nil {
296296
hookLog.WithError(errTracker).Warn("Error recording the hook in hook tracker")
297297
}
@@ -534,6 +534,11 @@ type PodExecRestoreHook struct {
534534
HookSource string
535535
Hook velerov1api.ExecRestoreHook
536536
executed bool
537+
// hookIndex contains the slice index for the specific hook from the restore spec
538+
// in order to track multiple hooks. Stored here because restore hook results are recorded
539+
// outside of the original slice iteration
540+
// for the same container
541+
hookIndex int
537542
}
538543

539544
// GroupRestoreExecHooks returns a list of hooks to be executed in a pod grouped by
@@ -561,12 +566,13 @@ func GroupRestoreExecHooks(
561566
if hookFromAnnotation.Container == "" {
562567
hookFromAnnotation.Container = pod.Spec.Containers[0].Name
563568
}
564-
hookTrack.Add(restoreName, metadata.GetNamespace(), metadata.GetName(), hookFromAnnotation.Container, HookSourceAnnotation, "<from-annotation>", HookPhase(""))
569+
hookTrack.Add(restoreName, metadata.GetNamespace(), metadata.GetName(), hookFromAnnotation.Container, HookSourceAnnotation, "<from-annotation>", HookPhase(""), 0)
565570
byContainer[hookFromAnnotation.Container] = []PodExecRestoreHook{
566571
{
567572
HookName: "<from-annotation>",
568573
HookSource: HookSourceAnnotation,
569574
Hook: *hookFromAnnotation,
575+
hookIndex: 0,
570576
},
571577
}
572578
return byContainer, nil
@@ -579,14 +585,15 @@ func GroupRestoreExecHooks(
579585
if !rrh.Selector.applicableTo(kuberesource.Pods, namespace, labels) {
580586
continue
581587
}
582-
for _, rh := range rrh.RestoreHooks {
588+
for i, rh := range rrh.RestoreHooks {
583589
if rh.Exec == nil {
584590
continue
585591
}
586592
named := PodExecRestoreHook{
587593
HookName: rrh.Name,
588594
Hook: *rh.Exec,
589595
HookSource: HookSourceSpec,
596+
hookIndex: i,
590597
}
591598
// default to false if attr WaitForReady not set
592599
if named.Hook.WaitForReady == nil {
@@ -596,7 +603,7 @@ func GroupRestoreExecHooks(
596603
if named.Hook.Container == "" {
597604
named.Hook.Container = pod.Spec.Containers[0].Name
598605
}
599-
hookTrack.Add(restoreName, metadata.GetNamespace(), metadata.GetName(), named.Hook.Container, HookSourceSpec, rrh.Name, HookPhase(""))
606+
hookTrack.Add(restoreName, metadata.GetNamespace(), metadata.GetName(), named.Hook.Container, HookSourceSpec, rrh.Name, HookPhase(""), i)
600607
byContainer[named.Hook.Container] = append(byContainer[named.Hook.Container], named)
601608
}
602609
}

‎internal/hook/item_hook_handler_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1151,6 +1151,7 @@ func TestGroupRestoreExecHooks(t *testing.T) {
11511151
WaitTimeout: metav1.Duration{Duration: time.Minute},
11521152
WaitForReady: boolptr.False(),
11531153
},
1154+
hookIndex: 0,
11541155
},
11551156
{
11561157
HookName: "hook1",
@@ -1163,6 +1164,7 @@ func TestGroupRestoreExecHooks(t *testing.T) {
11631164
WaitTimeout: metav1.Duration{Duration: time.Minute * 2},
11641165
WaitForReady: boolptr.False(),
11651166
},
1167+
hookIndex: 2,
11661168
},
11671169
{
11681170
HookName: "hook2",
@@ -1175,6 +1177,7 @@ func TestGroupRestoreExecHooks(t *testing.T) {
11751177
WaitTimeout: metav1.Duration{Duration: time.Minute * 4},
11761178
WaitForReady: boolptr.True(),
11771179
},
1180+
hookIndex: 0,
11781181
},
11791182
},
11801183
"container2": {
@@ -1189,6 +1192,7 @@ func TestGroupRestoreExecHooks(t *testing.T) {
11891192
WaitTimeout: metav1.Duration{Duration: time.Second * 3},
11901193
WaitForReady: boolptr.False(),
11911194
},
1195+
hookIndex: 1,
11921196
},
11931197
},
11941198
},

0 commit comments

Comments
 (0)