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 option to disable the use of inotify #92

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

Luap99
Copy link
Contributor

@Luap99 Luap99 commented Jun 16, 2021

Add a new InitCNINoInotify function to allow the use of OCICNI without
the use of inotify.

For some workloads it is not required to watch the cni config directory.
With podman v3.2 we started using OCICNI for rootless users as well.
However the use of inotify is restricted by sysctl values
(fs.inotify.max_user_instances and fs.inotify.max_user_watches).
By default only 128 processes can use inotify.

Since this limit is easy to reach and inotify is not required for our
use case it would be great to have this option to disable it.

see containers/podman#10686

Add new InitCNINoInotify function to disable the use of inotify.

Add a new InitCNINoInotify function to allow the use of OCICNI without
the use of inotify.

For some workloads it is not required to watch the cni config directory.
With podman v3.2 we started using OCICNI for rootless users as well.
However the use of inotify is restricted by sysctl values
(fs.inotify.max_user_instances and fs.inotify.max_user_watches).
By default only 128 processes can use inotify.

Since this limit is easy to reach and inotify is not required for our
use case it would be great to have this option to disable it.

see containers/podman#10686

Signed-off-by: Paul Holzinger <[email protected]>
@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. 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 Jun 16, 2021
@openshift-ci openshift-ci bot requested review from dcbw and rajatchopra June 16, 2021 16:23
@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 Jun 16, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 943565137

  • 11 of 18 (61.11%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.5%) to 65.331%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/ocicni/ocicni.go 11 18 61.11%
Files with Coverage Reduction New Missed Lines %
pkg/ocicni/ocicni.go 3 69.77%
Totals Coverage Status
Change from base Build 928682465: -0.5%
Covered Lines: 473
Relevant Lines: 724

💛 - Coveralls

@haircommander
Copy link
Member

/approve

LGTM

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2021
Copy link
Member

@saschagrunert saschagrunert 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
Copy link
Contributor

openshift-ci bot commented Jun 17, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, Luap99, 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:
  • OWNERS [haircommander,saschagrunert]

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

@Luap99
Copy link
Contributor Author

Luap99 commented Jun 17, 2021

@saschagrunert @haircommander Would it be possible that someone creates a branch without commit f9d4247 and 8480c61? I want to vendor this into podman to fix a critical bug but this would require a bump to CNI v1.0.0-rc1. This is not possible because breaking changes in CNI, podman and another library still depend on v0.8.1.

@saschagrunert
Copy link
Member

@saschagrunert @haircommander Would it be possible that someone creates a branch without commit f9d4247 and 8480c61? I want to vendor this into podman to fix a critical bug but this would require a bump to CNI v1.0.0-rc1. This is not possible because breaking changes in CNI, podman and another library still depend on v0.8.1.

We do not have a mechanism in place for something like this, yet. I'm not opposed in creating a new branch, WDYT @haircommander ?

@haircommander
Copy link
Member

I'm not oppsed! though I don't currently have permssions to do so. @mrunalp can you make a cni-0.x branch off of 10dec7e

@mrunalp
Copy link
Member

mrunalp commented Jun 17, 2021

@Luap99
Copy link
Contributor Author

Luap99 commented Jun 18, 2021

@mrunalp Thanks, I opened #93 to get this commit into the new branch.

@haircommander
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2021
@openshift-merge-robot openshift-merge-robot merged commit 5ce5cee into cri-o:master Jun 21, 2021
@Luap99 Luap99 deleted the no-inotify branch June 21, 2021 13:55
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. 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.

6 participants