New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for up/down detection of ping only devices. #7323

Merged
merged 31 commits into from Oct 28, 2017

Conversation

Projects
None yet
6 participants
@Zmegolaz
Member

Zmegolaz commented Sep 12, 2017

This allows the user to disable all SNMP checks and only ping a device. No extra service checks are needed and up/down status and alerting works as expected.
I have a few points I'd like to ask your opinion about:

  1. Should we add a "Disable SNMP" button to the "Add device" page too? snmp-scan.py (despite the name)?
  2. The OS is set to "Ping only" and the user can specify hardware. Should it be called something else? Should the user be able to specify more details?
  3. The disable SNMP setting is currently saved in a separate column and the code checks that value. However we could instead only set the OS of the device and use that field to decide if SNMP checks should be performed. It's not as clear and probably not as future proof, but it'd be a column less in the database.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926


This change is Reviewable

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 12, 2017

Member

Definitely a good starting point. I'll comment further when I've got more time.

Member

laf commented Sep 12, 2017

Definitely a good starting point. I'll comment further when I've got more time.

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Sep 12, 2017

Member

The Scrutinizer inspection mentions two bugs, a variable "does not seem to be defined for all execution paths leading up to this point", but it's wrong, that point can never be reached without the variable defined. It's very trivial to fix, but maybe a bit unnecessary. Up to you. :)

Member

Zmegolaz commented Sep 12, 2017

The Scrutinizer inspection mentions two bugs, a variable "does not seem to be defined for all execution paths leading up to this point", but it's wrong, that point can never be reached without the variable defined. It's very trivial to fix, but maybe a bit unnecessary. Up to you. :)

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Sep 13, 2017

Member

@Zmegolaz what if instead of a separate column, we just check that the OS is "ping"? Not sure if that is better or worse.

Also, some sort of icon would be nice :)

Member

murrant commented Sep 13, 2017

@Zmegolaz what if instead of a separate column, we just check that the OS is "ping"? Not sure if that is better or worse.

Also, some sort of icon would be nice :)

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Sep 13, 2017

Member

@murrant, Yeah, that's what I was thinking about in point 3. I can't make up my mind about what I think is the best way.

Yes! Icon. I'll get one.

Member

Zmegolaz commented Sep 13, 2017

@murrant, Yeah, that's what I was thinking about in point 3. I can't make up my mind about what I think is the best way.

Yes! Icon. I'll get one.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 13, 2017

Member

So my thought on this is that if a user adds a device which is only ICMP then we should at that stage allow them to select the OS it is - we will at that stage do nothing more with it other than make use of the logo and type, description etc.

It does need a few more areas:

  • Adding new hosts via CLI and WebUI need the option to allow just ICMP only.
  • A lot of graphs are going to be attempted to be shown including a lot of the usual tabs. We should hide those in the best way we can but I'm not sure what that is right now.

I've not checked code still yet, I think we should discuss how this PR should go before doing that.

Member

laf commented Sep 13, 2017

So my thought on this is that if a user adds a device which is only ICMP then we should at that stage allow them to select the OS it is - we will at that stage do nothing more with it other than make use of the logo and type, description etc.

It does need a few more areas:

  • Adding new hosts via CLI and WebUI need the option to allow just ICMP only.
  • A lot of graphs are going to be attempted to be shown including a lot of the usual tabs. We should hide those in the best way we can but I'm not sure what that is right now.

I've not checked code still yet, I think we should discuss how this PR should go before doing that.

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Sep 13, 2017

Member
  1. Letting the user select OS can surely be done. A dropdown might be too long, but maybe an autocomplete field?
  2. I agree, adding a host with with just ICMP should be supported, in all ways it's possible to add devices.
  3. Actually, very few invalid graphs and tabs are attempted. The only ones we need to hide are the Poller Modules Performance graph and the Maps tab.
Member

