search

rodjek / librarian-puppet

http://librarian-puppet.com
MIT License
693 stars 209 forks source link

Rsync option #254

Closed danzilio closed 9 years ago

danzilio commented 9 years ago

We're using librarian-puppet in a large-scale environment with lots of change and we've run into issues with librarian-puppet's rather liberal convergence strategy. Essentially we're running into an issue where a librarian-puppet update command will result in a Puppet master process losing it's CWD because of the install_path.rmtree call. This results in a catalog compilation failure. The install_path.rmtree method gets called regardless of whether there are changes to the module in install_path. This PR allows for the option to use rsync to converge the directories, which is ultimately a much more conservative convergence strategy. This may not be the most idiomatic implementation. I'm open to suggestions on how to improve it.

danzilio commented 9 years ago

I'll need to add some tests and documentation as well.

carlossg commented 9 years ago

Would your problem be solved if instead of rmtree the destdir it deletes destdir/* ?

The code looks good, would definitely need some docs and tests, it could even be the default and remove all the custom cp logic, permissions,... It would be nice to have tests that reproduce your issue to ensure it doesn't happen again.

njam commented 9 years ago

Cool cool cool. We've implemented such functionality within our puppet master code. But obviously it makes much more sense in librarian-puppet!

I would even vote to make this default, because when you create a puppet master's module-directory with librarian-puppet sooner or later the problem's gonna arise that puppet reads files that are being deleted at the same time.

cc @tomaszdurka @ppp0 @kris-lab

danzilio commented 9 years ago

The only reason I would hesitate about making this the default behavior is that we would not be able to control the dependency on rsync.

@carlossg I haven't tested the destdir/* scenario but I imagine it wouldn't solve the issue since the Puppet master process is likely reading from destdir/manifests at the time rmtree is called.

@njam thanks for chiming in. I was wondering if I was the only one seeing this behavior!

I'll add some tests and docs today.

danzilio commented 9 years ago

I just pushed a cucumber scenario that captures this, but it feels kind of hacky. Could I get some input on the Updating a module with the rsync configuration scenario in features/updates.feature? If this approach is sound, I'll add some more scenarios.

carlossg commented 9 years ago

Looks good to me, doesn't the same problem happen when running librarian-puppet install after the first time?

danzilio commented 9 years ago

It does, I'll add a couple more tests and docs soon. I've been swamped with PuppetConf stuff and I haven't had a chance to touch this for the past few days.

danzilio commented 9 years ago

What do you guys think? Is this code ready to go?

danzilio commented 9 years ago

Actually, I've found one issue with this. I'm going to add another test case and fix it.

carlossg commented 9 years ago

Can you squash the commits into one for merging?

danzilio commented 9 years ago

Squashed :)

carlossg commented 9 years ago

Thanks!

carlossg commented 9 years ago

Seems there are some timing issues ? https://travis-ci.org/rodjek/librarian-puppet/jobs/36945009#L188

danzilio commented 9 years ago

Strange. I'll dig into it!

danzilio commented 9 years ago

Do you want to revert for now?

carlossg commented 9 years ago

No if it doesn't take too long to fix. I've tried with 7506cedb but seems it didn't fix it

danzilio commented 9 years ago

I think it may be the logic in my test. Hopefully I'll have a commit for you in a few hours.

danzilio commented 9 years ago

Sorry, I've had a heck of a time replicating this.

carlossg commented 9 years ago

seems it only happens in the 1.x branch