file resource needs improvements #13

purpleidea · 2016-02-15T18:19:32Z

The file resource can (currently) only set file contents for a single file. It can't make directories or do anything fancy yet. That's okay, because the remaining parts are left as an introductory patch for a new golang hacker. Here's the design, people working on this code are welcome to suggest alternatives:

All the work should probably happen in Apply() and StateOK():
https://github.com/purpleidea/mgmt/blob/master/file.go#L339
NOTE: i'm considering changing the API to merge the two functions-- comments welcome. It might be preferable to delay this patch if you're not willing to rebase your work. Such is a pre 1.x release.

The resource offers up a list of file properties, and does "the right thing":
$name - if ends in / then it's a directory, otherwise it's a file
$content - for files, this is a string with the contents of the file
$content - for directories, this is the path to an existing directory, the structure/contents of which we'll want to copy in
$content - for directories, if this is an empty string, it means an empty directory
$recursive - if resource would require changing more than a single thing, this must be true, otherwise error.
we'll have to reconsider if this is necessary or not. only applies to directories.
$force - if resource would require a switch from file to directory or vice versa, then force must be true. otherwise it's an error. it can probably be thought of as the -f flag on rm.

Lastly (and can be a separate patch) the Watch() function will need support for watching recursively into the directories. Adding fanotify support ( https://github.com/go-fsnotify/fsnotify/issues/114 ) would also be welcome in addition.

Comments welcome by anyone who is going to hack on this!

dmitri-d · 2016-02-16T12:59:22Z

Picking this up...

purpleidea · 2016-02-16T15:05:29Z

@witlessbird Awesome! Please let me know if you have issues or questions, and your feelings about rebasing the two functions into one. I'm leaning fairly strongly towards doing that, although it shouldn't affect the file resource too much.

purpleidea · 2016-02-20T23:58:23Z

I have an unmerged API change. Instead of StateOK and Apply, there is now:

(obj FooType) CheckApply(apply bool) (stateok bool, err error)

The bool argument in: whether to apply or not (true means do the apply)
The bool return value: whether we were in the correct state or not.
If we aren't, then we obviously need to do the apply, assuming the bool is true.
If anything goes wrong anywhere, set error, although it should be much more common to error in the apply phase than in the state check phase.

Lastly, you should never return stateok == true, with a non nil error. This doesn't make sense, if the state is okay, there should be no error figuring that out. If you hit an error trying to apply, then the state shouldn't have been true! Think about it :)

NOTE: I haven't merged it yet, although it looks like it's a more logical fit. Any problems, let me know!

purpleidea · 2016-02-21T07:56:29Z

Yep, the new CheckApply API seems to work quite nicely so far. The reason why it's more elegant than two separate Check and Apply functions, is because for some resources, the whole "lead in" and "initialization" of the resource is shared between both functions, and in fact the output from the state checks can be used in the apply section. I realized this when wrapping package kit for the pkg resource. So they're all in one big happy function now.

dmitri-d · 2016-02-21T18:11:02Z

Makes sense/sounds good. Bikeshed painting bit -- I wonder if "CheckAndApply" is a clearer name (not sure how golang-compliant it is :) ).

purpleidea · 2016-02-21T18:15:00Z

On Sun, Feb 21, 2016 at 1:11 PM, Dmitri Dolguikh [email protected]
wrote:

Makes sense/sounds good. Bikeshed painting bit -- I wonder if
"CheckAndApply" is a clearer name (not sure how golang-compliant it is :) ).

I accept votes from people who write patches ;) I personally prefer the
shorter version, but anything is changeable. Cheers!

purpleidea · 2016-02-22T03:23:16Z

@witlessbird et al. I've committed the API change in: 58f41ed

I'm pretty happy with the change. It's probably safe to rebase now :) I don't have any expected breakages planned at the moment!

Cheers!

ffrank · 2016-03-20T20:37:22Z

@witlessbird how's progress? I've been playing with the code and would poach this if that's alright.

dmitri-d · 2016-03-20T20:44:22Z

The thing that is remaining is watching of directories for changes. Wanna pick that up?

ffrank · 2016-03-20T21:12:26Z

That looks good! Yep, I'll see about the watches.

purpleidea · 2016-03-20T21:40:11Z

It's mostly just the recursive watching part which is missing. Here's one existing method that could be poached to do it: https://github.com/xyproto/recwatch

LMK if anyone needs more info.

ffrank · 2016-03-20T22:02:38Z