Zmegolaz commented Sep 13, 2017

  1. Letting the user select OS can surely be done. A dropdown might be too long, but maybe an autocomplete field?
  2. I agree, adding a host with with just ICMP should be supported, in all ways it's possible to add devices.
  3. Actually, very few invalid graphs and tabs are attempted. The only ones we need to hide are the Poller Modules Performance graph and the Maps tab.
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 13, 2017

Member

Letting the user select OS can surely be done. A dropdown might be too long, but maybe an autocomplete field?

Yes an auto complete will be fine. We already have some code in the graph widget that does this and can be re-used.

Actually, very few invalid graphs and tabs are attempted. The only ones we need to hide are the Poller Modules Performance graph and the Maps tab.

The popups in the top right will need to be hidden. Not sure what else.

Member

laf commented Sep 13, 2017

Letting the user select OS can surely be done. A dropdown might be too long, but maybe an autocomplete field?

Yes an auto complete will be fine. We already have some code in the graph widget that does this and can be re-used.

Actually, very few invalid graphs and tabs are attempted. The only ones we need to hide are the Poller Modules Performance graph and the Maps tab.

The popups in the top right will need to be hidden. Not sure what else.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Sep 14, 2017

Member

@laf I think I would rather keep the os as "ping" that way it is more clear what data we have and we can customize the "ping" os to hide as much snmp stuff as possible.

However, we do need the ability for the user to customize the display of this a bit. Like setting an icon. So maybe, we let the use select an "os" but only set icon and some descriptive text, not the actual os value.

Member

murrant commented Sep 14, 2017

@laf I think I would rather keep the os as "ping" that way it is more clear what data we have and we can customize the "ping" os to hide as much snmp stuff as possible.

However, we do need the ability for the user to customize the display of this a bit. Like setting an icon. So maybe, we let the use select an "os" but only set icon and some descriptive text, not the actual os value.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 14, 2017

Member

However, we do need the ability for the user to customize the display of this a bit. Like setting an icon. So maybe, we let the use select an "os" but only set icon and some descriptive text, not the actual os value.

That's all dealt with by the OS so I really don't see the point in using a single OS. It limits the ability to do device groups and alerting on the correct OS all to save hiding some graphing info.

Member

laf commented Sep 14, 2017

However, we do need the ability for the user to customize the display of this a bit. Like setting an icon. So maybe, we let the use select an "os" but only set icon and some descriptive text, not the actual os value.

That's all dealt with by the OS so I really don't see the point in using a single OS. It limits the ability to do device groups and alerting on the correct OS all to save hiding some graphing info.

@Gorian

This comment has been minimized.

Show comment
Hide comment
@Gorian

Gorian Sep 15, 2017

Contributor

@Zmegolaz what if instead of a separate column, we just check that the OS is "ping"? Not sure if that is better or worse.

@murrant - I'd like to piggyback off of this code to add support for AWS services as OSes, so ideally you would add the code to disable SNMP checks, and other OSes could use that without having to be a "ping" OS...

Contributor

Gorian commented Sep 15, 2017

@Zmegolaz what if instead of a separate column, we just check that the OS is "ping"? Not sure if that is better or worse.

@murrant - I'd like to piggyback off of this code to add support for AWS services as OSes, so ideally you would add the code to disable SNMP checks, and other OSes could use that without having to be a "ping" OS...

@laf laf referenced this pull request Sep 16, 2017

Closed

Patch 3 #7342

1 of 1 task complete
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Sep 19, 2017

Member

@laf I'm not sure which would be better.

We could also go the route of separating ping devices entirely in the ui to keep them from making things crappy. Not sure if that is better or worse either.

Might need to try out various things and see which works the best.

Member

murrant commented Sep 19, 2017

@laf I'm not sure which would be better.

We could also go the route of separating ping devices entirely in the ui to keep them from making things crappy. Not sure if that is better or worse either.

Might need to try out various things and see which works the best.

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Sep 26, 2017

Member

I did a proof of concept for allowing the user to specify OS to see it in action. Personally, I think it turned out nice, it was relatively clean.

Member

Zmegolaz commented Sep 26, 2017

I did a proof of concept for allowing the user to specify OS to see it in action. Personally, I think it turned out nice, it was relatively clean.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 2, 2017

Member

Still no code review right now but testing it at present. Some more feedback:

  • Can you apply the select box UI we have (see misc)
  • We need addhost.php and Add Device in the UI to support adding non-snmp devices.
  • Not a fault of this PR but worth fixing, if ICMP is disabled then remove the ping response graph: /graphs/type=device_ping_perf/device=342/to=1506979800/from=1506893400/

I'm happy with how it works though. It's not quite right if you change an existing device over to be non-snmp due to all the data that did exist but I'm not sure that's one for this PR.

Member

laf commented Oct 2, 2017

Still no code review right now but testing it at present. Some more feedback:

  • Can you apply the select box UI we have (see misc)
  • We need addhost.php and Add Device in the UI to support adding non-snmp devices.
  • Not a fault of this PR but worth fixing, if ICMP is disabled then remove the ping response graph: /graphs/type=device_ping_perf/device=342/to=1506979800/from=1506893400/

I'm happy with how it works though. It's not quite right if you change an existing device over to be non-snmp due to all the data that did exist but I'm not sure that's one for this PR.

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Oct 5, 2017

Member

Whops, I broke it when fixing a Scrutinizer issue. Fixed again.

Member

Zmegolaz commented Oct 5, 2017

Whops, I broke it when fixing a Scrutinizer issue. Fixed again.

Zmegolaz added some commits Oct 5, 2017

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Oct 12, 2017

Contributor

Small question, "ping only" device will be listed like others? It will be possible to add services to this device? If yes, it will be really nice and will simplify understanding for services management.

Contributor

FTBZ commented Oct 12, 2017

Small question, "ping only" device will be listed like others? It will be possible to add services to this device? If yes, it will be really nice and will simplify understanding for services management.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 15, 2017

Member

@Zmegolaz Is this complete now as far as you're concerned so I can code review?

Member

laf commented Oct 15, 2017

@Zmegolaz Is this complete now as far as you're concerned so I can code review?

@laf laf added this to the 1.33 milestone Oct 15, 2017

@Zmegolaz

This comment has been minimized.

Show comment
Hide comment
@Zmegolaz

Zmegolaz Oct 15, 2017

Member

@laf Yes! Everything should be implemented now.

Member

Zmegolaz commented Oct 15, 2017

@laf Yes! Everything should be implemented now.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 19, 2017

Member

Alright got around to testing this.

The OS field should be clearly marked as optional. I think calling this OS is misleading here, but I'm not that worried about it.

I think the edit menu should be filtered. Things to remove: Port Settings, Applications, Modules, Storage, Processors, Memory, Components Not sure: IPMI, Misc

I haven't looked at the code yet. I was thinking about icons a bit and whipped one up. Not sure about it.

ping svg

Member

murrant commented Oct 19, 2017

Alright got around to testing this.

The OS field should be clearly marked as optional. I think calling this OS is misleading here, but I'm not that worried about it.

I think the edit menu should be filtered. Things to remove: Port Settings, Applications, Modules, Storage, Processors, Memory, Components Not sure: IPMI, Misc

I haven't looked at the code yet. I was thinking about icons a bit and whipped one up. Not sure about it.

ping svg

Zmegolaz added some commits Oct 19, 2017

Added (optional) to OS and hardware description. Hid Port Settings, A…
…pplications, Modules, Storage, Processors, Memory and Components from the edit menu
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 19, 2017

Member

The OS field should be clearly marked as optional. I think calling this OS is misleading here, but I'm not that worried about it.

As in choosing the OS for the device? Why would we have that as optional?

I haven't looked at the code yet. I was thinking about icons a bit and whipped one up. Not sure about it.

