#709 Remove anchors (and create_resources)

[alvagante]

Note: an anchor in init.pp has been temporarily left in place.
It has a purpose and might not trivial to replace.
Could remain in code base as far as I'm concerned.

[ghoneycutt]

This is a good first pass. Eventually I'd like to not use subclasses that do not provide value on their own, such as ::package and ::service and instead have those entries in the main sensu class.

[ghoneycutt]

syntax error here that is causing Travis to fail

[alvagante]

Now fixed

[ghoneycutt]

I'm a bit puzzled why the spec tests are failing.

[alvagante]

Travis errors were due to a mistake i was doing, forgetting to add the defaults params in the created resources. I'd not change/remove whole classes here, to avoid too many more or less radical changes all in one place. Eventually stuff for another ticket.
I wonder what's the actual use case for these hasrestart parameters ( I simplified a bit the passing of this parameter among classes): if service has restart or not should be a particularity of the implementation on the underlying OS, not something users may want to define.
Maybe is intended as a parameter to define if restart services automatically at config file changes, but if this was the intention, the implementation doesn't solve it.

[alvagante]

Squashed works on this on a single commit for possible merging, as these are some outstanding choices which might need further works:

• IF to keep the hasrestart parameter
• If to keep the anchor in site.pp (I'd say is a good sample of using anchors without the anchor patterns)
• What actually do with "Private classes": 1) Simply remove the check - 2) Remove those classes and move their contents in parent classes - 3) Something else?

[alvagante]

@ghoneycutt some input on the 3 above points is needed to finish this

My2c on them

• remove hasrestart parameter (unless there's a good logical reason for that ) This would imply a semver major version change, and it's just for style, so actually not a priority.
• I'd keep the remaining anchor in site.pp
• On private classes I'd just remove the check and eventually include the main class at the beginning (if this doesn't create circular dependencies)

[ghoneycutt]

I don't think we need hasrestart because the services do have the ability to restart them. Please confirm through Vagrant. If we have a service whose init file does not support restart, let's note it and file a bug against Sensu.

I'm OK with the single anchor.

The private subclasses should be moved into their parent classes.

[alvagante]

@ghoneycutt I've give a look at the number of private classes: more than I expected:

manifests/api/config.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/api/service.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/client/config.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/client/service.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/enterprise/dashboard/config.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/enterprise/dashboard/package.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/enterprise/dashboard/service.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/enterprise/package.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/enterprise/service.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/package.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/rabbitmq/config.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/redis/config.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/repo/apt.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/repo/yum.pp:    fail("Use of private class ${name} by ${caller_module_name}")
manifests/server/service.pp:    fail("Use of private class ${name} by ${caller_module_name}")

I'm not so sure it's a good idea to move their content ( i expect not few resources ) to the parent classes, considering the amount of tests that probably have to be changed, resources to be added to existing classes and management of dependencies.
I suppose is a matter of evaluating effort vs benefit here.

So I prefer a second confirmation (sorry :-) before starting the activity.

[ghoneycutt]

Please proceed, as we want to ditch the private classes and caller_module. This should be mostly cut/paste of resources and spec tests into other files.

[alvagante]

So, removed the checks for private classes, but not merged all of them in parent classes.

I just consolidated the sensu::enterprise::dashboard class for the moment.
I tried to be as conservative as possible while moving the code.

Consolidation of other classes on api, client e enterprise might be more impactful on tests (there were no tests for sensu::enterprise::dashboard subclasses). In any case for these cases a new class, to replace the existing ones has to be created: (ie manifests/api.pp to replace manifests/api/config.pp and manifests/api/service.pp).

has_restart is still there. Removing it would be backwards incompatible, so it would require a major version bump, if semver is followed. It doesn't harm but at the same time I wonder who needs it.

I'd plan its removal when a major version bump is done.

[ghoneycutt]

Please continue with the consolidation of classes. Please keep has_restart there for backward compatibility if at all possible. The spec tests have all the subclasses already consolidated, so they should not be affected much.

[alvagante]

@ghoneycutt consolidated api, client and enterprise classes.
There are few others which were private classes (now no more) but can't be merged into a single class (except eventually the repo ones):

manifests/package.pp
manifests/rabbitmq/config.pp
manifests/redis/config.pp 
manifests/repo/apt.pp 
manifests/repo/yum.pp 
manifests/server/service.pp 

I'd avoid to create giant classes just for the sake or reducing their numbers, considering also all the dependencies workarounds that have to be added.

So for me, this is ready to merge.

[ghoneycutt]

Hi @alvagante could you please rebase so this can be merged.

[alvagante]

@ghoneycutt rebased with pain and sweat, but should be ok.
Had to resolve some conflicts with @jeffmccune works
Given the impact on various files, a four/six eyes double check, especially on refactored relationships, would be welcomed.

Resolves: #763

Addressed

[ghoneycutt]

Tried to bring up the sensu-server in vagrant and getting errors

