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:

Refactor #7749

Bootstrapping Puppet in Ruby code has nasty scope cycles...

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

Status:ClosedStart date:06/01/2011
Priority:HighDue date:
Assignee:Chris Price% Done:

0%

Category:-
Target version:3.0.0
Affected Puppet version: Branch:https://github.com/puppetlabs/puppet/pull/571
Keywords:

We've Moved!

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


Description

So, in debugging an issue using code during early startup of Puppet, I found out that we have some very strange run-scope dependencies in bootstrapping.

Specifically, we depend on runtime code in Puppet::Util::CommandLine and Puppet::Application executing before we can safely execute load time code in Puppet.

This manifests as strange defaults in the configuration, because the run mode is configured in P::A during that run-scope code, but the defaults load in puppet earlier than that.

This also makes it difficult to just require 'puppet' and have things actually work. Coupled with a desire to be able to change run mode on the fly, and a longer term need for things that Faces abstract over to act as if in a run mode without the current hard-coded dependency, we kind of need to get this resolved.


Related issues

Related to Puppet - Feature #12085: The application name 'apply' is hardcoded and no other ap... Closed 01/23/2012
Related to Puppet - Feature #12968: acceptance tests for re-configuration Accepted 03/05/2012
Related to Puppet - Bug #7316: puppet face applications (subcommands) delivered via modu... Closed 05/02/2011
Related to Puppet - Bug #13536: Default rundir now /var/run/puppet and not home dir Rejected 03/30/2012
Related to Puppet - Bug #13588: log dir is not permissioned properly Closed 04/02/2012
Related to Puppet - Bug #15337: Do not merge configuration files in Telly Closed 07/02/2012
Related to Puppet - Feature #12126: make autoloader able to reload changed plugins Closed 01/24/2012
Related to Puppet - Feature #2935: Settings should use 'agent' and 'server' in addition to e... Closed 12/15/2009
Blocks Puppet - Bug #12494: When cwd is invalid, puppet prints a stack trace Accepted 02/07/2012

History

#1 Updated by Luke Kanies almost 5 years ago

Can you be a bit more specific about what’s happening that’s a problem? What’s the code in CommandLine and Application that has to run before core is loaded? Is this a fundamental flaw, or a happenstance of implementation?

#2 Updated by Anonymous almost 5 years ago

Can you be a bit more specific about what’s happening that’s a problem?

I can, indeed.

What’s the code in CommandLine and Application that has to run before core is loaded? Is this a fundamental flaw, or a happenstance of implementation?

This is happenstance; I can’t see any compelling reason we must behave the way we currently do, although my gut feeling is that it will be complicated to get to different behaviour. (Also, perhaps a little of this is always unavoidable; something, somewhere will have to bootstrap at some time.)

Further, I think that a lot of this complexity accreted over time. It probably started quite reasonably, years back, and as we added more capabilities, merged to the single binary, and modified our bootstrap process we ended up in this position. It has all the hallmarks of code flow that should have been cleaned up along the way and, for various good reasons like time pressure, just wasn’t.

I can’t give you an assurance that I have a perfect understanding of all the code. Parts of this are a black box, where I understood just enough to know what behaviour was occurring at a very high level, without understanding the ramifications fully. I do know enough to explain the obvious tip of the potential iceberg here. (…or, it could be this shallow. I /think/ it is, but I can’t be sure without a lot more time.)

So, the core problem is that when you require 'puppet' you implicitly run the code in puppet/defaults.rb in the context of the top level Puppet module. That, in turn, has dependencies on the run_mode and/or does setup for the defaults based on the application that is running.

Meanwhile, in Puppet::Util::CommandLine we actually load the application in the execute instance method. Loading the application file causes the desired run_mode to be declared – this is done at the time the file is loaded. The next thing that the execute method does is create an instance of the application class.

In the Puppet::Application initialize method a small set of things happen: we require the command line file, store that away, then call set_run_mode with the run_mode we stashed away earlier, at load time. (Here, of course, we are in execution time.)

Then, finally, that method will call require "puppet" after doing all the rest of the setup. This, in our bootstrap process, is when we load the top level class, and so evaluate the defaults: triggered at “load” time, but with that loading deliberately delayed until a bunch of “execution” time code has run to configure global state.

I introduced the problem by requiring puppet at the time the command line stuff was loaded, long before the code was executed and the objects instantiated that actually configured the global state of the code. This led to invalid values for various settings, breaking startup in limited ways that showed up in our acceptance tests.

To fix this, we should make sure that it is possible to just execute require "puppet" as the very first line of whatever loads the puppet code; that, in turn, implies that any library we write can just require the top level puppet module if they depend on it.

To get there, I can identify that we need to at least disambiguate the way we handle building the defaults, and the way they interact with the run_mode. On the plus side, this also means that it should be possible to change run_mode at runtime without any risk of things going wrong. :)

#3 Updated by Anonymous almost 5 years ago

  • Status changed from Unreviewed to Accepted

#4 Updated by Nigel Kersten over 4 years ago

  • Priority changed from Normal to High
  • Target version set to 3.x

This affects all sorts of other bits of functionality.

#5 Updated by Nigel Kersten over 4 years ago

Adding Josh as a watcher as we were chatting about this.

#6 Updated by Chris Price about 4 years ago

  • Assignee set to Chris Price

#7 Updated by Chris Price about 4 years ago

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

#8 Updated by Anonymous about 4 years ago

I had a small number of other polish changes to this, but it is pretty much ready to go in. Good work.

#9 Updated by Nigel Kersten about 4 years ago

Great to see this one get untangled!

#10 Updated by Chris Price about 4 years ago

Pull request updated based on Daniel’s feedback.

#11 Updated by Anonymous about 4 years ago

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

All merged. Awesome.

#12 Updated by Anonymous almost 4 years ago

  • Target version changed from 3.x to 3.0.0

#13 Updated by Anonymous almost 4 years ago

For the watchers of this ticket, as part of the work in 7749, the behavior of Telly is that ~/.puppet.conf is merged with /etc/puppet/puppet.conf. We’ve decided this is undesirable and complicates the problem of loading extension code so we’re going to undo this specific behavior change in Telly.

Please see #15337 for more information. Please add comments related to the configuration file merging in #15337 rather than this ticket.

#14 Updated by Matthaus Owens over 3 years ago

  • Status changed from Merged - Pending Release to Closed

Released in Puppet 3.0.0rc1.

Also available in: Atom PDF