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

Reduce useless log messages about default CNI network #208

Conversation

roman-kiselenko
Copy link
Member

@roman-kiselenko roman-kiselenko commented Jul 5, 2024

The logs with empty default CNI network updates are useless and produce much noise.

time="2024-06-25 19:33:12.783576005Z" level=info msg="Updated default CNI network name to "
time="2024-06-25 19:33:12.783958197Z" level=info msg="Updated default CNI network name to "
time="2024-06-25 19:33:12.784004164Z" level=info msg="Updated default CNI network name to "
time="2024-06-25 19:33:12.784321809Z" level=info msg="Updated default CNI network name to "
time="2024-06-25 19:33:12.784364691Z" level=info msg="Updated default CNI network name to "

This PR change log, so only actual changes are logged.

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Reduce useless log messages.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Reduce useless log messages about default CNI network changes

@openshift-ci openshift-ci bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jul 5, 2024
@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jul 5, 2024
@roman-kiselenko roman-kiselenko force-pushed the feature/ignore-empty-default-cni branch from caa5b92 to e4a886b Compare July 5, 2024 13:45
@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jul 5, 2024
@coveralls
Copy link

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9809293589

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 62.037%

Totals Coverage Status
Change from base Build 9657830375: 0.7%
Covered Lines: 536
Relevant Lines: 864

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9809302081

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 61.458%

Totals Coverage Status
Change from base Build 9657830375: 0.09%
Covered Lines: 531
Relevant Lines: 864

💛 - Coveralls

@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 5, 2024
@roman-kiselenko roman-kiselenko changed the title Reduce useless log message about default CNI network Reduce useless log messages about default CNI network Jul 5, 2024
@kwilczynski
Copy link
Member

kwilczynski commented Jul 8, 2024

Related:

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2024
Copy link
Contributor

openshift-ci bot commented Jul 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: roman-kiselenko, saschagrunert

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 0692f4e into cri-o:master Jul 8, 2024
5 checks passed
@kwilczynski
Copy link
Member

@roman-kiselenko, there is also a different angle to this problem.

The CNI plugins can be configured using .conf and .json files for a single network configuration and .conflict (there is no JSON file equivalent for conflist-style configuration) when configuring multiple networks. The directories that are usually involved in the whole CNI shebang are:

  • /etc/cni/net.d
  • /etc/kubernetes/cni/net.d
  • /var/lib/cni/networks
  • /var/lib/cni/results

We watch these directories which are also shared with other things, such as the specific CNI runtimes, etc. This can lead to a lot of spurious file system events, per:

An excerpt from the CRI-O log:

Jun 25 19:34:51 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:34:51.427866852Z" level=info msg="CNI monitoring event REMOVE        \"/var/lib/cni/bin/upgrade_5a6545b2-25e7-4511-8b40-e882c1305752\""
Jun 25 19:35:59 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:35:59.278298219Z" level=info msg="CNI monitoring event CREATE        \"/var/lib/cni/bin/upgrade_c1b67453-f6f3-4303-928e-b5fb53550991\""
Jun 25 19:35:59 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:35:59.278379200Z" level=info msg="Updated default CNI network name to "
Jun 25 19:35:59 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:35:59.404745277Z" level=info msg="CNI monitoring event CREATE        \"/var/lib/cni/bin/cert-approver\""
Jun 25 19:35:59 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:35:59.404839986Z" level=info msg="Updated default CNI network name to "
Jun 25 19:35:59 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:35:59.408682926Z" level=info msg="CNI monitoring event CREATE        \"/var/lib/cni/bin/install_multus\""
Jun 25 19:35:59 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:35:59.408748001Z" level=info msg="Updated default CNI network name to "
Jun 25 19:35:59 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:35:59.408900356Z" level=info msg="CNI monitoring event CREATE        \"/var/lib/cni/bin/kubeconfig_generator\""
Jun 25 19:35:59 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:35:59.408956448Z" level=info msg="Updated default CNI network name to "

Eventually, the proper configuration will land:

Jun 25 19:36:22 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:36:22.545735113Z" level=info msg="CNI monitoring event CREATE        \"/etc/kubernetes/cni/net.d/00-multus.conf\""
Jun 25 19:36:22 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:36:22.561614840Z" level=info msg="Found CNI network multus-cni-network (type=multus-shim) at /etc/kubernetes/cni/net.d/00-multus.conf"
Jun 25 19:36:22 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:36:22.561643272Z" level=info msg="Updated default CNI network name to multus-cni-network"
Jun 25 19:36:22 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:36:22.561664302Z" level=info msg="CNI monitoring event WRITE         \"/etc/kubernetes/cni/net.d/00-multus.conf\""
Jun 25 19:36:22 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:36:22.574319103Z" level=info msg="Found CNI network multus-cni-network (type=multus-shim) at /etc/kubernetes/cni/net.d/00-multus.conf"
Jun 25 19:36:22 kwilczynski-dev2-th748-worker-a-kg8lj crio[2219]: time="2024-06-25 19:36:22.574350829Z" level=info msg="Updated default CNI network name to multus-cni-network"

Thus, as much as suppressing the log line will make it "go away", the proper solution here might be an update to the code that handles file system events.

@roman-kiselenko roman-kiselenko deleted the feature/ignore-empty-default-cni branch July 8, 2024 08:41
@kwilczynski
Copy link
Member

@roman-kiselenko, thoughts on my comment?

@roman-kiselenko
Copy link
Member Author

@roman-kiselenko, thoughts on my comment?

🤔 I tried to determine which events can be ignored, but have not yet been able to find the desired configuration, I will try to investigate the problem more carefully, thanks.

Maybe separate issue would be good for cleanup work.

@kwilczynski
Copy link
Member

@roman-kiselenko, thoughts on my comment?

🤔 I tried to determine which events can be ignored, but have not yet been able to find the desired configuration, I will try to investigate the problem more carefully, thanks.

Maybe separate issue would be good for cleanup work.

@roman-kiselenko, the events are fine. What I suggested doing, would be to ignore everything that is not a valid CNI configuration file. There is little point having this code fire every time a CNI plugin downloads some files, or stored a temporary file to later delete it. All of this creates an event, to which the code reacts, and there is no need to reload CNI configuration cache, which would be empty, for some random binaries downloaded the CNI plugins, etc.

Does this make more sense now? I hope it does. 😄

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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants