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

Set dbpath_fix to false by default #347

Merged
merged 1 commit into from
Oct 20, 2017

Conversation

mwhahaha
Copy link
Contributor

Under Xenial, the current default value of dbpath_fix set to true leads
to idempotency issues because the data files may be created with the
nogroup group. This can lead to puppet repeatedly fixing permissions
which is not ideal. The files that are getting changed are set to 0600
so the group ownership does not help. By default, we should be setting
this to false because we should not need to manage the user/group
permissions with the default install provided by distrobutions.

@jurim76
Copy link

jurim76 commented Feb 8, 2017

Debian Jessie
puppetlabs-mongodb-0.17.0

mongodb/journal/lsn file with mongodb:nogroup permissions always calls dbpath_fix -> mongodb service restart.
This is very bad situation, especially for mongodb replica.

@rhoml
Copy link
Member

rhoml commented Feb 20, 2017

I can confirm this is a thing also in Ubuntu trusty.

@jurim76
Copy link

jurim76 commented Feb 21, 2017

FYI
Setting "dbpath_fix" to "false" does not help, setting "dbpath_fix" to "undef" fix this issue

@mwhahaha
Copy link
Contributor Author

boolean false should fix it based on https://github.com/puppetlabs/puppetlabs-mongodb/blob/2cef851409852134f00153eb9bb171179f4359d7/manifests/server/config.pp#L222 But if you pass "false" as a string it won't work

@jurim76
Copy link

jurim76 commented Feb 21, 2017 via email

@rhoml
Copy link
Member

rhoml commented Feb 21, 2017

I am afraid it is tru be default https://github.com/puppetlabs/puppetlabs-mongodb/blob/2cef851409852134f00153eb9bb171179f4359d7/manifests/params.pp#L15 making it false fixed it for me

@dandunckelman
Copy link

@dandunckelman
Copy link

I suggest adding a test to spec/classes/server_config_spec.rb to ensure the Exec['fix dbpath permissions'] resource is present when the value is true (https://github.com/puppetlabs/puppetlabs-mongodb/blob/0.17.0/manifests/server/config.pp#L222) and it is absent when the value is false.

@ekohl
Copy link
Member

ekohl commented Oct 2, 2017

I think this makes sense. Would you mind rebasing this to fix the merge conflict? A test would also be nice.

@mwhahaha
Copy link
Contributor Author

mwhahaha commented Oct 3, 2017

rebased and added a test

@ekohl ekohl removed the needs-rebase label Oct 3, 2017
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Would you mind expanding the 'with defaults' testcase to add is_expected.not_to contain_exec? Other than that I think it's good to go

:command => 'chown -R foo:bar /var/lib/mongo',
:path => ['/usr/bin', '/bin'],
:onlyif => "find /var/lib/mongo -not -user foo -o -not -group bar -print -quit | grep -q '.*'",
:subscribe => "File[/var/lib/mongo]")
Copy link
Member

Choose a reason for hiding this comment

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

It's better to test this with that_subscribes_to('File[/var/lib/mongo]') since that goes through the catalog compilation.

@mwhahaha mwhahaha force-pushed the idempotency-issues branch 2 times, most recently from 84e8c89 to 80538cf Compare October 3, 2017 16:47
@mwhahaha
Copy link
Contributor Author

mwhahaha commented Oct 5, 2017

Comments addressed, please take a minute to review. Thanks

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

LGTM

@wyardley
Copy link
Contributor

@mwhahaha sorry, we were waiting for some final changes during the transition to Vox. Would you mind rebasing one more time, and resolving the conflict?

Under Xenial, the current default value of dbpath_fix set to true leads
to idempotency issues because the data files may be created with the
nogroup group.  This can lead to puppet repeatedly fixing permissions
which is not ideal. The files that are getting changed are set to 0600
so the group ownership does not help.  By default, we should be setting
this to false because we should not need to manage the user/group
permissions with the default install provided by distrobutions.
@mwhahaha
Copy link
Contributor Author

@wyardley rebased

@wyardley wyardley merged commit 9e2cf80 into voxpupuli:master Oct 20, 2017
@wyardley
Copy link
Contributor

Sorry, I should have rebased again. this now has some rubocop errors, but I put in a separate fix for that in #407

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.

6 participants