Humm, if I read that BSD license correctly, then no - we can't. Unless we keep it in a dedicated code file that is BSD licensed. Sounds annoying.

I will look for inspiration, though - thanks for the pointer.

purpleidea · 2016-03-20T22:04:06Z

@ffrank what? not at all, it's definitely compatible.

dmitri-d · 2016-03-20T22:11:34Z

Yeah, I think it's an attribution license -- as long as we mention the author it should be fine.

ffrank · 2016-03-20T23:21:27Z

Right - come to think of it, this does look like a subset of GPL.

ffrank · 2016-03-28T23:24:37Z

When I realized that we can just import the recwatch module, I rejoiced. However, there's trouble. It relies on github.com/fsnotify/fsnotify rather than github.com/go-fsnotify/fsnotify. So it seems overly cumbersome to use it directly. (I just built using the former fsnotify, which does not seem to Just Work.)

So at this point we can either fork the recwatch repo or use some of the code in mgmt directly. Thoughts?

Btw @witlessbird do you feel like open a PR for your code at this point already? Looks like it could use another rebase, so that would make things easier.

purpleidea · 2016-03-28T23:26:22Z

@ffrank My recommendation is to take/learn what you can from the recwatch code and copy+paste it in, probably with modification. I mentioned it as a POC if someone wanted to avoid having to figure out the recursive stuff themselves.

purpleidea · 2016-03-29T01:46:01Z

@ffrank @witlessbird I just pushed a pretty big branch to git master. You'll want to run a rebase. Good news is, it probably won't affect much of your patches on this if any, because it was in unrelated parts of the code. Cheers!

ffrank · 2016-04-01T00:19:58Z

Hey @witlessbird, I pushed an initial PoC for watch code to https://github.com/ffrank/mgmt/tree/directory_support

I took the liberty to rebase to 0.0.3. Want to give that a spin? The examples work great, but then there seems to be trouble re-converging state when the managed directory is partially in sync (e.g., after removing a subdirectory or contained file).

Also, there are some semantics issues with the watching code that will require some more head scratching on my part.

ffrank · 2016-04-04T20:37:02Z

So I've looked into the syncing issues. It turns out that github.com/cf-guardian/guardian/kernel/fileutils trips up badly when the destination directory is partially in sync. Just ran this a number of times, interspersed with removing a directory from target:

package main
import (
        "github.com/cf-guardian/guardian/kernel/fileutils"
)

func main() {
        source := "/tmp/from"
        dest := "/tmp/to"
        err := fileutils.New().Copy(dest, source)
        if err != nil {
                panic(err)
        }
}

I feel that this routine is not quite adequate. Have you looked at https://github.com/Redundancy/go-sync ?

dmitri-d · 2016-04-04T20:44:40Z

I haven't, this was one of the first things I found. I'll take a look.

purpleidea · 2016-04-04T20:53:46Z

@ffrank I don't think we need to depend on either the guardian or the go-sync modules. We should implement this using the golang stdlib alone.

ffrank · 2016-04-04T21:27:43Z

I'm torn. Sure, it's nice to be self-contained, and I've been thinking about building this function. But then, it's tiresome to maintain this functionality. We're missing out on all the effort and bug fixing that has gone into existing modules. I also feel a bit silly reinventing wheels.

In the end, it's your call. I will say that we'll have a PoC sooner by (at least temporarily) sticking to a module.

dmitri-d · 2016-04-04T21:29:29Z

I needed a way to check for changes inside a dir, so I have a good chunk of dir copying functionality already...

purpleidea · 2016-04-04T21:29:54Z

@ffrank If you can't convince me that there's some complicated piece that's needed and provided by a module, but I don't think this is so bad. The sort of thing we need is built in:

https://golang.org/pkg/path/filepath/#Walk

Looking at how a diff algorithm works will probably provide a better solution too.

dmitri-d · 2016-04-05T21:49:54Z

Pushed changes to File.CheckApply that handle directories (both empty and with contents) and support for creating of Directories, both empty and with contents. The latter still relies on guardian/guardian/kernel file copying though. Also, rebased. There are two additional examples graph0a.yml and graph0b.yml that demonstrate creation of directories.

ffrank · 2016-04-05T22:32:47Z

Cool. I rebased to your's. Looks much better. No ugly error messages anymore. Still won't reliably sync up a partially existing destination tree though.

Mind if I take a crack at that?

dmitri-d · 2016-04-05T22:41:30Z