==> /var/log/sensu/sensu-api.log <==
{"timestamp":"2017-08-24T02:17:35.191064+0000","level":"warn","message":"config file applied changes","file":"/etc/sensu/conf.d/api.json","changes":{"api":[null,{"port":4567,"host":"127.0.0.1","bind":"0.0.0.0","user":"admin","password":"REDACTED"}]}}
{"timestamp":"2017-08-24T02:17:35.191078+0000","level":"warn","message":"loading config file","file":"/etc/sensu/conf.d/redis.json"}
{"timestamp":"2017-08-24T02:17:35.191096+0000","level":"warn","message":"config file applied changes","file":"/etc/sensu/conf.d/redis.json","changes":{"redis":[null,{"port":6379,"host":"127.0.0.1","reconnect_on_error":true,"db":0,"auto_reconnect":true}]}}
{"timestamp":"2017-08-24T02:17:35.191110+0000","level":"warn","message":"loading config file","file":"/etc/sensu/conf.d/client.json"}
{"timestamp":"2017-08-24T02:17:35.191136+0000","level":"warn","message":"config file applied changes","file":"/etc/sensu/conf.d/client.json","changes":{"client":{"address":[null,"192.168.56.10"],"deregister":[null,true],"deregistration":[null,{"handler":"deregister_client"}],"keepalive":[null,{}],"name":[null,"sensu-server.example.com"],"safe_mode":[null,false],"socket":[null,{"bind":"127.0.0.1","port":3030}],"subscriptions":[null,["all","roundrobin:poller"]]}}}
{"timestamp":"2017-08-24T02:17:35.191170+0000","level":"fatal","message":"check handlers must be an array","object":{"command":"PATH=$PATH:/usr/lib64/nagios/plugins check_ntp_time -H pool.ntp.org -w 30 -c 60","handlers":"default","interval":60,"standalone":true,"subscribers":"sensu-test","name":"check_ntp"}}
{"timestamp":"2017-08-24T02:17:35.191192+0000","level":"fatal","message":"check subscribers must be an array","object":{"command":"PATH=$PATH:/usr/lib64/nagios/plugins check_ntp_time -H :::address::: -w 30 -c 60","cron":"*/5 * * * *","handlers":"default","proxy_requests":{"client_attributes":{"subscriptions":"eval: value.include?(\"ntp\")"}},"standalone":false,"subscribers":"roundrobin:poller","name":"remote_check_ntp"}}
{"timestamp":"2017-08-24T02:17:35.191212+0000","level":"fatal","message":"check handlers must be an array","object":{"command":"PATH=$PATH:/usr/lib64/nagios/plugins check_ntp_time -H :::address::: -w 30 -c 60","cron":"*/5 * * * *","handlers":"default","proxy_requests":{"client_attributes":{"subscriptions":"eval: value.include?(\"ntp\")"}},"standalone":false,"subscribers":"roundrobin:poller","name":"remote_check_ntp"}}
{"timestamp":"2017-08-24T02:17:35.191242+0000","level":"fatal","message":"check subscribers must be an array","object":{"command":"/opt/sensu/embedded/bin/check-http.rb -u http://:::address:::","high_flap_threshold":60,"interval":300,"low_flap_threshold":20,"occurrences":2,"proxy_requests":{"client_attributes":{"subscriptions":"eval: value.include?(\"http\")"}},"refresh":600,"standalone":false,"subscribers":"roundrobin:poller","name":"remote_http"}}
{"timestamp":"2017-08-24T02:17:35.191261+0000","level":"fatal","message":"SENSU NOT RUNNING!"}

==> /var/log/uchiwa.log <==
{"timestamp":"2017-08-24T02:18:41.084590237Z","level":"info","message":"Updating the datacenter Site1"}
{"timestamp":"2017-08-24T02:18:41.085027048Z","level":"warn","message":"GET http://127.0.0.1:4567/stashes returned: dial tcp 127.0.0.1:4567: getsockopt: connection refused"}
{"timestamp":"2017-08-24T02:18:41.085054022Z","level":"warn","message":"Connection failed to the datacenter Site1"}

==> /var/log/messages <==
Aug 24 02:19:36 localhost systemd: sensu-api.service: main process exited, code=exited, status=2/INVALIDARGUMENT
Aug 24 02:19:36 localhost systemd: Unit sensu-api.service entered failed state.
Aug 24 02:19:36 localhost systemd: sensu-api.service failed.

[alvagante]

@ghoneycutt made some tests, it looks like we have the same issue with current master

[alvagante]

@ghoneycutt I think the error is due to how checks files are created:
if a single check is added in a conf.d/checks/ file then the json file should begin with:

{
    "check": {

and not

{
    "checks": {

manually changing the files in conf.d/checks/ on the Vagrant sensu-server vm allows the api service to start.

@jeffmccune FYI

[ghoneycutt]

@agoddard This seems like a sensu bug if you cannot specify checks with only a single check. The docs lead you to believe that the plural form is correct.

[ghoneycutt]

Seems this issue is not related to this PR, so merging this.

[ghoneycutt]

Released in v2.33.1