Puppet

I can confirm this issue occurring on a 2.7.1 client running against a 2.7.1 master using mod_passenger.

The error occurs every time a change is made to a file in one of the modules used by the client. On the second attempt the client runs fine. As such, it’s wonderfully reproducible. :–)

Upgrading the master to 2.7.2rc1 has solved the problem in my case. Could it be possible that this is a manifestation of bug #5318, which was fixed in 2.7.2rc1 ?

I can also confirm this. In my case downgrading to 2.6.9 fixed the problem.

I also noticed this primarily in modules using autoloading. None of my modules who (unfortunately) have all classes in init.pp had a problem. Only modules with the file name as the class name.

Additionally, are all of you suffering from this problem using “import” rather than “include” ?

If you’re using “import”, are you using globbing like “import foo/*” ?

The modules I had this problem on all had something like the following in manifests/init.pp

import “classes/.pp” import “definitions/.pp”

class zabbix { include zabbix::base }

The others varied in that there was nothing in the class defined in init.pp. So in my case I was using “import” and globbing.

Also wanted to add downgrading my puppetmaster server to 2.6.9 was able to get rid of the problem.

We’ve clearly regressed somewhat, but I’m curious as to why you’re using import rather than constructing classes in modules and letting the autoloader find the classes?

It sounds like you don’t have your classes laid out like:

$modulepath/foo/manifests/init.pp -> class foo
$modulepath/foo/manifests/bar.pp -> class foo::bar
$modulepath/foo/manifests/bar/baz.pp -> class foo::bar::baz

Is that right?

I encountered this bug, and I think I’ve found some information and a workaround.

I had a working (under puppet 2.6.3) module named “mysql”, with mysql_client & mysql_server classes. There was no mysql class. Since my upgrade to puppet 2.7.1, this module wasn’t working anymore, with a “Error 400 on SERVER: Could not find class mysql_server” message.

In order to regain a working mysql module with puppet 2.7.1, I had to declare an empty mysql class in the init.pp file of mysql module :

  class mysql {}
  import "mysql_client"
  import "mysql_server"

And include it in the nodes definition before the mysql_server class. For instance :

    node test {
      include mysql
      include mysql_server
    }

It seems that, for whatever reason, puppet 2.7.1 refuses to autoload a module who doesn’t contain a class with module name.

Following up on Nigel’s comment (#5), after downgrading my server back to 2.7.1 I managed to solve the problem by eliminating all “import *.pp” statements from my modules.

Once the module layout is such that the autoloader finds the classes automagically, the error disappears.

Nigel Kersten wrote:

We’ve clearly regressed somewhat, but I’m curious as to why you’re using import rather than constructing classes in modules and letting the autoloader find the classes?

It sounds like you don’t have your classes laid out like:

[…]

Is that right?

Yes, and I also see the problem. All my modules are laid out like:

$modulepath/foo/manifests/init.pp: import "foo.pp"
$modulepath/foo/manifests/foo.pp: class foo { ... }

Reason is that having tens of “init.pp” files open in emacs is impossible to navigate. It is just clean and convenient to have the foo class in foo.pp.

IMHO the autoloader could be hacked to look for foo.pp as well as init.pp, would only cause problems if you have a foo::foo class.

It seems the loader has become way more picky, most times only accepting classes in dirs/files with a corresponding name, and apparently not allowing multiple classes in the same files. I would not mind to have a pure one-class-per-file scheme, but then it should also extend to the base class, ie. drop init.pp.

so to be clear, we do still support multiple classes in the same file.

If you have the parent init.pp class laid out so that the autoloader will find it, and the other classes are declared in it, and you’re not using import, and you’re still hitting errors, lets separate out that into another bug please.

There should be no need to import at all within a module, and life really is significantly simpler if you work with the autoloader rather than against it. I’d suggest that a better approach would be:

# modulepath/foo/manifests/init.pp
class foo {
  include foo::foo
}

I do recognize that we’ve changed behavior, but I’m currently trying to disentangle changed defined behavior from changed undefined behavior in 2.6.x, and it’s unlikely we’re going to “fix” changes in undefined behavior with import unless we have a significant set of affected users.

I’m not completely opposed to the idea of alternate autoloader behavior like “foo.pp –> class foo {}”, but I’d like to separate out those feature requests from the bugs we look to be identifying here.

I am watching this issue closely as it’s clearly causing pain for users.

My email is nigel@puppetlabs.com, my twitter handle is @nigelkersten, and if people want to be able to talk over this in a private space, rant at me, or anything else that is going to add to the noise here and make it harder to solve the actual problems, please feel free to contact me.

I have to agree with Simon here: dozens of init.pp’s is simply a navigation nightmare (not only in emacs, but also in vi and Xcode).

The thing that should be done now is that the release notes for 2.7.? should be changed: Nowhere is visible that import behaviour is changed. This issue now needlessly costs me half a day.

Stephan Eggermont wrote:

I have to agree with Simon here: dozens of init.pp’s is simply a navigation nightmare (not only in emacs, but also in vi and Xcode).

The thing that should be done now is that the release notes for 2.7.? should be changed: Nowhere is visible that import behaviour is changed. This issue now needlessly costs me half a day.

Stephan, can you help with more details on specifically how your manifests needed to change? We need more concrete details for a release note update.

In my ntp recipe, I had two files in the manifests, init.pp and ntp.pp.

init.pp

import "*.pp"

and ntp.pp

class ntp::ntp {
package { "ntp":
ensure => installed
}
service { "ntp": 
ensure => running,
require => Package["ntp"],
}
}

I changed init.pp to

class ntp::ntp {
package { "ntp":
ensure => installed
}
service { "ntp": 
ensure => running,
require => Package["ntp"],
}
}

to make things work. The example is a bit contrived, but as soon as I have more classes in the recipe, I’ve used the import “*.pp” pattern in init.pp. This broke my vagrant 0.8 setup with a lucid64 box using puppet 2.7.1 on a mac os 10.6.8 host.

ok. I think we’ve got some details we can promote now.

Is there a reason why you were using import inside a module?

You were actually using the autoloader layout, so include ntp::ntp in init.pp would also have worked for you.

If you’re refactoring now, you should move to the canonical autoloader layout.

# init.pp

class ntp {
  include ntp::ntp
}

# ntp.pp

class ntp::ntp {
  ... 
  # your stuff
  ...
}

Does that make more sense?

The import made it easy to refactor the scripts, and not have so many files init.pp open. It could be that we ran into some autoloader bug originally and solved it that way, or just that we copied it from some tutorial.

Stephan Eggermont wrote:

The import made it easy to refactor the scripts, and not have so many files init.pp open. It could be that we ran into some autoloader bug originally and solved it that way, or just that we copied it from some tutorial.

Yep, understood. I believe the layout I outlined above also gives you the ability to not have a bunch of init.pp files open though, and works with the autoloader more closely.

Nigel Kersten wrote:

We’ve clearly regressed somewhat, but I’m curious as to why you’re using import rather than constructing classes in modules and letting the autoloader find the classes?

It sounds like you don’t have your classes laid out like:

[…]

Is that right?

Currently the modules I used “import” on where laid out like so..

$modulepath/foo/manifests/init.pp
$modulepath/foo/manifets/classes/bar.pp

So is the correct way to structure the directory for autoloading is to put all the class and definition files in manifests/. and not in subdirectories, or is subdirectories like “classes” and “definitions' acceptable?

I filed http://projects.puppetlabs.com/issues/8801 before I found this. Our earlier modules (there are dozens) use the old semi-standard (??) “import ‘*’” in modules/module/manifests/init.pp rather than declaring a base class in it.

In the case of simple modules with a single include, it’s easy enough to make that include become init.pp. For more complex ones, it is a hardship to have to refactor all of them at once. Downgrading to 2.6.9 would be acceptable if we didn’t have to deal with http://projects.puppetlabs.com/issues/5318 , but since we would, I for one would really like to see this behavior supported for a longer period of time (if not indefinitely) in newer versions.

I did also try eliminating globbing in imports and manually specifying .pp files, but it still yields the same issue.

Read all thread comments in more detail, and it looks like a refactor won’t be nearly so bad. Typical layout: import * in init.pp, class foo in foo.pp. As per Nigel’s comments, and since we also happen to be using autoload-compatible layout, class foo { include foo::foo } in init.pp and class foo::foo in foo.pp will do the trick.

If I were to guess, I would guess that most people running Puppet aren’t using Gems or staying current, and are using what their distro (or a place like EPEL) are providing. And I would also guess that, once those go to 2.7.x – if the import-in-module functionality is still silently deprecated – you may then receive “a significant set of affected users.” But by then, the horse would have already left the stable. Just saying :)

I don’t know what the status of this is, but this completely breaks our Puppet configuration. We have just spent six months building and deploying Puppet 2.6 on thousands of nodes, and it looks like we’re looking at a massive refactor effort if we expect to migrate to 2.7. I’m not even sure it is even possible to use this new structure with the complexity of our environment.

When was the decision made to eliminate “import”? Was this documented anywhere? It may be that we just don’t understand how modules are supposed to be used, but I don’t get the idea behind this at all. It seems to me what you are saying here is that if I want to create a module, let’s say for “foo”, and have some classes inside of that, I have to include them all on each node that uses anything in module “foo”.

For example:

foo::server (server setup of service foo) foo::client (client setup of service foo)

init.pp:

class foo {

include foo::server include foo::client

}

And in the node.pp:

node node.pp {

include foo

}

If we do this, we have to install both foo::server and foo::client on each node. So obviously I am missing something here, but we use our modules to configure various forms of functionality around a particular service (e.g., apache, nis, afs).

We need to be able to install foo::server on one node, and foo::client on another node.

Thus:

node client {

include foo::client

}

node server {

include foo::server

}

The way I read this is that I’ll need a separate module for each one, this:

modules/foo_server modules/foo_client

So what am I missing?

I can only speak for my own usage pattern, but –

OLD – modules/foo/manifests/init.pp: import “*” modules/foo/manifests/foo.pp: class foo { blah blah }

NEW – modules/foo/manifests/init.pp: class foo { include foo::foo } modules/foo/manifests/foo.pp: class foo::foo { blah blah }

Hope that helps :)

You better ask that on the puppet-users list. But in short:

When was the decision made to eliminate “import”? Was this documented anywhere? It may be that we just don’t understand how modules are supposed to be used, but I don’t get the idea behind this at all. It seems to me what you are saying here is that if I want to create a module, let’s say for “foo”, and have some classes inside of that, I have to include them all on each node that uses anything in module “foo”.

With the whole autoloading feature (which is documented quite good) there is simply no need for import.

We need to be able to install foo::server on one node, and foo::client on another node.

Thus:

node client {

include foo::client

}

node server {

include foo::server

}

The way I read this is that I’ll need a separate module for each one, this:

modules/foo_server modules/foo_client

So what am I missing?

No need for a separate module:

modules/foo/manifests/client.pp:
class foo::client {
  #do some client stuff
}
modules/foo/manifests/server.pp
class foo::server {
  # do some server stuff
}

# no need for an init.pp at all, as there is no class foo!!!

Then the nodes:

node client {
  include foo::client
}
node server {
  include foo::server
}

# no need to import anything!

If you follow these simply rules:

• only 1 class/define per file
• filename according the following rules:
  • a class foo::bar goes into modules/foo/manifests/bar.pp
  • a define foo::blub::bla goes into modules/foo/manifests/blub/bar.pp
  • a class foo::blub goes into modules/foo/manifests/blub.pp
  • class foo goes into modules/foo/manifests/init.pp

This is documented really well and also a lot of public available modules are following these guidelines (like the one I help to develop at http://git.puppet.immerda.ch), also afair it’s included in the best practices. Organizing your code like that also gives you much less headaches while searching for the correct class etc., as by following the conventions you will end up at the right place.

import is imho a left over of puppet’s dark ages and should in my humble opinion really not be used, there is simply no reason to use it. It caused various problems in the past, like the ones we are seeing here now. A year (or so) ago, there was also a lengthy discussion about that on the puppet-dev list and if I’m not completely wrong, there was afterwards also a RFC-mail on the puppet-users list. Both concluded that it makes sense to narrow the puppet loads code to avoid the various problems and performance issues, as the autoloading feature is working very well and quite good documented.

This so far from my humble memory. The rest should really better discussed on one of the lists.

Thanks. This isn’t as bad as it first appeared. We’ve worked out a script to convert our manifests over to this structure already.

Thanks Peter for the awesome response.

“import is imho a left over of puppet’s dark ages and should in my humble opinion really not be used, there is simply no reason to use it. It caused various problems in the past, like the ones we are seeing here now.”

That’s exactly right. We have very few reasons to need import now, and they’re all at site.pp level, where you want to define things like resource defaults and node definitions.

Life is significantly easier if you use include with the autoloader.

We haven’t deliberately broken anything here, but it is in a de-emphasized area of the code base, and people have been using import in many many different ways, so it’s been difficult to identify what actual fixes we need to make.

If it comes down to prioritizing fixes against include/autoloader vs fixes against import, the former is definitely going to win in most cases right now.

There is no current reason to use import, but there was a previous reason to use it.

A top hit for ‘scaling puppet’ and ‘git puppet’ is http://bitfieldconsulting.com/scaling-puppet-with-distributed-version-control . I think I was given the link on the #puppet IRC channel less than a year ago as a suggestion for both scaling and (switching to) modular design. That is how a lot of our manifests were made, for a while: “import *” in init.pp, while still using a module structure. People that only read Pulling Strings with Puppet might also get the same idea.

I’m all for progress, and justified deprecation. Even though I have a simple workaround for this on my own legacy manifests (the new ones are “fully” modular a la the current docs and Pro Puppet), the insider perspectives stated on this situation seriously concern me with regard to our own potential full-enterprise adoption. Syntax/API breakage, as opposed to warn-and-proceed, is something that I don’t think should be taken at all lightly, and it’s not clear to me from the contents of this ticket why phased deprecation isn’t on the table.

Right now you’re dealing with those that were running old versions, did make old manifests, decided to upgrade, located this bug, and decided to participate. How many more do you think might be running old versions, have old manifests, and haven’t upgraded because they’re still using what their distro handed them? When THEY show up, it could be in droves, and I don’t think the errors are sufficiently self-descriptive. I can speak only for myself, but I wasn’t pleased when I found out that the syntax was unceremoniously broken, and potentially for “you should do it the right way” reasons, while “the right way” is rapidly evolving.

Somewhat-related anecdote: we run hundreds of Ubuntu LTS servers, and got hammered by a ‘feature’ of dpkg. As it turns out, ext4 and btrfs systems performed slower than, say, ext3 with package operations. They modified it so that it issued a ‘sync’ call before doing an installation, which sped things up. Great, right? Well, sync is a whole-system sync, and there are folks out there, like us, with ultra-traffic NFS servers which can take up to an entire day to finish a sync process during live operation. The solution we’re employing is backporting dpkg from a future version, which adds a scary-sounding “force-unsafe-io” option. Now we seriously question sticking with a platform that enacts changes which hard-break previously-standard behavior, and don’t care enough to backport it themselves to the allegedly stable release.

Nick Lewis tracked this down to this commit:

https://github.com/puppetlabs/puppet/commit/4da88fb4cd57871f16649d50572240ac3f7420f0

James Turnbull wrote:

Gustavo – can you please try the RC candidate and see if that fixes it?

James,

sorry for the late reply..

I’ve had to left the migration to puppet 2.7.x behind for a while. I’ve just tried puppet 2.7.3 and the issue remains.

Cheers, Gustavo

Nigel Kersten wrote:

Nick Lewis tracked this down to this commit:

https://github.com/puppetlabs/puppet/commit/4da88fb4cd57871f16649d50572240ac3f7420f0

So, in short… what is the directions to take?

It seems that the “import” section described earlier in this thread will be deprecated in future.. is that right?

We’re going to be fixing this regardless of the import deprecation plan. It’s a change in behavior we need to address.

We should have some engineering resources on this in the next few weeks, and any help from the community to identify the specific bug/behavior change would be much appreciated.

Guess I better update my article :)

It’s rather unfortunate that code which was previously required to make modules work, now breaks them altogether.

Nigel Kersten wrote:

We’re going to be fixing this regardless of the import deprecation plan. It’s a change in behavior we need to address.

Thanks for the updates and clarification, Nigel. Very happy to hear it.

We should have some engineering resources on this in the next few weeks, and any help from the community to identify the specific bug/behavior change would be much appreciated.

As in, trying to isolate the specific checkin that changed the behavior? If so, how about something that rolls through all of the post-2.6.9 checkins sequentially, stopping at the first failure on an “import *”? Perhaps a framework for that could be useful for Puppet devs in general, if it doesn’t already exist.

I just had the issue in 2.7.6. I had previously worked with modules that require a server and a client part like this:

modules/foo/manifests/init.pp:
import "*"

modules/foo/manifests/server.pp:
class foo::server {
}

modules/foo/manifests/client.pp:
class foo::client {
}

So there was no class defined in init.pp, but I was under the impression that that was needed to load the other files and make autoloading foo work. After going through this issue, I realized I can now simply delete the whole init.pp and everything just works. Simple and easy, just cost me a bit of searching and reading to understand this issue :)

Edit: just noticed comment #24 also explained that. Guess I missed that one, my apologies.