Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MON-2900: add nodeExporter.collectors.netclass settings. #1893

Merged
merged 1 commit into from
Feb 27, 2023
Merged

MON-2900: add nodeExporter.collectors.netclass settings. #1893

merged 1 commit into from
Feb 27, 2023

Conversation

raptorsun
Copy link
Contributor

@raptorsun raptorsun commented Feb 15, 2023 •
edited
Loading

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

This PR adds toggle for netclass collector, based on PR#1888. It should be merged after PR#1888.

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

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

@raptorsun: This pull request references MON-2900 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 adds toggle for netclass collector, based on PR#1888. It should be merged after PR#1888.

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.

@openshift-ci openshift-ci bot requested review from jan--f and sthaha February 15, 2023 12:34
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2023
@openshift-ci-robot
Copy link
Contributor

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

@raptorsun: This pull request references MON-2900 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 adds toggle for netclass collector, based on PR#1888. It should be merged after PR#1888.

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
Copy link
Contributor Author

/hold
until #1888 is merged.

@openshift-ci 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 15, 2023
This was referenced Feb 15, 2023
Merged
Merged
@raptorsun raptorsun force-pushed the feature/MON-2900 branch 2 times, most recently from b6140ea to 2f00aea Compare February 15, 2023 21:13
@raptorsun
Copy link
Contributor Author

/retest-required

1 similar comment
@raptorsun
Copy link
Contributor Author

/retest-required

@raptorsun
Copy link
Contributor Author

/test e2e-agnostic-operator

1 similar comment
@raptorsun
Copy link
Contributor Author

/test e2e-agnostic-operator

@raptorsun
Copy link
Contributor Author

/test e2e-aws-single-node

@raptorsun
Copy link
Contributor Author

/unhold
since #1888 is merged.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2023
simonpasquier
simonpasquier reviewed Feb 20, 2023
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2023
@simonpasquier
Copy link
Contributor

/skip

@raptorsun
Copy link
Contributor Author

🙋‍♂️Hello @bburt-rh @Senthamilarasu-STA , could you please review this PR? If everything is good, could you stamp it with label docs-approved, px-approved?

