On Aug 30, 2013, at 4:35 PM, Nick Lewis <n...@puppetlabs.com> wrote:
> On Friday, August 30, 2013 at 3:58 PM, Luke Kanies wrote:
>> On Aug 30, 2013, at 10:47 AM, Andy Parker <a...@puppetlabs.com> wrote:
>>
>>> On Fri, Aug 30, 2013 at 10:24 AM, Luke Kanies <l...@puppetlabs.com> wrote:
>>> On Aug 30, 2013, at 9:11 AM, Andy Parker <a...@puppetlabs.com> wrote:
>>>
>>>> On Fri, Aug 30, 2013 at 1:05 AM, R.I.Pienaar <r...@devco.net> wrote:
>>>>>
>>>>>
>>>>> ----- Original Message -----
>>>>> > From: "Luke Kanies" <l...@puppetlabs.com>
>>>>> > To: puppet-dev@googlegroups.com
>>>>> > Sent: Thursday, August 29, 2013 11:27:00 PM
>>>>> > Subject: Re: Anchor pattern (was Re: [Puppet-dev] Puppet 4 discussions)
>>>>> >
>>>>> > On Aug 29, 2013, at 12:24 PM, John Bollinger <john.bollin...@stjude.org>
>>>>> > wrote:
>>>>> >
>>>>> > >
>>>>> > >
>>>>> > > On Wednesday, August 28, 2013 5:56:45 PM UTC-5, Andy Parker wrote:
>>>>> > > On Wed, Aug 28, 2013 at 3:22 PM, Luke Kanies <lu...@puppetlabs.com>
>>>>> > > wrote:
>>>>> > > On Aug 28, 2013, at 12:38 PM, Andy Parker <an...@puppetlabs.com>
>>>>> > > wrote:
>>>>> > >> On Wed, Aug 28, 2013 at 10:20 AM, Luke Kanies <lu...@puppetlabs.com>
>>>>> > >> wrote:
>>>>> > >> On Aug 28, 2013, at 8:45 AM, Andy Parker <an...@puppetlabs.com>
>>>>> > >> wrote:
>>>>> > >>
>>>>> > >> > * #8040 - anchor pattern. I think a solution is in sight, but it
>>>>> > >> > didn't make 3.3.0 and it is looking like it might be backwards
>>>>> > >> > incompatible.
>>>>> > >>
>>>>> > >> Why would it be incompatible?
>>>>> > >>
>>>>> > >> That implies that we can't ship it until 4.0, which would be a
>>>>> > >> tragedy
>>>>> > >> worth fighting hard to avoid.
>>>>> > >>
>>>>> > >>
>>>>> > >> The only possible problem, that I know of, would be that it would
>>>>> > >> change
>>>>> > >> the evaluation order. Once things get contained correctly that might
>>>>> > >> cause problems. We never give very strong guarantees between
>>>>> > >> versions of
>>>>> > >> puppet, but given the concern with manifest order, I thought that I
>>>>> > >> would
>>>>> > >> call this out as well.
>>>>> > >
>>>>> > > Do you mean, for 2 classes that should have a relationship but
>>>>> > > currently
>>>>> > > don't because of the bug (and the lack of someone using an anchor
>>>>> > > pattern
>>>>> > > to work around the bug), fixing that bug would cause them to have a
>>>>> > > relationship and thus change the order?
>>>>> > >
>>>>> > >
>>>>> > > No that shouldn't be a problem. I think we will be using making the
>>>>> > > resource syntax for classes ( class { foo: } ) create the containment
>>>>> > > relationship. That doesn't allow multiple declarations and so we
>>>>> > > shouldn't
>>>>> > > encounter the problem of the class being in two places.
>>>>> > >
>>>>> > >
>>>>> > > But it does allow multiple declarations, so long as only the first one
>>>>> > > parsed uses the parameterized syntax. There can be any number of
>>>>> > > other
>>>>> > > places where class foo is declared via the include() function or
>>>>> > > require()
>>>>> > > function.
>>>>> > >
>>>>> > >
>>>>> > > That is, you're concerned that the bug has been around so long it's
>>>>> > > considered a feature, and thus we can't change it except in a major
>>>>> > > release?
>>>>> > >
>>>>> > >
>>>>> > > More of just that the class will start being contained in another
>>>>> > > class and
>>>>> > > so it will change where it is evaluated in an agent run. That could
>>>>> > > cause
>>>>> > > something that worked before to stop working (it only worked before
>>>>> > > because of random luck). I'm also, right now, wondering if there are
>>>>> > > possible dependency cycles that might show up. I haven't thought that
>>>>> > > one
>>>>> > > through.
>>>>> > >
>>>>> > >
>>>>> > > Yes, it is possible that dependency cycles could be created where none
>>>>> > > existed before. About a week ago I added an example to the comments
>>>>> > > thread on this issue; it is part of a larger objection to the proposed
>>>>> > > solution: http://projects.puppetlabs.com/issues/8040#note-35. I also
>>>>> > > included a proposed alternative solution that could go into Puppet 3.
>>>>> >
>>>>> > As mentioned in my other email, the solution to this problem should not
>>>>> > in
>>>>> > any way require changes to containment semantics, and certainly
>>>>> > shouldn't
>>>>> > require class evaluation to indicate class containment. As I said, it
>>>>> > used
>>>>> > to do that for the first instance (but not for second, which led to some
>>>>> > inconsistencies and surprises, which is why I removed it). These days,
>>>>> > though, in general classes only contain resources, not other classes.
>>>>> > What
>>>>>
>>>>> I am not sure I follow and have missed some of this thread while on hols
>>>>> but
>>>>> here is why people use the anchor pattern:
>>>>>
>>>>> class one {
>>>>> include two
>>>>>
>>>>> notify{$name: }
>>>>> }
>>>>>
>>>>> class two {
>>>>> notify{$name: }
>>>>> }
>>>>>
>>>>> class three {
>>>>> notify{$name: require => Class["one"]}
>>>>> }
>>>>>
>>>>> include one, three
>>>>>
>>>>> $ puppet apply test.pp
>>>>> Notice: /Stage[main]/One/Notify[one]/message: defined 'message' as 'one'
>>>>> Notice: /Stage[main]/Three/Notify[three]/message: defined 'message' as
>>>>> 'three'
>>>>> Notice: /Stage[main]/Two/Notify[two]/message: defined 'message' as 'two'
>>>>> Notice: Finished catalog run in 0.11 seconds
>>>>>
>>>>> The desired outcome is that Notify[two] is before Notify[three]
>>>>>
>>>>
>>>> Thank you, RI. I was starting to wonder if Patrick and I were completely
>>>> misunderstanding the problem. We have the same understanding as you.
>>>>
>>>> Luke, the example you gave in which you claim that an empty class
>>>> disappears doesn't happen. I put together a test and reviewed the code,
>>>> just to make sure, and it works exactly as expected. I think your dislike
>>>> of whits is confusing you about what the problem actually is and how it
>>>> shows up.
>>>
>>> Can you share the test? I'd certainly like to get corrected if I'm
>>> misunderstanding it.
>>>
>>>
>>> it "preserves dependencies through empty containers" do
>>> relationships = compile_to_relationship_graph(<<-MANIFEST)
>>> class a { notify { "in a": } }
>>> class b { }
>>> class c { notify { "in c": } }
>>>
>>> include c, b, a
>>> Class[a] -> Class[b] -> Class[c]
>>> MANIFEST
>>>
>>> expect(order_resources_traversed_in(relationships)).to(
>>> include_in_order("Notify[in a]",
>>> "Notify[in c]"))
>>> end
>>>
>>>> My dislike of whits is based on the negative affects they've had; if they
>>>> have none, I'll happily dissolve that dislike.
>>>>
>>>
>>> My understanding is that early on they had the effect of showing up in
>>> output and they create another copy of the catalog. Those are problems and
>>> the first one has been addressed, the second one is still around. However,
>>> functionally they seem to do exactly what we want.
>>
>> They might functionally do what we want, but the last time I looked deeply
>> at them (a long-ass time ago), they were more of a hack than the right
>> solution.
>>
>> I think the right solution is to have a graph that supports both kinds of
>> edges, so they're unneeded; this fixes the whit problem, and my design flaw
>> from like 2006, and allows us to remove a bunch of code. At least, that's
>> what I concluded last time I looked at this, and I haven't seen anything
>> since then to conclude otherwise.
> I think in general, it would be nice to have a model of the graph that
> included various sorts of relationships (strict bidirectional ordering and
> containment among them). But I also don't think there's fundamentally
> anything *wrong* with using whits, since they accurately model the notion of
> containment, with respect to order: classes have a "start", and a "finish",
> and there's stuff in between. It doesn't really feel like a hack to me.
> Ironically, they *do* feel like a hack for solving the problem of
> many-to-many relationships, which was what they were intended to fix. But as
> far as representing containment goes, I think they function well.
The reason I think they're a hack is that they only solve the end-problem, and
there are other related problems we need to solve, too.
In particular, we make a bunch of claims about what users can get from the
catalog, but most of those claims aren't actually true for any catalogs in
puppet.
E.g., if you want to quickly get a list of all resources and their
dependencies, you… can't. You have to build a catalog, then splice it so it's
a relationship graph, and, then, um, wtf are these whit things and where did my
classes go?
Maybe the answer is to use whits in an entirely internal fashion, and never
expose them to the user, but at this point, the only place the user can see the
actual relationships in Puppet is in the relationship graph, which is so full
of whits it's not recognizable as the original catalog.
Maybe PuppetDB makes this all irrelevant because it actually builds a graph
that contains both, and makes that graph easily accessible, but… if it's good
for PuppetDB, why isn't it good for Puppet?
> It's easy to traverse the spliced graph with only a single type of edge, and
> more complex to traverse a graph with multiple kinds of edges. Hence why I
> would like to make sure to generalize it, rather than come up with an
> algorithm that works for require and contain and puts us in the same position
> as soon as we want a third kind of relationship.
>
> The biggest failing is really the loss of semantic information, since it's
> not as easy in the transformed graph to identify containment, which is a
> meaningful relationship.
I disagree that we need to generalize it. YAGNI.
We've used 2 kinds of edges for 8 years. What other edge types do we
realistically need right now?
>>
>> I.e., my distate for whits is rational, not emotional.
>>
>>>>
>>>>> So unless I am reading you wrong, the anchor pattern is used specifically
>>>>> because
>>>>> today many people have classes contained in other classes and it does not
>>>>> work
>>>>> as desired.
>>>>>
>>>>
>>>> Right, and it is a behavior that was explicitly removed (see
>>>> https://projects.puppetlabs.com/issues/2423). The catalog simply does not
>>>> track the information about a class being inside another class.
>>>
>>> Ok, so what you're saying is that the optimization using whits has nothing
>>> to do with the need for anchors, and it's entirely the fix for #2423?
>>>
>>>
>>> It has nothing at all to do with it. In fact the anchor pattern recreates
>>> exactly what the whits would produce if we tracked the containment edge for
>>> a class inside another class. So the anchor pattern is users having to
>>> produce the graph that puppet would do if we just put that edge in the
>>> catalog.
>>
>> Hmm. I'm essentially positive that there was a version of this bug that was
>> caused by whits cancelling out when classes were empty, but it sounds like
>> that form has been fixed. Either that, or the first time I did a deep dive
>> on this, I came away with a completely broken idea of the problem, which is
>> absolutely a possibility, especially given how long this has been sitting
>> around unsolved.
> This was #3382, which was fixed a few years ago. Actually it was fixed
> *before* the admissible and completed whits were introduced to represent
> containment.
>>
>>>>> Also I think fixing this only for the new resource like syntax and not for
>>>>> include would be a mistake - though i can see why that would be the chosen
>>>>> path as doing it otherwise would easily create loops etc.
>>>>>
>>>>
>>>> The reason for using the resource syntax to enforce containment is because
>>>> something can only be contained in one place. If containment is allowed in
>>>> multiple places you end up with inexplicable ordering and loops and are
>>>> back at #2423. Include has come to mean "I want this in the catalog with
>>>> no dependencies", and so adding containment to that would be a problem.
>>>> There is also the possibility of some new syntax to allow a "contained"
>>>> class (a function could be done, but then parameters would need to be
>>>> passed in a hash and it would be a bit odd).
>>>>
>>>> Here are the possibilities:
>>>>
>>>> * resource like syntax for classes expresses containment:
>>>>
>>>> class container { class { contained: parameter => value } }
>>>>
>>>> * a function declares the class *and* expresses containment
>>>>
>>>> class container { contain(contained, { parameter => value }) }
>>>>
>>>> * a function expresses containment, but does not declare the class
>>>>
>>>> class container { class { contained: parameter => value }
>>>> contain(contained) }
>>>>
>>>> * a new "resource type" expresses containment
>>>>
>>>> class container { contain { contained: parameter => value } }
>>>>
>>>> The two functions could actually be written right now and distributed in a
>>>> module, they just need to make sure that when the class is added to the
>>>> catalog that it also adds an edge from the container to the contained
>>>> class (Patrick spiked this and verified that it works).
>>>>
>>>> Now, the reason that I think that the existing resource syntax is the
>>>> right way is that it already allows the class to appear in one place,
>>>> which is what we need for reasonable containment. This also gives classes
>>>> containment of the exact same form as resources (because they currently
>>>> get contained in their class), which is a good symmetry to have. In
>>>> addition, there has been talk about allowing duplicate resources in the
>>>> catalog, which will open up the whole containment question for resources
>>>> (what does it mean for the resource to be in two places?). To solve that
>>>> we will probably need some way of "declaring without containment" for
>>>> resources, and having classes and resources agree on how to "declare
>>>> *with* containment" will make any solution there transfer better to both
>>>> classes and resources.
>>>
>>> I still don't understand why you wouldn't just use dependencies instead of
>>> containment here. It seems like either edge type works fine, so why focus
>>> on containment?
>>>
>>>
>>> Containment is just another form of dependencies, but with slightly
>>> different meaning. If class A contains something then it is more than a
>>> simple requires relationship because it means that anything that requires A
>>> (Class[a] -> Class[b]) needs to wait until everything that A contains has
>>> been executed (that is the transitive requires). That direction works with
>>> a simple requires.
>>>
>>> The other direction doesn't work with a simple requires. If class B,
>>> instead, needs to happen before class A (Class[b] -> Class[a]) the normal
>>> mental model is that nothing that A contains will happen until B is
>>> complete, then you would need to setup that relationship with everything
>>> contained inside A explicitly, which breaks encapsulation.
>>>
>>> The anchor pattern does this by setting up a resource in A, which will be
>>> contained using the above semantics, to explicitly setup relationships with
>>> classes that A wants to contain. These relationships are exactly what the
>>> relationship graph does when it is given a containment relationship.
>>>
>>> The semantics are:
>>>
>>> * Requires: A requires B means that all of A will be executed before B is
>>> executed
>>> * Contains: A contains B means that any resource C that requires A will
>>> only be executed after B is executed as well as any resource D that is
>>> required by A will be executed before B is executed
>>
>> Do you think John's recommendation of a new 'contain' function would solve
>> this? If not, why not?
>>
> A 'contain' function was the decision we came to last time I was involved in
> a conversation about this. That turned into ticket #13045, which was closed
> with the decision that it should be fixed in Puppet.
>
> I'm in favor of it as a simple, optional, backward-compatible fix. Basically,
> if you're using the anchor pattern now, you can use the contain function
> instead, and properly represents what's going on.
Wow, that's… embarrassing.
And I'd ship it in core, not stdlib.
>> We can't change the default semantics of 'include' or class declaration to
>> result in containment; the periodic cycles will just come right back, won't
>> they?
>>
>> If the new function doesn't work, what about a new top-level container that
>> defaults to containing, rather than the current model? Or what about both?
>> I've been wanting something like an 'application' label that was like a
>> class but with slightly different semantics for a while.
> Using more defines instead of classes gets you containment *and* more
> flexible modules! Just throwing that out there.
>>
>> --
>> Luke Kanies | http://about.me/lak | http://puppetlabs.com/ | +1-615-594-8199
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Puppet Developers" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to puppet-dev+unsubscr...@googlegroups.com.
>> To post to this group, send email to puppet-dev@googlegroups.com.
>> Visit this group at http://groups.google.com/group/puppet-dev.
>> For more options, visit https://groups.google.com/groups/opt_out.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to puppet-dev+unsubscr...@googlegroups.com.
> To post to this group, send email to puppet-dev@googlegroups.com.
> Visit this group at http://groups.google.com/group/puppet-dev.
> For more options, visit https://groups.google.com/groups/opt_out.
--
Luke Kanies | http://about.me/lak | http://puppetlabs.com/ | +1-615-594-8199
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to puppet-dev+unsubscr...@googlegroups.com.
To post to this group, send email to puppet-dev@googlegroups.com.
Visit this group at http://groups.google.com/group/puppet-dev.
For more options, visit https://groups.google.com/groups/opt_out.
