Issue #10437: derive velero server's graceful shutdown timeout from terminationGracePeriodSeconds - #10438

If derived is the default 30, wouldn't this forcibly shrink it to 20 effectively?

Good catch — fixed in 6d637fe: now short-circuits to the 30s default whenever terminationGracePeriodSeconds <= 30s, so default installs no longer shrink.

A logline to note the derived value may be helpful:

logger.WithFields(logrus.Fields{
    "gracefulShutdownTimeout": derived,
    "terminationGracePeriod": terminationGracePeriod,
    "safetyBuffer": cfg.GracefulShutdownSafetyBuffer,
}).Info("Resolved graceful shutdown timeout")

Added, thanks — logs gracefulShutdownTimeout, terminationGracePeriod, and safetyBuffer at Info level in 6d637fe.

validation to ensure this field is non-negative could be needed
otherwise we end up with a higher value that actual (3600 - (-30)) = 3630 which is higher that k8s value

Fixed — --graceful-shutdown-safety-buffer now rejects negative values via a custom pflag.Value, same pattern as --graceful-shutdown-timeout.

The derived timeout does not always preserve a safety margin.

When terminationGracePeriodSeconds <= 30s, this branch returns a full 30-second manager timeout:

For a 30-second Pod grace period, that consumes the entire termination budget; below 30 seconds, it exceeds it. Kubernetes may therefore send SIGKILL before controller-runtime finishes waiting.

Also, a preStop hook runs before SIGTERM and consumes the same termination grace period. The current derivation does not inspect the hook, so the default 10-second buffer may be insufficient when one is configured.

Could we ensure the resolved timeout is always strictly below the Pod’s grace period? At minimum, the documentation should state that --graceful-shutdown-safety-buffer must cover the maximum preStop duration plus a final safety margin. The explicit timeout override should carry the same warning because it can equal or exceed the Pod’s total budget.

Hey @kaovilai, really appreciate the careful read here — this caught a real gap. I pushed a fix in the latest commit that I'd love your eyes on:

• Removed the terminationGracePeriodSeconds <= 30s → return 30s short-circuit entirely. resolveGracefulShutdownTimeout now uses one uniform formula for every case, derived = terminationGracePeriodSeconds - safetyBuffer, with no exceptions — including pods at Kubernetes' own 30s default, which are no longer exempt. So the resolved timeout should now always be strictly below the pod's grace period, not just "never above" it.
• Lowered --graceful-shutdown-safety-buffer's default from 10s to 2s, so that change stays small for existing installs (30s → 28s instead of 30s → 20s).
• The derived <= 0 fallback (buffer doesn't fit within terminationGracePeriodSeconds at all) now returns terminationGracePeriodSeconds itself with zero margin, instead of a flat 30s — so it won't discard a grace period an operator deliberately raised above 30s just because the buffer happened not to fit.

I also went and checked the Kubernetes container lifecycle hooks docs, and they back up your point nicely: the grace period countdown starts before preStop runs, and its execution time counts against terminationGracePeriodSeconds rather than being additional to it — Kubernetes' own worked example (60s grace period, 55s hook, 10s app shutdown → killed before finishing) is pretty much the failure mode you described. One thing worth flagging, in case it's useful context: --graceful-shutdown-safety-buffer itself isn't a Kubernetes-documented mechanism as such — the k8s docs describe the operator's responsibility to size terminationGracePeriodSeconds generously, but don't have a notion of the app subtracting its own buffer. Ours is a Velero-side defensive margin for the "last mile" after controller-runtime's manager gives up waiting on reconciles (process exit, log flush, scheduling jitter), rather than something Kubernetes itself prescribes — I added a note about that distinction in customize-installation.md so it's not misread as an official guideline.

One question I'd genuinely like your take on: should we also try to read the preStop hook itself (e.g. Sleep.Seconds off the pod spec) and fold its duration into the derivation automatically? I'm currently leaning toward not doing that, mainly because:

• Kubernetes doesn't seem to model this as something an app introspects — the convention is that SIGTERM arrival is the "start shutting down" signal, and sizing the budget around a preStop hook reads more like the operator's job via the pod spec than the app's at runtime.
• Sleep is the only hook type with a bounded duration in the API; Exec/HTTPGet don't have one, so auto-detection would only ever cover a fraction of real configurations.
• Before this PR, velero server had zero awareness of terminationGracePeriodSeconds or preStop at all, so documenting the buffer's role (which I've now done) already feels like a solid improvement over the status quo.

But I hold that opinion loosely — if you think auto-detecting Sleep is worth the added complexity, happy to explore it further.

Thanks again for pushing on this, it made the fix noticeably more correct.