vmoperator: set default -http.shutdownDelay for HTTP-serving components#1882
vmoperator: set default -http.shutdownDelay for HTTP-serving components#1882endesapt wants to merge 10 commits intoVictoriaMetrics:masterfrom
Conversation
| } | ||
|
|
||
| func TestAddHTTPShutdownDelayArg(t *testing.T) { | ||
| t.Run("adds default derived from built-in readiness defaults", func(t *testing.T) { |
There was a problem hiding this comment.
please rewrite this using f-test pattern, like other tests even in this file
| } | ||
|
|
||
| delaySeconds := defaultReadinessPeriodSeconds * defaultReadinessFailureThreshold | ||
| if probes != nil && probes.ReadinessProbe != nil { |
There was a problem hiding this comment.
I don't think it's a good idea - this makes the results less predictable. terminationGracePeriodSeconds would be a factor which would influence this result.
Instead, I propose to set a valid default value (possibly separate for each component?) and set readinessPeriod / terminationGracePeriodSeconds unless they are already defined.
Also, we should apply this to new instances only - we should avoid unnecessary rollouts after operator upgrades
There was a problem hiding this comment.
I investigated a little bit and i am now concerned, if we even need readiness probe for that? As i read Pod Termination Flow it is said that when pod is terminated, it is instantly marked in EndpointSlices as ready:false which prevents all the traffic to it? So we need to rely only on terminationGracePeriodSeconds to understand how much time we have to process traffic that we already recieved.
So i guess the final scheme will be without readinessProbe and will be:
- if
spec.terminationGracePeriodSecondsis not set, set it to default(for each component or united) http.shutdownDelayis inherited from it if not defined by user.
There was a problem hiding this comment.
(possibly separate for each component?)
If i didn't miss anything, i can change the code accordingly, but i dont know what the values can be for each component (or even what the unified value should be), because i dont have much experience in maintaining VM components. Would be glad if you help me with that.
I can just start with setting unified default to 30s and then we will change it to appropriate value
There was a problem hiding this comment.
i am now concerned, if we even need readiness probe for that?
No, not necessarily, but my understanding is that readinessProbe needs to align with shutdownDelay - when the container goes down, we'll run x readiness checks + terminationGracePeriodSeconds. We want to minimize the downtime, so readinessProbe needs to run quite often (its cheap), wait for 2 failure and only afterwards mark the container as failed.
I don't think we need to adjust probe timings here, but if we do - we should do that here and preferably for new resources only.
I can just start with setting unified default to 30s and then we will change it to appropriate value
That sounds reasonable, make sure the implementation makes it easy to tweak it for, say, vmstorage later easily
There was a problem hiding this comment.
That sounds reasonable, make sure the implementation makes it easy to tweak it for, say, vmstorage later easily
Understand
we'll run x readiness checks + terminationGracePeriodSeconds. We want to minimize the downtime, so readinessProbe needs to run quite often (its cheap), wait for 2 failure and only afterwards mark the container as failed.
So still for shutdownDelay we will have the scheme i described earlier right? Readiness probe will fail during the termination cause of the flag, and it will be marked as ready: false; serving: "false" in EndpointSlice stopping the traffic.
I am talking about this:
- if
spec.terminationGracePeriodSecondsis not set, set it to default(for each component or united) http.shutdownDelayis inherited from it if not defined by user.
for new resources only.
I will try to do that, but it would be great if you can give some directions on how to do that? How to distinct new resources from older ones? By their annotations maybe?
There was a problem hiding this comment.
we will have the scheme i described earlier right?
Yes, I think we're on the same page. I don't think readiness probes need to be changed for now.
How to distinct new resources from older ones?
Most reconcile procedured have prevCR var - if its nil its an empty CR
There was a problem hiding this comment.
Also, we should apply this to new instances only - we should avoid unnecessary rollouts after operator upgrades
@vrutkovs i don't get your idea. why do we need it?
There was a problem hiding this comment.
If we apply new args as -http.shutdownDelay to Deployment spec, it will force Pods to rolling update. And as we are adding this to almost every vm operator component, it will make a lot of rolling updates.
I guess it is a lot of pressure on a cluster with lots of vm deployments that's why we are trying to avoid that.
| wantArgs: nil, | ||
| }) | ||
| f(opts{ | ||
| extraArgs: nil, |
There was a problem hiding this comment.
Test to add: shutdownDelay and probe both set
|
Changed I wanted to set it explicitly in daemonset,deployment,statefulset like that: f1e4300#diff-e7d6e0f45db6d4efd51be09d7ff37703298db2a37b4b0ee87054f1c549d77176R26-R28 But then i understood that it will make old deployments restart. I can try pass the check to the functions, but i think it will be messy. Do we need it? As for changing defaults for distinct components, it will be pretty easy. As operator/internal/controller/operator/factory/build/defaults.go Lines 339 to 341 in cf18409 |
vrutkovs
left a comment
vrutkovs
left a comment
There was a problem hiding this comment.
LGTM on the approach, few comments on the code
|
Hope i got you right |
vrutkovs
left a comment
vrutkovs
left a comment
There was a problem hiding this comment.
LGTM, @AndrewChubatiuk could you have a look? I'm not sure if defaulting style is ciorrect
endesapt
force-pushed
the
default-shutdown-delay
branch
from
5f4806c to
7cf7c8b
Compare
|
This change looks good, so no worries about that but I'm afraid I misinformed you: we would like to ensure that operator update will not cause unnecessary pod rollouts. This is a longstanding task and not limited to this PR (this is why I asked to update the PR and apply this change to new resources only). However, it appears this decision will later bite us - we'll have to handle removing this argument too, so its getting more and more complicated. The radical solution is to implement "dry run mode" - #1832, where users would be able to verify that operator update will cause rollouts and can plan their maintenance accordingly. As a result, we'll have to change the directions again - no need to take the "freshness" of the resource into account, we'll tackle the unexpected rollouts via different means. Sorry about this taking too long - we accidentally tripped a long-standing problem, nothing wrong with your code. I understand if you want to leave the PR as is - we'll update it accordingly in that case, no problem with that. |
I like to contribute to VictoriaMetrics - it is my way to learn and to say thanks to your project(i hope it helps). Changes is a part of it, so it is okay. I will revert the "freshness" when i will have time. |
AndrewChubatiuk
force-pushed
the
default-shutdown-delay
branch
12 times, most recently
from
7f09b5d to
7a4e4aa
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 49 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/operator/factory/vtcluster/insert.go">
<violation number="1">
P2: VTInsert no longer injects a default -http.shutdownDelay based on terminationGracePeriodSeconds, so graceful HTTP shutdown is lost unless users set extraArgs manually.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
AndrewChubatiuk
force-pushed
the
default-shutdown-delay
branch
from
7a4e4aa to
5d52b7a
Compare
AndrewChubatiuk
left a comment
AndrewChubatiuk
left a comment
There was a problem hiding this comment.
LGTM! Will merge this PR now without any changes to http.shutdownDelay or preStop hook as it already contains many changes/fixes, will update issue later with possible solution for it
AndrewChubatiuk
dismissed
their stale review
will move all my changes to a separate branch instead
…t, VMAlert, VMSingle
- change terminationGracePeriodSeconds default to 30 seconds. - refactor tests and add test for custom terminationGracePeriodSeconds - change default shutdownDelay to 30 seconds.
delete setting default terminationGracePeriodSeconds to 30s
AndrewChubatiuk
force-pushed
the
default-shutdown-delay
branch
from
e313aaa to
7de933c
Compare
Nice! Will be waiting for updates and would be glad to discuss and to implement our final decision later |
3 participants
Fixes: #1834
Created AddHTTPShutdownDelayArg func that adds -http.shutdownDelay based on the factors:
Applied it to every component that has http.shutdownDelay flag
Summary by cubic
Adds a default -http.shutdownDelay for all HTTP-serving components, matching the pod’s terminationGracePeriodSeconds (30s by default). The flag is added only for new resources and can be overridden per app; terminationGracePeriodSeconds defaults to 30s if not set.
Written for commit 7de933c. Summary will update on new commits.