Finishing touches for multi-instance support #343

fraenki · 2020-01-21T12:45:11Z

Pull Request (PR) description

This is essentially a stripped-down re-submission of #230. The original author of #230 seems to have abandoned his efforts (the repository is gone).

This PR contains the following enhancements:

Add an example for multiple instances to the README
Fix a dependency cycle by using camptocamp/systemd instead of Exec to reload systemd
Updates to the systemd template
Update tests accordingly

However, this re-submission does not include the redis-shutdown.erb template that was present in the original PR, because some users noted that this is not required. I can confirm this from my own testing.

Tested on CentOS 7.7 with Puppet 6.11 and Redis 5.

FWIW, this is the dependency cycle that may occur without the proposed modifications when using multiple instances:

Error: Found 1 dependency cycle:
(Exec[systemd-reload-redis] => Class[Redis] => Redis::Instance[6379] => File[/etc/systemd/system/redis-server-6379.service] => Exec[systemd-reload-redis])

This Pull Request (PR) fixes the following issues

n/a

ghoneycutt · 2020-01-22T19:25:54Z

@fraenki Great work! Could you please add some acceptance tests that show this is working with multiple instances? That would make this easy to merge and greatly help ensure that the functionality remains in tact.

fraenki · 2020-01-23T10:57:25Z

@ghoneycutt Sure, I'll add some acceptance tests :)

fraenki · 2020-01-28T21:36:52Z

@ghoneycutt So I took a look at the acceptance tests and the existing tests (this) already have good coverage for multi instance support and they ran successfully for this PR. Is there anything else I should add?

README.md

.fixtures.yml

manifests/instance.pp

README.md

templates/service_templates/redis.service.erb

fraenki · 2020-02-09T22:04:00Z

@ghoneycutt I've addressed all issues:

Update acceptance tests to use new example code from README
Re-enable multi-instance acceptance tests on Debian (because they are working fine)
Add spec test for systemd service file
Add condition to reload systemd only on Puppet < 6.1.0
Rebase against current master

vox-pupuli-tasks · 2020-02-17T17:31:14Z

Dear @fraenki, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

vox-pupuli-tasks · 2020-02-17T17:31:14Z

Dear @fraenki, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

templates/service_templates/redis.service.erb

manifests/instance.pp

spec/acceptance/suites/default/redis_multi_instances_one_host_spec.rb

spec/classes/redis_spec.rb

templates/service_templates/redis.service.erb

ekohl

On a related note, I'm sure you've noticed that more recent Debian (Buster) and Ubuntu (Bionic) versions have started to ship the redis(-server)@ service. I'd like to use those since they also contain various security hardening features. Not a blocker to this, but have you had a look at this and what are your thoughts?

spec/acceptance/suites/default/redis_multi_instances_one_host_spec.rb

spec/classes/redis_spec.rb

manifests/ulimit.pp

fraenki · 2020-02-18T11:20:40Z

On a related note, I'm sure you've noticed that more recent Debian (Buster) and Ubuntu (Bionic) versions have started to ship the redis(-server)@ service. I'd like to use those since they also contain various security hardening features. Not a blocker to this, but have you had a look at this and what are your thoughts?

Not at all, actually I'm not doing anything with Debian or Ubuntu.
So I'd prefer if this could be integrated with another PR. :)

ekohl · 2020-02-18T11:34:58Z

Not at all, actually I'm not doing anything with Debian or Ubuntu.
So I'd prefer if this could be integrated with another PR. :)

Absolutely. However, I do wonder if we could deploy a redis(-server)@ unit file on platforms that are missing it and utilize that. Since you're not a Debian/Ubuntu user, how would you feel about that?

ekohl

Small last nitpicks, but this is shaping up to be a good PR.

manifests/ulimit.pp

metadata.json

ekohl · 2020-02-18T13:27:02Z

README.md

+    service_ensure => 'running',
+    port           => $port,
+    bind           => $facts['networking']['ip'],
+    dbfilename     => "${port}-dump.rdb",


This makes me wonder if we should have some code in instance.pp that does this and appendfilename so they're unique. Defaulting to the same file for multiple instances sounds like a bad idea. We can pass the default in config.pp as we do for other *file parameters.

While I agree, I think having $port as the one and only unique identifier for Redis instances is sufficient. Adding $bind to the mix by introducing it as unique identifier could make things pretty complicated. I think this would only add choice (which is usually a good thing), but without real benefits.

.fixtures.yml

fraenki · 2020-02-18T13:51:54Z

Absolutely. However, I do wonder if we could deploy a redis(-server)@ unit file on platforms that are missing it and utilize that. Since you're not a Debian/Ubuntu user, how would you feel about that?

I took a look at this. It sure is an improvement. I took note and will implement it in a follow-up PR, because this one is already too big and I don't want to see all checks break again. :) (The follow-up PR would also be a good opportunity to convert the service template to EPP, as suggested by @bastelfreak.)

Now that all checks are working... is this ready for merge? :)

spec/classes/redis_spec.rb

ekohl

LGTM. Trivial nit, but feel free to leave that. I'll also give other reviewers that were involved time for a last look (@bastelfreak, @ghoneycutt).

ekohl · 2020-02-18T15:04:52Z

spec/classes/redis_spec.rb

