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 cpu and mempolls EdgeCore switch #7850

Merged
merged 12 commits into from Jan 11, 2018

Conversation

Projects
None yet
4 participants
@erotel
Copy link
Contributor

erotel commented Dec 4, 2017

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

ECS4510
ECS3528
ECS4120 - NOT TESTED (I do not have this device)
ECS4210
ECS3510

Fixes: #7616

$temp_total = snmp_get($device, '.1.3.6.1.4.1.259.10.1.24.1.39.3.1.0', '-Ovqn');//ECS4510
if (!empty($temp_total)) {
$total = snmp_get($device, '.1.3.6.1.4.1.259.10.1.24.1.39.3.1.0', '-Ovqn');

This comment has been minimized.

@laf

laf Dec 4, 2017

Member

No point in running this snmp_get, you already have this data.

You then should check if $total and $avail are set for the rest of these so you don't continuously send snmp gets if you match early on.

Also, can you not do anything with sysObjectID to work out what the OIDs are?

This comment has been minimized.

@erotel

erotel Dec 4, 2017

Author Contributor

The use of sysObjectID is not possible because it is not known yet in discovery

#### Load disco module processors ####
Array
(
    [device_id] => 39
    [hostname] => 192.168.224.254
    [sysName] => 192.168.224.254
    [ip] =>.
    [community] => public
    [authlevel] =>.
    [authname] =>.
    [authpass] =>.
    [authalgo] =>.
    [cryptopass] =>.
    [cryptoalgo] =>.
    [snmpver] => v2c
    [port] => 161
    [transport] => udp
    [timeout] =>.
    [retries] =>.
    [snmp_disable] => 0
    [bgpLocalAs] =>.
    [sysObjectID] =>.
    [sysDescr] =>.
    [sysContact] =>.
    [version] =>.
    [hardware] =>.
    [features] =>.
    [location] =>.
    [os] => edgecos
    [status] => 1
    [status_reason] =>.
    [ignore] => 0
    [disabled] => 0
    [uptime] =>.
    [agent_uptime] => 0
    [last_polled] =>.
    [last_poll_attempted] =>.
    [last_polled_timetaken] =>.
    [last_discovered_timetaken] =>.
    [last_discovered] =>.
    [last_ping] =>.
    [last_ping_timetaken] =>.
    [purpose] =>.
    [type] => network
    [serial] =>.
    [icon] => edge-core.png
    [poller_group] => 0
    [override_sysLocation] => 0
    [notes] =>.
    [port_association_mode] => 1
    [attribs] => Array
        (
        )

    [snmp_max_repeaters] =>.
)

This comment has been minimized.

@laf

laf Dec 4, 2017

Member

It should be and if it isn't / doesn't we should make sure it is.

You could however just do the sysObjectID lookup once anyway to still save all of this but I favor fixing disco.

This comment has been minimized.

@erotel

erotel Dec 5, 2017

Author Contributor

I will try to repair the disco, but I do not know if I can do it

This comment has been minimized.

@laf

laf Dec 5, 2017

Member

Should already be available in $device['sysObjectID']

This comment has been minimized.

@erotel

erotel Dec 5, 2017

Author Contributor

When adding a new device, $ device ['sysObjectID'] is only available after the first polling

$oid = '.1.3.6.1.4.1.259.10.1.24.1.39.2.1.0';
$descr = 'Processor';
$usage = snmp_get($device, $oid, '-Ovqn');
$usage = snmp_get($device, '.1.3.6.1.4.1.259.10.1.24.1.39.2.1.0', '-Ovqn');//ECS4510

This comment has been minimized.

@laf

laf Dec 4, 2017

Member

Same goes for this file really, no point in running snmp_gets all the time, just check if $oid is set before you do.

Check you can't sysObjectID to work these out as well. Will save a lot of code and snmp queries.

* the source code distribution for details.
*/
if (starts_with($device['sysObjectID'], 'enterprises.259.10.1.24.')) { //ECS4510

This comment has been minimized.

@laf

laf Dec 4, 2017

Member

Ok so you've answered my question above, you can use the sysObjectID - you need to port this to discovery.

Can you not used the named OIDs in all of this?

You should convert these to snmp_get_multi_oid() when running more than one get.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Dec 8, 2017

@erotel here is how these things should work. In discovery, you should figure out the numeric oid that holds the data, save that in the discover_x() call and then the poller will just use that to poll it without any tricky logic.

Rather than depending on sysObjectID, you could snmpget an oid and if that doesn't return data, try another oid.

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 8, 2017

Rather than depending on sysObjectID, you could snmpget an oid and if that doesn't return data, try another oid.

In this case though that's not the best course of action. Using sysObjectID to build the query is better as it then only needs one get or walk, we just need to make sure sysObjectID is available, which it should be.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Dec 9, 2017

I'm a bit cynical as I've been looking though all the processor discovery files, many are broken to some degree.

Sure you can use sysObjectID to save a query, but I don't like the way it is used here. Using it to select certain models leaves out unknown or future models.

@erotel

This comment has been minimized.

Copy link
Contributor Author

erotel commented Dec 9, 2017

Maybe I misunderstood (my English is terrible). In my opinion sysObjectID snmpget in discovery saves the number of queries. sysObjectID in the poller is right for me. Why load snmp oid when the type can be determined by the sysObjectID we know. This saves a lot of snmp queries (up to 4 queries for the latest model).
As for the new or unknown model, support must be added to yaml and other files.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Dec 9, 2017

Sure, but please support devices you have not explicitly defined.

Also, you can access sysObjectID here $device['sysObjectID']

@erotel

This comment has been minimized.

Copy link
Contributor Author

erotel commented Dec 9, 2017

The oid is always for a particular line.
eg SNMPv2-SMI :: enterprises.259.10.1.24. is for the ECS4510 series of switches.
  tested on ECS4510-52T ECS4510-28F

When adding a new device, $ device ['sysObjectID'] is only available after the first polling

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 9, 2017

Sure you can use sysObjectID to save a query, but I don't like the way it is used here. Using it to select certain models leaves out unknown or future models.

Which would have been the same with querying the OID anyway, the difference here is we aren't attempting to query what is possibly an unused OID until we find the one we want.

However, we can fix the concern here by just appending the last few octets of the OID to sysObjectID as it's only that bit which changes, that will then cover for any that we don't know about.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Dec 10, 2017

When adding a new device, $ device ['sysObjectID'] is only available after the first polling

I think that should be fixed :-). But it is a little tricky. Please use that for now and let it only work for the discovery after the first poll.

Seems reasonable @laf

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 10, 2017

I think that should be fixed :-). But it is a little tricky. Please use that for now and let it take two discovers for new devices.

