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

Check Process limit on node start up #1503

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Check Process limit on node start up #1503

wants to merge 16 commits into from

Conversation

atchapcyp
Copy link
Contributor

ref : #1495

  • I added unit tests for any code that added
  • I updated the CHANGELOG.md
  • All IP is original and not copied from another source
  • I assign all copyright to Loom Network for the code in the pull request

config/config.go Outdated Show resolved Hide resolved
cmd/loom/loom.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
cmd/loom/loom.go Outdated
return err
}
if rlimit.Cur < min {
return errors.New("insufficient file descriptor")
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this line to
return fmt.Errorf("Current file descriptor limit (%d) is too low; minimum file descriptor limit is %d", rlimit.Cur, min)

config/config.go Outdated
CallEnabled: true,
DPOSVersion: 3,
AllowNamedEvmContracts: false,
MinimumFileDescriptorLimit: 512,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure 512 is enough but 500000 is definitely not practical.

Copy link
Contributor

Choose a reason for hiding this comment

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

500k is what we use on all our nodes, and what we instruct people to use in the jump-start docs. 500k might be overkill, lets set the minimum to 100k.

config/config.go Outdated
CallEnabled: true,
DPOSVersion: 3,
AllowNamedEvmContracts: false,
MinimumFileDescriptorLimit: 512,
Copy link
Contributor

Choose a reason for hiding this comment

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

500k is what we use on all our nodes, and what we instruct people to use in the jump-start docs. 500k might be overkill, lets set the minimum to 100k.

config/config.go Outdated
@@ -141,6 +141,9 @@ type Config struct {

// Dragons
EVMDebugEnabled bool

//Minimum number of file descriptor limit
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after // please

cmd/loom/loom.go Outdated
@@ -337,6 +337,13 @@ func newRunCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "run [root contract]",
Short: "Run the blockchain node",
PreRunE: func(cmd *cobra.Command, args []string) error {
if err := checkFileDescriptorLimit(cfg.MinimumFileDescriptorLimit); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to check cfg was actually parsed successfully before attempting to use it. Just move this code to RunE below.

Copy link
Contributor

Choose a reason for hiding this comment

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

And skip the limit check if MinimumFileDescriptorLimit == 0

config/config.go Outdated Show resolved Hide resolved
cmd/loom/loom.go Outdated Show resolved Hide resolved
cmd/loom/loom.go Outdated Show resolved Hide resolved
@atchapcyp atchapcyp self-assigned this Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants