vmoperator: set default -http.shutdownDelay for HTTP-serving components#1882
vmoperator: set default -http.shutdownDelay for HTTP-serving components#1882endesapt wants to merge 9 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
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| } | ||
|
|
||
| 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
…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
endesapt
force-pushed
the
default-shutdown-delay
branch
from
5f4806c to
7cf7c8b
Compare
|
brought branch up to date to master, edited LastAppliedSpec according to the review |
|
@vrutkovs @AndrewChubatiuk Can you approve requested changes if everything is alright? |
|
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. |
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
Set a default -http.shutdownDelay for all HTTP-serving components equal to the pod’s termination grace period. Applies only to new resources, defaults to 30s when the pod value isn’t set, and respects extraArgs overrides.
Written for commit d91d083. Summary will update on new commits.