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

fixed tests of raised fd limits #5496

Merged
merged 3 commits into from
Sep 25, 2018
Merged

fixed tests of raised fd limits #5496

merged 3 commits into from
Sep 25, 2018

Conversation

rob-deutsch
Copy link
Contributor

@rob-deutsch rob-deutsch commented Sep 20, 2018 •
edited
Loading

Raising FD limits was erroring when the OS's max was at the maximum signed integer value. Switched the code to using uint64 instead of int64.

Could someone please review, in particular, the FreeBSD parts? Integer checks are not my forte, and I don't have a FreeBSD machine to test on.

fixed #5495

Bonus: The ManageFdLimit() function contained a fmt.Print statement. That's really annoying because it was printing during tests. I've changed the ManageFdLimit() function so that it returns the new limit, and the calling function can do the fmt.Print statement (if it wishes).

@magik6k
Copy link
Member

magik6k commented Sep 20, 2018

Fails to cross-compile to FreeBSD: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fgo-ipfs/detail/PR-5496/3/pipeline/123

@rob-deutsch
Copy link
Contributor Author

Thanks @magik6k . I pushed a new set of commits with the bug fixed, and an extra commit which adds FreeBSD compilation to the make test command. Why should Jenkins have all the fun? :P

Would still like someone to review the FreeBSD code. I've just amended it to make it compile, but would like someone more knowledgeable to check the integer handling.

@magik6k
Copy link
Member

magik6k commented Sep 20, 2018

Sharness still fails, should be easy fix: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fgo-ipfs/detail/PR-5496/5/tests

> diff -u expected_daemon actual_daemon
--- expected_daemon 2018-09-20 12:31:42.345528549 +0000
+++ actual_daemon 2018-09-20 12:31:40.581579936 +0000
@@ -1,4 +1,5 @@
Initializing daemon...
+Successfully raised file descriptor limit to 0.
Swarm listening on /ip4/127.0.0.1/tcp/36394
Swarm listening on /p2p-circuit/ipfs/QmZkiWzie2TUr3KMTqXsRSBNxJ8xakWw1c5pqdwcsyLArT
Swarm announcing /ip4/127.0.0.1/tcp/36394

Stebalien
Stebalien reviewed Sep 20, 2018
View reviewed changes
mk/util.mk
@@ -20,6 +20,8 @@ SUPPORTED_PLATFORMS += linux-amd64
SUPPORTED_PLATFORMS += darwin-386
SUPPORTED_PLATFORMS += darwin-amd64

SUPPORTED_PLATFORMS += freebsd-amd64
Copy link
Member

Choose a reason for hiding this comment

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

👍 I believe we didn't have this before due to some reuseport issues (like usual...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for commenting. I'm not sure how this relates to reuseport though. This just directs make test to see whether it can compile a binary for FreeBSD.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, compiling on FreeBSD was broken at the time these tests were added.

magik6k
magik6k reviewed Sep 21, 2018
View reviewed changes
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

1 nitpick, otherwise looks good.

(can't get my freebsd vm to boot, so I'm going to assume this will work)

cmd/ipfs/util/ulimit_freebsd.go Outdated Show resolved Hide resolved
Stebalien
Stebalien reviewed Sep 24, 2018
View reviewed changes
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Two nits then LGTM.

cmd/ipfs/util/ulimit_freebsd.go Outdated Show resolved Hide resolved
cmd/ipfs/util/ulimit_freebsd.go Outdated Show resolved Hide resolved
@Stebalien Stebalien added the need/author-input Needs input from the original author label Sep 24, 2018
@rob-deutsch
fixed test of raised fd limits
182507d 
Raising FD limits was erroring when the OS's max was at the maximum signed integer value. Switched the code to using uint64 instead of int64.

fixed #5495

License: MIT
Signed-off-by: Rob Deutsch <rdeutschob@gmail.com>
@rob-deutsch
supressed fd util printing to output
8cca572 
Moved the fmt.Printf call from ManageFdLimit() to the calling code. ManageFdLimit() is called by tests and its annoying to have it output text

License: MIT
Signed-off-by: Rob Deutsch <rdeutschob@gmail.com>
@rob-deutsch
added freebsd cross-compile to 'make check' tests
aada0cc 
License: MIT
Signed-off-by: Rob Deutsch <rdeutschob@gmail.com>
@rob-deutsch
Copy link
Contributor Author

Correct me if I'm wrong, but I think the code is clean and its failed due to intermittent Travis CI issues, right?

@Stebalien Stebalien merged commit 1e0d53f into ipfs:master Sep 25, 2018
@Stebalien Stebalien removed the need/author-input Needs input from the original author label Sep 25, 2018
@Stebalien
Copy link
Member

(yes).

Thanks!

@rob-deutsch rob-deutsch deleted the fix/5495 branch September 25, 2018 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magik6k magik6k magik6k left review comments

@Stebalien Stebalien Stebalien approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cmd/ipfs/util max fd tests don't work on MacOS
3 participants
@rob-deutsch @magik6k @Stebalien