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

Add Junos VirtuallChassis ports polling #9879

Merged
merged 18 commits into from Mar 12, 2019

Conversation

Projects
None yet
4 participants
@rmagomedov
Copy link
Contributor

commented Feb 26, 2019

Junos EX/QFX switches can be stacked together by so called VC ports. It's good to be able to monitor utilization of all these VC ports. With IF-MIB it is only possible to see ports of VirtuallChassis Master switch which is not enough. There is JUNOS-VIRTUALlCHASSIS-MIB containing interface counters of all VC ports for system.

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
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

rmagomedov added some commits Feb 26, 2019

@CLAassistant

This comment has been minimized.

Copy link

commented Feb 26, 2019

CLA assistant check
All committers have signed the CLA.

@label-actions

This comment has been minimized.

Copy link

commented Feb 26, 2019

Please add test data so we can ensure your change is not broken in the future.

Read the docs to find out how: https://docs.librenms.org/Developing/os/Test-Units

@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Is it ok if I add required snmp data to existing test file or it's better to create a new one? If it's fine, I updated tests/snmpsim/junos_ex.snmprec file. I cannot rebuild junos_ex.json with 'scripts/save-test.data.php -o junos -v ex'. When I run it, it stucks on 'Listening at UDP/IPv4 endpoint 127.1.6.1:1161, transport ID 1.3.6.1.6.1.1.0' and nothing happens after.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Hi @rmagomedov ,
You should disable your firewall on 127.0.0.0/8 (all the loopback interfaces) and it will succeed.

@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

Test data is updated.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

Hi,
It seems that the JSON file is not built against the SNMPREC file that is attached to the Pull request. If you look at the Travis CI results, you can see there is an issue.
Could you first try to do : ./scripts/pre-commit.php -o junos
That one should run tests for junos (all variants, including 'ex') and you can fine tune the test in order to see a success here.
Then you can commit and push again the correct files to the PR.
PipoCanaja

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

for some reasons, the snmprec file in this PR does not create any ports, and your JSON file does have ports. So either the snmprec file is not the same in your environnement than here, or your php code is not the same.

@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

I checked out my working directory to the add-junos-vc-ports branch, the branch of the pull request. And all steps I do, I do it from this directory. The junos_ex.snmprec is updated there and I generate junos_ex.json with ./scripts/save-test-data.php -o junos -v ex command and it goes well - the JSON file is updated and it contains all expected VC ports. Right after that, from the same directory, I run unit tests ./scripts/pre-commit.php --db --snmpsim -p -u and it fails. So generating the .json file works well(expected ports are in place), but at the same time the unit tests show empty port array and fail

@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Here is how part of debug of ./scripts/save-test-data.php -o junos -v ex -m ports --debug looks like.

save-test-data-debug.txt

It looks like the unit test script is not even trying to poll jnxVirtualChassisPortTable of JUNIPER-VIRTULCHASSIS-MIB.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

The save-test-data.php should always be run without the -m option, cause that's how the tests are run in the automated tests ...

If that does not help, as junos_ex.snmprec was existing before your change, here is a suggestion :

  • Could you try to restore the original junos_ex.snmprec and junos_ex.json
  • Collect a new snmprec (junos_exPortPoll.snmprec for instance)
  • Run a save-test-data without specifying any module (so you ensure all modules are run).
  • Then try the pre-commit.php -o junos that should succeed.

If all is good that way, then the issue is the mix of old snmprec data and new, and you can keep the new test file and leave the existing one untouched.

@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Sorry for the delay. I tried all of the above, nothing helped. I collected new separate junos_qfx.snmprec with ./scripts/collect-snmp-data.php -h <IP> -o junos -v qfx command. Like before, save-test-data generates .json with expected VC ports, but pre-commit fails.

I am not sure but it looks like 'pre-commit' even is not trying to run includes/polling/ports/junos-vc-ports.inc.php needed for polling of VC ports while 'save-test-data' is. I expected that 'pre-commit' as well as 'save-test-data' should run my VC port polling php code. I checked it by adding some output with the d_echo function.

In the 'includes/polling/ports.inc.php' file I tried to replace require_once 'ports/junos-vcp.inc.php'; with require 'ports/junos-vcp.inc.php'; and the unit tests finally passed in my environment. Looks like require_once for some reason is not including my php code when ran by 'pre-commit'.

@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Like I said, yesterday I found out that if I include my VC port polling php code with 'require' instead of 'require_once' tests pass.

Today I also noticed that tests pass even if the code is included with 'requite_once' but when test unit do not use OS variants. For example:

  • Moved JUNIPER-VIRTUALCHASSIS-MIB data from junos_ex.snmprec to junos.snmprec
  • Renamed junos_qfx.snmprec I collected myself to qfx.snmprec
@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

In order to complete this pull request I can add the MIB data to junos.snmprec instead of junos_ex.snmprec, I think it's ok but a little illogical because looks like this junos.snmprec was collected from Juniper SRX firewall which I believe cannot have VirtuallChassis ports, but probably it does not matter for the unit tests.

I can also add that before I ran into this problem with unit tests. The test passed fine on my other production server on the php56 brach - Debian 9, PHP7.0

rmagomedov added some commits Mar 7, 2019

@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

I have moved the snmp data from junos_ex.snmprec to junos.snmprec and now the tests passed

@murrant

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

Yes, it should be require (because it is being used like a function).
Looks like you got the unit tests working, nice. Just one style fix and this should be g2g.

rmagomedov added some commits Mar 9, 2019

@rmagomedov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

The tests passed

// dummy integer index of it.
if (preg_match('#^(\d{1,2})\.vcp-255/(\d)/(\d{1,2})$#', $index, $matches)) {
if (strlen($matches[1]) == 1) {

This comment has been minimized.

Copy link
@murrant

murrant Mar 12, 2019

Member

Instead of these if statements, you could just us str_pad():

$fpc = str_pad($matches[1], 2, '0', STR_PAD_LEFT)

1 => 01
99 => 99
432 => 432

This comment has been minimized.

Copy link
@rmagomedov

rmagomedov Mar 12, 2019

Author Contributor

Yeah, this looks more compact and easier to read. Done

@murrant

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Looks good just a small change to reduce complexity a bit

@murrant murrant merged commit 019c57b into librenms:master Mar 12, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details
license/cla Contributor License Agreement is signed.
Details

funzoneq added a commit to funzoneq/librenms that referenced this pull request Apr 30, 2019

Add Junos VirtuallChassis ports polling (librenms#9879)
* Update ports.inc.php

* Create junos-vcp.inc.php

* Fix some formatting errors in junos-vcp.inc.php

* Update junos_ex.snmprec

* Update junos_ex.json

* Update tests/data/junos_ex.jso

* Rollback junos_ex data set

* Update ports.inc.php

* Update junos-vcp.inc.php~

* Update tests/snmpsim/junos.snmprec

* Generate new .json files

* Update includes/polling/ports.inc.php

* Rollback tests/data/junos_ex.json

* Update junos.snmprec

* Update junos.snmprec

* Use str_pad in junos-vcp.inc.php

@lock lock bot locked as resolved and limited conversation to collaborators May 11, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.