The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Bug #8040

Classes should be able to contain other classes to provide a self contained module

Added by Anonymous almost 5 years ago. Updated over 2 years ago.

Status:ClosedStart date:06/22/2011
Priority:ImmediateDue date:
Assignee:Patrick Carlisle% Done:

0%

Category:compiler
Target version:3.4.0
Affected Puppet version:2.6.0 Branch:https://github.com/puppetlabs/puppet/pull/1878
Keywords:anchor containment contain graph modules module self-contained dependency reuse usability forge backlog customer

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com


Description

Overview

As a module author, I want to build collections of classes for end users shipped as a module.

As a module end-user, I want to manage resources before and that require the collection of classes as a self contained unit of functionality.

For example, the end user wants to use a module I write in the following way:

node default {
  class { 'java': }
  ->
  class { 'activemq': }
  ->
  class { 'mcollective: }
}

Where java, activemq, and mcollective are all discrete modules with multiple classes each. For example, a each module has a class for the packages, a class for the configuration files, and a class for the running service if there is a service.

With Puppet 2.6, when a class declares another class, the classes are not related to each other in any way, containment or dependency.

Expected Behavior

The example illustrates the expectation that all resources in the activemq module are managed after all resources in the java module and before all resources in the mcollective module.

Actual Behavior

Without the Anchor Pattern, when class activemq::service is declared from within class activemq, the resources float away and are not transitively related to java or mcollective.

Suggested Implementation

It has been expressed that it may be a viable solution for module authors to be able to specify containment edges in the graph from within the Puppet DSL. With Puppet 2.6.x and 2.7.x this is not possible. The Anchor Pattern works around this problem by specifying relationship edges to a resource contained within the composite class.

Work Around

The Anchor Pattern is the current work around for Puppet 2.6.x When a class declares other classes, it should contain them using this pattern:

class activemq {

  anchor { 'activemq::begin': }
  anchor { 'activemq::end': }

  class { 'activemq::package':
    require => Anchor['activemq::begin'],
  }
  class { 'activemq::config':
    require => Class['activemq::config'],
    notify => Class['activemq::service'],
  }
  class { 'activemq::service':
    before => Anchor['activemq::end'],
  }

}

— Jeff McCune Puppet Labs @0xEFF

Anchor_no_relationships.png - The Problem - Disconnected Graphs (229 KB) Anonymous, 06/22/2011 11:35 am

Anchor_both_anchors.png - The Work Around - Anchor Pattern (329 KB) Anonymous, 06/22/2011 11:35 am


Related issues

Related to Standard Library - Bug #8844: document anchor type of the stdlib module Closed 08/08/2011
Related to Puppet - Bug #7709: expanded_relationship.dot should not have both containmen... Accepted 05/27/2011
Related to Standard Library - Feature #13045: stdlib module should have a contains function Closed 03/09/2012
Related to Puppet - Bug #3382: Class require chain thru class with no resources Closed 03/17/2010
Related to Puppet - Bug #11339: Class ordering bug? Closed 12/12/2011
Related to Puppet - Refactor #3691: The Catalog should include both dependency and containmen... Duplicate 04/27/2010
Related to Puppet - Bug #2423: Intermittent dependency cycle Closed 07/18/2009
Duplicated by Puppet - Bug #14578: Puppet does not respect implied class dependencies Duplicate 05/18/2012

History

#1 Updated by Anonymous almost 5 years ago

#2 Updated by Anonymous almost 5 years ago

Graph View

To visualize the problem. Please consider the graph produced when the work around is not used.

class activemq {
  class { 'activemq::pacakge': }
  ->
  class { 'activemq::config': }
  ->
  class { 'activemq::service': }
}

When the user declares this class in their node definitions, they want activemq to be managed after Java is managed and before MCollective is managed:

node default {
  class { 'java': }
  ->
  class { 'activemq': }
  ->
  class { 'mcollective': }
}

Here is the resulting graph with this code:

The Problem

The Work Around

To work around the problem, the module author contains all of the classes in the MCollective Module using anchor resources:

class activemq {
  anchor { 'activemq::begin': }
  ->
  class { 'activemq::pacakge': }
  ->
  class { 'activemq::config': }
  ->
  class { 'activemq::service': }
  ->
  anchor { 'activemq::end': }
}

And we get the following graph. Note the resources the end user has declared come after all of the resources in the activemq module.

Anchor Work Around

#3 Updated by Nan Liu over 4 years ago

A decision regarding class containment will also impacts stage behavior. See #8263

#4 Updated by Anonymous over 4 years ago

  • Category set to compiler
  • Status changed from Unreviewed to Accepted
  • Target version set to 3.x

Accepted

I think the “Anchor Pattern” has been sufficiently vetted at this point to warrant this ticket being accepted. I think it’s now clear that class containment is an issue in need of a solution in core.

If anyone disagrees, please feel free to change to “Needs more information,” let me know what’s not clear and assign back to me.

-Jeff

#5 Updated by Anonymous over 4 years ago

  • Status changed from Accepted to Needs Decision
  • Assignee set to Nigel Kersten
  • Target version deleted (3.x)

Nigel

Following Daniel’s suggestion I’m setting to Needs Decision and assigning to Nigel for placement on the road map. I’d like to see this fixed in 2.7.x but I think it can certainly wait until Telly. I think it must be solved in Telly though.

#6 Updated by Anonymous over 4 years ago

Following Daniel’s suggestion I’m setting to Needs Decision and assigning to Nigel for placement on the road map. I’d like to see this fixed in 2.7.x but I think it can certainly wait until Telly. I think it must be solved in Telly though.

To be honest, my gut feeling is that we would do well to look at this as a part of taking a serious look at how we structure and process the catalog, to try and get to a point where containers are full, first class parts of the graph at all times. Using that, and a model with pre and post “action” points on the vertex, and we get to the point that we can, for example, mostly ditch some of the current sorting and processing we do, and have a more complete model for processing content.

This might also flow to things like global completion ordering around the suggested package / file / service processing elsewhere, etc. So, I agree we must solve the problem, but I would also like to see if it can be done as part of a bigger rehabilitation of the catalog graph ordering that accounted for all the changes we are interested in.

#7 Updated by Garrett Honeycutt over 4 years ago

This is a very clever solution to a problem that adds extra complexity to one’s manifests. Puppet already has ways to describe dependency relationships. While this appears to work, I do not think it is a pattern that we should propagate.

#8 Updated by Anonymous over 4 years ago

Garrett Honeycutt wrote:

This is a very clever solution to a problem that adds extra complexity to one’s manifests. Puppet already has ways to describe dependency relationships. While this appears to work, I do not think it is a pattern that we should propagate.

Totally agree Garrett. This ticket is precisely so we don’t have to use the Anchor Pattern, which is just a horrible hack and a way to work around the problem.

We’ve always said we shouldn’t teach the anchor pattern and instead solve the problem in the core product, hence this ticket.

Until it’s fixed in core, however, the anchor pattern should be used whenever a class declares another class inside of a module. Otherwise, the end user of the module cannot use the dependency relationships you mentioned.

#9 Updated by Nigel Kersten about 4 years ago

  • Status changed from Needs Decision to Needs More Information
  • Assignee changed from Nigel Kersten to Anonymous

I swear we already decided this in another ticket Daniel is assigned to… am I wrong there?

#10 Updated by Anonymous about 4 years ago

  • Status changed from Needs More Information to Accepted

I thought so, too, but I can’t put my finger on which one, and searching turns up nothing.

We should support class to class dependencies. This is a major change to the graph on the agent side, so won’t be happening in the next little while, unfortunately.

#11 Updated by Nick Lewis about 4 years ago

I believe the decision we came to was to add a contains function to the stdlib module, but I think the actual work slipped through the cracks.

#12 Updated by Nigel Kersten about 4 years ago

Can someone file the issue around a contains function then?

Would that function automatically include a class if it hasn’t been declared? Or just specify the relationship?

#13 Updated by Anonymous about 4 years ago

Nigel Kersten wrote:

Can someone file the issue around a contains function then?

Would that function automatically include a class if it hasn’t been declared? Or just specify the relationship?

Filed as #13045

I commented on this ticket via email too… Hopefully it shows up here eventually.

-Jeff

#14 Updated by Chris Price almost 4 years ago

Jeff—starting to look at this and would love to pick your brain about it for a few minutes sometime soon. My most pressing question at the moment is this:

Your updated graph shows a relationship between “Anchor[java::*]” and “Anchor[activemq::begin]”. I’m confused as to how this relationship gets established. It seems like it would have to be specified explicitly somewhere… is that correct? And, if so, where does that happen? User’s manifest? Or in the ActiveMQ module?

#15 Updated by Anonymous almost 4 years ago

Chris Price wrote:

Jeff—starting to look at this and would love to pick your brain about it for a few minutes sometime soon. My most pressing question at the moment is this:

Sure, come on over to my desk anytime. Even if I have my headphones on.

Your updated graph shows a relationship between “Anchor[java::*]” and “Anchor[activemq::begin]”. I’m confused as to how this relationship gets established. It seems like it would have to be specified explicitly somewhere… is that correct? And, if so, where does that happen? User’s manifest? Or in the ActiveMQ module?

Here’s how that happens:

node foo {
  include mcollective
}

class mcollective {
  class { java: } -> class { activemq: }
}

So, in the line class { java: } -> class { activemq: } all of the resources in the Java class will have a relationship to all of the resources in the activemq class.

#16 Updated by Chris Price almost 4 years ago

  • Assignee changed from Anonymous to Chris Price

Andy and I are both working on this. I’m currently scouting for the original tickets where the whits-related changes were introduced, and will link them to this ticket accordingly.

Ticket #3382 seems to be one of the original reports of issues around this. It appears from reading that ticket that this was presumed to be fixed as of this commit:

https://github.com/puppetlabs/puppet/commit/59a23d69c20a8ab1f7dc9ff1d109305f9027641c

I plan to dig through the history a bit more, and then probably recreate some of the example manifests and test them against a few different versions of puppet.

#17 Updated by Chris Price almost 4 years ago

Also related: ticket #5200 …. commit:

https://github.com/puppetlabs/puppet/commit/2a6c6cb8fabf82d2f2127c90db670c1a856427c5

#18 Updated by Chris Price almost 4 years ago

We’ve had some discussions around this lately, so I’ll try to update the ticket for posterity:

  • There seems to be general consensus that we should be able to get by with only two types of “inclusions”; a simple one-level dependency—ala existing “require” function—and a “contains” dependency, which basically means “anything that has a dependency relationship with me also has the same dependency relationship with everything that I am expressing that I ‘contain’”. For the sake of conversation, we’ve been using the term “require” to discuss the first type of dependency and the term “contain” to describe the second type of dependency.

  • The current semantics of the “include” keyword (basically: dump this object in the catalog without any ordering or dependency information) should not be necessary once we have a working implementation of the “require” and “contain” semantics. In fact, the current semantics of “include” probably lead to manifests that are not explicit enough, and are prone to causing ordering issues that can be difficult to resolve.

  • In an ideal world we might like to overlay the new semantics on top of the existing terminology; in other words, “include” and the “class {‘foo’}” syntax would simply change over to the new “contains” meaning. However, there is obvious concern about the backward compatibility issues that this would cause.

  • Regardless of the final decision about how this new functionality should be expressed in the language, there seems to be general consensus that we should try to build a working implementation of it soon. This could be done as a module or function, so as to be non-invasive but available to solicit user testing. We would want to apply this to as many modules on the forge as possible (especially those that use the anchor pattern) and ensure that we don’t come across any unforeseen problems.

  • The technical details for the implementation basically just involve making sure that every “class” or “container” resource is broken into two nodes in the graph: a “before” node and an “after” node. We will establish implicit relationships between contained resources and these “anchor” points automatically. We may try to re-purpose, rename, and clean up the existing “whit” nodes, which appear to be a first attempt at doing exactly this… or we may get rid of them and modify the “component” nodes to behave this way.

  • We intend to change the graph output (dot/graphviz files) to explicitly illustrate these before/after anchor points in the graph—right now the graphs attempt to hide these.

  • We would like to leave the existing catalog format intact as much as possible; however there will probably be at least one change required: we need a way to distinguish between a “require” edge and a “contain” edge.

#19 Updated by Chris Price almost 4 years ago

  • Assignee deleted (Chris Price)

#20 Updated by John Bollinger over 3 years ago

Chris Price wrote:

We’ve had some discussions around this lately, so I’ll try to update the ticket for posterity:

  • There seems to be general consensus that we should be able to get by with only two types of “inclusions”; a simple one-level dependency—ala existing “require” function—and a “contains” dependency, which basically means “anything that has a dependency relationship with me also has the same dependency relationship with everything that I am expressing that I ‘contain’”. For the sake of conversation, we’ve been using the term “require” to discuss the first type of dependency and the term “contain” to describe the second type of dependency.

Are you perhaps forgetting an important class of people when you declare a “general consensus”? Inasmuch as I don’t recall anything about this idea having been discussed on puppet-users, and in particular inasmuch as I personally disagree, it seems a bit of a stretch. I am prepared to eat crow if somehow I missed this, but I don’t miss much that passes through the list.

  • The current semantics of the “include” keyword (basically: dump this object in the catalog without any ordering or dependency information) should not be necessary once we have a working implementation of the “require” and “contain” semantics. In fact, the current semantics of “include” probably lead to manifests that are not explicit enough, and are prone to causing ordering issues that can be difficult to resolve.

I beg to differ. Perhaps it would be possible to limp along without the functionality currently provided by “include”, but those semantics are worthwhile. Among its more common uses, “include” may express

  • a logical dependency arising because the class in which it appears refers to variables or resources declared in the included class (which does not inherently have order-of-application implications), or
  • a client-side functional dependency without order-of-application constraints

Unneeded relationships should be avoided, because each relationship requires Puppet to consume more memory to track the relationship and to expend more effort to choose an application order, and because unneeded relationships may create cycles that otherwise would not exist.

I particularly object to the premise that the semantics of “include” are prone to causing ordering issues. Ordering issues arise because manifest authors either under- or over-specify resource relationships. I don’t see how one can reasonably expect that reducing authors' expressive options will reduce ordering problems. In any case, the semantics of “include” are not responsible for any such problem. Every ordering problem can be laid squarely at the feet of one or more manifest authors.

Moreover, I don’t see how forcing manifest authors to express unneeded relationships is likely to make ordering issues less frequent or easier to resolve. With repeatable ordering it may be trickier to detect ordering issues arising from missing relationships, but once they are detected they are usually fairly easy to resolve. Certainly, they are no harder to resolve with “include” being present than if it were unavailable, and I’m inclined to think that “include” makes resolving such issues easier. On the other hand, resource cycles, excessive memory usage, and excessive CPU usage arising from excess relationships are more difficult problems.

I don’t have sufficient visibility to confidently judge whether manifests tend to under-specify relationships, but I don’t think it’s as simple as that. Discussion on the user list leads me to believe that it’s not so uncommon for people to over-specify relationships as well. That seems to happen when manifest authors confuse functional dependency (resource A must be configured for resource B to operate properly) with order-of-application dependency (resource A must be configured for resource B to be configured). The former does not require any special treatment by Puppet.

  • In an ideal world we might like to overlay the new semantics on top of the existing terminology; in other words, “include” and the “class {‘foo’}” syntax would simply change over to the new “contains” meaning. However, there is obvious concern about the backward compatibility issues that this would cause.

Surely you’re joking. What would even be desirable about changing the well-established semantics of an existing function to something markedly different? Notwithstanding the loss of the useful functionality that would be replaced, how is backward compatibility not an overriding concern? The usual practice for this sort of thing is to deprecate old functionality prior to removing it, and to introduce new functionality in a way that avoids conflict with the old. Not that I want to see “include” deprecated, but I absolutely do not want to see it replaced by something different under the same name.

#21 Updated by Chris Price over 3 years ago

John, thanks for the comments. Rest assured that backwards compatibility is a huge concern for us. The last comment that you replied to was speaking more to the idea of “how would the language look if we were designing it from scratch?” It is unlikely that we will drastically change the meaning of any of the existing keywords, and if we change them at all there will most certainly be a deprecation warning for an extended period of time first.

This ticket was being used to discuss ideas around this topic, but no action has been taken and none is scheduled at this time. Your feedback is appreciated and I expect that this will be discussed on the mailing lists in detail before any final decisions are made.

#22 Updated by Andre Nathan over 3 years ago

So does that mean that after an year there’s still no decision taken about this?

#23 Updated by Nick Fagerlund over 3 years ago

A proposed solution has come up a few times in convo w/ Dan Bode, and I want to make sure it’s captured here:

Why not just say that resource-like declarations (class {'myclass':}) always cause containment? This should be possible without conflicting with any existing semantics.

  • Multiple non-nested containment is scary because of dependency cycles, and we don’t want to allow it. Fine: Resource-like declarations already only allow you to declare a class once, so that’s taken care of for free.
  • Include should continue to not cause containment, and I think that’s sane.
  • I’ve tried and tried to come up with a situation where you’d want to cause a class to be contained but NOT have the class be wholly “owned” by its container. I can’t think of one.
  • This would also make resource-like declarations act more like normal resources, which is cool.

(This would almost definitely reveal currently hidden dependency cycles in peoples' code. That means we should implement it in a major version with a temporary pref to turn the behavior off, just so people can upgrade and start fixing their breakages one at a time. Ideally, the deactivated version of the behavior would log warnings when it sees things that would be cyclic.)

#24 Updated by Jeff Goldschrafe over 3 years ago

Nick Fagerlund wrote:

(This would almost definitely reveal currently hidden dependency cycles in peoples' code. That means we should implement it in a major version with a temporary pref to turn the behavior off, just so people can upgrade and start fixing their breakages one at a time. Ideally, the deactivated version of the behavior would log warnings when it sees things that would be cyclic.)

As an end-user, I’d much rather see this in a production release as soon as possible, with a setting to turn it on, rather than for some far-off 4.0 release. The anchor pattern causes a really gigantic amount of clutter in parameterized classes that have a lot of knobs to turn things on and off. We’d love to see it burn.

#25 Updated by eric sorenson over 3 years ago

  • Priority changed from Normal to High
  • Target version set to 3.x
  • Keywords changed from anchor containment contain graph modules module self-contained dependency reuse usability forge to anchor containment contain graph modules module self-contained dependency reuse usability forge backlog

#26 Updated by eric sorenson about 3 years ago

  • Priority changed from High to Immediate

#27 Updated by Charlie Sharpsteen about 3 years ago

  • Assignee set to Charlie Sharpsteen

#28 Updated by Charlie Sharpsteen about 3 years ago

  • Keywords changed from anchor containment contain graph modules module self-contained dependency reuse usability forge backlog to anchor containment contain graph modules module self-contained dependency reuse usability forge backlog customer

#30 Updated by Markus Uckelmann almost 3 years ago

  • Support Urls deleted (https://support.puppetlabs.com/tickets/772 https://support.puppetlabs.com/tickets/1659)

I just stumbled over this bug and it took me two days to figure it out. Could you please solve this issue?

#31 Updated by Dennis Jacobfeuerborn almost 3 years ago

  • Support Urls deleted (https://support.puppetlabs.com/tickets/772 https://support.puppetlabs.com/tickets/1659)

This bug is two years old now so is this actually looked into? AFAIK most modules on Puppet Forge don’t actually use the proposed workaround and thus are not usable to construct larger enveloping classes which makes more complex scenarios rather difficult to work with. For me this is probably the most important Puppet bug right now.

#32 Updated by Andre Nathan almost 3 years ago

  • Support Urls deleted (https://support.puppetlabs.com/tickets/772 https://support.puppetlabs.com/tickets/1659)

Completely agree with Dennis. It’s a pain to deal with this, it should be given a higher priority.

#33 Updated by eric sorenson almost 3 years ago

  • Support Urls deleted (https://support.puppetlabs.com/tickets/772 https://support.puppetlabs.com/tickets/1659)

#34 Updated by Patrick Carlisle over 2 years ago

  • Assignee changed from Charlie Sharpsteen to Patrick Carlisle

#35 Updated by John Bollinger over 2 years ago

Nick Fagerlund wrote:

A proposed solution has come up a few times in convo w/ Dan Bode, and I want to make sure it’s captured here:

Why not just say that resource-like declarations (class {'myclass':}) always cause containment? This should be possible without conflicting with any existing semantics.

I guess I wasn’t paying attention to this issue, or maybe I overlooked the suggestion above, so I apologize for throwing in my own two cents so long after the suggestion was originally posted.

I take the suggestion to be that if a class declares another via the parameterized-style syntax then that implies containment of the declared class by the declaring one. I disagree that that avoids conflict with existing semantics, because the fact that containment is NOT currently implied by parameterized-style class declarations must be considered part of their semantics. For example, the following currently works, but would produce a cycle under the proposed change:

class a { }

class b { class { ‘a’: } }

class c { include ‘a’

anchor { ‘c_start’: before => Class[‘a’] } anchor { ‘c_end’: require => Class[‘a’] } }

node foo { include ‘b’ include ‘c’ Class[‘b’] –> Class[‘c’] }

That the change could break existing code contradicts the premise that it does not involve any change to existing semantics.

  • Multiple non-nested containment is scary because of dependency cycles, and we don’t want to allow it. Fine: Resource-like declarations already only allow you to declare a class once, so that’s taken care of for free.

That’s a fair point, but perhaps it would be ok for Puppet to be a little less obstructionist. That multiple non-nested containment is indeed scary and may easily contribute to dependency cycles does not mean that Puppet must actively prevent people from declaring it.

  • Include should continue to not cause containment, and I think that’s sane.

Thank you, I think that’s sane, too.

  • I’ve tried and tried to come up with a situation where you’d want to cause a class to be contained but NOT have the class be wholly “owned” by its container. I can’t think of one.

“Ownership” meaning that the owned class should not be declared by other classes? You may be right — though I’m not persuaded — but if the proposal is implemented then you’ll suddenly have a lot of contained classes that don’t need to be owned, but are so simply because parameterized-style class declarations are popular.

  • This would also make resource-like declarations act more like normal resources, which is cool.

I can see the attraction. On the other hand, I’m not sure it’s such a good idea to make class declarations more like ordinary resource declarations while not making them exactly like ordinary resource declarations. It trades off user surprise in one area for user surprise in another, and it’s not clear to me that there is any net gain.

(This would almost definitely reveal currently hidden dependency cycles in peoples' code. That means we should implement it in a major version with a temporary pref to turn the behavior off, just so people can upgrade and start fixing their breakages one at a time. Ideally, the deactivated version of the behavior would log warnings when it sees things that would be cyclic.)

I guess I would put that differently: the proposal would create dependency cycles in people’s code that are not there now. Perhaps the result would be worth it, but I’m inclined to think not. Indeed, I think overloading the desired containment declaration semantics on top of existing syntax seems unwise. Whether a class needs to be contained or not is a separate concern from whether parameterized-style class declaration syntax can or should be used. What do you intend to say to those users who want to user the parameterized syntax, but need non-containment semantics? Why shouldn’t they be able to have both?

I’m sure I suggested this on puppet-users before, but what about a new built-in function instead? We already have the require() function, which does two thirds of the job (declares the class and establishes a relationship between declaring and declared classes). Why not add a built-in contains() function in the same mold? That would be a lot simpler for people to use than is the anchor pattern. It would be a straight substitution where people already use ‘include’ or ‘require’ to declare classes, but it could also be used easily with parameterized-style declarations, requiring only that users observe the restriction that the parameterized-style declaration comes before any other declaration of the same class (such as the contains() function would provide). And this wouldn’t need to wait for Puppet 4.

#36 Updated by Andre Nathan over 2 years ago

Completely agree with John. A “contains” built-in function would avoid breaking current semantics and allow us to have this very much needed feature sooner.

#37 Updated by Luke Kanies over 2 years ago

(As sent to the list.)

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 can’t remember is how inheritance affects containment, if at all.

Let’s be clear on what the original problem was that led us to this:

Fundamentally, we only have a problem in a circumstance like this:

class a { file { “/a”: ensure => present } class b {} class c { file { “/c”: ensure => present }

Class[a] –> Class[b] –> Class[c]

Because Class[b] is empty, the relationship between the resources in A and the resources in C get lost.

This is a bug that was introduced around 2.7 or 2.8, and it was introduced because we made an optimization in the graph.

The optimization was needed because the current graph code can have either containment or dependency edges in it, but not both. The catalog we ship from the server has containment edges, and all dependencies are specified as parameters, not edges in the graph.

The transaction creates what we call the relationship graph, which has those dependency edges, but no containment edges. The way it used to accomplish this was by essentially multiplying appropriate containment by appropriate dependency edges. E.g., in this case:

class a { file { [“/a1”, “/a2”]: ensure => present } } –> class b { file { [“/b1”, “/b2”]: ensure => present } }

We end up turning our one dependency (Class[a] –> Class[b]) into 4 edges: File[a1] –> File[b1] File[a1] –> File[b2] File[a2] –> File[b1] File[a2] –> File[b2]

In small catalogs, this isn’t really a problem, and might actually result in fewer edges (e.g., we actually had 4 containment edges in the first graph, and now have 4 dependency edges, so it’s equal).

In large catalogs, though, this multiplication (and note I’m using that word loosely; you can see the details in the ‘splice’ method on Catalog) results in a massive increase in edges, which significantly slows things down.

The optimization done was to add whits as a single collector for those edges, and thus reduce the overall count.

Unfortunately, this optimization introduced the bug we’re all discussing, which requires the anchor pattern.

The correct fix for this is relatively simple to understand, and should involve the removal of quite a bit of code, although it might be a bit hard to implement:

Change the graph handling code so it can correctly handle both dependency and containment edges. This is relatively simple: Just change the traversal (and cycle detection) to prefer dependencies over containment.

This way we don’t have to multiply edges, which means we don’t need whits (yay!), which means we don’t need anchors. Done. We also get to remove the relationship graph and all related code, which is also a win.

I tried this about 2 years ago, but I couldn’t get the cycle detection working.

#38 Updated by Josh Partlow over 2 years ago

  • Status changed from Accepted to In Topic Branch Pending Review
  • Branch set to https://github.com/puppetlabs/puppet/pull/1878

Merged Patrick’s contain function to master in 0cf6a31

#39 Updated by eric sorenson over 2 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

#40 Updated by Rob Terhaar over 2 years ago

Two years to discuss it, and two weeks to fix it ;)

I am really happy this is finally fixed. Thank you everyone!

#41 Updated by Ruslan Lutsenko over 2 years ago

So, with this fix we have to use:

class container {
  contain contained
 }

Just curious, how will it look in case when parameterized class need to be included and contained:

class container {
   class {contained:}
 }

Will all classes have optional boolean attribute “contained”, or parameterized classes are not meant to be contained?:

#42 Updated by John Bollinger over 2 years ago

Ruslan Lutsenko wrote:

Will all classes have optional boolean attribute “contained”, or parameterized classes are not meant to be contained?

It is already accounted for:

class container {
  class { 'contained':
      param => value
  }
  contain 'contained'
}

Of course, if you don’t need to bind parameter values via the declaration, then you can write it as

class container {
  contain 'contained'
}

despite the fact that class contained is parameterized, just like you could declare it without containment via ‘include’.

In fact, there is nothing inherently wrong with

class container {
  include 'contained'
  contain 'contained'
}

though it is redundant.

#43 Updated by Roman Esq over 2 years ago

Is it expected that the fix will be included in 2.6.19 version? Or is there any other possibility to use “contain” fix with 2.6 branch?

#44 Updated by eric sorenson over 2 years ago

Roman Esq wrote:

Is it expected that the fix will be included in 2.6.19 version? Or is there any other possibility to use “contain” fix with 2.6 branch?

No guarantees whatsoever, because 2.6 is past end of life. But the ‘contain’ function is just a standalone file that could be pluginsynced from a module.

#45 Updated by Andre Nathan over 2 years ago

In code like this:

node foo {
  class { 'base_server':
    ...
  }
  class { 'web_server':
    ...
  }
}

If I want to make sure all code in “base_server” runs before the code in “web_server”, what’s the preferred way to ensure that? Should I use “contains” in the “web_server” class definition, ie,

class web_server(...) {
  contains 'base_server'
  ...
}

Will a “contains => Class[‘base_server’]” meta-parameter be added in the future?

#46 Updated by John Bollinger over 2 years ago

Andre Nathan wrote:

In code like this:

node foo {
  class { 'base_server':
    ...
  }
  class { 'web_server':
    ...
  }
}

If I want to make sure all code in “base_server” runs before the code in “web_server”, what’s the preferred way to ensure that? Should I use “contains” in the “web_server” class definition, ie,

class web_server(...) {
  contains 'base_server'
  ...
}

Will a “contains => Class[‘base_server’]” meta-parameter be added in the future?

The puppet-users group would be a more appropriate forum for discussion about how best to use the new feature. The short answer is you would use the same ordering mechanisms Puppet already had: chaining operators and relationship metaparams. The ‘contains’ function would be used inside the base_server and web_server classes to mark the other classes those already declare as contained.

If you want a fuller, more nuanced answer then ask on puppet-users.

#47 Updated by Patrick Carlisle over 2 years ago

  • Target version changed from 3.x to 3.4.0

#48 Updated by Melissa Stone over 2 years ago

  • Status changed from Merged - Pending Release to Closed

Released in Puppet 3.4.0-rc1

#49 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

#50 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

#51 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

#52 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

#53 Updated by Melissa Stone over 2 years ago

Released in Puppet 3.4.0-rc1

Also available in: Atom PDF