On Aug 30, 2013, at 10:47 AM, Andy Parker <[email protected]> wrote:
> On Fri, Aug 30, 2013 at 10:24 AM, Luke Kanies <[email protected]> wrote: > On Aug 30, 2013, at 9:11 AM, Andy Parker <[email protected]> wrote: > >> On Fri, Aug 30, 2013 at 1:05 AM, R.I.Pienaar <[email protected]> wrote: >> >> >> ----- Original Message ----- >> > From: "Luke Kanies" <[email protected]> >> > To: [email protected] >> > 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 <[email protected]> >> > 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 <[email protected]> >> > > wrote: >> > > On Aug 28, 2013, at 12:38 PM, Andy Parker <[email protected]> wrote: >> > >> On Wed, Aug 28, 2013 at 10:20 AM, Luke Kanies <[email protected]> >> > >> wrote: >> > >> On Aug 28, 2013, at 8:45 AM, Andy Parker <[email protected]> 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.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. >> 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? 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. -- 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 [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/puppet-dev. For more options, visit https://groups.google.com/groups/opt_out.
