MON-2903: add nodeExporter.collectors.systemd settings. #1892

raptorsun · 2023-02-14T14:21:43Z

This PR is held for the moment, systems collector requires more settings, it will come together later in this PR.

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

This PR is based on top of #1876 , we should merge that before this one.

openshift-ci-robot · 2023-02-14T14:21:47Z

@raptorsun: This pull request references MON-2903 which is a valid jira issue.

In response to this:

I added CHANGELOG entry for this change.

No user facing changes, so no entry in CHANGELOG was needed.

This PR is based on top of #1876 , we should merge that before this one.

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 kubernetes/test-infra repository.

simonpasquier · 2023-02-14T16:39:01Z

/hold

openshift-ci-robot · 2023-02-14T17:08:17Z

@raptorsun: This pull request references MON-2903 which is a valid jira issue.

In response to this:

This PR is held for the moment, systems collector requires more settings, it will come together later in this PR.

I added CHANGELOG entry for this change.

No user facing changes, so no entry in CHANGELOG was needed.

This PR is based on top of #1876 , we should merge that before this one.

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 kubernetes/test-infra repository.

raptorsun · 2023-04-11T14:02:40Z

/retest

raptorsun · 2023-04-11T19:07:25Z

/retest-required

raptorsun · 2023-04-12T09:45:46Z

when activating systemd collector 8 new metrics are collected:

name	cardinality per node
node_systemd_socket_accepted_connections_total,	~ 15 sockets
node_systemd_socket_current_connections,	~ 15 sockets
node_systemd_socket_refused_connections_total,	~ 15 sockets
node_systemd_system_running,	1 running state
node_systemd_timer_last_trigger_seconds,	3 timers
node_systemd_unit_state,	~ 220 service x 4 states
node_systemd_units,	4 states
node_systemd_version	1 version

The metric node_systemd_unit_state will generate many series in proportion to the number of systemd service per node.

simonpasquier · 2023-05-03T13:31:31Z

My take is that enabling the systemd collector should only expose metrics which have a fixed cardinality: node_systemd_system_running, node_systemd_unit_state, node_systemd_units and node_systemd_version.

By default CMO should set --collector.systemd.unit-include="" meaning that no node_systemd_unit_state metric is exposed. Users can configure which units they want to collect metrics for. Maybe via a list of regexps?

nodeExporter:
  collectors:
    systemd:
      enabled: true
      units:
      - iscsi-init.*
      - sshd.service

node_systemd_socket_* and node_systemd_timer_* are automatically exposed for units which are included.

Tai-RedHat · 2023-05-17T02:30:57Z

@simonpasquier, I guess Haoyu is not back yet, is this PR ready for testing?

Tai-RedHat · 2023-05-17T03:41:51Z

test PR with cluster-bot, new features implemented
/label qe-approved

simonpasquier

in case you've missed, I had a comment about metrics cardinality and how to limit it.

simonpasquier · 2023-05-31T12:45:55Z

pkg/manifests/types.go

