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

fdlimit: improve fdlimit autoraising tests #3149

Merged
merged 2 commits into from
Sep 3, 2016

Conversation

whyrusleeping
Copy link
Member

Better tests for this, at least on linux we can check it externally. I don't know of a good way to do this on OSX

@whyrusleeping whyrusleeping added status/in-progress In progress need/review Needs a review and removed status/in-progress In progress labels Aug 30, 2016
@Kubuxu
Copy link
Member

Kubuxu commented Aug 30, 2016

LGTM

@jbenet do you want to also give it a review?

awk '{ print $4 }' $1
}

if [ `uname` == "Linux" ]; then
Copy link
Member

@jbenet jbenet Aug 31, 2016

Choose a reason for hiding this comment

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

  • this is nice
  • but we really need to also test in osx. and we shouldn't trust what the limits say, should test for sure that we can indeed open that many fds.
  • http://stackoverflow.com/questions/31115764/setrlimit-doesnt-work-on-os-x-maverick-and-yosemite
    • setrlimit may not work on osx, and many times osx reports incorrect fd limits
  • we can test things for real (in both osx and linux) with a tool like open-fds <fds> <multiaddr> which uses nc to hang fds open.
    • then count the fds with lsof. maybe: lsof -p $1 | grep -v " txt " | wc -l
    • then kill the open-fds process.

Copy link
Member

Choose a reason for hiding this comment

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

The open-fds process would have to start from go-ipfs process, to inherit the fd limit.
Otherwise we are testing nothing.

Other approach would be to make the fd rising separate package, produce a small binary that tries opening files with and without the fd limit raised.

Copy link
Member Author

Choose a reason for hiding this comment

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

"this is nice"

image

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm using syscall.Setrlimit in https://github.com/jbenet/hang-fds/blob/master/hang-fds.go#L32-L63

> ulimit -Sn 256
> ulimit -Sn
256
> ./hang-fds 2048 /ip4/127.0.0.1/tcp/1234
hanging 2048 fds at /ip4/127.0.0.1/tcp/1234
error: failed to raise fd limit to 2058 (still 256)

Copy link
Member Author

Choose a reason for hiding this comment

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

@jbenet you never actually change the fd limit in that code. you call getrlimit, you check the value, and then you you call setrlimit with the same object.

@jbenet jbenet removed their assignment Aug 31, 2016
@jbenet jbenet mentioned this pull request Sep 1, 2016
58 tasks
License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
…ally raised

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@whyrusleeping whyrusleeping merged commit 1f387af into version/0.4.3-rc4 Sep 3, 2016
@whyrusleeping whyrusleeping deleted the test/fdlimit-raising branch September 3, 2016 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants