Import vs autoload: the puppet parser is a bad joke that stopped being funny years ago.
Closed, ResolvedPublic

Description

This patch: https://gerrit.wikimedia.org/r/#/c/254107/ caused:

Error: Could not retrieve catalog from remote server: Error 400 on SERVER: Role class role::puppetmaster::frontend not found at /etc/puppet/manifests/site.pp:2116 on node palladium.eqiad.wmnet

So the puppet parser refuses to work correctly if you have an "import" that includes classes in a specific namespace, then you have some other classes from the same module/namespace but in autoload layout. This is only a problem it the classes in autoload layout are in the first-level directory, modules/$modulename/manifests/someclass.pp

yuvipanda updated the task description. (Show Details)
yuvipanda raised the priority of this task from to Needs Triage.
yuvipanda added projects: Operations, Puppet.
yuvipanda added a subscriber: yuvipanda.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptNov 19 2015, 6:13 AM
yuvipanda assigned this task to Joe.Nov 19 2015, 6:14 AM
yuvipanda set Security to None.
Dzahn added a subscriber: Dzahn.EditedDec 14 2015, 7:50 PM

afaik the classes should instead be: module/role/puppetmaster/something.pp and we are fixing this by moving all things from manifests/role into module/role.

That fixes the autoload layout warnings and classes are found. I already moved some of them that were easy because they have no "init.pp".

Joe triaged this task as Low priority.Feb 10 2016, 10:44 AM
scfc added a subscriber: scfc.Feb 10 2016, 10:18 PM

This doesn't have anything to do with the role function. The autoload layout is only applied to modules/. What manifests are pulled from manifests/ is determined by the explicit imports in manifests/site.pp:

# vim: set tabstop=4 shiftwidth=4 softtabstop=4 expandtab textwidth=80 smarttab
# site.pp

import 'realm.pp' # These ones first
import 'misc/*.pp'
import 'network.pp'
import 'role/*.pp'
import 'role/analytics/*.pp'

[…]

These don't match manifests/role/puppetmaster/backend.pp, so a patch creating a new subdirectory in manifests/role needs to patch manifests/site.pp as well.

In the end that is one of the advantages of moving role classes to the autoload layout beneath modules/role/: They are no longer always imported, but only when needed.

@scfc is absolutely correct, it's the import lines in site.pp and yes, move them to modules/role/ is the fix. we know how to for all classes with the structure "role foo::bar" just not for the ones that are just "role foo" yet.

scfc added a comment.Feb 10 2016, 11:11 PM

(@Dzahn, could you point to an example or a task for "role foo" not working with modules/role/?)

@scfc So.. if it's a role foo::bar (or more levels than that), we move it to modules/role/manifests/foo/bar.pp and that works fine, i have moved a couple already, it generally just compiles fine, is noop and lint is happy.

But if you have a role foo, you wonder how you move it. modules/role/manifests/foo.pp is still not correct in the autoload layout, then you might think (like i did), it becomes modules/role/manifests/foo/init.pp by convention, but no, that doesn't work.

So one way to get around it is to just make sure all roles have this 2-level naming structure, for example a "cheat" is to do something like renaming "role phabricator" to "role phabricator::server" or "role bla" to "role bla::main" or whatever. In some cases it made a little sense and i did it, but in other cases it seems wrong to to just make something up.

scfc added a comment.Feb 10 2016, 11:32 PM

For role foo, modules/role/manifests/foo.pp should definitely work. /init.pp only works for the "base" class, in this case "role"; cf. http://docs.puppetlabs.com/puppet/3/reference/lang_namespaces.html for the details:

namefile path
apache<MODULE DIRECTORY>/apache/manifests/init.pp
apache::mod<MODULE DIRECTORY>/apache/manifests/mod.pp
apache::mod::passenger<MODULE DIRECTORY>/apache/manifests/mod/passenger.pp

Could you point out the patch that failed?

Dzahn added a comment.EditedFeb 12 2016, 5:08 PM

Could you point out the patch that failed?

https://gerrit.wikimedia.org/r/#/c/270105/
http://puppet-compiler.wmflabs.org/1750/ruthenium.eqiad.wmnet/

https://gerrit.wikimedia.org/r/#/c/270107/
http://puppet-compiler.wmflabs.org/1749/rcs1001.eqiad.wmnet/

afaik it's a limitation of the role keyword hack, which brings us back to the topic of this ticket

scfc added a comment.Feb 24 2016, 9:57 PM

