[cluster-autoscaler] Publish node group min/max metrics #4022
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @amrmahdi! |
k8s-ci-robot
added
the
size/M
|
+1 as well to add those metrics. could you fix the CI failures? |
amrmahdi
force-pushed
the
amrh/nodegroupminmaxmetrics
branch
2 times, most recently
from
51f0613 to
0e97a40
Compare
|
/test |
|
@amrmahdi: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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/test-infra repository. |
|
/test |
|
@feiskyer: No presubmit jobs available for kubernetes/autoscaler@master In response to this:
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/test-infra repository. |
|
@mwielgus @MaciekPytel any ideas the tests didn't run now for this PR? |
i'm curious if we could get some consensus around using node group IDs in the metrics. i think there are other metrics that could benefit from using these identifiers, but i have been warned against using them in the past. i think if we are going to agree that node group IDs are acceptable to use as metrics labels, then we should also establish guidance as to when it is appropriate. for example, this PR proposes a metric that will only get updated once (although this misses the notion of node groups being adjusted after initialization), is this usage of node group IDs acceptable? if this usage is acceptable, when is it unacceptable to use them? |
amrmahdi
force-pushed
the
amrh/nodegroupminmaxmetrics
branch
2 times, most recently
from
51b4dc9 to
42ce8b7
Compare
Can we get help on the CI? It seems it is waiting for a maintainer.
|
|
@mwielgus @MaciekPytel could you help to approve the github actions? The new changes from github actions make it required of approval first from maintainers (this is also true for other PRs). |
|
/assign @vivekbagade |
k8s-ci-robot
added
the
needs-rebase
amrmahdi
force-pushed
the
amrh/nodegroupminmaxmetrics
branch
from
42ce8b7 to
2b4f971
Compare
amrmahdi
force-pushed
the
amrh/nodegroupminmaxmetrics
branch
from
2b4f971 to
2bd7f0e
Compare
k8s-ci-robot
removed
the
needs-rebase
amrmahdi
force-pushed
the
amrh/nodegroupminmaxmetrics
branch
from
f2b343b to
3ac32b8
Compare
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
Yes I missed that. Thanks |
|
Left one more comment, otherwise lgtm. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amrmahdi, elmiko, feiskyer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
Each test works in isolation, but they cause panic when the entire suite is run (ex. make test-in-docker), because the underlying metrics library panics when the same metric is registered twice.
Skipping metrics tests added in #4022
Each test works in isolation, but they cause panic when the entire suite is run (ex. make test-in-docker), because the underlying metrics library panics when the same metric is registered twice.
…e-1.21-nodegroup-minmax [cluster-autoscaler] backport #4022 Publish node group min/max metrics into 1.21
* Set maxAsgNamesPerDescribe to the new maximum value While this was previously effectively limited to 50, `DescribeAutoScalingGroups` now supports fetching 100 ASG per calls on all regions, matching what's documented: https://docs.aws.amazon.com/autoscaling/ec2/APIReference/API_DescribeAutoScalingGroups.html ``` AutoScalingGroupNames.member.N The names of the Auto Scaling groups. By default, you can only specify up to 50 names. You can optionally increase this limit using the MaxRecords parameter. MaxRecords The maximum number of items to return with this call. The default value is 50 and the maximum value is 100. ``` Doubling this halves API calls on large clusters, which should help to prevent throttling. * Break out unmarshal from GenerateEC2InstanceTypes Refactor to allow for optimisation * Optimise GenerateEC2InstanceTypes unmarshal memory usage The pricing json for us-east-1 is currently 129MB. Currently fetching this into memory and parsing results in a large memory footprint on startup, and can lead to the autoscaler being OOMKilled. Change the ReadAll/Unmarshal logic to a stream decoder to significantly reduce the memory use. * use aws sdk to find region * Merge pull request kubernetes#4274 from kinvolk/imran/cloud-provider-packet-fix Cloud provider[Packet] fixes * Fix templated nodeinfo names collisions in BinpackingNodeEstimator Both upscale's `getUpcomingNodeInfos` and the binpacking estimator now uses the same shared DeepCopyTemplateNode function and inherits its naming pattern, which is great as that fixes a long standing bug. Due to that, `getUpcomingNodeInfos` will enrich the cluster snapshots with generated nodeinfos and nodes having predictable names (using template name + an incremental ordinal starting at 0) for upcoming nodes. Later, when it looks for fitting nodes for unschedulable pods (when upcoming nodes don't satisfy those (FitsAnyNodeMatching failing due to nodes capacity, or pods antiaffinity, ...), the binpacking estimator will also build virtual nodes and place them in a snapshot fork to evaluate scheduler predicates. Those temporary virtual nodes are built using the same pattern (template name and an index ordinal also starting at 0) as the one previously used by `getUpcomingNodeInfos`, which means it will generate the same nodeinfos/nodes names for nodegroups having upcoming nodes. But adding nodes by the same name in an existing cluster snapshot isn't allowed, and the evaluation attempt will fail. Practically this blocks re-upscales for nodegroups having upcoming nodes, which can cause a significant delay. * Improve misleading log Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr> * dont proactively decrement azure cache for unregistered nodes * annotate fakeNodes so that cloudprovider implementations can identify them if needed * move annotations to cloudprovider package * Cluster Autoscaler 1.21.1 * CA - AWS - Instance List Update 03-10-21 - 1.21 release branch * CA - AWS - Instance List Update 29-10-21 - 1.21 release branch * Cluster-Autoscaler update AWS EC2 instance types with g5, m6 and r6 * CA - AWS Instance List Update - 13/12/21 - 1.21 * Merge pull request kubernetes#4497 from marwanad/add-more-azure-instance-types add more azure instance types * Cluster Autoscaler 1.21.2 * Add `--feature-gates` flag to support scale up on volume limits (CSI migration enabled) Signed-off-by: ialidzhikov <i.alidjikov@gmail.com> * [Cherry pick 1.21] Remove TestDeleteBlob UT Signed-off-by: Zhecheng Li <zhechengli@microsoft.com> * cherry-pick kubernetes#4022 [cluster-autoscaler] Publish node group min/max metrics * Skipping metrics tests added in kubernetes#4022 Each test works in isolation, but they cause panic when the entire suite is run (ex. make test-in-docker), because the underlying metrics library panics when the same metric is registered twice. (cherry picked from commit 52392b3) * cherry-pick kubernetes#4162 and kubernetes#4172 [cluster-autoscaler]Add flag to control DaemonSet eviction on non-empty nodes & Allow DaemonSet pods to opt in/out from eviction. * CA - AWS Cloud Provider - 1.21 Static Instance List Update 02-06-2022 * fix instance type fallback Instead of logging a fatal error, log a standard error and fall back to loading instance types from the static list. * Cluster Autoscaler - 1.21.3 release * FAQ updated * Sync_changes file updated Co-authored-by: Benjamin Pineau <benjamin.pineau@datadoghq.com> Co-authored-by: Adrian Lai <aidy@loathe.me.uk> Co-authored-by: darkpssngr <shreyas300691@gmail.com> Co-authored-by: Kubernetes Prow Robot <k8s-ci-robot@users.noreply.github.com> Co-authored-by: Sylvain Rabot <sylvain@abstraction.fr> Co-authored-by: Marwan Ahmed <marwanad@microsoft.com> Co-authored-by: Jakub Tużnik <jtuznik@google.com> Co-authored-by: GuyTempleton <guy.templeton@skyscanner.net> Co-authored-by: sturman <4456572+sturman@users.noreply.github.com> Co-authored-by: Maciek Pytel <maciekpytel@google.com> Co-authored-by: ialidzhikov <i.alidjikov@gmail.com> Co-authored-by: Zhecheng Li <zhechengli@microsoft.com> Co-authored-by: Shubham Kuchhal <shubham.kuchhal@india.nec.com> Co-authored-by: Todd Neal <tnealt@amazon.com>
Each test works in isolation, but they cause panic when the entire suite is run (ex. make test-in-docker), because the underlying metrics library panics when the same metric is registered twice.
This change publishes the node groups min and max count set on the cluster autoscaler. The current use case that these metrics are needed for is to create alerts when the number of nodes in the cluster reach a certain percentage of the max allowed nodes for example.