From 1e26cf7ca0b30959542bcfe22e602bde1a9d5d0a Mon Sep 17 00:00:00 2001 From: Nitish Malang <71919457+nitishmalang@users.noreply.github.com> Date: Sat, 22 Aug 2026 00:45:30 +0530 Subject: [PATCH] =?UTF-8?q?fix(restore=5Ffinalizer):=20bound=20WaitRestore?= =?UTF-8?q?ExecHook=20poll=20with=20resourceT=E2=80=A6=20(#10280)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: reuse DefaultResourceTimeout from server config for hook wait Signed-off-by: Nitish Malang <71919457+nitishmalang@users.noreply.github.com> * Add changelog for PR 10280 Signed-off-by: Tiger Kaovilai --------- Signed-off-by: Nitish Malang <71919457+nitishmalang@users.noreply.github.com> Signed-off-by: Tiger Kaovilai Co-authored-by: Tiger Kaovilai --- changelogs/unreleased/10280-nitishmalang | 1 + pkg/cmd/server/config/config.go | 7 ++++++- .../restore_finalizer_controller.go | 15 +++++++++++-- .../restore_finalizer_controller_test.go | 21 +++++++++++++++++++ 4 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/10280-nitishmalang diff --git a/changelogs/unreleased/10280-nitishmalang b/changelogs/unreleased/10280-nitishmalang new file mode 100644 index 000000000..f0a5e49b9 --- /dev/null +++ b/changelogs/unreleased/10280-nitishmalang @@ -0,0 +1 @@ +Bound WaitRestoreExecHook polling with resourceTimeout to avoid an infinite wait when restore exec hooks never complete. diff --git a/pkg/cmd/server/config/config.go b/pkg/cmd/server/config/config.go index 2cc7bac4e..5198adcbc 100644 --- a/pkg/cmd/server/config/config.go +++ b/pkg/cmd/server/config/config.go @@ -28,6 +28,11 @@ const ( defaultPodVolumeOperationTimeout = 240 * time.Minute defaultResourceTerminatingTimeout = 10 * time.Minute + // DefaultResourceTimeout is the default for --resource-timeout. It matches + // defaultResourceTerminatingTimeout so controller fallbacks stay aligned with + // server defaults (see pkg/cmd/server/config/config.go). + DefaultResourceTimeout = defaultResourceTerminatingTimeout + // server's client default qps and burst defaultClientQPS float32 = 100.0 defaultClientBurst int = 100 @@ -41,7 +46,7 @@ const ( defaultCSISnapshotTimeout = 10 * time.Minute defaultItemOperationTimeout = 4 * time.Hour - resourceTimeout = 10 * time.Minute + resourceTimeout = defaultResourceTerminatingTimeout defaultMaxConcurrentK8SConnections = 30 defaultDisableInformerCache = false diff --git a/pkg/controller/restore_finalizer_controller.go b/pkg/controller/restore_finalizer_controller.go index 43e41d963..d9acd0a09 100644 --- a/pkg/controller/restore_finalizer_controller.go +++ b/pkg/controller/restore_finalizer_controller.go @@ -39,6 +39,7 @@ import ( "github.com/vmware-tanzu/velero/internal/hook" "github.com/vmware-tanzu/velero/internal/volume" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + serverconfig "github.com/vmware-tanzu/velero/pkg/cmd/server/config" "github.com/vmware-tanzu/velero/pkg/constant" "github.com/vmware-tanzu/velero/pkg/itemoperation" "github.com/vmware-tanzu/velero/pkg/metrics" @@ -577,8 +578,18 @@ func (ctx *finalizerContext) WaitRestoreExecHook() (errs results.Result) { log := ctx.logger.WithField("restore", ctx.restore.Name) log.Info("Waiting for restore exec hooks starts") - // wait for restore exec hooks to finish - err := wait.PollUntilContextCancel(context.Background(), 1*time.Second, true, func(context.Context) (bool, error) { + // Bound the wait by resourceTimeout (the same budget Velero already + // applies to other finalizer phases). Previously this poll had no + // deadline, so a hook that was registered via Add() but never + // recorded as executed left the restore stuck in Finalizing forever + // and blocked every other restore on the cluster. + timeout := ctx.resourceTimeout + if timeout <= 0 { + timeout = serverconfig.DefaultResourceTimeout + } + pollCtx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + err := wait.PollUntilContextCancel(pollCtx, 1*time.Second, true, func(context.Context) (bool, error) { log.Debug("Checking the progress of hooks execution") if ctx.multiHookTracker.IsComplete(ctx.restore.Name) { return true, nil diff --git a/pkg/controller/restore_finalizer_controller_test.go b/pkg/controller/restore_finalizer_controller_test.go index 832eb494c..226a2283c 100644 --- a/pkg/controller/restore_finalizer_controller_test.go +++ b/pkg/controller/restore_finalizer_controller_test.go @@ -482,6 +482,10 @@ func TestWaitRestoreExecHook(t *testing.T) { hookFailed, hookErr := true, fmt.Errorf("hook failed") hookTracker3.Add(restoreName3, podNs, podName, container, source, hookName, hook.PhasePre, 0) + hookTracker4 := hook.NewMultiHookTracker() + restoreName4 := "restore4" + hookTracker4.Add(restoreName4, "ns", "pod", "con1", "s1", "h1", hook.PhasePre, 0) + tests := []struct { name string hookTracker *hook.MultiHookTracker @@ -497,6 +501,8 @@ func TestWaitRestoreExecHook(t *testing.T) { hookName string hookFailed bool hookErr error + resourceTimeout time.Duration + expectTimeoutErr bool }{ { name: "no restore exec hooks", @@ -530,6 +536,16 @@ func TestWaitRestoreExecHook(t *testing.T) { hookFailed: hookFailed, hookErr: hookErr, }, + { + name: "hook never recorded should timeout instead of hanging", + hookTracker: hookTracker4, + restore: builder.ForRestore(velerov1api.DefaultNamespace, restoreName4).Result(), + expectedHooksAttempted: 0, + expectedHooksFailed: 0, + expectedHookErrs: 1, + resourceTimeout: 3 * time.Second, + expectTimeoutErr: true, + }, } for _, tc := range tests { @@ -542,6 +558,7 @@ func TestWaitRestoreExecHook(t *testing.T) { crClient: fakeClient, restore: tc.restore, multiHookTracker: tc.hookTracker, + resourceTimeout: tc.resourceTimeout, } require.NoError(t, ctx.crClient.Create(t.Context(), tc.restore)) @@ -553,6 +570,10 @@ func TestWaitRestoreExecHook(t *testing.T) { } errs := ctx.WaitRestoreExecHook() + if tc.expectTimeoutErr { + assert.NotEmpty(t, errs.Namespaces, "expected timeout error but got none") + continue + } assert.Len(t, errs.Namespaces, tc.expectedHookErrs) updated := &velerov1api.Restore{}