It definitely should. I'd like to try and fix it for this PR so leave it with me.

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 19, 2017

#7922 adds $device['sysObjectID'] and $device['sysDescr'], when it's merged you can update this PR to use those (or update before hand ready).

@laf

laf approved these changes Dec 26, 2017

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 26, 2017

This just needs to wait for #7922 now and then an update to be made to the code. enterprises.259.10.1.24. would now be the full numerical OID even in polling. The linked PR standardises the use of $device['sysObjectID'] throughout the codebase.

Will leave the blocker label in place until ready.

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 26, 2017

Thanks @erotel. Just needs the other PR merging now before this can go in.

@murrant murrant added this to the 1.36 milestone Dec 27, 2017

@murrant murrant removed the Blocker 🚫 label Jan 9, 2018

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Jan 9, 2018

@erotel The sysObjectID PR is now merged. But, we also merged a functional testing framework.

Would you be willing to add some test data?
https://docs.librenms.org/Developing/os/Test-Units/

You would probably need several variants to represent the different types you have here.

@erotel

This comment has been minimized.

Copy link
Contributor Author

erotel commented Jan 9, 2018

I hope I understand what you need.
But the script tells me the error.

error save-test-data.php

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Jan 9, 2018

@erotel you need to install snmpsim to generate the json files. If you want, I might be able to do that from the snmprec files you uploaded.

If you need any help, ask on Discord :)

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Jan 9, 2018

Also, you need to rebase this to pull in the sysObjectID changes (and delete cache/os_defs.cache after you do that).

@erotel

This comment has been minimized.

Copy link
Contributor Author

erotel commented Jan 9, 2018

@murrant

I might be able to do that from the snmprec files you uploaded.

Please.
Somehow I can not install snmpsim.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Jan 9, 2018

@erotel the processor oids are not include in your previously uploaded snmprec files because you had not rebased. You'll need to rebase and re-capture the data. Verify the processor oid is present in each snmprec ;)

I forgot about mempools too. So when you run the capture do -m processors,mempools

erotel added some commits Jan 10, 2018

@laf

This comment has been minimized.

Copy link
Member

laf commented Jan 10, 2018

@erotel Still needs the data @murrant mentioned in his last comment :)

@erotel

This comment has been minimized.

Copy link
Contributor Author

erotel commented Jan 11, 2018

I added new json files where it was used
/opt/librenms/scripts/save-test-data.php -h 192.168.224.192 -m processors,mempools

@laf

This comment has been minimized.

Copy link
Member

laf commented Jan 11, 2018

I could be missing it but I don't see the data in the snmprec file though for mem/cpu.

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jan 11, 2018

The inspection completed: 671 Issues, 25 Patches

@laf

This comment has been minimized.

Copy link
Member

laf commented Jan 11, 2018

Awesome work @erotel. Thanks for doing this

@laf laf merged commit 9a0b06e into librenms:master Jan 11, 2018

2 checks passed

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

@laf laf referenced this pull request Jan 11, 2018

Merged

Major Processors rewrite #8066

1 of 1 task complete
@lock

This comment has been minimized.

Copy link

lock bot commented May 16, 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 16, 2018

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