Adding metrics for Maxunavailable feature in StatefulSet #130951
Conversation
|
/triage accepted |
|
/retest |
|
This PR may require stable metrics review. Stable metrics are guaranteed to not change. Please review the documentation for the requirements and lifecycle of stable metrics and ensure that your metrics meet these guidelines. |
|
LGTM in general after the presubmit check failure is fixed. @soltysh would you like to take a look as well? |
|
LGTM label has been added. Git tree hash: 30677f32c92a34a1acfbc1b72625aa1bb7a63803
|
|
/assign |
|
/triage accepted |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Edwinhr716, janetkuo, soltysh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| "statefulSet", klog.KObj(set), | ||
| "unavailablePods", unavailablePods, | ||
| "maxUnavailable", maxUnavailable) | ||
| if unavailablePods > maxUnavailable { | ||
| metrics.MaxUnavailableViolations.WithLabelValues(set.Namespace, set.Name).Inc() | ||
| } |
There was a problem hiding this comment.
This is always triggered by Parallel StatefulSets multiple times during the initial rollout (depending on the number of replicas, minReadySeconds), so I am not sure how useful this metric is.
IMO, we should remove the logging here when we graduate to beta, or at least make it less verbose (4?), to prevent the spam.
Also, during the OrderedReady rollout there is a period of time where we have unavailable pods, but we don't log that. We also don't notice loss of availability in future StatefulSet updates.
There was a problem hiding this comment.
This is always triggered by Parallel StatefulSets multiple times during the initial rollout
By this you mean if PodManagementPolicy is set to Parallel? If so. could you expand on this? I don't see how it is triggered multiple times. There will be at most maxUnavailable unavailable pods no? Even using minReadySeconds?
IMO, we should remove the logging here when we graduate to beta, or at least make it less verbose (4?), to prevent the spam.
Makes sense, especially if we keep the metric.
Also, during the OrderedReady rollout there is a period of time where we have unavailable pods, but we don't log that
Are you suggesting we log it? Wouldn't that just be logging it everytime there is an unavailable pod?
There was a problem hiding this comment.
This is always triggered by Parallel StatefulSets multiple times during the initial rollout (depending on the number of replicas, minReadySeconds), so I am not sure how useful this metric is.
This part is being fixed in #130909, where we're only missing unit test to properly account unavailable pods with minReadySeconds taken into account.
IMO, we should remove the logging here when we graduate to beta, or at least make it less verbose (4?), to prevent the spam.
That seems reasonable.
Also, during the OrderedReady rollout there is a period of time where we have unavailable pods, but we don't log that. We also don't notice loss of availability in future StatefulSet updates.
We don't strive to log the time for how long the pods will be unavailable. As you pointed in your first question, this will vary from one statefulset to another, and by the nature of statefulsets it's hard to use that as a reasonable metric. This was discussed several times in the past.
There was a problem hiding this comment.
+1 to remove those logs. Even if we're keeping the logs, we don't need to log when unavailablePods == maxUnavailable given that it's a valid case.
There was a problem hiding this comment.
By this you mean if PodManagementPolicy is set to Parallel? If so. could you expand on this? I don't see how it is triggered multiple times. There will be at most maxUnavailable unavailable pods no? Even using minReadySeconds?
Yes, in case the PodManagementPolicy is set to Parallel and MaxUnavailableStatefulSet FG enabled. It happens before the statefulset reconciles to the final state.
Yes, but it takes time for all the pods to reach the minReadySeconds.
This part is being fixed in #130909, where we're only missing unit test to properly account unavailable pods with minReadySeconds taken into account.
Even when I test it with #130909, it still happens. Parallel policy hits this point +/- 18 times for StatefulSet with 5 pods for me. It depends on the kubelet/apiserver and other variables how many reconciles we hit.
Also, during the OrderedReady rollout there is a period of time where we have unavailable pods, but we don't log that
Are you suggesting we log it? Wouldn't that just be logging it everytime there is an unavailable pod?
We don't strive to log the time for how long the pods will be unavailable. As you pointed in your first question, this will vary from one statefulset to another, and by the nature of statefulsets it's hard to use that as a reasonable metric. This was discussed several times in the past.
I do not think we necessarily have to log that. Just saying there is difference between OrderedReady and Parallel.
There was a problem hiding this comment.
Yes, but it takes time for all the pods to reach the minReadySeconds.
Sure, but we take that into account when we determine how many pods are unavailable since they are only set as available once minReadySeconds has passed. So when we determine how many pods to delete
it shouldn't delete more (causing them to be unavailable) unless more than maxUnavailable pods are available after minReadySeconds have passed.|
/hold |
Co-authored-by: Filip Křepinský <[email protected]>
|
New changes are detected. LGTM label has been removed. |
|
/retest |
|
@Edwinhr716: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
I believe the comment is being addressed in #130909. /hold cancel |
|
Added additional info: #130951 (comment) |
|
/assign |
Filip's comment is on-point, this will need to be addressed. |
@richabanker any feedback from the sig-instrumentation pov? |
whoops, super sorry for the late reply, I think I was just curious, why is the new metric starting off at BETA stabilityLevel? |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds a metric to track how many times there has been a
maxunavailableviolation, requirement for kubernetes/enhancements#961 beta graduation.Which issue(s) this PR fixes:
Part of kubernetes/enhancements#961
Special notes for your reviewer:
This is a follow up to the discussion on the KEP update PR kubernetes/enhancements#4474 (comment).
General consensus seems to be that this metric should be in tree instead of in kube-state-metrics.
Open question:
cc @atiratree @dgrisonnet @wojtek-t who were part of the original discussion.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: