Tails

#16 Updated by bertagaz 2016-03-21 09:59:39

bertagaz wrote:
> * all systems memory usage is fine.

There’s no memory plugin shipped with Icinga2 or the Nagios plugins. There’s a check_load one though. I found one on the web that can check for system memory: https://github.com/justintime/nagios-plugins. Do you think we should use this one? Or shall we stick to the check_load one, even if that’s not really what we drafted?

#18 Updated by bertagaz 2016-03-23 14:10:02

intrigeri wrote:
> We need to monitor disk usage (bytes) and disk usage (inodes). We need to be notified if one of those goes wrong. If what you implemented already satisfies these requirements already, then fine. Or do we need to clarify these requirements?

Nop, forget about it, it was blurry to me how the check_disk plugin works, hence my not-so-clear question. But in the end, it seems the plugins does what we want: check disks bytes and inodes and report if one of this value is out of bound (that last point was the one blurry to me).

> I see a crapload of memory plugins so I suggest you ask our many friends who use Nagios/Icinga which one they prefer and use.

TBH, only 3 or 4 of them are interesting in our case. I’ve send an email to get input from our friends, we’ll see. I suspect from their puppet manifest they don’t check the memory, or use a check_mk feature to do that.

> > Or shall we stick to the check_load one, even if that’s not really what we drafted?
>
> If you think that we actually don’t need to monitor memory usage, please argue about it (monitoring system load has very little to do with that AFAICT; sorry if I didn’t guess right what a plugin called check_load does).

That was just to be sure how flexible the blueprint was on this kind of items.

#19 Updated by bertagaz 2016-03-23 14:11:31

bertagaz wrote:
> * all systems disk usage is fine (bytes and inodes).

Ok, added a check for the basic partitions of every systems (/boot and /), as well as checks for custom partitions dedicated to services.

#20 Updated by bertagaz 2016-03-23 14:17:51

Also, in commit puppet-tails:2b0e864 I’ve added a bunch of regexp which I hope do not stink too much.

#21 Updated by intrigeri 2016-03-25 18:33:03

> Also, in commit puppet-tails:2b0e864 I’ve added a bunch of regexp which I hope do not stink too much.

OK, so I had a quick look:

• $partitions: it seems overly complicated, using capturing parenthesis in places where I doubt we need even grouping parenthesis at all, escaping chars that I doubt need to be escaped, and generally it’s not clear to me what you are trying to do here, so I can’t review this; I could decide that you’re probably right and look elsewhere, but given what I know about your regexp skills, I’m not sure that the benefit potentially brought by this regexp is worth its current complexity; if we’re going to keep a regexp here, it might be better if I look closer. So I see two options:
  • document what the regexp should, and should not, match, so I can review it (and I’ll be happy to improve it if you want)
  • drop the regexp and just validate_string
• $wfree and friends, i.e. ^(\d{1,3}%|\d+)$: I think you can drop the parenthesis, which would be less confusing (parenthesis actually do something, so a reader like me wastes time trying to understand why you added them, because if they were useless you would not add them, presumably; see? :)
• $name: this seems to be duplicated from a similar regexp found in tails::monitoring::service, and at first glance it shares the same bugs (that we’ve been discussing on another ticket)

#22 Updated by intrigeri 2016-03-25 18:35:37

> That was just to be sure how flexible the blueprint was on this kind of items.

IMO the blueprint is flexible in the sense that if it has useless or incorrect stuff, we can drop or correct it. Replacing checks we initially thought were needed (and still seem pretty important to me), with mostly unrelated ones, needs to be justified. I hope we’re now on the same page :)

#23 Updated by bertagaz 2016-03-31 14:20:54

intrigeri wrote:
>
> * $partitions: it seems overly complicated, using capturing parenthesis in places where I doubt we need even grouping parenthesis at all, escaping chars that I doubt need to be escaped, and generally it’s not clear to me what you are trying to do here, so I can’t review this; I could decide that you’re probably right and look elsewhere, but given what I know about your regexp skills, I’m not sure that the benefit potentially brought by this regexp is worth its current complexity; if we’re going to keep a regexp here, it might be better if I look closer. So I see two options:
> document what the regexp should, and should not, match, so I can review it (and I’ll be happy to improve it if you want)
> drop the regexp and just validate_string

In the end I dropped this regexp, as this parameter is already documented. You could have guessed what I’ve tried to capture by reading it btw.

> * $wfree and friends, i.e. ^(\d{1,3}%|\d+)$: I think you can drop the parenthesis, which would be less confusing (parenthesis actually do something, so a reader like me wastes time trying to understand why you added them, because if they were useless you would not add them, presumably; see? :)

Done, commit puppet-tails:9007871