@ffrank: nah, go for it. You can do a PR against my branch if you'd like... I'll add tests for the new functionality in the meantime.

This is the 1st commit message: lang: Split FuncValue into FuncValue and SimpleFn Representing an MCL function value as a golang function from Value to Value was a mistake, it should be a function from Vertex to Vertex. Here is why this is a mistake: The output of a function like $f = fn(x) { Shell(Sprintf("seq %d", x)) } varies over time, while a single Value does not. Thus, code like Map($f, list(1, 2)) would first produce the value list("1", "1"), but then it would _not_ update to list("1", "2") when "seq 2" produces its second line. That's because with the mistaken design, when Map receives a new FuncValue or a new input list of N elements, Map calls the function from Value to Value N times and produces a single output list of N elements. Here is why the corrected design is better: Here's what happens with this new design when Map receives a new FuncValue or a new input list of N elements. First, Map constructs N item-input nodes, each of which extracts a different entry from the list. Then, Map calls the function from Vertex to Vertex N times, once for each item-input node, and thus obtain N item-output nodes. Finally, Map constructs an item-collecting node which constructs a list out of all of its inputs, and Map connects the N item-output nodes to the item-collecting node. This item-collecting node is the output of Map. The Vertex to Vertex function constructs and connects its own nodes; in this case, it constructs an Sprintf node and connects the item-input node to it, and then constructs a Shell node and connects the Sprintf node to it, and then returns the Shell node as the item-output node. The two Shell node in this sub-graph emit a first value "1", which propagates to the item-collecting node and causes it to output a first value list("1", "1"). Then, the second Shell node emits a second value "2", which propagates to the item-collecting node and causes it to output a second value list("1", "2"), as desired. Here is how this commit brings us closer to the above plan: Changing FuncValue throughout the codebase is a big change. One of the difficulties is that it is not just nodes which are emitting FuncValues, there are also many other places in the code where FuncValue is used to hold a golang function from Value to Value. Some of those places now need to hold a golang function from Vertex to Vertex, but other places still need to hold a golang function from Vertex to Vertex. Thus, as a first step, we need to split FuncValue into two types. This commit splits the old FuncValue into two types: 1. The new FuncValue will hold a function from Vertex to Vertex. FuncValue is a Value. 2. A new type named "SimpleFn" will hold a function from Value to Value. SimpleFn is not a Value. This commit replaces occurrences of the old FuncValue with one of those two new types, as appropriate. This commit does not yet adapt the surrounding code to make use of the new representation; that will be done in future commits. I have annotated the missing parts with the following panic message in order to make it easy to find which parts still need to be implemented. The "..." part explains what needs to be implemented. panic("TODO [SimpleFn]: ..."); Here's where I need help: One part of the code which is not clear to me are the parts which use reflection. I don't understand the purpose of that code well enough to explain what needs to be implemented. I have annotated those "known unknown" parts of the remaining work with the following panic message in order to make it easy to find which parts still need more thinking and planning: panic("TODO [SimpleFn] [Reflect]: ..."); This is the commit message #2: lang: Add the core func graph Txn API This will eventually let functions change the running graph via a transaction API. At the moment the core Lock and Unlock primitives aren't implemented. This is the commit message #3: lang: A reversible wrapper around Txn This is useful for the common case in which we call one FuncValue to construct a bunch of nodes, and later we switch to a different FuncValue and so we want to remove all the nodes added by the first FuncValue and replace them by the nodes added by the second FuncValue. This is the commit message #4: lang: move FuncValue to its own package This is the commit message #5: lang: combine lang/func/structs and ast Merging those two packages allows us to avoid import cycles when a Func needs to add an Expr to the graph. This is the commit message #6: lang: CallExpr must generate a subgraph FuncValues are now manipulating the graph instead of manipulating values, so the logic for calling a FuncValue must now follow suit. This is the commit message #7: lang: ExprFunc does not need to store its value This is the commit message #8: spelling This is the commit message #9: convert between SimpleFn, FuncValue, and Func This is the commit message #10: no need for ExprCall.argVertices it's the exact same thing as ExprCall.Args This is the commit message #11: ExprFunc.Func()'s three cases This is the commit message #12: FunctionFunc is no longer used This is the commit message #13: ExprBool.MergedGraph() This is the commit message #14: ExprIf.MergedGraph() This is the commit message #15: ExprFunc.MergedGraph() This is the commit message #16: lang: ast, interfaces: Add MergedGraph signature and implementation This adds a MergedGraph signature to the Expr interface. The txn type is interface{} until we have that merged. This is the commit message #17: lang: ast, interfaces: Add MergedGraph signature and implementation This adds a MergedGraph signature to the Stmt interface. The txn type is interface{} until we have that merged. This is the commit message #18: Use MergedGraph signature and implementation This puts it into play, but doesn't initialize the input args at all. This is the commit message #19: Restore ExprBool.Graph() and Func() This reverts commit 3ea3845. This is the commit message #20: Restore ExprIf.Graph() This reverts commit a62889e. This is the commit message #21: restore ExprFunc::Graph() and Func() This is the commit message #22: fix type errors This is the commit message #23: ExprCall.MergedGraph() This is the commit message #24: a more precise type for args This is the commit message #25: GraphTxn This is the commit message #26: ReversibleTxn.AddGraph This is the commit message #27: ExprFunc.MergedGraph() This is the commit message #28: ExprVar.MergedGraph() This is the commit message #29: sub-graph is spelled subgraph in this codebase This is the commit message #30: ExprFunc.mkFunc() was unused This is the commit message #31: CallFunc.Stream() should add Funcs to the graph, not Exprs This is the commit message #32: move ConstFunc to lang.funcs This is the commit message #33: move conversion functions to lang.funcs.simple I wrote some conversion functions from SimpleFn to FuncValue to Func, and I want to call them from Func.Stream() implementations, so those conversion functions should be somewhere in lang.funcs. This is the commit message #34: it is the responsibility of the function engine to call Init on the nodes This is the commit message #35: FuncValue.Call() This is the commit message #36: drop MergedGraph's unused txn parameter This is the commit message #37: move CallFunc to funcs.simple This is the commit message #38: Func from channel This is the commit message #39: ChannelBased{Source,Sink}Func This is the commit message #40: MapFunc This is the commit message #41: remove unused CallFunc field This is the commit message #42: [REVERT ME] remove unrelated files to make VS Code happy This is the commit message #43: [REVERT ME] remove unrelated files to make VS Code happy This is the commit message #44: extend environment with StmtProg's local variables This is the commit message #45: delete unused VarFunc This is the commit message #46: appease govet This is the commit message #47: add imported variables to the environment This is the commit message #48: add builtins to the environment This is the commit message #49: accumulate all the imported variables previously, we were only keeping the imported variables from the last import statement. This is the commit message #50: XXX: wip new function engine This is the commit message #51: comments This is the commit message #52: remove Engine.{Lock,Unlock} placeholders This is the commit message #53: f(...) support for ExprCall This is the commit message #54: [REVERT ME] remove unrelated files to make VS Code happy This is the commit message #55: don't recreate the subgraphInput This is the commit message #56: CallFunc now takes a single input, the FuncValue Also, by using the map_func logic, CallFunc now detects when it can no longer emits new values downstream. This is the commit message #57: GraphTxn must take a pointer in order to modify the graph This is the commit message #58: GraphTxn is unused

