-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kube-state-metrics breaking release aka 2.0 #569
Comments
Agree with do a breaking release to clean up the kube-state-metrics. Actually performance optimizations is a feature. I am fine with a 2.1 release to add that feature.
I am fine with this change. But it seems that black-/whitelist names are also ok. What is the motivation for moving to allow/deny-list. And this apparently is an additional breaking change.
Fine with this. I also think if we do this, we need to also deprecated the |
The performance improvements I would prefer to release in a 1.x release, and once we’re comfortable everything works correctly, we go ahead and do these breaking changes. Renaming black/whitelist is for ethical reasons. I know it has been a used term but it doesn’t even describe well what it does, but it seems it’s not contentious :) . I think keeping the telemetry host and port is fine, just the default port should be something other than 81. |
Seems reasonable to me. allow/deny is more accurate.
Any suggestion about the candidates? 82 or some value else? IMHO, 81 is good enough to do this job. :) |
Agreed. |
Sorry I should have been more clear about why I think we should change ports. Anything lower than 1024 requires root on Linux (or at least have the CAP_NET_ADMIN capability). That’s why default ports of kube-state-metrics should be higher than that. And beyond that, whatever we use should be consistent across the pure binary and the container. |
Understood clearly. @brancz |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
I would also like to propose that we update the k8s dependencies to 1.13 at the very least. Given that it has the potential of introducing backwards-incompatible/breaking changes, it would be a good candidate for the 2.0 release. |
Do we have known incompatibilities? So far we only have the policy to support the past 4 versions of Kubernetes, as long as we stick with that it's not so much a "breaking" release. |
Nothing off the top my head. The client-go version support matrix is unfortunately not very clear in what they support/do not support. I can experiment with the latest k8s client-go dep and see how that turns out with the previous releases. By past 4 versions of Kubernetes - Do you mean that with respect to the latest Kubernetes version out there or the latest Kubernetes version depped into kube-state-metrics? |
The later. Forward compatibility should work, but we can't guarantee it due to client-go. |
@brancz I'd like to object! We've got a use case for grouping Pods and other objects that lack any controller. This logic only works on ingestion with recording rules if we have a metric that clearly specifies the lack of owners on an object. We can't solve this riddle with Please, do not think ill of me after looking at a screenshot of this abomination: |
I might sound silly, but maybe we can unify the single/plural form of options |
Haha, if anything I have more respect! That's a pretty nice recording rule, would you mind contributing that to the kubernetes-mixin? I think you have a fair point, let's keep it. In theory this could be done with a join as well, but feels too difficult to get right. Thanks for bringing this up! @dohnto :
Not at all silly. The |
…empty" This reverts commit 15b93c4. Based on Brancz's decision here: kubernetes#569 (comment) Signed-off-by: Andrey Klimentyev <andrey.klimentyev@flant.com>
@sylr brought to my attention, that for a number of pod metrics, there is the kube-state-metrics/internal/collector/pod.go Line 511 in aed9485
|
Added "consider renaming" |
LGTM. |
Added
Thanks to @xieyanker for noticing here: #777 (comment) |
I would add to the list, renaming all the leftover user-facing occurrences of |
Great suggestion! Added. |
Let's add the Sharding feature to the list as well. |
Sharding isn’t breaking so I feel it can be added in a backward compatible way in 1.x or 2.x. It’s fairly close to being ready I would say though so I’d like to see it go into a 1.x release. |
Came here to +1 removal of high-cardinality I'm intrigued: what does anyone use this metric for, in its current form? |
@bboreham you can also just blacklist those metrics for now until they are removed? |
Doesn’t a new major release show that there are breaking changes? Of course we need to properly document these changes. |
All coresponding issues from above were created under the
v2
Google doc for v2 release that we discussed during kubecon. https://docs.google.com/document/d/1lCvbvOAVFai7ciP_heZrJ_QXLOEaFu8ZBmwVVeCwf54/edit?usp=sharing |
One more thing that came up during kubecon: Before we do the v2 release we probably want to do another round of scalability tests. I believe Google volunteered to do this. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
@brancz anyone to ping regarding scalability tests? Or should we bring it up in the SIG call maybe? @tariq1890 |
SIG call sounds good |
Looks like we've done everything on the list, let's get a pre-release started then! :) |
Should the kube-state-metrics module path not become |
You're right, we forgot about that. @omegas27 do you want to take care of that? |
Sure 👍 |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle rotten We are almost there 🎉 |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
We finished and release is cut 🎉 Thank you all!! |
We have accumulated a number of deprecated metrics and odd behaviors that I believe may justify a 2.0 release. I'd like to take this issue to discuss whether people think this is a good idea and collect what we would potentially like to break should we do a breaking release.
Off the top of my head breaking changes I would like to do:
horizontalpodautoscaler
nomenclature, to match the rest of the exposed metrics v2: Rename hpa metrics to use full horizontalpodautoscaler nomenclature #977--namespace
flag to--namespaces
v2: Rename --namespace flag to --namespaces #978consider renaming kube-state-metrics to kubernetes-exporterwe decided against this onekube_secret_metadata_resource_version
,kube_configmap_metadata_resource_version
andkube_ingress_metadata_resource_version
expose the resource version as a string in its set of labels. This value can change often and would therefore create huge cardinality. This should be a number or not existing at all. Move resource version from metric label to metric number value #997kube_*_labels
metrics to have explicit lists of labels to expose passed in via a flag to prevent unnecessary high cardinality Allow list labels per resource #1047I would see a breaking release at least 3 months out there, as I would like to validate the performance optimizations independently first. Further thoughts?
@andyxning @zouyee @mxinden
The text was updated successfully, but these errors were encountered: