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.

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

Add ExcludeInterfaceRegexp to Net Dev monitor #675

Merged

Conversation

mmiranda96
Copy link
Contributor

@mmiranda96 mmiranda96 commented Jun 3, 2022

What does this PR do?

As a solution for #659, this PR adds field ExcludeInterfaceRegexp to network device monitor. This field is used to stop tracking metrics for certain network interfaces.

This change also edits config net-cgroup-system-stats-monitor.json to exclude Calico interfaces (according to this doc, cali and tunl interfaces) and veth, which are interfaces created dynamically and only add noise to the metrics.

Additionally, I refactored the code in order to make it more testable and added some unit tests.

How to test this change?

  1. Create a Kubernetes cluster that uses Calico (I personally used GKE and followed instructions here). Ensure the cluster uses the new NPD version and config (in my case, I manually copied the binaries and config to the node).
  2. Run some workload, trigger new network interfaces creation, and ensure metrics are not being reported for Calico interfaces.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 3, 2022
@mmiranda96 mmiranda96 changed the title Add ExcludeInterfaceRegexp to Net Dev monitor WIP: Add ExcludeInterfaceRegexp to Net Dev monitor Jun 3, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2022
@mmiranda96 mmiranda96 force-pushed the feat/net-monitor-groupings branch 2 times, most recently from 3ee4018 to 0577a7d Compare June 15, 2022 00:16
@mmiranda96 mmiranda96 changed the title WIP: Add ExcludeInterfaceRegexp to Net Dev monitor Add ExcludeInterfaceRegexp to Net Dev monitor Jun 15, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2022
@mmiranda96
Copy link
Contributor Author

/cc @bobbypage

@bobbypage
Copy link
Member

Change LGTM, a few small nits about the test. Thanks for adding the the exclude feature and test coverage, it's a big improvement!

@mmiranda96
Copy link
Contributor Author

Addressed your comments @bobbypage. Thanks for the review!

@bobbypage
Copy link
Member

bobbypage commented Jun 21, 2022

Thanks!

/lgtm

/assign @Random-Liu

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2022
@Random-Liu
Copy link
Member

@vteratipally Could you take a look at this change? Thanks!

@vteratipally
Copy link
Collaborator

@mmiranda96 Did you happen to check if adding the filter improving the time reporting the metrics

@mmiranda96
Copy link
Contributor Author

@vteratipally I didn't check that specifically, I just ensured that metrics are properly filtered.

@vteratipally
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobbypage, mmiranda96, vteratipally

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2022
@k8s-ci-robot k8s-ci-robot merged commit 72f1672 into kubernetes:master Jun 29, 2022
@mmiranda96 mmiranda96 deleted the feat/net-monitor-groupings branch June 29, 2022 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants