Should files autorequire all parents?

[Ian Ward Comfort]

I've just put an RFC feature request into the tracker, and I'm curious what other people think about it. In short, I'd like to have file resources autorequire all their ancestor directories, not just the direct parent. Input appreciated.

http://projects.puppetlabs.com/issues/6368

--
Ian Ward Comfort <icom...@stanford.edu>
Systems Team Lead, Academic Computing Services, Stanford University

[Luke Kanies]

On Feb 17, 2011, at 10:48 PM, Ian Ward Comfort wrote:

> I've just put an RFC feature request into the tracker, and I'm curious what other people think about it. In short, I'd like to have file resources autorequire all their ancestor directories, not just the direct parent. Input appreciated.
>
> http://projects.puppetlabs.com/issues/6368

My only real concern here is that edge growth could quickly become exponential. That is often not a problem, but it certainly can be.

--
I have lost friends, some by death... others through sheer inability
to cross the street. -- Virginia Woolf
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199

[Daniel Pittman]

I think I am in general agreement with this - or, at least, something that can't be disstinguished from this externally.  However, to be sure I understand your proposal correctly:

The goal is that dependencies of an ancestor directory are implicitly added to a directory node, and nothing more?

You do not expect this to implicitly create directories on disk, or anything that would be externally observable, just to influence dependency execution order?

Regards,
    Daniel
--
Puppet Labs Developer –http://puppetlabs.com
Daniel Pittman <dan...@puppetlabs.com>
Contact me via gtalk, email, or phone: +1 (877) 575-9775
Sent from a mobile device. Please forgive me if this is briefer than usual.

> --
> You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
> To post to this group, send email to puppe...@googlegroups.com.
> To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
>

[Daniel Pittman]

On Feb 18, 2011 10:30 AM, "Luke Kanies" <lu...@puppetlabs.com> wrote:
> On Feb 17, 2011, at 10:48 PM, Ian Ward Comfort wrote:
>
> > I've just put an RFC feature request into the tracker, and I'm curious what other people think about it. In short, I'd like to have file resources autorequire all their ancestor directories, not just the direct parent. Input appreciated.
> >
> >       http://projects.puppetlabs.com/issues/6368
>
> My only real concern here is that edge growth could quickly become exponential.  That is often not a problem, but it certainly can be.

For what it is worth, I understand the problem statement to support triming most of those edges without changing behaviour, so we can optimize those dependencies away silently and have the same behaviour.

A suggestion for exactly that was already filed on the ticket, in fact. :)

[Ian Ward Comfort]

On 18 Feb 2011, at 10:32 AM, Daniel Pittman wrote:
> I think I am in general agreement with this - or, at least, something that can't be disstinguished from this externally. However, to be sure I understand your proposal correctly:
>
> The goal is that dependencies of an ancestor directory are implicitly added to a directory node, and nothing more?
>
> You do not expect this to implicitly create directories on disk, or anything that would be externally observable, just to influence dependency execution order?

Correct on all counts.

[Ian Ward Comfort]

On 18 Feb 2011, at 10:34 AM, Daniel Pittman wrote:
> On Feb 18, 2011 10:30 AM, "Luke Kanies" <lu...@puppetlabs.com> wrote:
>> On Feb 17, 2011, at 10:48 PM, Ian Ward Comfort wrote:
>>>
>>> I've just put an RFC feature request into the tracker, and I'm curious what other people think about it. In short, I'd like to have file resources autorequire all their ancestor directories, not just the direct parent. Input appreciated.
>>>
>>> http://projects.puppetlabs.com/issues/6368
>>>
>>
>>
>> My only real concern here is that edge growth could quickly become exponential. That is often not a problem, but it certainly can be.
>
> For what it is worth, I understand the problem statement to support triming most of those edges without changing behaviour, so we can optimize those dependencies away silently and have the same behaviour.
>
> A suggestion for exactly that was already filed on the ticket, in fact. :)

Yeah, requiring just the first managed parent, as suggested, would be sufficient. I wasn't sure how to implement this, though, from a quick glance through the code. The logic that iterates over possible autorequires and adds relationships for anything found in the catalog is up in Puppet.Type.

Any ideas for how to do that trimming?

[Daniel Pittman]

Nope. :)

I was interested in characterizing the problem fully, so we understand
what the request was, but I have no idea what the solution is
presently. That part is just code, though, so how hard can it be? ;)

Regards,
Daniel
--
⎋ Puppet Labs Developer – http://puppetlabs.com
✉ Daniel Pittman <dan...@puppetlabs.com>
✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775

♲ Made with 100 percent post-consumer electrons

[Ian Ward Comfort]

On 18 Feb 2011, at 11:19 AM, Daniel Pittman wrote:
> On Fri, Feb 18, 2011 at 10:48, Ian Ward Comfort <icom...@stanford.edu> wrote:
>> Yeah, requiring just the first managed parent, as suggested, would be sufficient. I wasn't sure how to implement this, though, from a quick glance through the code. The logic that iterates over possible autorequires and adds relationships for anything found in the catalog is up in Puppet.Type.
>>
>> Any ideas for how to do that trimming?
>
> Nope. :)
>
> I was interested in characterizing the problem fully, so we understand what the request was, but I have no idea what the solution is presently. That part is just code, though, so how hard can it be? ;)

Heh, right. I was trying to think of a clean way to give types a way to say, "autorequire the first of these dependencies that's found in the catalog, and ignore the rest". Adding an autorequirefirst method would do it in a backwards-compatible way, but that feels icky.

[Markus Roberts]

Heh, right. I was trying to think of a clean way to give types a way to say, "autorequire the first of these dependencies that's found in the catalog, and ignore the rest". Adding an autorequirefirst method would do it in a backwards-compatible way, but that feels icky.

Can you explain what feels icky about it?  It seems like a legitimate (and not particularly open-ended) extension of the api.  If there were risk that we'd be opening the door to later adding "autorequireallbutfirst" and "autorequirethird" and such could see objecting strongly (heck, I'd be objecting strongly myself) but in this case it seems like a semantically sound addition that isn't likely to get out of hand and could plausibly be useful elsewhere.

-- M
-----------------------------------------------------------
When in trouble or in doubt, run in circles,
scream and shout. -- 1920's parody of the 
maritime general prudential rule
------------------------------------------------------------

[Ian Ward Comfort]

On 18 Feb 2011, at 12:04 PM, Markus Roberts wrote:
>> Heh, right. I was trying to think of a clean way to give types a way to say, "autorequire the first of these dependencies that's found in the catalog, and ignore the rest". Adding an autorequirefirst method would do it in a backwards-compatible way, but that feels icky.
>
> Can you explain what feels icky about it? It seems like a legitimate (and not particularly open-ended) extension of the api. If there were risk that we'd be opening the door to later adding "autorequireallbutfirst" and "autorequirethird" and such could see objecting strongly (heck, I'd be objecting strongly myself) but in this case it seems like a semantically sound addition that isn't likely to get out of hand and could plausibly be useful elsewhere.

Maybe it's not really so icky. I wanted something that treated autorequires in more unified fashion, and was trying to think of a way to push information down into the type and/or yield items selectively from the autorequire blocks. But even if I came up with something it would almost certainly break the existing API.

Shall I code up a weather-balloon patch for autorequirefirst?

[Markus Roberts]

Fine by me.

[Luke Kanies]

On Feb 18, 2011, at 11:37 AM, Ian Ward Comfort wrote:

> On 18 Feb 2011, at 11:19 AM, Daniel Pittman wrote:
>> On Fri, Feb 18, 2011 at 10:48, Ian Ward Comfort <icom...@stanford.edu> wrote:
>>> Yeah, requiring just the first managed parent, as suggested, would be sufficient. I wasn't sure how to implement this, though, from a quick glance through the code. The logic that iterates over possible autorequires and adds relationships for anything found in the catalog is up in Puppet.Type.
>>>
>>> Any ideas for how to do that trimming?
>>
>> Nope. :)
>>
>> I was interested in characterizing the problem fully, so we understand what the request was, but I have no idea what the solution is presently. That part is just code, though, so how hard can it be? ;)
>
> Heh, right. I was trying to think of a clean way to give types a way to say, "autorequire the first of these dependencies that's found in the catalog, and ignore the rest". Adding an autorequirefirst method would do it in a backwards-compatible way, but that feels icky.

