Safe check rlimit syscall and refactoring #3801

hoenirvili · 2017-03-19T22:10:26Z

This PR contains:

this contains refactoring
this adds safe checking around rlimit syscall
more doc string

Kubuxu · 2017-03-19T23:09:21Z

Ok, few things:

please split refactors, added tests and docs into separate comments.
if you are changing the rlimit logic please also change it for freebsd, or preferably extract the code that has to be different on freebsd vs rest (getting and setting the rlimit) and reuse the rest.
the dependency of yours is not available - please coordinate with me on IRC to pin it on our nodes
more in review comments

EDIT: I see that freebsd code is in sync, but it would be best to DRY it up as the logic got a bit more complex. It should be safe to use uint64 in general and just have helper methods that would execute those calls (and case it to int64 in case of freebsd).

Kubuxu · 2017-03-19T23:09:50Z

cmd/ipfs/runmain_test.go

 )

 // this abuses go so much that I felt dirty writing this code
 // but it is the only way to do it without writing custom compiler that would
 // be a clone of go-build with go-test
-func TestRunMain(t *testing.T) {
+func (m mainRun) TestRunMain(c *gc.C) {


Not sure if it will fly with coverage collection code. We will see when dependency thing is fixed.

This change is breaking coverage collection func TestRunMain(t *testing.T) is run when testing starts and it takes over the testing process. In this case it just executes ipfs process not the tests. It is guided by the build flag of this file.

Kubuxu · 2017-03-19T23:14:50Z

The ulimit raising is tested in sharness, so the unit tests are not needed 100% but useful nevertheless. If it causes problems with coverage collection we can move the ulimit raising to separate package and test it alone.

hoenirvili · 2017-03-19T23:19:24Z

I'm open to any good solution. From my standpoint of view I think every file should contains a *_test.go. It's kind of scary that the project is missing so much unit testing.

Kubuxu · 2017-03-19T23:21:37Z

Yes we are constantly trying to push for more unit testing. We have also quite a big sharness testing suite. Checkout test/sharness/

Kubuxu · 2017-03-19T23:22:32Z

cmd/ipfs/main_test.go

 	"github.com/ipfs/go-ipfs/commands"
+
+	gc "gopkg.in/check.v1"


This dependency has to be rewritten, please run gx-go rewrite

whyrusleeping · 2017-03-20T16:50:28Z

This needs to be broken up into logical atomic changesets

Kubuxu · 2017-03-20T16:55:41Z

cmd/ipfs/ulimit_freebsd.go

+				return fmt.Errorf("error setting ulimit without hard limit: %s", err)
+			}
+
+			log.Infof("Successfully raised file descriptor limit to %d.\n", maxFds)


Info is not default on channel so it won't be visible. We want this message to be visible during daemon start, that is the reason it was using fmt.Printf before.

hsanjuan · 2017-03-21T15:08:59Z

Sorry to party-poop too, but there has been previous proposals to bring-in testing helper libraries and so far they have not moved forward. This would effectively introduce testing deps. See #3498

hoenirvili · 2017-03-21T17:01:56Z

@hsanjuan check #3498

whyrusleeping · 2017-03-21T17:46:37Z

cmd/ipfs/main_test.go

-	if !isClientError(&commands.Error{Code: commands.ErrClient}) {
-		t.Errorf("misidentified pointer")
-	}
+func (m mainRun) TestIsCientErr(c *gc.C) {


This new way of testing this is not shorter, cleaner, or easier to understand. I'm not on board with this.

I disagree completely.
I think more mantainers of this project should take a look and give more feedback.
The c.Assert besides of line number and fail message it contains these lines as well "expected: value, obtained: value". which it gives us way more information on "what went wrong". If you want I can use testify to write these tests.
But keep relaying and using the testing pkg from std lib it's not good idea, in the long run. (in my opinion).

You are free to have your opinions, but to convince me that this is worthwhile and not just some random contributor trying to push their opinions on a project they just found you'll have to do better convincing me of this.

You need to choose better examples to prove your point, because this specific one is not helped by shoving a test lib into it. "expected: true, got: false" Is quite clear from reading the existing test and the debuggability is not helped by adding in the extra cognitive overhead of this framework.

I have to agree with @whyrusleeping on this, in this case misidentified value is much more descriptive than expected: true, got: false.

Also replacing the c.Assert(ok, gc.Equals, true) with

if !ok { t.Error("misidentified value") }

Is easier and faster to type, conveys more information and everybody knowns what is going on.
There is no value added of c.Assert in this part of code.

hoenirvili · 2017-03-21T21:55:58Z

@whyrusleeping @Kubuxu now it's ok?

whyrusleeping · 2017-03-21T22:26:53Z

cmd/ipfs/init.go

@@ -9,6 +9,7 @@ import (
 	"path"

 	context "context"
+


while we're modifying this, lets go ahead and drop the named import (holdover from "golang.org/x/net/context") and move it up with the rest of the stdlib

whyrusleeping · 2017-03-21T22:27:26Z

cmd/ipfs/ulimit.go

@@ -5,15 +5,25 @@ import (
 	"strconv"
 )

-var ipfsFileDescNum = uint64(1024)
+// maxFds is the maximum number of file descriptors that the go ipfs


s/the go ipfs/go-ipfs/

whyrusleeping · 2017-03-21T22:27:55Z

cmd/ipfs/ulimit.go

@@ -5,15 +5,25 @@ import (
 	"strconv"
 )

-var ipfsFileDescNum = uint64(1024)
+// maxFds is the maximum number of file descriptors that the go ipfs
+// can use. Default values is 1024. This can be overwritten by the


s/Default values/The default value/

whyrusleeping · 2017-03-21T22:28:15Z

cmd/ipfs/ulimit.go

 		}
+
+		// assign the


useless comment

whyrusleeping · 2017-03-21T22:30:26Z

cmd/ipfs/ulimit_freebsd.go

+	// in the range from 0 up to the hard limit
+	if err = syscall.Setrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {
+		// the process does not have permission so we should only set the soft value
+		if err == syscall.EPERM {


reverse the logic here: if err != syscall.EPERM { ...

whyrusleeping · 2017-03-21T22:31:46Z

cmd/ipfs/ulimit_test.go

+	t.Logf("Testing file descriptor invalidity")
+	var err error
+	if err = os.Unsetenv("IPFS_FD_MAX"); err != nil {
+		t.Errorf("Cannot unset the IPFS_FD_MAX env variable")


Use fatal for these, the rest of the test is invalid if this assertion (or the others) fails.

whyrusleeping · 2017-03-21T22:32:02Z

cmd/ipfs/ulimit_test.go

+		t.Errorf("Managefd should return an error")
+	}
+
+	if err != nil {


whyrusleeping · 2017-03-21T22:33:49Z

cmd/ipfs/ulimit_windows.go

+
+package main
+
+// managefd checks for the maximum number of file descriptors that the system supports


This comment is incorrect.

whyrusleeping · 2017-03-21T22:34:20Z

cmd/ipfs/ulimit_unix.go

+	// the hard limit acts as a ceiling for the soft limit
+	// an unprivileged process may only set its soft limit to a value
+	// in the range from 0 up to the hard limit
+	if err = syscall.Setrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {


This logic is nearly identical to the other freebsd logic. I'm sure we can DRY it up a bit

whyrusleeping

A few comments, lets try to DRY up the limit setting code so we don't mess up changes to it later

Kubuxu · 2017-03-21T22:51:51Z

cmd/ipfs/ulimit_test.go

+
+	rlimit := syscall.Rlimit{}
+	if err = syscall.Getrlimit(syscall.RLIMIT_NOFILE, &rlimit); err != nil {
+		t.Errorf("Cannot get the file descriptor count")


Fatal here too as rest is useless if this fails

Kubuxu · 2017-03-21T23:06:33Z

I would suggest pulling the fd managing code to separate package (it has nothing to do with ipfs main process itself) and also switching from directly referencing functions under build tag to using init to inject the platform specific code into a private variable. This way we don't have to have build tag for every not supported OS.

Example how it could be done:
Core file (no build tags):

// hasFDManage is used to determine if we should run tests
// (if we have generic, platform antagonistic test (open a lot of empty files)).
var hasFDManage bool = false
var manageFDs func() error = func() { return nil }

func ManageFDs() error {
    return manageFDs()
}

Then file that builds on unix:

func init() {
hasFDManage = true
manageFDs = unixManageFDs()
}

func unixManageFDs()

Then as freebsd is special, one file for non freebsd unix:

// +build unix,!freebsd

func getFDlimits...
func setFDlimits...

and separate one for freebsd:

// +build freebsd

func getFDlimits...
func setFDlimits...

whyrusleeping · 2017-03-23T04:52:39Z

cmd/ipfs/util/ulimit.go

+			return
+		}
+
+		// assign the


whats this comment here for?

Sorry, I will delete it.

hoenirvili · 2017-03-23T22:05:11Z

Why not use the runtime.GOOS ?

whyrusleeping · 2017-03-23T22:22:26Z

@hoenirvili I'm assuming youre asking why use build tags over a switch on runtime.GOOS. And my answer to that is "you can", but at some point you will have to deal with the fact that the syscall.RLimit has different types on different systems. The only decent way to deal with that is to use build tags and normalize it with some small wrapper code.

hoenirvili · 2017-03-23T22:29:23Z

Can you please take a look one more time at the code? What changes should I make?

whyrusleeping · 2017-03-23T22:43:29Z

@hoenirvili Something like this: https://gist.github.com/whyrusleeping/8c3bae34723f209665480f39d2163848

Let me know if i should be more detailed

whyrusleeping · 2017-03-23T22:48:35Z

cmd/ipfs/util/ulimit.go

+	"os"
+	"strconv"
+
+	"github.com/prometheus/common/log"


This is the wrong logging package

Import logging "github.com/ipfs/go-logging" and then do a global var log = logging.Logger("ulimit")

whyrusleeping · 2017-03-23T22:50:19Z

The reasoning for the construction in the linked gist is to ensure that when trying this for new unsupported platforms, one: it doesnt fail to build because we forgot a special file with its particular build tags, and two: its much easier to implement support on new systems, just by implementing the getFDLimit and setFDLimit functions on the new platform.

hoenirvili · 2017-04-02T10:48:41Z

Sorry for taking so much time... I was very busy these couple of weeks. I've updated the patch, you can take a look and tell what improvements I can add further.

hoenirvili · 2017-04-10T15:20:24Z

What about now ?

whyrusleeping · 2017-04-12T05:26:29Z

@hoenirvili Hey, I'll take another look at this tomorrow. Sorry for taking so long to get back to you here, i've been out for a week

hoenirvili · 2017-04-12T11:10:08Z

@whyrusleeping Thanks !

Kubuxu

See the comments and also CodeClimate issue.

Kubuxu · 2017-04-12T19:30:30Z

cmd/ipfs/util/ulimit.go

+// maxFds is the maximum number of file descriptors that go-ipfs
+// can use. The default value is 1024. This can be overwritten by the
+// IPFS_FD_MAX env variable
+var maxFds = uint64(1024)


Master has increased it to 2048.

Kubuxu · 2017-04-12T19:31:11Z

cmd/ipfs/util/ulimit.go

+	}
+}
+
+func ManageFdLimit() (err error) {


remove the naming of the return value here

Kubuxu · 2017-04-12T19:31:24Z

cmd/ipfs/util/ulimit.go

+
+func ManageFdLimit() (err error) {
+	if !supportsFDManagement {
+		return


and do explicit nil or error return here

Kubuxu · 2017-04-12T19:33:05Z

cmd/ipfs/util/ulimit.go

+	// the hard limit acts as a ceiling for the soft limit
+	// an unprivileged process may only set it's soft limit to a
+	// alue in the range from 0 up to the hard limit
+	if err = setLimit(max, max); err != nil {


I am not sure this is needed. Just setting it to max, hard should be ok (and before checking if max is < than hard).

Kubuxu · 2017-04-12T19:33:35Z

cmd/ipfs/util/ulimit.go

+		// set the soft value
+		if max > hard {
+			return errors.New(
+				"cannot set rlimit, IPFS_FD_MAX is larger than the hard limit",


That might be not true, the builtin default rise might be larger than the hard limit

Kubuxu · 2017-04-12T19:35:58Z

cmd/ipfs/util/ulimit.go

+	"strconv"
+	"syscall"
+
+	logging "github.com/ipfs/go-log"


run gx-go rw to fix it and allow tests to run

Kubuxu · 2017-04-21T12:54:30Z

Sharness test checking for the message is failing.

hoenirvili · 2017-04-21T19:38:22Z

@Kubuxu Yes, I know but, I don't have experience with the sharness framework to modify the code in order to make the sharness test logic pass. Can you provide some advice on this topic?

whyrusleeping · 2017-05-18T05:08:19Z

Hey @hoenirvili! Sorry for taking nearly a month to get back to you here. I pulled your branch down and figured out how to get the tests passing, here was my diff:

diff --git a/cmd/ipfs/util/ulimit.go b/cmd/ipfs/util/ulimit.go
index ed7f37fc1..d4669631a 100644
--- a/cmd/ipfs/util/ulimit.go
+++ b/cmd/ipfs/util/ulimit.go
@@ -49,12 +49,17 @@ func ManageFdLimit() error {
        }
 
        setMaxFds()
-       _, hard, err := getLimit()
+       soft, hard, err := getLimit()
        if err != nil {
                return err
        }
 
        max := int64(maxFds)
+
+       if max <= soft {
+               return nil
+       }
+
        // the soft limit is the value that the kernel enforces for the
        // corresponding resource
        // the hard limit acts as a ceiling for the soft limit
@@ -77,9 +82,8 @@ func ManageFdLimit() error {
                        return fmt.Errorf("error setting ulimit wihout hard limit: %s", err)
                }
 
-               fmt.Printf("Successfully raised file descriptor limit to %d.\n", max)
-               return nil
        }
+       fmt.Printf("Successfully raised file descriptor limit to %d.\n", max)
 
        return nil
 }

I also removed the file cmd/ipfs/ulimit.go since it is no longer needed

hoenirvili · 2017-06-07T09:16:05Z

@whyrusleeping changed.

The soft limit is the value that the kernel enforces for the corresponding resource The hard limit acts as a ceiling for the soft limit an unprivileged process may only set its soft limit to a value in the range from 0 up to the hard limit. So in order to make the change in fds count without any error we should inform the user to make the process have CAP_SYS_RESOURCE capability in order to set the hard limit. License: MIT Signed-off-by: hoenirvili <[email protected]>

kevina · 2017-12-04T22:30:29Z

@whyrusleeping I just rebased

whyrusleeping · 2017-12-05T17:34:35Z

@kevina thanks!

@hoenirvili Thanks for the changes! Sorry about the delay, i'm working on getting through the mountain of PRs now.

Kubuxu reviewed Mar 19, 2017

View reviewed changes

Kubuxu reviewed Mar 20, 2017

View reviewed changes

whyrusleeping reviewed Mar 21, 2017

View reviewed changes

hoenirvili changed the title ~~Safe check rlimit syscall, refactoring and new dependency~~ Safe check rlimit syscall and refactoring Mar 21, 2017

whyrusleeping reviewed Mar 21, 2017

View reviewed changes

cmd/ipfs/ulimit.go Outdated

}

// assign the

Copy link

Member

whyrusleeping Mar 21, 2017

Choose a reason for hiding this comment

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

useless comment

whyrusleeping reviewed Mar 21, 2017

View reviewed changes

cmd/ipfs/ulimit_test.go Outdated

t.Errorf("Managefd should return an error")

}

if err != nil {

Copy link

Member

whyrusleeping Mar 21, 2017

Choose a reason for hiding this comment

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

else if

whyrusleeping reviewed Mar 21, 2017

View reviewed changes

whyrusleeping requested changes Mar 21, 2017

View reviewed changes

Kubuxu reviewed Mar 21, 2017

View reviewed changes

whyrusleeping reviewed Mar 23, 2017

View reviewed changes

whyrusleeping self-assigned this Apr 12, 2017

Kubuxu unassigned whyrusleeping Apr 12, 2017

Kubuxu requested changes Apr 12, 2017

View reviewed changes

ghost assigned kevina Dec 4, 2017

ghost added the status/in-progress In progress label Dec 4, 2017

whyrusleeping approved these changes Dec 5, 2017

View reviewed changes

whyrusleeping merged commit 715bd16 into ipfs:master Dec 5, 2017

ghost removed the status/in-progress In progress label Dec 5, 2017


		package main

		// managefd checks for the maximum number of file descriptors that the system supports

Safe check rlimit syscall and refactoring #3801

Safe check rlimit syscall and refactoring #3801

Conversation

hoenirvili commented Mar 19, 2017 • edited Loading

Kubuxu commented Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Mar 19, 2017

hoenirvili commented Mar 19, 2017

Kubuxu commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Mar 20, 2017

Choose a reason for hiding this comment

hsanjuan commented Mar 21, 2017

hoenirvili commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

hoenirvili commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoenirvili commented Mar 23, 2017

whyrusleeping commented Mar 23, 2017

hoenirvili commented Mar 23, 2017

whyrusleeping commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Mar 23, 2017

hoenirvili commented Apr 2, 2017

hoenirvili commented Apr 10, 2017

whyrusleeping commented Apr 12, 2017

hoenirvili commented Apr 12, 2017

Kubuxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Apr 21, 2017

hoenirvili commented Apr 21, 2017 • edited Loading

whyrusleeping commented May 18, 2017

hoenirvili commented Jun 7, 2017

kevina commented Dec 4, 2017

whyrusleeping commented Dec 5, 2017

hoenirvili commented Mar 19, 2017 •

edited

Loading

Kubuxu commented Mar 19, 2017 •

edited

Loading

Kubuxu Mar 21, 2017 •

edited

Loading

Kubuxu commented Mar 21, 2017 •

edited

Loading

Kubuxu Apr 12, 2017 •

edited

Loading

hoenirvili commented Apr 21, 2017 •

edited

Loading