Short recap of (failed) trials: The issue does not lie with the role() function as the error occurs with plain include role::something (one level, i. e. not include role::something::xyz) as well. It is also not an error specific to role::ntp as if that manifest is also moved, the error shifts to role::diamond and then probably others. It may be specific to the role namespace, but I'm not sure.

@scfc joe looked at it earlier and said "the problem is "import" is implemented awkwardly.. there is no way to fix it,"

now this:

< _joe_> mutante: we could just rename ::role::base::diamond and ::role::base::ntp
< _joe_> better, base::role
< mutante> _joe_: ok, let's try that

Change 273209 had a related patch set uploaded (by Giuseppe Lavagetto):
standard: move to own module

https://gerrit.wikimedia.org/r/273209

Joe renamed this task from 'role' function doesn't find classess in autoload layout in manifests/role to Import vs autoload: the puppet parser is a bad joke that stopped to be funny years ago..Feb 29 2016, 10:01 AM
Joe added a subscriber: Joe.
Joe added a comment.Feb 29 2016, 10:11 AM

For the record, I isolated the issue.

It has nothing to do with the role function or anything outside the puppet parser.

Here is my example:

With this directory structure:

.
├── manifests
│   ├── role
│   │   └── foo.pp
│   └── site.pp
└── modules
    └── role
        └── manifests

where manifests/role/foo.pp is

