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

use the config's ulimits if set and is > max_clients. #234

Merged
merged 1 commit into from
Oct 19, 2016

Conversation

popsikle
Copy link
Contributor

I spent a bit trying to figure out why setting config['ulimit'] was capping out at 10k (max_clients). This fixes that, which lets someone control ulimits on the redis user while protecting against too small of a ulimit.

@popsikle popsikle mentioned this pull request Dec 22, 2015
@martinb3 martinb3 added this to the 2.4.1 Bugfixing milestone Dec 23, 2015
@majuscule
Copy link

+1 for this merge. The ulimit also needs to be set in the unit file from recipes/configure.rb for systemd machines. I'd be happy to make the changes once this is merged... Until then, for anyone who finds this, the solution is a drop-in (described in systemd.unit(5)):

node['redisio']['servers'].each do |service|
    if service.has_key?('ulimit')
        directory "/usr/lib/systemd/system/redis@#{service['port']}.service.d/"
        template "/usr/lib/systemd/system/redis@#{service['port']}.service.d/limits.conf" do
            source 'limits.conf.erb'
            variables({ :ulimit => service['ulimit'] })
            notifies :run, 'execute[systemctl-daemon-reload]', :immediately
        end
    end
end

execute 'systemctl-daemon-reload' do
  command '/bin/systemctl daemon-reload'
  action :nothing
end
[Service]
LimitNOFILES=<%= @ulimit %>

@popsikle
Copy link
Contributor Author

sigh

@brianbianco
Copy link
Contributor

Frankly I don't find adding Sigh to a pull request as constructive in the least. While it did draw my attention to the fact that this needs to be merged, I found it to be a bit inconsiderate.

@majuscule Would you mind doing a PR for your additions after I merge this?

@brianbianco brianbianco merged commit d722065 into sous-chefs:master Oct 19, 2016
@majuscule
Copy link

Hey @brianbianco. I'm no longer using chef day to day. I'd be happy to drop these additions into the install script, but I don't have a proper environment to test the changes. I took a quick look and noticed there is already

templates/default/[email protected]
10:LimitNOFILE=<%= @limit_nofile %>

from commit e6468d0, so it looks like someone already implemented the fix without merging this pull request :-(.

@shortdudey123 shortdudey123 modified the milestones: 2.6.0, 2.4.1 Bugfixing Apr 25, 2017
shortdudey123 added a commit that referenced this pull request May 10, 2017
- Update 'bind' config comments ([#293](#293))
- Add disable_os_default recipe ([#224](#224))
- Use the config's ulimits if set and is > max_clients ([#234](#234))
- Add Travis config ([#299](#299))
- Fix test failures (FoodCritic and Rubocop) ([#298](#298))
- Fix TravisCI builds ([#300](#300))
- Add repl-backlog-size, repl-backlog-ttl, and aof-load-truncated options ([#278](#278))
- Add sentinel_bind to bind sentinel to different IPs ([#306](#306))
- Cleanup deprecation warnings ([#301](#301))
- Fix version detection with epoch version numbers from deb/ubuntu ([#294](#294))
- Restrict VM redis config to <= 2.4 ([#322](#322))
- Rename_commands should be checked for nil before empty ([#311](#311))
- Fixup foodcritic, rubocop, and kitchen testing ([#324](#324))
  - Note: this drops support for Chef < 11
- Add min-slaves redis options ([#313](#313))
- Allow /etc/init start after sigterm from system or user ([#310](#310))
- Check user existence with Etc, not ohai node attributes ([#303](#303))
- Various systemd-related improvements ([#302](#302))
- Update serverspec testing with correct OS's for systemd ([#329](#329))
- Add kitchen-dokken testing to Travis ([#330](#330))
- Add fedora-25 to kitchen testing and clean up kitchen config ([#331](#331))
- Fix systemd paths for sentinel service ([#332](#332))
- Add redis-package and sentinel to Travis kitchen verify ([#334](#334))
- Add breadcrumb-file creation condition as attribute ([#268](#268))
- Fix cluster options in README ([#333](#333))
- Fix systemd loader to use descriptors instead of max_clients+32 ([#338](#338))
- Add SELinux support ([#305](#305))
- Make source of redis.conf template configurable ([#341](#341))
- Support sentinel notification-script and client-reconfig-script ([#342](#342))
@lock
Copy link

lock bot commented Nov 28, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants