Amy Rich [:arr] [:arich] Reporter
	Description • 9 years ago

There are a few nxlog modules out there for puppet, and we can probably combine a few of these and make some modifications to have a solid module we can use internally and contribute back to puppetforge (I didn't see any there).


https://github.com/Intelliplan/puppet-nxlog/
https://github.com/opentable/puppet-nxlog/
https://github.com/theyosef/puppet-nxlog/

	Amy Rich [:arr] [:arich] Reporter
	Comment 1 • 9 years ago

Similar to what we're doing for bug 1097374

	Rob Thijssen [:grenade (EET/UTC+0300)] Assignee
	Comment 2 • 9 years ago

Here's my first attempt at the windows nxlog module/package installer. Please let me know what you think.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 3 • 9 years ago

Comment on attachment 8623704 [details] [diff] [review]
nxlog-module-windows.diff

Review of attachment 8623704 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good -- just a bit of refactoring to reduce duplicated code.

::: modules/nxlog/manifests/conf.pp
@@ +10,5 @@
> +            case $::hardwaremodel {
> +                # (facter should return 'x64' -- https://github.com/puppetlabs/facter/blob/2.x/lib/facter/hardwaremodel.rb#L50)
> +                'x64': {
> +                    file {
> +                        'C:/Program Files (x86)/nxlog/conf/nxlog.conf':

You could move this into a setting and determine it once:

class nxlog::settings {
    $root_dir = $::hardwaremodel ? {
        x64     => 'C:/Program Files (x86)/nxlog',
        default => 'C:/Program Files/nxlog',
    }
}

(also, isn't that backward? x64 corresponds to Program Files?)

then substitute that value into paths:

  file {
    "${nxlog::settings::root_dir}/conf/nxlog.conf":
        content => template("nxlog/nxlog.conf")
  }

and into the template:

define ROOT <%= scope.lookup('nxlog::settings::root_dir') %>

at which point you only need one template.  You could do a similar thing for the service in nxlog::svc.

@@ +12,5 @@
> +                'x64': {
> +                    file {
> +                        'C:/Program Files (x86)/nxlog/conf/nxlog.conf':
> +                            require => Class [ 'packages::nxlog' ],
> +                            replace => true,        

nit: watch for trailing spaces on lines.  Bugzilla makes them obvious in the splinter review tool

	Rob Thijssen [:grenade (EET/UTC+0300)] Assignee
	Comment 4 • 9 years ago

Updated patch with suggestions applied.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 5 • 9 years ago

Comment on attachment 8623754 [details] [diff] [review]
nxlog-module-windows.diff

Review of attachment 8623754 [details] [diff] [review]:
-----------------------------------------------------------------

_~
    _~ )_)_~
    )_))_))_)
    _!__!__!_         it.
    \______t/
  ~~~~~~~~~~~~~

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 6 • 9 years ago

darn whitespace

	Rob Thijssen [:grenade (EET/UTC+0300)] Assignee
	Comment 7 • 9 years ago

Thanks Dustin, I found the whitespace and removed it. When you have time, can you commit this change for me? I don't yet have commit rights on hg.mozilla.org. The change is on BB: https://bitbucket.org/grenade/mozilla-puppet-again/commits/1c6328749f10f624b70426bc991c972712260635?at=default

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 8 • 9 years ago

http://hg.mozilla.org/build/puppet/rev/1c6328749f10
http://hg.mozilla.org/build/puppet/rev/47c8d8d20470

	Rob Thijssen [:grenade (EET/UTC+0300)] Assignee
	Comment 9 • 9 years ago

Commit is on BB at: https://bitbucket.org/grenade/mozilla-puppet-again/commits/ff196b47962eb5e1e7a29f33f16b1aeb822f4ec6

The change includes nxlog installer as the Windows config for log_aggragator::client, removes the windows exclusion for the same and patches some syntax errors in the nxlog config template.

If appropriate, please merge to hg.mozilla.org, where I have no commit access.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 10 • 9 years ago

Comment on attachment 8624737 [details] [diff] [review]
trigger from log_aggragator::client

Review of attachment 8624737 [details] [diff] [review]:
-----------------------------------------------------------------

I like what you're saying about log_aggregator just indicating it needs nxlog installed.  However, the nxlog module has log_aggregator-specific stuff in it as it stands (@log_aggregator and @logging_port).  If you want to keep that split, you'd need to do something where that configuration comes from the log_aggregator module, and hypothetically some other log_printer module could provide its own output module.  The rsyslog module is set up that way (rsyslog::config), but we actually use rsyslog in a variety of different ways throughout the infra, so that's necessary.  For nxlog, I suspect this will be the only way we ever use it.

Bottom line - I'd suggest just merging the nxlog install and conf file into the log_aggregator module. 

Incidentally, I see @log_aggregator and @logging_port in modules/nxlog/templates/nxlog.conf.erb but I don't see them in modules/nxlog/manifests/conf.pp which is what calls template(..).  Are those variables actually getting filled in correctly?

	Amy Rich [:arr] [:arich] Reporter
	Comment 11 • 9 years ago

Comment on attachment 8624737 [details] [diff] [review]
trigger from log_aggragator::client

Review of attachment 8624737 [details] [diff] [review]:
-----------------------------------------------------------------

Dustin has already made the points we were discussing on irc.

	Rob Thijssen [:grenade (EET/UTC+0300)] Assignee
	Comment 12 • 9 years ago

New patch handles aggragetor specific nxlog config from the log_aggregator client module. Please comment or merge if appropriate.
https://bitbucket.org/grenade/mozilla-puppet-again/commits/7af7800416d82b362cbdf09ebd7be3604d79624f

	Amy Rich [:arr] [:arich] Reporter
	Comment 13 • 9 years ago

Since we can split out into multiple config files, I think this looks like a reasonable solution (having the base package install a config that doesn't do anything).

You don't need the following, since we don't have any secrets that we're trying to keep in the config file:

show_diff => false,

You combine all the multiple file blocks into one (see modules/vnc/manifests/init.pp for an example of that)


This is the default, so not necessary:

replace => true,

	Jake Watkins [:dividehex]
	Comment 14 • 9 years ago

Comment on attachment 8626092 [details] [diff] [review]
trigger nxlog install from parent log_aggregator::client

Review of attachment 8626092 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good.  Just pull out the fluff

::: modules/log_aggregator/manifests/client.pp
@@ +38,5 @@
> +                include nxlog::settings
> +                file {
> +                    "${nxlog::settings::root_dir}/conf/nxlog_source_eventlog.conf":
> +                        require => Class [ 'packages::nxlog' ],
> +                        replace => true,

You can remove replace since it is true by default

@@ +39,5 @@
> +                file {
> +                    "${nxlog::settings::root_dir}/conf/nxlog_source_eventlog.conf":
> +                        require => Class [ 'packages::nxlog' ],
> +                        replace => true,
> +                        show_diff => false,

Show diff is only used when hiding secrets

	Rob Thijssen [:grenade (EET/UTC+0300)] Assignee
	Comment 15 • 9 years ago

Suggestions implemented.
https://bitbucket.org/grenade/mozilla-puppet-again/commits/bce48ab5c3d0cacaed660fba6b3f4e873e2610e8

	Amy Rich [:arr] [:arich] Reporter
	Comment 16 • 9 years ago

Comment on attachment 8626329 [details] [diff] [review]
trigger nxlog install from parent log_aggregator::client

Review of attachment 8626329 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

	Amy Rich [:arr] [:arich] Reporter
	Comment 17 • 9 years ago

remote:   https://hg.mozilla.org/build/puppet/rev/c328e7b63312
remote:   https://hg.mozilla.org/build/puppet/rev/3f33e6834846

	Rob Thijssen [:grenade (EET/UTC+0300)] Assignee
	Comment 18 • 9 years ago

these commits conclude the fun and games