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

Improved FortiManager support #8102

Merged
merged 1 commit into from Jan 24, 2018

Conversation

Projects
None yet
4 participants
@tukezor
Contributor

tukezor commented Jan 17, 2018

Improved basic OS polling to include hardware, version, and serial number information
Added new mempool for accurate memory usage
Added Log Rate graph for FortiAnalyzer feature enabled FortiManagers
Updated FortiManager/FortiAnalyzer mib file

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

@CLAassistant

This comment has been minimized.

CLAassistant commented Jan 17, 2018

CLA assistant check
All committers have signed the CLA.

@laf

Some inline comments and alos the ones below.

thank for submitting this :)

How come mibs/fortinet/FORTINET-FORTIANALYZER-MIB.mib has been removed?

Please rename mibs/fortinet/FORTINET-FORTIMANAGER-FORTIANALYZER-MIB.mib to mibs/fortinet/FORTINET-FORTIMANAGER-FORTIANALYZER-MIB

<?php
if ($device['os'] == 'fortios') {
echo 'FORTIOS-MEMORY-POOL: ';

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

You need to do a snmp_get here to check the device has mempools available before registering discover_mempool.

// FortiOS Mempool
echo 'FortiOS Mempool';
$mempool['total'] = (snmp_get($device, 'FORTINET-FORTIMANAGER-FORTIANALYZER-MIB::fmSysMemCapacity.0', '-OvQ') * 1024);

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

Please convert these two snmp_get calls to snmp_get_multi_oid(). You need to specify the MIB in the correct arg of that new function rather than tagging it onto the OID.

$mempool['perc'] = (($mempool['used'] / $mempool['total']) * 100);
$mempool['free'] = ($mempool['total'] - $mempool['used']);
echo '(U: '.$mempool['used'].' T: '.$mempool['total'].' F: '.$mempool['free'].') ';

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

Either remove this line or change it to use d_echo() so it only appears in debug.

$serial = snmp_get($device, 'fnSysSerial', '-OQv', 'FORTINET-FORTIANALYZER-MIB');
use LibreNMS\RRD\RrdDefinition;
$serial = snmp_get($device, 'FORTINET-CORE-MIB::fnSysSerial.0', '-Ovq');

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

Please convert both of these to snmp_get_multi_oid() and also drop the MIB name from the oid and put it as an arg to the function.

}
//Log rate only for FortiAnalyzer features enabled FortiManagers
$entModes = snmpwalk_cache_oid($device, 'fmDeviceEntMode', null, 'FORTINET-FORTIMANAGER-FORTIANALYZER-MIB');

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

How big is this walk? the idea in polling is to minimise the number of snmp calls. Can you not just test using $log_rate = snmp_get($device, '.1.3.6.1.4.1.12356.103.2.1.9.0', '-Ovq'); seeing as that's all you care about?

This comment has been minimized.

@tukezor

tukezor Jan 17, 2018

Contributor

FortiManager and FortiAnalyzer are separate products, but you can enable most of the Analyzer features inside Manager. So the idea is to poll the log rate only if those Analyzer features have been enabled. There is no specific oid for that.

That code checks if there are any Administrative Domains in fmg-faz mode. But you are right, in very large deployments that walk can get big. I think it should be safe to just get the first ADOM's mode and check if there is 'faz' in it, so I will do that.

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

Or just do the log_rate snmp query. Running one snmp query to determine if we should run another is the same amount of time as just running the second one only and in fact will be slower if the first query tells us we should then run the second.

In short, you should only do what you're doing if we need the data from both queries which it doesn't look like we do here.

This comment has been minimized.

@tukezor

tukezor Jan 18, 2018

Contributor

Log rate is only relevant if those Analyzer features are enabled. The log rate oid is present are those features enabled or not. So if we don't check that in any way, FortiManagers without analyzer features will get the log rate graph also. And its going to be constant 0.

I understand that we want to keep snmp queries to the minimum. If you want it to be like you suggested, I will do it.

This comment has been minimized.

@laf

laf Jan 18, 2018

Member

Can the value of the log rate oid be 0 if it's enabled still? If not then you can check if it's > 0. If it can then you will need to find the best way to do a simple snmp get for the initial test, walking a tree which could be large shouldn't be done in polling.

@@ -0,0 +1 @@
INSERT INTO `graph_types`(`graph_type`, `graph_subtype`, `graph_section`, `graph_descr`, `graph_order`) VALUES ('device', 'fortios_lograte', 'analyzer', 'Log Rate', 0);

This comment has been minimized.

@laf

laf Jan 17, 2018

Member

You will need to move this to 231.sql

@tukezor

This comment has been minimized.

Contributor

tukezor commented Jan 17, 2018

That old Analyzer MIB was back from 2009. FortiManager and FortiAnalyzer do not have separate MIB's anymore, so there is just that combined MIB for both products.

Thanks for the comments. I'll make the changes.

@laf

This comment has been minimized.

Member

laf commented Jan 17, 2018

That old Analyzer MIB was back from 2009. FortiManager and FortiAnalyzer do not have separate MIB's anymore, so there is just that combined MIB for both products.

Ok thanks, it can stay deleted then.

@laf

This comment has been minimized.

Member

laf commented Jan 21, 2018

Ok, that looks all good now.

The only thing left is unit test. https://docs.librenms.org/#Developing/os/Test-Units/ If you can add that then we can get this merged.

Improved FortiManager support
Modified with suggested changes

Test data for FortiOS
@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 23, 2018

The inspection completed: No new issues

@tukezor

This comment has been minimized.

Contributor

tukezor commented Jan 23, 2018

Added the test data. I Hope that's what you were asking for.

@@ -1 +1,41 @@
1.3.6.1.2.1.1.1.0|4|

This comment has been minimized.

@laf

laf Jan 24, 2018

Member

This is perfect with the exception of sysDescr - is it genuinely blank?

This comment has been minimized.

@tukezor

tukezor Jan 24, 2018

Contributor

It's blank unless you manually set it.

This comment has been minimized.

@laf

laf Jan 24, 2018

Member

Ok, thanks :)

@laf

laf approved these changes Jan 24, 2018

@laf laf merged commit 0a6f113 into librenms:master Jan 24, 2018

2 checks passed

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

This comment has been minimized.

Member

laf commented Jan 24, 2018

image

@lock

This comment has been minimized.

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.