What's the icon for? If it's for the ping.yaml then imho that should be binned off unless you want it as just another OS option people can choose for ping only devices.

Member

laf commented Oct 19, 2017

The OS field should be clearly marked as optional. I think calling this OS is misleading here, but I'm not that worried about it.

As in choosing the OS for the device? Why would we have that as optional?

I haven't looked at the code yet. I was thinking about icons a bit and whipped one up. Not sure about it.

What's the icon for? If it's for the ping.yaml then imho that should be binned off unless you want it as just another OS option people can choose for ping only devices.

Zmegolaz added some commits Oct 19, 2017

@laf

@librenms/reviewers Anyone else want to code review this?

Show outdated Hide outdated addhost.php
Show outdated Hide outdated addhost.php
Show outdated Hide outdated html/ajax_ossuggest.php
Show outdated Hide outdated html/pages/addhost.inc.php
Show outdated Hide outdated html/pages/device/edit/snmp.inc.php
Show outdated Hide outdated snmp-scan.py

Zmegolaz added some commits Oct 23, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 25, 2017

Member

@librenms/reviewers I'd like to merge this tomorrow - any further comments?

Member

laf commented Oct 25, 2017

@librenms/reviewers I'd like to merge this tomorrow - any further comments?

@murrant

Mostly minor comments except for snmp-scan.py. I propose it always tries snmp before adding ping only.

Show outdated Hide outdated html/ajax_ossuggest.php
@@ -407,7 +407,7 @@ function delete_device($id)
* @throws InvalidPortAssocModeException The given port association mode was invalid

This comment has been minimized.

@murrant

murrant Oct 26, 2017

Member

Can you update the phpdoc for addHost to describe the $additional argument?

@murrant

murrant Oct 26, 2017

Member

Can you update the phpdoc for addHost to describe the $additional argument?

Show outdated Hide outdated includes/functions.php
Show outdated Hide outdated includes/polling/functions.inc.php
@@ -120,7 +121,7 @@ def scan_host(ip):
pass
try:
add_output = check_output(['/usr/bin/env', 'php', 'addhost.php', hostname or ip])

This comment has been minimized.

@murrant

murrant Oct 26, 2017

Member

I feel link snmp-scan.py option should be allow ping only devices to be added, this way it tries snmp first.

@murrant

murrant Oct 26, 2017

Member

I feel link snmp-scan.py option should be allow ping only devices to be added, this way it tries snmp first.

This comment has been minimized.

@Zmegolaz

Zmegolaz Oct 26, 2017

Member

I think I prefer to keep it as ping only. If someone really want to do a combined scan they could start with the SNMP scan to add all SNMP devices and then do a ping scan to add the rest. Otherwise we would lose the ability to add devices as ping only if they happen to reply to SNMP.

It would also take longer time to scan it, but I don't think that matter much.

@Zmegolaz

Zmegolaz Oct 26, 2017

Member

I think I prefer to keep it as ping only. If someone really want to do a combined scan they could start with the SNMP scan to add all SNMP devices and then do a ping scan to add the rest. Otherwise we would lose the ability to add devices as ping only if they happen to reply to SNMP.

It would also take longer time to scan it, but I don't think that matter much.

Zmegolaz added some commits Oct 27, 2017

Always try SNMP in snmp-scan.py, new option for it in addhost.php. Sl…
…ice instead of chunk in ajax_ossuggest.php. Other minor style changes.
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Oct 27, 2017

The inspection completed: 1 updated code elements

scrutinizer-notifier commented Oct 27, 2017

The inspection completed: 1 updated code elements

@laf

laf approved these changes Oct 27, 2017

LGTM

@laf laf removed this from the 1.33 milestone Oct 27, 2017

@murrant murrant merged commit f8d7ccf into librenms:master Oct 28, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@Zmegolaz Zmegolaz deleted the Zmegolaz:icmp_up-down branch Oct 30, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 17, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.