It's actually pretty straightforward to do the trimming, I think - we currently just return the parent class, but we should instead search through possible parent dirs and see which ones are in the catalog. Something like:

autorequire(:file) do
dirs = File.dirname(self[:path]).split(File::SEPARATOR)[1..-1].inject([""]) { |list, d| list << File.join(list[-1], d); list }[1..-1].reverse
parent = dirs.find { |dir| catalog.resource(:file, dir) }
parent # either nil or a filename; works the same either way
end

--
Someday I want to be rich. Some people get so rich they lose all
respect for humanity. That's how rich I want to be. --Rita Rudner

[Ian Ward Comfort]

On 18 Feb 2011, at 12:29 PM, Luke Kanies wrote:
> On Feb 18, 2011, at 11:37 AM, Ian Ward Comfort wrote:
>> Heh, right. I was trying to think of a clean way to give types a way to say, "autorequire the first of these dependencies that's found in the catalog, and ignore the rest". Adding an autorequirefirst method would do it in a backwards-compatible way, but that feels icky.
>
> It's actually pretty straightforward to do the trimming, I think - we currently just return the parent class, but we should instead search through possible parent dirs and see which ones are in the catalog. Something like:
>
> autorequire(:file) do
> dirs = File.dirname(self[:path]).split(File::SEPARATOR)[1..-1].inject([""]) { |list, d| list << File.join(list[-1], d); list }[1..-1].reverse
> parent = dirs.find { |dir| catalog.resource(:file, dir) }
> parent # either nil or a filename; works the same either way
> end

Ah, hmm. Is that legit? Type.autorequire takes rel_catalog, which appears to be the relative catalog against which autorequires are to be generated, while this always uses the resource's catalog. But maybe rel_catalog is only supposed to be for picking the final resources... or, actually, it never seems to be used anyway.

OK, I'll give this approach a shot.

[Luke Kanies]

Are we looking at the same code? I'm comparing to the existing autorequire in File:

autorequire(:file) do
basedir = File.dirname(self[:path])
if basedir != self[:path]
basedir
else
nil
end
end

--
Risk! Risk anything! Care no more for the opinions of others, for those
voices. Do the hardest thing on earth for you. Act for yourself. Face
the truth. -- Katherine Mansfield

[Ian Ward Comfort]

On 18 Feb 2011, at 2:08 PM, Luke Kanies wrote:
> On Feb 18, 2011, at 1:20 PM, Ian Ward Comfort wrote:
>> On 18 Feb 2011, at 12:29 PM, Luke Kanies wrote:
>>> It's actually pretty straightforward to do the trimming, I think - we currently just return the parent class, but we should instead search through possible parent dirs and see which ones are in the catalog. Something like:
>>>
>>> autorequire(:file) do
>>> dirs = File.dirname(self[:path]).split(File::SEPARATOR)[1..-1].inject([""]) { |list, d| list << File.join(list[-1], d); list }[1..-1].reverse
>>> parent = dirs.find { |dir| catalog.resource(:file, dir) }
>>> parent # either nil or a filename; works the same either way
>>> end
>>
>> Ah, hmm. Is that legit? Type.autorequire takes rel_catalog, which appears to be the relative catalog against which autorequires are to be generated, while this always uses the resource's catalog. But maybe rel_catalog is only supposed to be for picking the final resources... or, actually, it never seems to be used anyway.
>>
>> OK, I'll give this approach a shot.
>
> Are we looking at the same code? I'm comparing to the existing autorequire in File:
>
> autorequire(:file) do
> basedir = File.dirname(self[:path])
> if basedir != self[:path]
> basedir
> else
> nil
> end
> end

I was looking at lib/puppet/type.rb:

# Figure out of there are any objects we can automatically add as
# dependencies.
def autorequire(rel_catalog = nil)
rel_catalog ||= catalog
raise(Puppet::DevError, "You cannot add relationships without a catalog") unless rel_catalog
...

But I don't think anything ever supplies a rel_catalog. (Should it be removed?) Anyway, your approach does work. I rewrote a little, hoping that this would be more robust across POSIX and non-POSIX:

autorequire(:file) do
basedir = File.dirname(self[:path])
if basedir != self[:path]

parents = []
until basedir == parents.last
parents.push basedir
basedir = File.dirname(basedir)
end
# The filename of the first parent found, or nil
parents.find { |dir| catalog.resource(:file, dir) }
else
nil
end
end

Now I'm just struggling to get rspec up and running on my RHEL box so I can add some tests. (Is there any dev documentation I'm missing on this topic?)

[Luke Kanies]

autorequire is a bit of a weird beast, so I know why you're having problems.

My code was just about changing the behavior of existing autorequire, rather than adding a new kind of autorequire.

The code I sent was just replacing the *call* to that method you're looking at - weird, but true.

Glad to hear it seems to work. And I notice that your code is far simpler and clearer than mine. :)

--
Humphrey's Law of the Efficacy of Prayer:
In a dangerous world there will always be more people around whose
prayers for their own safety have been answered than those whose
prayers have not.

[Ian Ward Comfort]

The File type will now autorequire the nearest ancestor directory found in the
catalog, not just the file's parent directory. This is useful for setting up
transitive relationships in cases when a package or other resource creates a
large directory hierarchy, e.g.

package { 'foo': ensure => present }
file { '/var/lib/foo': require => Package['foo'] }

This will make File resources at arbitrarily deep levels under /var/lib/foo
automatically (transitively) require the foo package.

Only the nearest ancestor is autorequired, to prevent explosion of the
relationship graph.

Signed-off-by: Ian Ward Comfort <icom...@stanford.edu>
---

On 18 Feb 2011, at 2:20 PM, Luke Kanies wrote:
> autorequire is a bit of a weird beast, so I know why you're having problems.
>
> My code was just about changing the behavior of existing autorequire, rather
> than adding a new kind of autorequire.
>
> The code I sent was just replacing the *call* to that method you're looking
> at - weird, but true.
>
> Glad to hear it seems to work. And I notice that your code is far simpler
> and clearer than mine. :)

OK, I was definitely quite confused before, but having spent some time
*actually* learning Ruby and swimming about this part of the codebase, I think
I've got it. Given the late stage at which Type#autorequire gets called, this
strategy works just fine.

This patch is based on master, but I can rebase if that's wrong. It can also
be pulled from:

git://github.com/icomfort/puppet.git file-autorequire-first-ancestor

lib/puppet/type/file.rb | 10 ++++++++--
spec/unit/type/file_spec.rb | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 963b9e5..da09392 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -244,11 +244,17 @@ Puppet::Type.newtype(:file) do
newvalues(:first, :all)
end

- # Autorequire any parent directories.
+ # Autorequire the nearest ancestor directory found in the catalog.

autorequire(:file) do
basedir = File.dirname(self[:path])
if basedir != self[:path]

- basedir
+ parents = []
+ until basedir == parents.last
+ parents << basedir
+ basedir = File.dirname(basedir)
+ end
+ # The filename of the first ancestor found, or nil
+ parents.find { |dir| catalog.resource(:file, dir) }
else
nil
end
diff --git a/spec/unit/type/file_spec.rb b/spec/unit/type/file_spec.rb
index 539782f..cbd6812 100755
--- a/spec/unit/type/file_spec.rb
+++ b/spec/unit/type/file_spec.rb
@@ -169,6 +169,19 @@ describe Puppet::Type.type(:file) do
reqs[0].target.must == file
end

+ it "should autorequire its nearest ancestor directory" do
+ file = Puppet::Type::File.new(:path => "/foo/bar/baz")
+ dir = Puppet::Type::File.new(:path => "/foo")
+ root = Puppet::Type::File.new(:path => "/")
+ @catalog.add_resource file
+ @catalog.add_resource dir
+ @catalog.add_resource root
+ reqs = file.autorequire
+ reqs.length.must == 1
+ reqs[0].source.must == dir
+ reqs[0].target.must == file
+ end
+
it "should not autorequire its parent dir if its parent dir is itself" do
file = Puppet::Type::File.new(:path => "/")
@catalog.add_resource file
@@ -242,6 +255,19 @@ describe Puppet::Type.type(:file) do
reqs[0].target.must == file
end

+ it "should autorequire its nearest ancestor directory" do
+ file = Puppet::Type::File.new(:path => "X:/foo/bar/baz")
+ dir = Puppet::Type::File.new(:path => "X:/foo")
+ root = Puppet::Type::File.new(:path => "X:/")
+ @catalog.add_resource file
+ @catalog.add_resource dir
+ @catalog.add_resource root
+ reqs = file.autorequire
+ reqs.length.must == 1
+ reqs[0].source.must == dir
+ reqs[0].target.must == file
+ end
+
it "should not autorequire its parent dir if its parent dir is itself" do
file = Puppet::Type::File.new(:path => "X:/")
@catalog.add_resource file
@@ -303,6 +329,19 @@ describe Puppet::Type.type(:file) do
reqs[0].target.must == file
end

+ it "should autorequire its nearest ancestor directory" do
+ file = Puppet::Type::File.new(:path => "//server/foo/bar/baz/qux")
+ dir = Puppet::Type::File.new(:path => "//server/foo/bar")
+ root = Puppet::Type::File.new(:path => "//server/foo")
+ @catalog.add_resource file
+ @catalog.add_resource dir
+ @catalog.add_resource root
+ reqs = file.autorequire
+ reqs.length.must == 1
+ reqs[0].source.must == dir
+ reqs[0].target.must == file
+ end
+
it "should not autorequire its parent dir if its parent dir is itself" do
file = Puppet::Type::File.new(:path => "//server/foo")
@catalog.add_resource file
--
1.7.3.2

[Paul Berry]

FYI, this patch is associated with ticket 6368 (http://projects.puppetlabs.com/issues/6368)

[Jacob Helwig]

Thanks for doing this, Ian! Josh and I have merged the following
additional tests into the patch.

diff --git i/spec/unit/type/file_spec.rb w/spec/unit/type/file_spec.rb
index cbd6812..223873f 100755
--- i/spec/unit/type/file_spec.rb
+++ w/spec/unit/type/file_spec.rb
@@ -182,6 +182,12 @@ describe Puppet::Type.type(:file) do

reqs[0].target.must == file
end

+ it "should not autorequire anything when there is no nearest ancestor directory" do

+ file = Puppet::Type::File.new(:path => "/foo/bar/baz")

+ @catalog.add_resource file
+ file.autorequire.should be_empty

+ end
+
it "should not autorequire its parent dir if its parent dir is itself" do
file = Puppet::Type::File.new(:path => "/")
@catalog.add_resource file

@@ -268,6 +274,12 @@ describe Puppet::Type.type(:file) do

reqs[0].target.must == file
end

+ it "should not autorequire anything when there is no nearest ancestor directory" do

+ file = Puppet::Type::File.new(:path => "X:/foo/bar/baz")

+ @catalog.add_resource file
+ file.autorequire.should be_empty

+ end
+
it "should not autorequire its parent dir if its parent dir is itself" do
file = Puppet::Type::File.new(:path => "X:/")
@catalog.add_resource file

@@ -342,6 +354,12 @@ describe Puppet::Type.type(:file) do

reqs[0].target.must == file
end

+ it "should not autorequire anything when there is no nearest ancestor directory" do

+ file = Puppet::Type::File.new(:path => "//server/foo/bar/baz/qux")

+ @catalog.add_resource file
+ file.autorequire.should be_empty

+ end
+
it "should not autorequire its parent dir if its parent dir is itself" do
file = Puppet::Type::File.new(:path => "//server/foo")
@catalog.add_resource file

--
Jacob Helwig

[Ian Ward Comfort]

On 22 Apr 2011, at 11:26 AM, Jacob Helwig wrote:
> Thanks for doing this, Ian! Josh and I have merged the following additional tests into the patch.

That looks good; thanks!