Skip to content
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

Added options to check interface by ifDescr regex, and also by ifAlias regex #4

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

pablodav
Copy link

Tested in real case, example:

user@host:~$ nm_check_admin_up_oper_down  -P 2c -C public -H HOSTNAME -c
5 interfaces down
StackPort1
GigabitEthernet1/0/46 - *** some int description ***
GigabitEthernet1/0/47 - UPLINK to SWITCHX
StackSub-St1-1
StackSub-St1-2

user@host:~$ nm_check_admin_up_oper_down  -P 2c -C public -H HOSTNAME -c -d 'GigabitEthernet(\d+)/0/(4[78]|[5][0-2])'
1 interfaces down
GigabitEthernet1/0/47 - UPLINK to SWITCHX

user@host:~$ nm_check_admin_up_oper_down  -P 2c -C public -H HOSTNAME -c -al 'UPLINK'
1 interfaces down
GigabitEthernet1/0/47 - UPLINK to SWITCHX

@pablodav
Copy link
Author

Have you seen this?

@ogenstad
Copy link
Member

Yes I have. Sorry, I've had way to little time and these repos haven't been prioritised. Hope to find time soon. Thank you for your contributions!

@ogenstad
Copy link
Member

Hi, sorry about the delay..

What problem did you run into regarding setuptools in setup.py? 'setuptools>=35.0.2'

Can you change the version to 1.3.6, and specify the name of the plugin in HISTORY.rst?

@pablodav
Copy link
Author

Hello @ogenstad

I have done the updates to 1.3.6 on history and init file.

Add one more version to 1.3.7 due to a bug reported in nelsnmp networklore/nelsnmp#8

The setuptools outdated made problems to install nelmon, It was not possible to install it before upgrading, to make things easier and smooth for users adding this dependency makes everything work out of the box.

I have tested in production nagios servers now 1.3.7 and everything is working fine.

@pablodav
Copy link
Author

will create new pull request, as it doesn't looks updated

@pablodav pablodav closed this Jul 12, 2017
@pablodav
Copy link
Author

sorry reopenning as it looks fine, was required a refresh from my browser.

@pablodav pablodav reopened this Jul 12, 2017
@ogenstad
Copy link
Member

Which version of setuptools were you using? Or on what platform did you see that problem?

Since version 1.3.6 hasn't been released there's no reason to bump the version to 1.3.7, can you change it back?

For now the timeout issue could probably just be fixed by setting the default of 1 with a line self.timeout = 1 somewhere here:
https://github.com/networklore/nelmon/blob/master/lib/nelmon/snmp/handler.py#L49-L50

I.e not tying the nelsnmp version.

@pablodav
Copy link
Author

Hello @ogenstad
thanks for the review and comments,

I have done more tests with ubuntu 14.04 (the affected system).
cloning in a fresh install and doing pip install --users . didn't got errors so seems that there is no issue in that case, the issue seems to came in my installation without pip package from ansible role:

https://github.com/CoffeeITWorks/ansible_nagios4_server_plugins/blob/master/defaults/main.yml#L22

Now I have done the workaround in that role and reverted the change in this software. I did the installation using git+https on my role due that I don't want to duplicate nelmon on pip, so I prefer if the actual pip package just got upgraded from original author, but I need the latest version because I'm using it in production.

I have also put back change to 1.3.6 version.

i didn't change anything with self.timeout = 1 because it need more testing, now added as "workaround" until we can test more in next version.

@pablodav
Copy link
Author

hello @ogenstad did you got this message?

@ogenstad
Copy link
Member

Hi Pablo,

Yes I saw the issue. However I wanted to have this issue fixed in nelmon instead of tying the version in nelsnmp. Looking at the code in nelmon now I will probably rewrite it "sometime" since a lot of the code is replicated from nelsnmp and that's what's causing the issue.

I've just released nelmon 1.3.6 which contains the bug fix for the timeout issue. Currently there's a merge conflict which needs to be resolved before this could be merged. Could you take a look at that and I'll promise that I won't take a long time to review the PR.

Best regards
Patrick

@pablodav
Copy link
Author

Thanks you, I will merge your changes and remove the conflicts

@pablodav
Copy link
Author

I have resolved the conflicts now

@@ -24,6 +25,17 @@ def main():
help='Return Warning if interfaces are down')
argparser.parser.add_argument('-c', action='store_true',
help='Return Critical if interfaces are down')
argparser.parser.add_argument('-d', '--descr', dest='ifdescr_arg', default=None, const=None,
help='Search over Interface descr with regex'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these descriptions get quite long with strange linebreaks. Try using nm_check_admin_up_oper_down -h. It should be enough to add a few \n

help='Search over Interface alias with regex'
'example: UPLINK'
'matches any interfaces with keyword UPLINK on its alias')
argparser.parser.add_argument('-id', '--ignore_descr', dest='ifdescr_ignore_arg', default=None, const=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I know all the "good letters" are already taken I'd like the arguments to include only one letter. Can you think of any other alternatives instead of -al and -id?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have forgot to answer to it.

what do you think about it:

instead of '-al' for alias, use: '-s' or '-a'
instead of '-id' for ignore description, use: '-x'

What do you prefer?

@ogenstad
Copy link
Member

ogenstad commented Oct 7, 2017

@pablodav, I've reviewd this now and it looks good. I would like you to address the small issues above though.

I'm open to other suggestions about the arguments, can't think of anything at the moment.

Thanks.

@pablodav
Copy link
Author

hello @ogenstad ,

sorry I have forgotten to answer before, now I have added my answer above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants