-
Notifications
You must be signed in to change notification settings - Fork 127
[CONTINT-4643] Add an option to configure KSM custom resource metrics collection #1927
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
base: main
Are you sure you want to change the base?
Conversation
0bd388f
to
d0de66f
Compare
d0de66f
to
16e29f7
Compare
…ctCustomResources`
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1927 +/- ##
==========================================
+ Coverage 38.97% 39.10% +0.13%
==========================================
Files 253 254 +1
Lines 26208 26275 +67
==========================================
+ Hits 10214 10275 +61
- Misses 15386 15390 +4
- Partials 608 610 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
bef05e6
to
03a75b1
Compare
d2757df
to
ac5089f
Compare
ac5089f
to
654136f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for configuring custom resource metrics collection in the Kubernetes State Metrics (KSM) core check. It introduces a new collectCustomResources
field to the DatadogAgent specification, allowing users to define custom resources and their associated metrics that should be collected by the kube-state-metrics core check.
Key changes include:
- Addition of new API types for custom resource configuration including Resource, Generator, and Metric types
- Implementation of RBAC permissions generation for custom resources
- Integration of custom resource configuration into the KSM check configuration YAML
- Comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
api/datadoghq/v2alpha1/datadogagent_types.go | Defines new API types for custom resource metrics configuration |
internal/controller/datadogagent/feature/kubernetesstatecore/feature.go | Integrates custom resource collection into the KSM feature |
internal/controller/datadogagent/feature/kubernetesstatecore/rbac.go | Generates RBAC permissions for custom resources |
internal/controller/datadogagent/feature/kubernetesstatecore/configmap.go | Updates KSM configuration to include custom resources |
internal/controller/datadogagent/feature/kubernetesstatecore/indent_writer.go | Utility for proper YAML indentation |
test/e2e/manifests/datadog-agent-ccr-enabled.yaml | E2E test manifest with custom resource configuration |
test/e2e/tests/k8s_suite/k8s_suite_test.go | E2E test validation for custom resource metrics |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
// +optional | ||
Conf *CustomConfig `json:"conf,omitempty"` | ||
|
||
// `CollectCustomResources` defines custom resources for the kube-state-metrics core check to collect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add config instructions for []resource
in this description so that it'll be populated in the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve added a link to the documentation of the details of the fields of this structure here: 28c60bd#diff-bb61879086cb5d8e4bd2d033770ca5943d58d914262b1c1f6cab53ad5a1b263a.
// `CollectCustomResources` defines custom resources for the kube-state-metrics core check to collect. | ||
// +optional | ||
// +listType=atomic | ||
CollectCustomResources []Resource `json:"collectCustomResources,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are investing in simplifing migration could we align configs as closely as possible with helm?
https://github.com/DataDog/helm-charts/blob/ff7cf3fbee4c440bc493a105fae27a2e2c1c3958/charts/datadog/values.yaml#L178-L185
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve renamed the field to make it match the Helm configuration in 28c60bd.
// GroupVersionKind is the Kubernetes group, version, and kind of a resource. | ||
type GroupVersionKind struct { | ||
Group string `json:"group" yaml:"group"` | ||
Version string `json:"version" yaml:"version"` | ||
Kind string `json:"kind" yaml:"kind"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this struct be imported from kube source code ?
it will never change but for simplicity ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the definition of GroupVersionKind
in apimachinery
:
https://github.com/kubernetes/apimachinery/blob/268a6d0fb19c92c9665e8f5cd85b564557038dc1/pkg/apis/meta/v1/group_version.go#L82-L90.
Unfortunately, it misses the yaml
tags that we need here to embed a GroupVersionKind
in a custom resource.
// Use the resource plural if specified, otherwise derive it from the Kind | ||
resourceName := cr.ResourcePlural | ||
if resourceName == "" { | ||
resourceName = strings.ToLower(flect.Pluralize(cr.GroupVersionKind.Kind)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we force to lower the plural name we build here, should we force the plural name we read from the custom resource too ? is it always in lowercase by rune from kubernetes or something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from the core Kubernetes concepts.
The GVK is what we find inside resource manifests. The kind is the object type. It’s capitalized and singular.
For ex., a resource contains:
apiVersion: apps/v1
kind: Deployment
The GVR is what is used in RESTful API path. The resource is lower cased and plural.
For ex., the API to operate on the above resource is:
/apis/apps/v1/namespaces/{ns}/deployments
The logic here is the same as the one implemented in kube-state-metrics
upstream: https://github.com/kubernetes/kube-state-metrics/blob/e332175940b8d8f76648c24d79c1e0d59a9b7926/pkg/customresourcestate/config.go#L82-L90
…ator into lenaic/CONTINT-4643
/merge |
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 79133870 took longer than expected. The current limit for the base branch 'main' is 120 minutes. |
What does this PR do?
Add an datadog agent parameter to configure custom resources metrics collection introduced in DataDog/datadog-agent#31715.
Motivation
Additional Notes
This the
datadog-operator
equivalent of DataDog/helm-charts#1883.Minimum Agent Versions
Are there minimum versions of the Datadog Agent and/or Cluster Agent required?
kubernetes-state-core
is running on cluster checks runners)Describe your test plan
Create a
DatadogAgent
CR with the following piece of KSM configuration:And validate that the corresponding custom resource metric is properly emitted:
Checklist
bug
,enhancement
,refactoring
,documentation
,tooling
, and/ordependencies
qa/skip-qa
label