-
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
Allow optional VK in CR metrics #1851
Conversation
91acc37
to
09bec65
Compare
8a8d82c
to
7bc684a
Compare
b5d4b01
to
f973b82
Compare
There seems to be angry bots yelling about things in the integration tests. |
@logicalhan Yes, checks were passing with the older implementation (till the first fixup commit), but due to the half-baked rewrite they are failing now (support for multi-valued VKs is not yet fully implemented here). I wanted to reach a consensus first on the approach we should be going with (in the thread above), as I wasn't fully sure, before going futher with the code changes. |
After proposing the following plan to @mrueg in order to have a much more performant implementation in place, I'm reworking this PR to adhere to the following:
However, currently, we have the CRD types to implement this feature, but if I'm not mistaken
False alarm, I missed this, resuming work on this. |
c9bea21
to
6e2e65f
Compare
(for other reviewers) |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logicalhan, rexagod 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 |
FYI @\reviewers, just pushed #1851 (comment) and #1851 (comment) in the latest commit, nothing else. |
Add support for variable VKs in CRS config, while maintaining a cache of discovered GVKs in the cluster, and updating it every 30s. Signed-off-by: Pranshu Srivastava <[email protected]>
New changes are detected. LGTM label has been removed. |
🥳 thanks for your work on this @rexagod |
Thank you for the all the top-notch reviews that helped shape this PR, @mrueg @logicalhan @dgrisonnet! 🥳 |
PR#1851 introduced accidental breakage for custom metrics on resources with an empty empty, e.g. Node. Fix it, and add tests to catch the next breakage. Reference: kubernetes#1851 Signed-off-by: Robin H. Johnson <[email protected]> Fixes: kubernetes#2434
PR#1851 introduced accidental breakage for custom metrics on resources with an empty empty, e.g. Node. Fix it, and add tests to catch the next breakage. Reference: kubernetes#1851 Reference: kubernetes#2434 Signed-off-by: Robin H. Johnson <[email protected]>
PR#1851 introduced accidental breakage for custom metrics on resources with an empty empty, e.g. Node. Fix it, and add tests to catch the next breakage. Reference: kubernetes#1851 Reference: kubernetes#2434 Signed-off-by: Robin H. Johnson <[email protected]>
PR#1851 introduced accidental breakage for custom metrics on resources with an empty empty, e.g. Node. Fix it, and add tests to catch the next breakage. ``` demo_timestamp{customresource_group="",customresource_kind="Node",customresource_version="v1",node="kind-control-plane"} 1.719618433e+09 ``` Reference: kubernetes#1851 Reference: kubernetes#2434 Signed-off-by: Robin H. Johnson <[email protected]>
PR#1851 introduced accidental breakage for custom metrics on resources with an empty empty, e.g. Node. Fix it, and add tests to catch the next breakage. ``` demo_timestamp{customresource_group="",customresource_kind="Node",customresource_version="v1",node="kind-control-plane"} 1.719618433e+09 ``` Reference: kubernetes#1851 Reference: kubernetes#2434 Signed-off-by: Robin H. Johnson <[email protected]>
What this PR does / why we need it: Allow optional VK in CR metrics, while only requiring the group to be fixed. This would allow users to define custom metrics for:
How does this change affect the cardinality of KSM: Increases, or stays the same, depending on the cardinality of the version(s) and/or kind(s), present under a group.
Which issue(s) this PR fixes: Fixes #1848