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".

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.

(@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.

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:

Could you point out the patch that failed?

In T119042#2017204, @scfc wrote:

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

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

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

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.

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...

@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

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

modulepath = /etc/puppet/private/modules:/etc/puppet/modules:/etc/puppet/role

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*.

mediawiki roles moved https://gerrit.wikimedia.org/r/#/c/256574/

ci roles moved https://gerrit.wikimedia.org/r/#/c/260939/

maps roles moved https://gerrit.wikimedia.org/r/#/c/249059/

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

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

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

In T119042#2521941, @faidon wrote:

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

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

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