Closed
Bug 1146324
Opened 9 years ago
Closed 9 years ago
Add nxlog support to windows machines with puppet
Categories
(Infrastructure & Operations :: RelOps: Puppet, task)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: arich, Assigned: grenade)
References
Details
(Whiteboard: [windows])
Attachments
(1 file, 4 obsolete files)
8.26 KB,
patch
|
arich
:
review+
|
Details | Diff | Splinter Review |
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/
Reporter | ||
Comment 1•9 years ago
|
||
Similar to what we're doing for bug 1097374
Assignee: relops → rthijssen
Updated•9 years ago
|
Blocks: t-xp32-ix-006
Reporter | ||
Updated•9 years ago
|
Whiteboard: [windows]
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.
Attachment #8623704 -
Flags: review?(mcornmesser)
Attachment #8623704 -
Flags: review?(dustin)
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
Attachment #8623704 -
Flags: review?(dustin) → feedback+
Updated•9 years ago
|
Attachment #8623704 -
Flags: review?(mcornmesser)
Assignee | ||
Comment 4•9 years ago
|
||
Updated patch with suggestions applied.
Attachment #8623704 -
Attachment is obsolete: true
Attachment #8623754 -
Flags: review?(dustin)
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/ ~~~~~~~~~~~~~
Attachment #8623754 -
Flags: review?(dustin) → review+
Comment 6•9 years ago
|
||
darn whitespace
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
Comment 8•9 years ago
|
||
http://hg.mozilla.org/build/puppet/rev/1c6328749f10 http://hg.mozilla.org/build/puppet/rev/47c8d8d20470
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.
Attachment #8623754 -
Attachment is obsolete: true
Attachment #8624737 -
Flags: review?(dustin)
Attachment #8624737 -
Flags: review?(arich)
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?
Attachment #8624737 -
Flags: review?(dustin)
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.
Attachment #8624737 -
Flags: review?(arich) → review-
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
Attachment #8624737 -
Attachment is obsolete: true
Attachment #8626092 -
Flags: review?(dustin)
Attachment #8626092 -
Flags: feedback?(arich)
Assignee | ||
Updated•9 years ago
|
Attachment #8626092 -
Flags: review?(dustin) → review?(jwatkins)
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,
Reporter | ||
Updated•9 years ago
|
Attachment #8626092 -
Flags: feedback?(arich) → feedback+
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
Attachment #8626092 -
Flags: review?(jwatkins) → review+
Assignee | ||
Comment 15•9 years ago
|
||
Suggestions implemented. https://bitbucket.org/grenade/mozilla-puppet-again/commits/bce48ab5c3d0cacaed660fba6b3f4e873e2610e8
Attachment #8626092 -
Attachment is obsolete: true
Attachment #8626329 -
Flags: review?(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!
Attachment #8626329 -
Flags: review?(arich) → review+
Reporter | ||
Comment 17•9 years ago
|
||
remote: https://hg.mozilla.org/build/puppet/rev/c328e7b63312 remote: https://hg.mozilla.org/build/puppet/rev/3f33e6834846
Assignee | ||
Comment 18•9 years ago
|
||
these commits conclude the fun and games
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•