+            # Only necessary for Puppet < 6.1.0,
+            # See https://github.com/puppetlabs/puppet/commit/f8d5c60ddb130c6429ff12736bfdb4ae669a9fd4
+            if Puppet.version < '6.1'
+              is_expected.to contain_augeas('Systemd redis ulimit').
+                with_incl("/etc/systemd/system/#{service_name}.service.d/limit.conf").
+                with_lens('Systemd.lns').
+                with_changes(['defnode nofile Service/LimitNOFILE ""', 'set $nofile/value "7777"']).
+                that_notifies('Class[systemd::systemctl::daemon_reload]')
+            else
+              is_expected.to contain_augeas('Systemd redis ulimit').
+                with_incl("/etc/systemd/system/#{service_name}.service.d/limit.conf").
+                with_lens('Systemd.lns').
+                with_changes(['defnode nofile Service/LimitNOFILE ""', 'set $nofile/value "7777"'])
+            end


Alternative with less duplication

is_expected.to contain_augeas('Systemd redis ulimit'). with_incl("/etc/systemd/system/#{service_name}.service.d/limit.conf"). with_lens('Systemd.lns'). with_changes(['defnode nofile Service/LimitNOFILE ""', 'set $nofile/value "7777"']) # Only necessary for Puppet < 6.1.0, # See https://github.com/puppetlabs/puppet/commit/f8d5c60ddb130c6429ff12736bfdb4ae669a9fd4 if Puppet.version < '6.1' is_expected.to contain_augeas('Systemd redis ulimit'). that_notifies('Class[systemd::systemctl::daemon_reload]')

You can probably even add an else with not_to

Finishing touches for multi-instance support

ghoneycutt requested changes Jan 30, 2020

View reviewed changes

README.md Show resolved Hide resolved

.fixtures.yml Outdated Show resolved Hide resolved

manifests/instance.pp Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

templates/service_templates/redis.service.erb Outdated Show resolved Hide resolved

bastelfreak added enhancement New feature or request needs-work not ready to merge just yet labels Jan 30, 2020

bastelfreak removed the needs-work not ready to merge just yet label Feb 9, 2020

bastelfreak approved these changes Feb 9, 2020

View reviewed changes

vox-pupuli-tasks bot added merge-conflicts labels Feb 17, 2020

This comment has been minimized.

Sign in to view

vox-pupuli-tasks bot removed the merge-conflicts label Feb 17, 2020

bastelfreak reviewed Feb 17, 2020

View reviewed changes

templates/service_templates/redis.service.erb Show resolved Hide resolved

bastelfreak approved these changes Feb 17, 2020

View reviewed changes

ekohl reviewed Feb 17, 2020

View reviewed changes

ekohl reviewed Feb 18, 2020

View reviewed changes

spec/acceptance/suites/default/redis_multi_instances_one_host_spec.rb Show resolved Hide resolved

spec/classes/redis_spec.rb Show resolved Hide resolved

manifests/ulimit.pp Outdated Show resolved Hide resolved

bastelfreak approved these changes Feb 18, 2020

View reviewed changes

ekohl requested changes Feb 18, 2020

View reviewed changes

manifests/ulimit.pp Outdated Show resolved Hide resolved

metadata.json Outdated Show resolved Hide resolved

ekohl reviewed Feb 18, 2020

View reviewed changes

.fixtures.yml Show resolved Hide resolved

ekohl reviewed Feb 18, 2020

View reviewed changes

spec/classes/redis_spec.rb Outdated Show resolved Hide resolved

add finishing touches to multi-instance support

d779b75

fraenki requested review from ekohl and ghoneycutt February 18, 2020 14:58

ekohl approved these changes Feb 18, 2020

View reviewed changes

ghoneycutt approved these changes Feb 18, 2020

View reviewed changes

ghoneycutt merged commit ef7c4e6 into voxpupuli:master Feb 18, 2020

ekohl linked an issue May 11, 2020 that may be closed by this pull request

Type forking causes service timeout #260

Closed

cegeka-jenkins pushed a commit to cegeka/puppet-redis that referenced this pull request Feb 16, 2021

Merge pull request voxpupuli#343 from fraenki/multi_instance

520e217

Finishing touches for multi-instance support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finishing touches for multi-instance support #343

Finishing touches for multi-instance support #343

fraenki commented Jan 21, 2020 •

edited

Loading

ghoneycutt commented Jan 22, 2020

fraenki commented Jan 23, 2020

fraenki commented Jan 28, 2020

fraenki commented Feb 9, 2020

This comment has been minimized.

vox-pupuli-tasks bot commented Feb 17, 2020

This comment has been minimized.

vox-pupuli-tasks bot commented Feb 17, 2020

ekohl left a comment

fraenki commented Feb 18, 2020

ekohl commented Feb 18, 2020

ekohl left a comment

ekohl Feb 18, 2020

fraenki Feb 18, 2020

fraenki commented Feb 18, 2020

ekohl left a comment

ekohl Feb 18, 2020

Finishing touches for multi-instance support #343

Finishing touches for multi-instance support #343

Conversation

fraenki commented Jan 21, 2020 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

ghoneycutt commented Jan 22, 2020

fraenki commented Jan 23, 2020

fraenki commented Jan 28, 2020

fraenki commented Feb 9, 2020

This comment has been minimized.

vox-pupuli-tasks bot commented Feb 17, 2020

This comment has been minimized.

vox-pupuli-tasks bot commented Feb 17, 2020

ekohl left a comment

Choose a reason for hiding this comment

fraenki commented Feb 18, 2020

ekohl commented Feb 18, 2020

ekohl left a comment

Choose a reason for hiding this comment

ekohl Feb 18, 2020

Choose a reason for hiding this comment

fraenki Feb 18, 2020

Choose a reason for hiding this comment

fraenki commented Feb 18, 2020

ekohl left a comment

Choose a reason for hiding this comment

ekohl Feb 18, 2020

Choose a reason for hiding this comment

fraenki commented Jan 21, 2020 •

edited

Loading