purpleidea added the mgmtlove label Feb 15, 2016

ffrank mentioned this issue Mar 31, 2016

File watches can become excessive? #20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

file resource needs improvements #13

file resource needs improvements #13

purpleidea commented Feb 15, 2016

dmitri-d commented Feb 16, 2016

purpleidea commented Feb 16, 2016

purpleidea commented Feb 20, 2016

purpleidea commented Feb 21, 2016

dmitri-d commented Feb 21, 2016

purpleidea commented Feb 21, 2016

purpleidea commented Feb 22, 2016

ffrank commented Mar 20, 2016

dmitri-d commented Mar 20, 2016

ffrank commented Mar 20, 2016

purpleidea commented Mar 20, 2016

ffrank commented Mar 20, 2016

purpleidea commented Mar 20, 2016

dmitri-d commented Mar 20, 2016

ffrank commented Mar 20, 2016

ffrank commented Mar 28, 2016

purpleidea commented Mar 28, 2016

purpleidea commented Mar 29, 2016

ffrank commented Apr 1, 2016

ffrank commented Apr 4, 2016

dmitri-d commented Apr 4, 2016

purpleidea commented Apr 4, 2016

ffrank commented Apr 4, 2016

dmitri-d commented Apr 4, 2016

purpleidea commented Apr 4, 2016

dmitri-d commented Apr 5, 2016

ffrank commented Apr 5, 2016

dmitri-d commented Apr 5, 2016