class role::foo { notice('foo!'}

and manifests/site.pp is

import 'role/*.pp'

node 'test' {
    include role::foo
}

In this situation, puppet compiles the catalog just fine:

test$ puppet apply --modulepath=modules manifests/site.pp 
Warning: The use of 'import' is deprecated at manifests/site.pp:4. See http://links.puppetlabs.com/puppet-import-deprecation
   (at /usr/lib/ruby/vendor_ruby/puppet/parser/parser_support.rb:110:in `import')
Notice: Scope(Class[Role::Foo]): foo!
Notice: Compiled catalog for test in environment production in 0.02 seconds
Notice: Finished catalog run in 0.08 seconds

Now I just touch an empty file in modules/role/manifests and MAGIC! the catalog compilation fails:

test$ touch modules/role/manifests/bar.pp
test$ puppet apply --modulepath=modules manifests/site.pp 
Warning: The use of 'import' is deprecated at manifests/site.pp:4. See http://links.puppetlabs.com/puppet-import-deprecation
   (at /usr/lib/ruby/vendor_ruby/puppet/parser/parser_support.rb:110:in `import')
Error: Could not find class role::foo for test on test
Error: Could not find class role::foo for test on node test

What is more funny is that if I add a file in a subdirectory of modules/role/manifests instead, the catalog compiles fine!

test$ rm modules/role/manifests/bar.pp
test$ mkdir modules/role/manifests/bar
test$ touch modules/role/manifests/bar/baz.pp
test$ puppet apply --modulepath=modules manifests/site.pp 
Warning: The use of 'import' is deprecated at manifests/site.pp:4. See http://links.puppetlabs.com/puppet-import-deprecation
   (at /usr/lib/ruby/vendor_ruby/puppet/parser/parser_support.rb:110:in `import')
Notice: Scope(Class[Role::Foo]): foo!
Notice: Compiled catalog for test in environment production in 0.01 seconds
Notice: Finished catalog run in 0.05 seconds
Joe updated the task description. (Show Details)Feb 29 2016, 10:14 AM
Joe added a comment.Feb 29 2016, 10:17 AM

So, it looks like we're left with the option of either:

  1. migrate all second-level roles (like role::mediawiki::appserver) and then migrate the first-level ones in one bang
  2. rename temporarily the first-level roles with a middle-prefix like role::ntp => role::temp::ntp or something (bikeshedding welcome)

if we want to migrate roles in autoload layout and get rid of that annoying import warning.

Joe changed the task status from Open to Stalled.Feb 29 2016, 10:18 AM
Joe renamed this task from Import vs autoload: the puppet parser is a bad joke that stopped to be funny years ago. to Import vs autoload: the puppet parser is a bad joke that stopped being funny years ago..
Joe added a comment.Feb 29 2016, 10:31 AM

Another possible solution: we move the master to be puppet 3.7 or later (so upgrading to jessie would do) and we use the future parser, removing the import directive from site.pp:

test$ touch modules/role/manifests/bar.pp
test$ puppet apply --parser=future --modulepath=modules manifests/
Notice: Scope(Class[Role::Foo]): foo!
Notice: Compiled catalog for test in environment production in 0.46 seconds
Notice: Finished catalog run in 0.05 seconds

But this has rather large implications...

scfc added a comment.EditedFeb 29 2016, 10:52 AM

@Joe: One thing I found intriguing with my tests was that when I defined a class toollabs::xyz in manifests/role/rcstream.pp that printed a notification and included that class in manifests/site.pp's node default, it worked despite modules/toollabs/manifests/*.pp existing. The issue only occured with modules/role/manifests/*.pp, so I'm not sure if there is not something special about the role module.

Change 273209 merged by Giuseppe Lavagetto:
standard: move to own module

https://gerrit.wikimedia.org/r/273209

Ottomata added a subscriber: Ottomata.EditedMar 1 2016, 4:21 PM

We should just make a new special modulepath for roles!!!!!!!

modulepath = /etc/puppet/private/modules:/etc/puppet/modules:/etc/puppet/role
Ottomata added a comment.EditedMar 1 2016, 4:29 PM

I suppose this would conflict with a lot of currently named role classes, since there are role classes that have similar names to module classes, e.g. ::role::kafka* vs ::kafka*.

Dzahn added a comment.EditedMar 18 2016, 12:46 AM

microsites roles moved https://gerrit.wikimedia.org/r/#/c/275034/

ores roles moved (by tim landscheidt) https://gerrit.wikimedia.org/r/#/c/270102/

Dzahn added a subscriber: Mschon.Apr 20 2016, 2:21 AM
faidon added a subscriber: faidon.Aug 4 2016, 10:50 AM

Why can't we just move all the roles from manifests/roles into modules/roles in one single commit and be done with it? It should be *just* a file rename, no functional changes or anything, no?

Renaming each role separately felt already like a big burden to me, even more so if we need to (even temporarily) rename the roles to add arbitrary prefixes/suffixes.

Change 311194 had a related patch set uploaded (by Paladox):
archiva: Fix it not being a autoload module

https://gerrit.wikimedia.org/r/311194

Joe removed Joe as the assignee of this task.Oct 5 2016, 7:57 AM
Joe edited projects, added User-Joe; removed Patch-For-Review.

Why can't we just move all the roles from manifests/roles into modules/roles in one single commit and be done with it? It should be *just* a file rename, no functional changes or anything, no?

Renaming each role separately felt already like a big burden to me, even more so if we need to (even temporarily) rename the roles to add arbitrary prefixes/suffixes.

I agree heartily. Here's my plan:

  1. Make sure that all files in manifests/role are in autoload format. There are only three files left - logstash.pp, eventlogging.pp and mariadb.pp. These will be simple noop renames that we can verify with puppet-compiler
  2. Move all of manifests/role/*.pp en-masse to modules/role/manifests.
  3. Find places where we had 'invented' class names, and rename them back to sane names.

I'm primarily doing it to remove confusion about where files are, and to stop the proliferation of invented class names...

I'll take a shot at this next week.

Change 315343 had a related patch set uploaded (by Dzahn):
mariadb: split role classes into separate files

https://gerrit.wikimedia.org/r/315343

Change 311194 abandoned by Paladox:
archiva: Fix it not being a autoload module

https://gerrit.wikimedia.org/r/311194

Change 315343 abandoned by Dzahn:
mariadb: split role classes into separate files

https://gerrit.wikimedia.org/r/315343

Joe moved this task from Backlog to Doing on the User-Joe board.Oct 25 2016, 2:24 PM

Change 315343 restored by Giuseppe Lavagetto:
mariadb: split role classes into separate files

Reason:
This Patch or something similar will be needed.

https://gerrit.wikimedia.org/r/315343

Change 317837 had a related patch set uploaded (by Giuseppe Lavagetto):
puppet: kill manifests/roles, move under $modulepath

https://gerrit.wikimedia.org/r/317837

Change 317837 merged by Giuseppe Lavagetto:
puppet: kill manifests/roles, move under $modulepath

https://gerrit.wikimedia.org/r/317837

Joe closed this task as Resolved.Oct 25 2016, 5:24 PM
Joe claimed this task.

Change 315343 abandoned by Dzahn:
mariadb: split role classes into separate files

Reason:
Jaime is doing this in separate steps at https://phabricator.wikimedia.org/T150850 3 files are already split out, also the whole role/mariadb.pp file has moved to role/manifests/mariadb/

https://gerrit.wikimedia.org/r/315343