MON-4115: expose label metrics for jobs and cronjobs#2553
MON-4115: expose label metrics for jobs and cronjobs#2553rexagod wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
@rexagod: This pull request references MON-4115 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature request to target the "4.19.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
approved
|
/jira refresh |
|
@rexagod: This pull request references MON-4115 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature request to target the "4.19.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@rexagod: This pull request references MON-4115 which is a valid jira issue. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
rexagod
force-pushed
the
MON-4115
branch
3 times, most recently
from
c25eb30 to
7ad69ad
Compare
|
/retest-required |
|
I'm having some issues running |
|
/retest |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@rexagod: This pull request references MON-4115 which is a valid jira issue. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
left a comment on the ticket. |
|
[from the f2f] @machine424 suggested we have the default allowlist, in addition to a set of "allowed" resources that the users could choose to enable from. The allowlist itself will thus be customizable from CMO's config. |
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
Support extending the labels-allowlist based on this discussion. |
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
removed
the
needs-rebase
| // the default labels. Currently, only `jobs` and `cronjobs` resources are | ||
| // supported due to cardinality concerns. Each entry specifies a resource | ||
| // name and a list of label names (use `*` to expose all labels). | ||
| AdditionalResourceLabels []ResourceLabels `json:"additionalResourceLabels,omitempty"` |
There was a problem hiding this comment.
Please note, this was renamed from AdditionalLabelsAllowList to this.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/manifests/manifests.go (1)
791-802: Mutate the deployment args directly instead of the ranged copy.This works today only because the copied
container.Argsslice still aliases the live deployment args. That’s easy to break later with anappendor re-slice and hard to spot in review.Suggested refactor
additionalResourceLabels := f.config.ClusterMonitoringConfiguration.KubeStateMetricsConfig.AdditionalResourceLabels if len(additionalResourceLabels) > 0 { - for j := range container.Args { - if strings.HasPrefix(container.Args[j], "--metric-labels-allowlist=") { - var parts []string - for _, rl := range additionalResourceLabels { - parts = append(parts, fmt.Sprintf("%s=[%s]", rl.Resource, strings.Join(rl.Labels, ","))) - } - container.Args[j] += "," + strings.Join(parts, ",") - } - } + parts := make([]string, 0, len(additionalResourceLabels)) + for _, rl := range additionalResourceLabels { + parts = append(parts, fmt.Sprintf("%s=[%s]", rl.Resource, strings.Join(rl.Labels, ","))) + } + + args := d.Spec.Template.Spec.Containers[i].Args + for j := range args { + if strings.HasPrefix(args[j], "--metric-labels-allowlist=") { + args[j] += "," + strings.Join(parts, ",") + break + } + } + d.Spec.Template.Spec.Containers[i].Args = args }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 59d001fb-5b7a-46cb-be1c-050ffcbf0f1b
📒 Files selected for processing (9)
Documentation/api.mdDocumentation/openshiftdocs/index.adocDocumentation/openshiftdocs/modules/kubestatemetricsconfig.adocDocumentation/openshiftdocs/modules/resourcelabels.adocpkg/manifests/config.gopkg/manifests/config_test.gopkg/manifests/manifests.gopkg/manifests/manifests_test.gopkg/manifests/types.go
✅ Files skipped from review due to trivial changes (2)
- Documentation/openshiftdocs/index.adoc
- Documentation/openshiftdocs/modules/resourcelabels.adoc
🚧 Files skipped from review as they are similar to previous changes (2)
- Documentation/openshiftdocs/modules/kubestatemetricsconfig.adoc
- pkg/manifests/config_test.go
|
/test golangci-lint |
|
/retest-required |
Adds `AdditionalResourceLabels` to KSM config. This works as follows: ``` additionalResourceLabels: - resource: jobs labels: - foo - bar - resource: cronjobs labels: - * ``` Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
|
Updated commit and PR descriptions to reflect the newer field name. |
|
@rexagod: This pull request references MON-4115 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature request to target either version "4.22." or "openshift-4.22.", but it targets "openshift-5.0" instead. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/manifests/manifests.go (1)
791-802: Mutate deployment args directly instead of relying on range-copy aliasing.This block mutates
container.Argsfrom the range variable copy. It works today because of slice aliasing, but it’s fragile and harder to maintain. Write back throughd.Spec.Template.Spec.Containers[i].Argsexplicitly.Suggested refactor
additionalResourceLabels := f.config.ClusterMonitoringConfiguration.KubeStateMetricsConfig.AdditionalResourceLabels if len(additionalResourceLabels) > 0 { - for j := range container.Args { - if strings.HasPrefix(container.Args[j], "--metric-labels-allowlist=") { - var parts []string - for _, rl := range additionalResourceLabels { - parts = append(parts, fmt.Sprintf("%s=[%s]", rl.Resource, strings.Join(rl.Labels, ","))) - } - container.Args[j] += "," + strings.Join(parts, ",") - } - } + var parts []string + for _, rl := range additionalResourceLabels { + parts = append(parts, fmt.Sprintf("%s=[%s]", rl.Resource, strings.Join(rl.Labels, ","))) + } + + args := d.Spec.Template.Spec.Containers[i].Args + for j := range args { + if strings.HasPrefix(args[j], "--metric-labels-allowlist=") { + args[j] += "," + strings.Join(parts, ",") + break + } + } + d.Spec.Template.Spec.Containers[i].Args = args }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 000b3f8e-c9b1-4bee-b54c-0b9e5c0cb35e
📒 Files selected for processing (9)
Documentation/api.mdDocumentation/openshiftdocs/index.adocDocumentation/openshiftdocs/modules/kubestatemetricsconfig.adocDocumentation/openshiftdocs/modules/resourcelabels.adocpkg/manifests/config.gopkg/manifests/config_test.gopkg/manifests/manifests.gopkg/manifests/manifests_test.gopkg/manifests/types.go
✅ Files skipped from review due to trivial changes (4)
- Documentation/openshiftdocs/modules/resourcelabels.adoc
- pkg/manifests/config_test.go
- Documentation/openshiftdocs/index.adoc
- pkg/manifests/config.go
🚧 Files skipped from review as they are similar to previous changes (3)
- Documentation/openshiftdocs/modules/kubestatemetricsconfig.adoc
- pkg/manifests/types.go
- pkg/manifests/manifests_test.go
| #### Description | ||
|
|
||
| The `ResourceLabels` resource defines which Kubernetes labels to expose as metrics for a given resource type. | ||
|
|
||
| #### Required | ||
| - ` resource ` | ||
| - ` labels ` |
There was a problem hiding this comment.
Fix heading increment for markdownlint compliance.
Line 562 uses #### directly under a ## section, which triggers MD001. Since this file is generated, update the source code comments/templates so this renders as ### (and same for the paired “Required” heading).
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 562-562: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
pkg/manifests/manifests_test.go
Outdated
|
|
||
| d, err := f.KubeStateMetricsDeployment() | ||
| if err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
(nit) can we use require.NoError(t, err) which is less verbose? Same for the other calls to t.Fatal(f).
| return fmt.Errorf("%w: additionalResourceLabels: duplicate resource %q", ErrConfigValidation, rl.Resource) | ||
| } | ||
| seen[rl.Resource] = true | ||
| if len(rl.Labels) == 0 { |
There was a problem hiding this comment.
should we also check for non-empty values?
There was a problem hiding this comment.
Added checks for empty and duplicate non-empty labels.
|
TODO: Introduce this field in openshift/api#2778 once this is in. |
|
@rexagod: This pull request references MON-4115 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature request to target either version "4.22." or "openshift-4.22.", but it targets "openshift-5.0" instead. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@rexagod: This pull request references MON-4115 which is a valid jira issue. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rexagod, simonpasquier The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-aws-ovn-techpreview |
1 similar comment
|
/test e2e-aws-ovn-techpreview |
|
/verified by tests |
openshift-ci-robot
added
the
verified
|
@rexagod: This PR has been marked as verified by DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-ovn-techpreview |
|
@rexagod: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
7 participants
Adds jobs and cronjobs to the exposed set of label metrics, i.e.,
--metric-labels-allowlist=pods=[*],nodes=[*],namespaces=[*],persistentvolumes=[*],persistentvolumeclaims=[*],poddisruptionbudgets=[*],jobs=[*],cronjobs=[*]by introducing a wrapperAdditionalResourceLabelsfield (see #2553 (comment) for details).The linked issue is a feature request by a customer that requested these metrics to build their dashboards. I'm not sure what the qualifying factors were for the inclusion of the existing set of exposed label metrics
--metric-labels-allowlist=pods=[*],nodes=[*],namespaces=[*],persistentvolumes=[*],persistentvolumeclaims=[*],poddisruptionbudgets=[*], but I'm assuming we do so when there's an explicit and reasonable request to expose them.Nonetheless, I've opened this PR to set a ground for discussion if we want to incorporate this, or not.
TODO: Introduce this field in openshift/api#2778 once this is in.