file resource needs improvements

[purpleidea]

These are the remaining non-obvious issues with the File resource after closing #13 and after 598c746

1) Should this API still be used?

$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

My original notes in #13 mentioned this, but I forgot about it when I wrote the code. If it's still logical, we should change it, and remove the use of Source which I used for dirs.

2) Are nested file resources an issue?

As pointed out by @ffrank What if we have two File resources (including dirs) where one is nested inside the other? In particular when the nested member is either a file or another directory. Similarly, the same question could apply if a nested directory itself contains another nested file or directory...

Well, after some consideration I think the answer should be as follows:

We limit the CheckApply() and also the Watch() functions to only work on a subset of their recursive contents if indeed they have recursive == true, and also if there is another file resource found to be contained within them. To get this information, the BaseRes will have to grow a method to either return all the file resources, OR more generally to return all other resources. I think I prefer the later. It can then loop through this list and find which paths are subsets.

Is this still the right approach? It needs to be investigated. The autoedges API now exists, so it might be worth considering that it could be helpful.

3) The recurse variable isn't respected for some deletions. This is a simple enhancement that should be added.

I used os.RemoveAll to keep things simple. We'll want to do this recursively with os.Remove, so that we can add a recurseMax (or similarly named) parameter.

/cc @ffrank @witlessbird

[purpleidea]

Another point which I apparently forgot: What do we do for the situation where we want to have a file, but from a single source. Do we use some sort of file() function (similar to the template function) but which returns the file contents, or do we make the file resource path aware for single files (which is what we do for directories).

[ffrank]

Don't add special meaning to things like content: /tmp/frlzsdne412.tmp. This kind of thing is just bound to blow up in your face when eventually and inevitably someone will need to ensure that a file contains a path. We used to have this type of fun with Puppet.

[purpleidea]

@ffrank Good point, and nice reference. I think I agree with you, but I should probably mull it over in my brain for a little while. Does anyone have any suggestions on an improved interface here?

[purpleidea]

Recent comments from: #411

TBH file resource (being literally the first thing I wrote in mgmt) needs some cleanups! Eg:

mgmt/engine/resources/file.go

Line 181 in b2d7260

// FIXME: Also watch the source directory when using obj.Source !!!

I'm waiting on the upcoming filesystem changes in my big module patch to add new functions that can read content on disk and return strings, eg:

# normal
file "/tmp/foo" {
        content => "hello\n",
}

# coming soon!
file "/tmp/bar" {
        content => file("/etc/passwd"), # copy that file from disk here
}

# also coming soon!
file "/tmp/bar" {
        content => files("static/passwd"), # copy that file from our deploy filesystem
}

# or perhaps fancier:
file "/tmp/bar" {
        content => template("templates/passwd.tmpl"), # template something
}

# or directories?? (uncertain)
file "/tmp/dir/" {
        content => "/var/foo/", # or do we use source =>
        # what about copying a dir *from* the deploy filesystem???
}

[ffrank]

Trying to address as many points as I can right now:

1) Should this API still be used?

$content - for files, this is a string with the contents of the file

Yes and I believe it's working.

$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

content is not allowed for directories, which feels correct to me. Should always be source.

My original notes in #13 mentioned this, but I forgot about it when I wrote the code. If it's still logical, we should change it, and remove the use of Source which I used for dirs.

I don't see a need for this. When source as equal semantics for files and directories, why change it?

2) Are nested file resources an issue?

...

Is this still the right approach? It needs to be investigated. The autoedges API now exists, so it might be worth considering that it could be helpful.

I've mulled this over.

file "/path/to/something/" { ensure => "absent", recurse => true, }
file "/path/to/something/deep/within" { ensure => "exists", }

These resources are in conflict and at runtime (i.e., CheckApply time) the file should enter a failed state. It will have an AutoEdge on the directory, so this will work consistently.

My previous concern was coming from a Puppet point of view, where a purge approach will remove only unmanaged files from a tree. This is not what's happening here, and in fact a Puppet transaction will break in the same way (I think).

3) The recurse variable isn't respected for some deletions. This is a simple enhancement that should be added.

I used os.RemoveAll to keep things simple. We'll want to do this recursively with os.Remove, so that we can add a recurseMax (or similarly named) parameter.

Oof I dont recall...was this about crossing file system boundaries? The fact that we cannot unlink active mount points?

I sort of feel that if the resulting behavior is "deletes everything except the mount points and their parent directories", that is quite fine. (I can confirm this before we close.) The vertex should enter a failed state after the partial Apply.

Not sure how I feel about recurseMax, but it might be nice to have a safer option that stops at filesystem boundaries. Where available (Linux) we could even shell out to rm --one-fileystem for this. Icky but trivial to implement, and saves us from implementing safety code ourselves, traversing directory trees etc.

Either way, that should be a separate ticket.

Recent comments from: #411

TBH file resource (being literally the first thing I wrote in mgmt) needs some cleanups! Eg:

mgmt/engine/resources/file.go

Line 181 in b2d7260

// FIXME: Also watch the source directory when using obj.Source !!!

I'm waiting on the upcoming filesystem changes in my big module patch to add new functions that can read content on disk and return strings, eg:

normal

file "/tmp/foo" {
content => "hello\n",
}

This works.

coming soon!

file "/tmp/bar" {
content => file("/etc/passwd"), # copy that file from disk here
}

You have the code in the drawer already?

also coming soon!

file "/tmp/bar" {
content => files("static/passwd"), # copy that file from our deploy filesystem
}

"files" though? Or should it be file and the magic is in the missing leading slash? That seems reasonable then.

or perhaps fancier:

file "/tmp/bar" {
content => template("templates/passwd.tmpl"), # template something
}

or directories?? (uncertain)

file "/tmp/dir/" {
content => "/var/foo/", # or do we use source =>
# what about copying a dir from the deploy filesystem???
}

If file("non-absolute/path" has special meaning, then we could expand that to the source parameter. source => "var/foo/" feels much more consistent than suddenly allowing content for directories after all.

[ffrank]

So essentially, I feel that we could do the following:

1. confirm that things are failing the way we expect
2. open new ticket about recursive file delete semantics and/or safety
3. open a new ticket about copying data from the virtual filesystem

Then close.