Repository navigation
MON-4527: ClusterMonitoring NodeExporterConfig logic - #2919
Conversation
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds detection of a top-level nodeExporter key in the ClusterMonitoring ConfigMap and extends mergeClusterMonitoringCRD to accept a nodeExporterDeclaredByConfigMap flag. It implements helpers to detect empty NodeExporter specs/collectors, maps CR collectionPolicy values into enabled/disabled collector flags, and conditionally merges CRD NodeExporter fields (resources, maxProcs, ignored devices, collector enablement) only when the ConfigMap omits nodeExporter. Unit tests for detection, emptiness checks, and merge precedence were added. go.mod updates the github.com/openshift/api version. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| // Defines the nodes on which the Pods are scheduled. | ||
| NodeSelector map[string]string `json:"nodeSelector,omitempty"` | ||
| // Defines tolerations for the pods. | ||
| Tolerations []v1.Toleration `json:"tolerations,omitempty"` |
There was a problem hiding this comment.
should we hide theses settings from the "official" doc?
I understand that we added these fields for consistency but I have a hard time seeing a use case for node selector and tolerations in the scope of a daemonset like node_exporter.
There was a problem hiding this comment.
yep, I was trying to clarify this with @danielmellado this morning.
Do you agree with Simon?
There was a problem hiding this comment.
The api design added that for consistency among all the other components as requested by the api tram. That said, we might for now not directly use them in CMO and that wouldn't hurt.
There was a problem hiding this comment.
we should probably revisit if there's no concrete use case because while the API would be consistent, it would still be confusing for users. I agree that for now, we should just drop the fields and not port them to the existing config.
|
/retest-required |
4da24ae to
f1ac5ee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/manifests/config_merge_test.go (1)
341-345: ⚡ Quick winWeak assertion:
Softirqs.Enableddefaults tofalse, so this test passes even ifDoNotCollectmapping is never exercised.
Softirqsis not set in thecmcinitialization defaults, so its zero value is alreadyfalse. The assertionrequire.False(t, ...)cannot distinguish "mapping worked correctly" from "code path was never reached."A more conclusive test uses a collector whose default is
true—NetDev(Enabled: trueby default) orNetClass(Enabled: true) — and asserts that settingDoNotCollectflips it tofalse:✅ Stronger proposed assertion
-t.Run("CR maps DoNotCollect to disabled", func(t *testing.T) { - c, err := NewConfigFromStringAndClusterMonitoringResource("{}", softirqsCR(configv1alpha1.NodeExporterCollectorCollectionPolicyDoNotCollect)) - require.NoError(t, err) - require.False(t, c.ClusterMonitoringConfiguration.NodeExporterConfig.Collectors.Softirqs.Enabled) -}) +t.Run("CR maps DoNotCollect to disabled", func(t *testing.T) { + // NetDev defaults to Enabled:true, so DoNotCollect must flip it to false. + cm := &configv1alpha1.ClusterMonitoring{ + Spec: configv1alpha1.ClusterMonitoringSpec{ + NodeExporterConfig: configv1alpha1.NodeExporterConfig{ + Collectors: configv1alpha1.NodeExporterCollectorConfig{ + NetDev: configv1alpha1.NodeExporterCollectorNetDevConfig{ + CollectionPolicy: configv1alpha1.NodeExporterCollectorCollectionPolicyDoNotCollect, + }, + }, + }, + }, + } + c, err := NewConfigFromStringAndClusterMonitoringResource("{}", cm) + require.NoError(t, err) + require.False(t, c.ClusterMonitoringConfiguration.NodeExporterConfig.Collectors.NetDev.Enabled) +})
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bacadb7f-7bbf-4aeb-abb4-4546c420ea2b
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_cluster_operator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modpkg/manifests/config.gopkg/manifests/config_merge.gopkg/manifests/config_merge_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
| // configMapYAMLDeclaresNodeExporter reports whether the cluster-monitoring-config body includes a | ||
| // top-level nodeExporter key (including explicit null). Phase 1: if set, the ConfigMap wins for | ||
| // the whole nodeExporter stanza and the ClusterMonitoring CR's nodeExporterConfig is ignored. | ||
| func configMapYAMLDeclaresNodeExporter(cmYAML string) bool { |
There was a problem hiding this comment.
The merge logic shouldn't depend on the input YAML. Can we refactor the config creation logic to avoid this?
openshift-ci
Bot
added
the
needs-rebase
1f054af to
c455b9d
Compare
openshift-ci
Bot
removed
the
needs-rebase
c455b9d to
fa3dac9
Compare
| func NewConfigFromStringAndClusterMonitoringResource(content string, cmr *configv1alpha1.ClusterMonitoring) (*Config, error) { | ||
| nodeExporterDeclaredByConfigMap := configMapDeclaresNodeExporter(content) | ||
|
|
||
| cmc := ClusterMonitoringConfiguration{ |
There was a problem hiding this comment.
would it be possible to avoid initialization of the node_exporter fields and move this to applyDefaults()? it would remove the need for a different treatment when merging the node_exporter configuration.
|
/test e2e-aws-ovn |
openshift-ci
Bot
added
the
needs-rebase
fa3dac9 to
4850982
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test generate |
1 similar comment
|
/test generate |
|
/test e2e-hypershift-conformance |
simonpasquier
left a comment
simonpasquier
left a comment
There was a problem hiding this comment.
another option would be turn the bool fields controlling the enabled-by-default collector to pointers.
type NodeExporterCollectorNetDevConfig struct {
// A Boolean flag that enables or disables the `netdev` collector.
Enabled *bool `json:"enabled,omitempty"`
}
...
type NodeExporterCollectorNetClassConfig struct {
// A Boolean flag that enables or disables the `netclass` collector.
Enabled *bool `json:"enabled,omitempty"`
// A Boolean flag that activates the `netlink` implementation of the `netclass` collector.
// Its default value is `true`: activating the netlink mode.
// This implementation improves the performance of the `netclass` collector.
UseNetlink *bool `json:"useNetlink,omitempty"`
}
And inside applyDefaults(), set the fields to true if they are undefined/nil.
(the systemd collector doesn't need to be included because it's disabled by default)
Phase 1: if cluster-monitoring-config YAML declares nodeExporter, keep the ConfigMap and ignore the CR for that block; otherwise map CR fields into ClusterMonitoringConfiguration (collectors, resources, maxProcs, devices, nodeSelector, tolerations). Pass ConfigMap YAML into mergeClusterMonitoringCRD for key presence checks. Apply nodeSelector and tolerations on the node-exporter DaemonSet when set. Bump github.com/openshift/api and refresh generated API docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Use a nil *NodeExporterConfig to detect ConfigMap overrides instead of re-parsing the cluster-monitoring-config YAML during CR merge. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
8d2bcef to
64c07a6
Compare
Use pointer bools for netdev and netclass so omitted ConfigMap fields keep their enabled-by-default values, and apply defaults in applyDefaults() instead of re-unmarshaling the ConfigMap. Co-authored-by: Cursor <cursoragent@cursor.com>
Iterate collection policies in a loop instead of repeating the same condition for each collector. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/skip |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marioferh, 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 |
|
/unhold |
|
/test e2e-aws-ovn |
openshift-ci
Bot
removed
the
do-not-merge/hold
marioferh
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@marioferh: This pull request references MON-4527 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 story to target the "5.0.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. |
|
/verified by tests |
openshift-ci-robot
added
the
verified
|
@simonpasquier: 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. |
|
/retest-required |
|
/test e2e-aws-ovn |
|
@marioferh: 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. |
4 participants
No description provided.