(PUP-2520) implements 'manual' environment directories

[masterzen]

Hi,

Please find here an implementation of the 'manual' type of directory environments, along with a puppet face to list the environements and flush them.

I tried to cover the whole feature with as much tests I can, but I didn't provide any acceptance tests.

It's been a long time since I contributed some code to Puppet, so please bear with me and do not hesitate to comment the following patches!

Thanks!
Brice

[hlindberg]

Don't think it is a good idea to use the file_watcher (quite a bit of overhead), and it is unclear why it is needed here - I see no instantiation of Puppet::Util::FileWatcher.

[masterzen]

it's a left-over from a previous implementation that was using the file watcher. The thing wasn't really working correctly so I backtracked to a regular check but forgot the require.
I'll remove it soon.

[puppetcla]

CLA signed by all contributors.

[hlindberg]

A general comment: the "manual" is not 100% manual since I think the ctime changes if there are changes to the content of the directory. This means that the cache is flushed for the env at other times than the manual touching of the directory itself. This in turn means that a user that expects to be able to do several operations before manually flushing may be surprised and that the resulting catalog may be based on partial, incomplete input.

[masterzen]

@hlindberg at least on my mac and linux boxes, both ctime and mtime change if I add a file to the root of the environment directory. Any other changes don't change ctime nor mtime. A change below the first directory level don't change ctime nor mtime.
I used ctime because that's what the Puppet File Watcher was using, so I thought it had a good reason to use that:
If you really want to have an atomic deployment based on watching the directory, then you need to update in a temp directory and move the directory in place. This operation only changes the ctime.

[masterzen]

@hlindberg do you prefer the fixes as a new commit or I rebase and push everything?

[hlindberg]

The distinction between ctime and mtime is kind of fuzzy, and I don't know the exact semantics for a directory, but I think ctime is fine (you get file +/- in the watched dir, plus changes to the directory's permissions etc). To make an atomic operation a user needs to switch the directories, a copy is not atomic where there is more than one file to copy, it would change the ctime multiple times, and worst case is that the environment does not contain everything that it is supposed to contain. (i.e. using symlinks, the changing of the symlink is atomic).

You can push new commits if you like.

[masterzen]

Hi, I've rebased the commits with the following changes:

• @hlindberg English comments in the Puppet face help
• rate limit the stat(2) calls to no more than 1 per seconds
• limit the face list command to list only directory environment

Let me know if you have more comments!
Thanks!
Brice

[jpartlow]

Macro typo: loader_get_environment_dir :)

[masterzen]

Good catch!
I will amend the patch ASAP

[masterzen]

I just repushed the PR to take into account @jpartlow comment.

[ferventcoder]

environment misspelled.

[masterzen]

@ferventcoder good catch!

[masterzen]

I rebased the commits on the current master, and fixed the misspelling @ferventcoder found.

[jpartlow]

We were discussing this in the pr triage today, and we are concerned about the time it will take to get this tested on our various platforms (specifically passing on Windows :). With all the work still outstanding for 3.7, we would like to leave this be until we are able to begin work on Puppet 4.

There was also discussion that the docs should be adjusted to warn that manually refreshing environments has the added problem that it won't reload ruby code (compared to a graceful restart of the server).

[masterzen]

@jpartlow ok, thanks for the information. I can't promise I'll be able to maintain this PR until you'll be ready to look at it :)

Regarding Windows, the current code is using the exact same semantics as the Puppet file watcher. If this last one works on Windows, then there's very great chances this code will work as-is :)

Regarding reloading ruby code, the issue is then the same when environment_timeout is set to a numerical value, is that right?

[zaphod42]

@masterzen yeah, the ruby reloading is the same with any of the evictions that try to reload the environment within a process. That is why I pushed so much so keep it to the timeouts and unlimited, with the preference for users to use unlimited. I was concerned that the "manual" mode would look too much like unlimited with a graceful restart, but not be able to achieve any of its guarantees.

[masterzen]

@zaphod42 sorry, but I fail to see how this is a different behavior than with the legacy dynamic environments?

People lived with that for many years and to my knowledge didn't complain that loudly (at least not to me). Directory environments weren't really created to solve the ruby reloading issue (or I missed the memo :), but to have a sane and direct configuration mechanism based on a proven convention (that mimics a lot what people were doing).

Anyway, if this PR proves to not be such a good idea, let's get rid of it before I spend too much time fixing the comments so that it get accepted :)

[jpartlow]

Hi Brice,

We discussed this more at the triage today, and in the end we don't want to take on the overhead of maintenance for this when we are agreed that the preferred method for a restart should be an unlimited timeout and a graceful restart of the server. All of the restart options have potential issues, but the manual timeout has the most problems. Apologies for getting you started on this; we should have come to a clear decision before opening the original ticket.