bburt-rh
bburt-rh reviewed Feb 21, 2023
View reviewed changes
Documentation/api.md Outdated
@@ -192,6 +193,7 @@ The `NodeExporterCollectorConfig` resource defines settings for individual colle
| cpufreq | [NodeExporterCollectorCpufreqConfig](#nodeexportercollectorcpufreqconfig) | Defines the configuration of the `cpufreq` collector, which collects CPU frequency statistics. Disabled by default. |
| tcpstat | [NodeExporterCollectorTcpStatConfig](#nodeexportercollectortcpstatconfig) | Defines the configuration of the `tcpstat` collector, which collects TCP connection statistics. Disabled by default. |
| netdev | [NodeExporterCollectorNetDevConfig](#nodeexportercollectornetdevconfig) | Defines the configuration of the `netdev` collector, which collects network devices statistics. Enabled by default. |
| netclass | [NodeExporterCollectorNetClassConfig](#nodeexportercollectornetclassconfig) | Defines the configuration of the `netclass` collector, which collects network devices information. Enabled by default. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| netclass | [NodeExporterCollectorNetClassConfig](#nodeexportercollectornetclassconfig) | Defines the configuration of the `netclass` collector, which collects network devices information. Enabled by default. |
| netclass | [NodeExporterCollectorNetClassConfig](#nodeexportercollectornetclassconfig) | Defines the configuration of the `netclass` collector, which collects information about network devices. Enabled by default. |

bburt-rh
bburt-rh reviewed Feb 21, 2023
View reviewed changes
Documentation/openshiftdocs/modules/nodeexportercollectorconfig.adoc Outdated
@@ -24,6 +24,8 @@ Appears in: link:nodeexporterconfig.adoc[NodeExporterConfig]

|netdev|link:nodeexportercollectornetdevconfig.adoc[NodeExporterCollectorNetDevConfig]|Defines the configuration of the `netdev` collector, which collects network devices statistics. Enabled by default.

|netclass|link:nodeexportercollectornetclassconfig.adoc[NodeExporterCollectorNetClassConfig]|Defines the configuration of the `netclass` collector, which collects network devices information. Enabled by default.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
|netclass|link:nodeexportercollectornetclassconfig.adoc[NodeExporterCollectorNetClassConfig]|Defines the configuration of the `netclass` collector, which collects network devices information. Enabled by default.
|netclass|link:nodeexportercollectornetclassconfig.adoc[NodeExporterCollectorNetClassConfig]|Defines the configuration of the `netclass` collector, which collects information about network devices. Enabled by default.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2023
@raptorsun
Copy link
Contributor Author

Thank for the review @bburt-rh :)
The requested modifications have been pushed into the new commit.

@bburt-rh
Copy link
Contributor

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Feb 21, 2023
@simonpasquier
Copy link
Contributor

/skip

simonpasquier
simonpasquier reviewed Feb 22, 2023
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2023
@raptorsun
Copy link
Contributor Author

🙋‍♂️Hello @Tai-RedHat @Senthamilarasu-STA , could you please review this PR? If everything is good, could you stamp it with label qe-approved, px-approved?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2023
@raptorsun
Copy link
Contributor Author

Merge conflict is resolved. Need stamp lgtm again 🙋‍♂️

@Tai-RedHat
Copy link

tested PR with cluster-bot, new feature implemented.
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 23, 2023
@jan--f
Copy link
Contributor

jan--f commented Feb 23, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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,simonpasquier]

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

@Tai-RedHat
Copy link

@raptorsun , shouldn't we make netclass enabled by default?

| netclass | [NodeExporterCollectorNetClassConfig](#nodeexportercollectornetclassconfig) | Defines the configuration of the `netclass` collector, which collects information about network devices. Enabled by default. |

@raptorsun
Copy link
Contributor Author

raptorsun commented Feb 23, 2023 •
edited
Loading

@raptorsun , shouldn't we make netclass enabled by default?

| netclass | [NodeExporterCollectorNetClassConfig](#nodeexportercollectornetclassconfig) | Defines the configuration of the `netclass` collector, which collects information about network devices. Enabled by default. |

Yes, the netclass collector is enabled by default, retaining the same behavior as before.
Its default value is set here: https://github.com/openshift/cluster-monitoring-operator/pull/1893/files#diff-420c938cae65d6e9c4d4e386d768532276257149dbcaf476a90884a29fe36bd4R211

@Tai-RedHat
Copy link

Tai-RedHat commented Feb 23, 2023 •
edited
Loading

@raptorsun , shouldn't we make netclass enabled by default?

| netclass | [NodeExporterCollectorNetClassConfig](#nodeexportercollectornetclassconfig) | Defines the configuration of the `netclass` collector, which collects information about network devices. Enabled by default. |

Yes, the netclass collector is enabled by default, retaining the same behavior as before.

ok, but what you wrote in Jira Description is the default value is false.
https://issues.redhat.com/browse/MON-2900

@raptorsun
Copy link
Contributor Author

@raptorsun , shouldn't we make netclass enabled by default?

| netclass | [NodeExporterCollectorNetClassConfig](#nodeexportercollectornetclassconfig) | Defines the configuration of the `netclass` collector, which collects information about network devices. Enabled by default. |

Yes, the netclass collector is enabled by default, retaining the same behavior as before.

ok, but what you wrote in Jira Description is the default value is false. https://issues.redhat.com/browse/MON-2900

oops, my bad, it is corrected now in the JIRA ticket.
Thank you very much :)

@raptorsun
Copy link
Contributor Author

🙋‍♂️ Hello @Senthamilarasu-STA, the PR just needs a px-approved label before merging. Could you please have a look?

@Senthamilarasu-STA
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Feb 27, 2023
@simonpasquier
Copy link
Contributor

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 27, 2023

@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-merge-robot openshift-merge-robot merged commit c3066c0 into openshift:master Feb 27, 2023
@raptorsun raptorsun deleted the feature/MON-2900 branch June 9, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonpasquier simonpasquier simonpasquier left review comments

@bburt-rh bburt-rh bburt-rh left review comments

@jan--f jan--f Awaiting requested review from jan--f

@sthaha sthaha Awaiting requested review from sthaha

Assignees

@jan--f jan--f

@simonpasquier simonpasquier

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@raptorsun @openshift-ci-robot @simonpasquier @bburt-rh @Tai-RedHat @jan--f @Senthamilarasu-STA @openshift-merge-robot