> * $name: this seems to be duplicated from a similar regexp found in tails::monitoring::service, and at first glance it shares the same bugs (that we’ve been discussing on another ticket)

Yep, fixed too.

#24 Updated by bertagaz 2016-03-31 14:23:01

intrigeri wrote:
> IMO the blueprint is flexible in the sense that if it has useless or incorrect stuff, we can drop or correct it. Replacing checks we initially thought were needed (and still seem pretty important to me), with mostly unrelated ones, needs to be justified. I hope we’re now on the same page :)

We are! :)

#28 Updated by intrigeri 2016-04-13 03:48:45

• https://icingaweb2.tails.boum.org/monitoring/service/show?host=apt.lizard&service=apt-snapshots-disk should allow for a larger disk usage (90% maybe?) before warning
• https://icingaweb2.tails.boum.org/monitoring/service/show?host=apt-proxy.lizard&service=apt-cacher-ng-disk ← same

#29 Updated by intrigeri 2016-04-13 04:21:41

FWIW https://icingaweb2.tails.boum.org/monitoring/service/history?host=ecours.tails.boum.org&service=4mvq3pnvid3awjln.onion seems to reliably fail once out of three attempts. I don’t know how this will translate into feedback to sysadmins (dashboard/notifications), in a way that allows us to distinguish “service is unstable” from “oh it’s normal that the check fails once out of three times”; time will tell (I don’t think the feedback part was submitted for review yet so I’ll wait). In passing, it seems a bit intense to query that server every 30 seconds, no?

#35 Updated by bertagaz 2016-04-26 09:28:43

intrigeri wrote:
> https://icingaweb2.tails.boum.org/monitoring/list/hosts doesn’t list all our VMs (at least puppet-git.lizard is missing), so it looks like the checks that are about “all systems” are not set up completely yet. Please double-check that everything required has been implemented and then I’ll happpily review it :)

Right, I didn’t activate the agents on the other VMs yet. Will do soon.

> Regarding the WhisperBack check (and any other fragile one, really): either consider it’s part of this ticket, and then make sure it satisfies our needs before your next request for review; or, file a dedicated subtask of Feature #8652 about it. Your call.

Ack, will report about it on this ticket then. Note that I previously reported some bits on Feature #8652 already.

> Regarding the apt-cacher-ng-disk check: the fact that we’re back to a usage level that doesn’t show up on icinga2 right now doesn’t resolve the problem I’ve raised. I doubt we can afford ensuring that a fifth of each of our large data filesystems is constantly free, so I still am in favour of monitoring settings that don’t force us to waste this much disk space.

Ok, updated it in commit b8c273f in our main repo.

> Regarding the apt-cacher-ng-disk check: sounds good, let’s try it out this way!

That’s about the apt-snapshots-disk check here I guessed. If so, fine with me.

#36 Updated by bertagaz 2016-04-26 13:26:01

bertagaz wrote:
> Right, I didn’t activate the agents on the other VMs yet. Will do soon.

Added the agent on every remaining VMs, their checks are slowly getting reported. Only the isobuilders one are broken, because their Puppet manifest doesn’t apply cleanly for other reasons (ikiwiki apt_preferences related problems). I’ll investigate tomorrow.

#37 Updated by bertagaz 2016-04-27 02:54:03

bertagaz wrote:
> Only the isobuilders one are broken, because their Puppet manifest doesn’t apply cleanly for other reasons (ikiwiki apt_preferences related problems). I’ll investigate tomorrow.

Fixed. The isobuilders checks are added slowly.

#39 Updated by intrigeri 2016-04-28 12:51:01

>> Regarding the apt-cacher-ng-disk check: […]

> Ok, updated it in commit b8c273f in our main repo.

Looks good, thank you.

>> Regarding the apt-cacher-ng-disk check: sounds good, let’s try it out this way!

> That’s about the apt-snapshots-disk check here I guessed. If so, fine with me.

Yes.

#40 Updated by intrigeri 2016-04-28 12:57:45

> Fixed. The isobuilders checks are added slowly.

Good. Only problems I see:

• https://icingaweb2.tails.boum.org/monitoring/service/show?host=puppet-git.lizard&service=iso-history-gitolite-disk seems to still be broken
• same for https://icingaweb2.tails.boum.org/monitoring/service/show?host=puppet-git.lizard&service=torbrowser-gitolite-disk — I think it’s fine to set more relaxed permissions on the root directory of this FS, as long as the sensitive subdirs (e.g. the .git one, if any) are protected
• system_disks check on puppet-git that I’ll reply to separately

#42 Updated by intrigeri 2016-04-28 14:07:56

>> So I guess it’s time I do a code review of all these checks, right?

> Yep, it is!

Here we go! I have to first say this is high quality work, on which I don’t have much to say except some (mostly style) nitpicking. Congrats :)

• globals.return_servicegroup_host is buggy if service.vars.real_host is true but service.host_name != service.vars.real_host is false: it won’t return anything. The if (service.host_name != service.vars.real_host) condition seems to be useless, and introduces this bug.
• files/monitoring/icinga2/plugins/check_ssh_or_sftp_account: I’m surprised you had to write a custom plugin for this. But the code is good, and simple enough, so well :)
• Please read the “Autorequires” section on https://docs.puppet.com/puppet/latest/reference/type.html#file, it’ll help you improve the signal/noise ratio of your Puppet code (you’re sometimes explicitly adding requires on the owner user, or on the parent directory, which as you’ll see is not needed; and you’re not doing it consistently, which is a bit troubling for crazy folks like me).
• “$real_host must resolve to the host where the service is running” ← do you really mean “resolve to”, as in DNS-speak, or just “be”?
• files/monitoring/icinga2/plugins/check_rsync doesn’t exactly respect current Perl best practices, but well, let’s say it’s good enough.
• files/monitoring/icinga2/plugins/check_mem.pl could use libraries that are kept up-to-date instead of reinventing wheels, but well, let’s say it’s good enough.
• in files/monitoring/icinga2/plugins/check_torbrowser_archive, in the get_url function: I’m aware that generally bare exceptions are Bad™, so on the one hand I’m glad you handle only the exceptions you expect, but in this specific case I wonder what happens if we ever hit a type of exception that’s not handled. Perhaps we should catch all exceptions here? Otherwise, good job, you’re making progress at programming these days, it’s a pleasure for me :)
• Nitpicking: “Url” (typo for “URL”) is now called “URI” these days, I think. Ignore that if you want for already written code, but please take it into account in the future ;)
• s/unix/Unix/
• s/Host name/Hostname/
• s/IP Address/IP address/
• Grammar mistake in “that will be checks” (checked)
• “Take care to use a uniq $name” ← please clarify within what set it must be unique (and s/uniq/unique/).

#43 Updated by bertagaz 2016-04-29 02:34:48

intrigeri wrote:
> Here we go! I have to first say this is high quality work, on which I don’t have much to say except some (mostly style) nitpicking. Congrats :)

Glad you like it.

> * globals.return_servicegroup_host is buggy if service.vars.real_host is true but service.host_name != service.vars.real_host is false: it won’t return anything. The if (service.host_name != service.vars.real_host) condition seems to be useless, and introduces this bug.

Hmmm, in the case you describe, in fact I believe it will return service.hostname. But you’re right, this condition is useless, it’s there only because I thought to add vars.real_host in every service check configuration, but chose a different path in the end. Fixed in puppet-tails:35664d4

> * files/monitoring/icinga2/plugins/check_ssh_or_sftp_account: I’m surprised you had to write a custom plugin for this. But the code is good, and simple enough, so well :)

There’s plugin to check if a SSH server is running, but it’s not able to do more than that and try to connect to a specific user account.

> * Please read the “Autorequires” section on https://docs.puppet.com/puppet/latest/reference/type.html#file, it’ll help you improve the signal/noise ratio of your Puppet code (you’re sometimes explicitly adding requires on the owner user, or on the parent directory, which as you’ll see is not needed; and you’re not doing it consistently, which is a bit troubling for crazy folks like me).

Right, this is still a bit blurry to me what this autorequire feature does actually require. :)

Commit puppet-tails:e2f57ee

> * “$real_host must resolve to the host where the service is running” ← do you really mean “resolve to”, as in DNS-speak, or just “be”?

Be, commit puppet-tails:b34e35e

> * files/monitoring/icinga2/plugins/check_rsync doesn’t exactly respect current Perl best practices, but well, let’s say it’s good enough.

That’s the most relevant plugin I found, but it’s a bit outdated.

> * files/monitoring/icinga2/plugins/check_mem.pl could use libraries that are kept up-to-date instead of reinventing wheels, but well, let’s say it’s good enough.

Same, often plugins are of quite bad quality in term of coding practices.

> * in files/monitoring/icinga2/plugins/check_torbrowser_archive, in the get_url function: I’m aware that generally bare exceptions are Bad™, so on the one hand I’m glad you handle only the exceptions you expect, but in this specific case I wonder what happens if we ever hit a type of exception that’s not handled. Perhaps we should catch all exceptions here? Otherwise, good job, you’re making progress at programming these days, it’s a pleasure for me :)

True, I did it in the other custom plugin, I should have done it same in this one. Fixed in commit puppet-tails:1e656d9

> * Nitpicking: “Url” (typo for “URL”) is now called “URI” these days, I think. Ignore that if you want for already written code, but please take it into account in the future ;)