+// Among them, the `node_systemd_unit_state` metric is the most useful show the state of each  systemd unit. So its cardinality cound be high.
+// If you enable this collector, watch the prometheus-k8s deployment closely for excessive memory usage.
+type NodeExporterCollectorSystemdConfig struct {
+	// A Boolean flag that enables or disables the `systemd` colletor.


Suggested change

// A Boolean flag that enables or disables the `systemd` colletor.

// A Boolean flag that enables or disables the `systemd` collector.

raptorsun · 2023-06-01T10:51:28Z

in case you've missed, I had a comment about metrics cardinality and how to limit it.

Thanks for reminding me of limiting cardinality, I will later push a new version with these filters.

bburt-rh · 2023-06-08T13:18:35Z

pkg/manifests/types.go

@@ -297,6 +297,9 @@ type NodeExporterCollectorConfig struct {
 	// Defines the configuration of the `buddyinfo` collector, which collects statistics about memory fragmentation from the `node_buddyinfo_blocks` metric. This metric collects data from `/proc/buddyinfo`.
 	// Disabled by default.
 	BuddyInfo NodeExporterCollectorBuddyInfoConfig `json:"buddyinfo,omitempty"`
+	// Defines the configuration of the `systemd` collector, which collects statistics on systemd daemon and its managed services.


Suggested change

// Defines the configuration of the `systemd` collector, which collects statistics on systemd daemon and its managed services.

// Defines the configuration of the `systemd` collector, which collects statistics on the `systemd` daemon and its managed services.

bburt-rh · 2023-06-08T13:20:50Z

pkg/manifests/types.go

+// `node_systemd_unit_state`,
+// `node_systemd_units`,
+// `node_systemd_version`.
+// Among them, the `node_systemd_unit_state` metric is the most useful show the state of each  systemd unit. So its cardinality cound be high.


Suggested change

// Among them, the `node_systemd_unit_state` metric is the most useful show the state of each systemd unit. So its cardinality cound be high.

// Of these metrics, the `node_systemd_unit_state` metric is the most useful because it shows the state of each `systemd` unit. However, note that the cardinality for this metric might be high.

bburt-rh · 2023-06-08T13:21:36Z

pkg/manifests/types.go

+// `node_systemd_units`,
+// `node_systemd_version`.
+// Among them, the `node_systemd_unit_state` metric is the most useful show the state of each  systemd unit. So its cardinality cound be high.
+// If you enable this collector, watch the prometheus-k8s deployment closely for excessive memory usage.


Suggested change

// If you enable this collector, watch the prometheus-k8s deployment closely for excessive memory usage.

// If you enable this collector, closely monitor the `prometheus-k8s` deployment for excessive memory usage.

raptorsun · 2023-06-19T22:10:24Z

/hold
adding systemd unit selection in the config.

raptorsun · 2023-06-23T15:36:57Z

@bburt-rh @simonpasquier Thank you for the timely review :D

All points has been addressed. The PR is ready for another review now.

pkg/manifests/types.go

raptorsun · 2023-06-27T07:06:05Z

ready to review again.
/unhold

simonpasquier · 2023-06-27T09:46:21Z

pkg/manifests/manifests.go

+		units := f.config.ClusterMonitoringConfiguration.NodeExporterConfig.Collectors.Systemd.Units
+		for idx, unit := range units {


I wouldn't modify the config slice.

Suggested change

units := f.config.ClusterMonitoringConfiguration.NodeExporterConfig.Collectors.Systemd.Units

for idx, unit := range units {

patternUnits := make([]string, len(f.config.ClusterMonitoringConfiguration.NodeExporterConfig.Collectors.Systemd.Units))

for i, unit := range f.config.ClusterMonitoringConfiguration.NodeExporterConfig.Collectors.Systemd.Units {

after removing the addition of brackets, this will not be needed, the unit list will be read only in this function.

pkg/manifests/manifests.go

simonpasquier · 2023-06-27T09:50:24Z

pkg/manifests/manifests.go

+			if err != nil {
+				return nil, fmt.Errorf("invalid regexp for systemd unit: %s", unit)
+			}
+			units[idx] = fmt.Sprintf("(%s)", unit)


do we need the enclosing brackets?

After a second thought, any pattern that needs brackets to limit cannot pass the regex check just before this line. No bracket will be added.

test/e2e/node_exporter_test.go

pkg/manifests/manifests.go

bburt-rh · 2023-06-28T18:55:18Z

/label docs-approved

raptorsun · 2023-07-04T07:25:29Z

/retest-required

jan--f · 2023-07-05T08:38:08Z

This lgtm but will let @simonpasquier apply the label.

jan--f · 2023-07-10T06:46:33Z

/lgtm

openshift-ci · 2023-07-10T06:47:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bburt-rh, jan--f, raptorsun

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:

~~OWNERS~~ [jan--f,raptorsun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-07-10T09:14:17Z

@raptorsun: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 14, 2023

openshift-ci bot requested review from jan--f and JoaoBraveCoding February 14, 2023 14:22

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2023

raptorsun force-pushed the feature/MON-2903 branch from 6e8e2f9 to 315a4ba Compare February 14, 2023 14:23

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2023

raptorsun force-pushed the feature/MON-2903 branch from 315a4ba to 332d5ed Compare April 11, 2023 10:44

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2023

raptorsun force-pushed the feature/MON-2903 branch from 332d5ed to a161f24 Compare April 11, 2023 14:44

raptorsun force-pushed the feature/MON-2903 branch from a161f24 to 8639c4a Compare April 12, 2023 09:37

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label May 17, 2023

simonpasquier reviewed May 31, 2023

View reviewed changes

bburt-rh reviewed Jun 8, 2023

View reviewed changes

raptorsun force-pushed the feature/MON-2903 branch from 8639c4a to c8b5911 Compare June 19, 2023 22:09

raptorsun changed the title ~~MON-2903: add nodeExporter.collectors.systemd settings.~~ [WIP] MON-2903: add nodeExporter.collectors.systemd settings. Jun 19, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2023

bburt-rh reviewed Jun 23, 2023

View reviewed changes

pkg/manifests/types.go Outdated Show resolved Hide resolved

raptorsun force-pushed the feature/MON-2903 branch from d0fe5b2 to c06a027 Compare June 25, 2023 21:49

raptorsun requested review from simonpasquier and bburt-rh June 27, 2023 07:05

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2023

simonpasquier reviewed Jun 27, 2023

View reviewed changes

raptorsun force-pushed the feature/MON-2903 branch 2 times, most recently from bc71bc7 to 882073c Compare June 28, 2023 10:11

simonpasquier reviewed Jun 28, 2023

View reviewed changes

pkg/manifests/manifests.go Outdated Show resolved Hide resolved

raptorsun force-pushed the feature/MON-2903 branch from 882073c to 2470f9c Compare June 28, 2023 18:30

bburt-rh approved these changes Jun 28, 2023

View reviewed changes

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jun 28, 2023

raptorsun force-pushed the feature/MON-2903 branch from 2470f9c to cbdcbd7 Compare July 3, 2023 21:30

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2023

raptorsun force-pushed the feature/MON-2903 branch from cbdcbd7 to 86515bf Compare July 5, 2023 10:12

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2023

add nodeExporter.collectors.systemd settings.

5cc05fa

raptorsun force-pushed the feature/MON-2903 branch from 86515bf to 5cc05fa Compare July 5, 2023 13:19

raptorsun requested a review from simonpasquier July 6, 2023 22:06

openshift-ci bot assigned jan--f Jul 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2023

openshift-merge-robot merged commit b886eb2 into openshift:master Jul 10, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MON-2903: add nodeExporter.collectors.systemd settings. #1892

MON-2903: add nodeExporter.collectors.systemd settings. #1892

raptorsun commented Feb 14, 2023 •

edited

openshift-ci-robot commented Feb 14, 2023 •

edited by openshift-ci bot

simonpasquier commented Feb 14, 2023

openshift-ci-robot commented Feb 14, 2023 •

edited by openshift-ci bot

raptorsun commented Apr 11, 2023

raptorsun commented Apr 11, 2023

raptorsun commented Apr 12, 2023

simonpasquier commented May 3, 2023

Tai-RedHat commented May 17, 2023

Tai-RedHat commented May 17, 2023

simonpasquier left a comment

simonpasquier May 31, 2023

raptorsun commented Jun 1, 2023

bburt-rh Jun 8, 2023

bburt-rh Jun 8, 2023

bburt-rh Jun 8, 2023

raptorsun commented Jun 19, 2023

raptorsun commented Jun 23, 2023

raptorsun commented Jun 27, 2023

simonpasquier Jun 27, 2023

raptorsun Jun 27, 2023

simonpasquier Jun 27, 2023

raptorsun Jun 27, 2023

bburt-rh commented Jun 28, 2023

raptorsun commented Jul 4, 2023

jan--f commented Jul 5, 2023

jan--f commented Jul 10, 2023

openshift-ci bot commented Jul 10, 2023

openshift-ci bot commented Jul 10, 2023

	// A Boolean flag that enables or disables the `systemd` colletor.
	// A Boolean flag that enables or disables the `systemd` collector.

	// Defines the configuration of the `systemd` collector, which collects statistics on systemd daemon and its managed services.
	// Defines the configuration of the `systemd` collector, which collects statistics on the `systemd` daemon and its managed services.

	// Among them, the `node_systemd_unit_state` metric is the most useful show the state of each systemd unit. So its cardinality cound be high.
	// Of these metrics, the `node_systemd_unit_state` metric is the most useful because it shows the state of each `systemd` unit. However, note that the cardinality for this metric might be high.

	// If you enable this collector, watch the prometheus-k8s deployment closely for excessive memory usage.
	// If you enable this collector, closely monitor the `prometheus-k8s` deployment for excessive memory usage.

		units := f.config.ClusterMonitoringConfiguration.NodeExporterConfig.Collectors.Systemd.Units
		for idx, unit := range units {

MON-2903: add nodeExporter.collectors.systemd settings. #1892

MON-2903: add nodeExporter.collectors.systemd settings. #1892

Conversation

raptorsun commented Feb 14, 2023 • edited

openshift-ci-robot commented Feb 14, 2023 • edited by openshift-ci bot

simonpasquier commented Feb 14, 2023

openshift-ci-robot commented Feb 14, 2023 • edited by openshift-ci bot

raptorsun commented Apr 11, 2023

raptorsun commented Apr 11, 2023

raptorsun commented Apr 12, 2023

simonpasquier commented May 3, 2023

Tai-RedHat commented May 17, 2023

Tai-RedHat commented May 17, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raptorsun commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raptorsun commented Jun 19, 2023

raptorsun commented Jun 23, 2023

raptorsun commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bburt-rh commented Jun 28, 2023

raptorsun commented Jul 4, 2023

jan--f commented Jul 5, 2023

jan--f commented Jul 10, 2023

openshift-ci bot commented Jul 10, 2023

openshift-ci bot commented Jul 10, 2023

raptorsun commented Feb 14, 2023 •

edited

openshift-ci-robot commented Feb 14, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Feb 14, 2023 •

edited by openshift-ci bot