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

Fix the GOMAXPROCS warning for Upstart-based systems #39

Merged
merged 3 commits into from
Oct 28, 2014

Conversation

tehranian
Copy link

Description

Improvements to the Upstart startup script:

  • Set default value of GOMAXPROCS=2
  • Make GOMAXPROCS configurable via /etc/default/consul
  • Follow Upstart idioms like using setuid/setgid

Testing Done
Tested on Ubuntu 12.04:

  • Saw no more warnings about GOMAXPROCS in /var/log/upstart/consul.log using the default config

  • If I set GOMAXPROCS=1 in /etc/default/consul, the warning comes back so the sourcing of that file is working.

  • Saw that consul was started with the correct uid/gid :

    $ ps -eafww  | grep consul
    consul   15473     1  0 17:20 ?        00:00:03 /usr/local/bin/consul agent -config-dir /etc/consul
    
  • Verified that service consul restart and service consul reload both work as expected.

Improvements to the Upstart startup script:

  * Set default value of GOMAXPROCS=2
  * Make GOMAXPROCS configurable via "/etc/default/consul"
  * Follow Upstart idioms like using setuid/setgid
@solarkennedy
Copy link
Contributor

I appreciate the GOMAXPROCS setting

  • setuid / setgid settings are not supported on Ubuntu Lucid, which is a supported OS for this module
  • Tests are failing.

@tehranian
Copy link
Author

Sorry about that. I'll setup a VM with a proper Ruby development environment to run the tests and undo the setuid/setgid change.

Dan Tehranian added 2 commits October 26, 2014 19:46
…tu 10.04

Use `exec sudo ...` instead of Upstart's setuid/setgid directives
as those directives are not supported on older versions on Ubuntu.
@tehranian
Copy link
Author

  • Setup my Ruby dev VM w/necessary Docker support

  • Undid the setuid/setgid modification (Sorry!)

  • Tests now succeed:

    $ bundle exec rake test
    Your Gemfile lists the gem beaker (~> 1.11.0) more than once.
    You should probably keep only one of them.
    While it's not a problem now, it could cause errors if you change the version of just one of them later.
    ---> syntax:manifests
    ---> syntax:templates
    ---> syntax:hiera:yaml
    Cloning into 'spec/fixtures/modules/stdlib'...
    remote: Counting objects: 5661, done.
    remote: Compressing objects: 100% (19/19), done.
    remote: Total 5661 (delta 5), reused 0 (delta 0)
    Receiving objects: 100% (5661/5661), 1.13 MiB | 636 KiB/s, done.
    Resolving deltas: 100% (2365/2365), done.
    HEAD is now at 9dea092 Merge branch '4.3.x', add tempfile back for resource validate_cmd and validate_augeas
    Cloning into 'spec/fixtures/modules/staging'...
    remote: Counting objects: 591, done.
    remote: Compressing objects: 100% (7/7), done.
    remote: Total 591 (delta 0), reused 0 (delta 0)
    Receiving objects: 100% (591/591), 1.31 MiB | 842 KiB/s, done.
    Resolving deltas: 100% (239/239), done.
    HEAD is now at ffc158d Merge pull request #43 from mhaskel/strict_variables
    /home/vagrant/.rvm/rubies/ruby-1.9.3-p547/bin/ruby -S rspec spec/classes/init_spec.rb spec/defines/consul_check_spec.rb spec/defines/consul_service_spec.rb spec/defines/consul_watch_spec.rb --color
    .............................................................................
    
    Finished in 23.9 seconds
    77 examples, 0 failures
    
  • Fixed a few broken acceptance tests while I was in there too

  • Did some manual checking to make sure that service consul restart and service consul reload still work.

solarkennedy added a commit that referenced this pull request Oct 28, 2014
Fix the GOMAXPROCS warning for Upstart-based systems
@solarkennedy solarkennedy merged commit a7e8a5b into voxpupuli:master Oct 28, 2014
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.

2 participants