Right. Old school here. :)

Note that the urllib python module still talk about URLs.

> * s/unix/Unix/
> * s/Host name/Hostname/
> * s/IP Address/IP address/

Thanks for this good catch. commit puppet-tails:327a0e0

> * Grammar mistake in “that will be checks” (checked)

Oops, commit puppet-tails:0da5d7c

> * “Take care to use a uniq $name” ← please clarify within what set it must be unique (and s/uniq/unique/).

commit puppet-tails:3741247

#44 Updated by bertagaz 2016-04-29 03:05:02

intrigeri wrote:
> * https://icingaweb2.tails.boum.org/monitoring/service/show?host=puppet-git.lizard&service=iso-history-gitolite-disk seems to still be broken
> * same for https://icingaweb2.tails.boum.org/monitoring/service/show?host=puppet-git.lizard&service=torbrowser-gitolite-disk — I think it’s fine to set more relaxed permissions on the root directory of this FS, as long as the sensitive subdirs (e.g. the .git one, if any) are protected

Yeah, I think my workaround didn’t change anything, the nagios user probably don’t have enough access to the block device too. Will try the other permission change workaround.

#45 Updated by bertagaz 2016-04-29 08:22:59

bertagaz wrote:
> intrigeri wrote:
> > * https://icingaweb2.tails.boum.org/monitoring/service/show?host=puppet-git.lizard&service=iso-history-gitolite-disk seems to still be broken
> > * same for https://icingaweb2.tails.boum.org/monitoring/service/show?host=puppet-git.lizard&service=torbrowser-gitolite-disk — I think it’s fine to set more relaxed permissions on the root directory of this FS, as long as the sensitive subdirs (e.g. the .git one, if any) are protected
>
> Yeah, I think my workaround didn’t change anything, the nagios user probably don’t have enough access to the block device too. Will try the other permission change workaround.

I did commit puppet-tails:35667c9 and puppet-tails:b579632, and it seems to solve the problem as the torbrowser-gitolite-disk check state change shows.

#46 Updated by bertagaz 2016-04-29 08:27:37

intrigeri wrote:
> I must say I’m a little bit surprised, as I’ve never seen any hidden service I’m regularly using perform this poorly. So I’m curious: where does “what our users sometimes do” come from, exactly?

From my own experience, when I tried to send a bug report, and as Bug #11136 seems to indicate it sometimes happen.

But you were right, as discussed on IRC it seems the Tor running on whisperback.li was the Wheezy version, while our system is Jessie, so it was crashing every few minutes because it was compiled with older openssl and libevent versions than the one installed on the system. We’ll see how it behaves now that I’ve upgraded it to the Jessie version, and that it doesn’t restart so often.

#48 Updated by intrigeri 2016-04-29 12:39:56

> Note that the urllib python module still talk about URLs.

Yes, and I do to in many other places :)

Wrt. all other changes you mention in the comment I’m replying to: great!

#49 Updated by intrigeri 2016-04-29 12:44:28

> I did commit puppet-tails:35667c9 and puppet-tails:b579632, and it seems to solve the problem as the torbrowser-gitolite-disk check state change shows.

Assuming you’re talking of these commits in our manifests repo (and not in the puppet-tails one): that’s a pretty big hammer you used there, and I’m worried this will grant the nagios more permissions than what we would like to, now or later. I would really prefer making the root directory of each of those filesystems o+r (and the parent directories o+x). What do you think?

#50 Updated by intrigeri 2016-04-29 12:48:25

> But you were right, as discussed on IRC it seems the Tor running on whisperback.li was the Wheezy version, while our system is Jessie, so it was crashing every few minutes because it was compiled with older openssl and libevent versions than the one installed on the system. We’ll see how it behaves now that I’ve upgraded it to the Jessie version, and that it doesn’t restart so often.

Indeed. Please reassign to me once you’ve confirmed it improves things to a point when it seems production-ready (or delegate to a subtask of Feature #8652).

#52 Updated by bertagaz 2016-04-30 04:59:42

intrigeri wrote:
> > But you were right, as discussed on IRC it seems the Tor running on whisperback.li was the Wheezy version, while our system is Jessie, so it was crashing every few minutes because it was compiled with older openssl and libevent versions than the one installed on the system. We’ll see how it behaves now that I’ve upgraded it to the Jessie version, and that it doesn’t restart so often.
>
> Indeed. Please reassign to me once you’ve confirmed it improves things to a point when it seems production-ready (or delegate to a subtask of Feature #8652).

This check didn’t fail since the Tor upgrade on whisperback.li. Given its failure rate before, it should already have failed. So it really was a bug in the service, not in the check, and it seems to be fixed now. \o/