Cacti / cacti

Cacti ™
http://www.cacti.net
GNU General Public License v2.0
1.63k stars 404 forks source link

Automation discovery : New devices added by automation discovery have empty SNMP community field #1300

Closed g1augusto closed 6 years ago

g1augusto commented 6 years ago

Hi Everyone,

Since last version update I noticed that for Automation discovery all our newly added devices discovered by a scan are added to CACTI but without the SNMP community

Before it was correctly adding the SNMP community from the Automation discovery SNMP Option matching SNMP community

netniV commented 6 years ago

In your network discovery, is it using the correct option set?

g1augusto commented 6 years ago

Yes, the option were not changed and I had these discoveries working well before the 1.1.33 update

netniV commented 6 years ago

What version were you on prior to the 1.1.33 update?

g1augusto commented 6 years ago

1.1.28

netniV commented 6 years ago

OK, I'll check out the differences between the two release tags later today and see if anything is likely to impact on this issue.

g1augusto commented 6 years ago

thanks a lot, let me know if I can help with the testing anyway

netniV commented 6 years ago

So I can see a lot of changes to the automation to move from snmp_readstring to snmp_community. I think it may be related to this.

netniV commented 6 years ago

One thing, can you make sure that your automation tables are right. Do a SHOW CREATE TABLE tablename \G against each fo them and paste here.

g1augusto commented 6 years ago

If it's OK with you I ran DESCRIBE instead (let me know if you need the other command):

automation_tables.txt

netniV commented 6 years ago

OK, I believe that I have found the problem. It's in lib/api_automation.php and it assigns automation_snmp_items.snmp_readstring to automation_devices.snmp_community

This is wrong on two levels because for starters, snmp_readstring on automation_snmp_items is now snmp_community; and because snmp_community is just community on automation_devices;

I think if you look at automation_devices, you'll find that community is not populated for your devices.

g1augusto commented 6 years ago

TBH the automation automatically adds the devices to CACTI in my installation, so I found out looking at new devices with missing SNMP community, but I guess it's the same as you are looking at.

Funny is that the device is queried with the right SNMP community and then it's not saved with any community, as you pointed out.

netniV commented 6 years ago

Do this SQL query:

select community, count(*) from automation_devices;
g1augusto commented 6 years ago

image

netniV commented 6 years ago

So they all have a community. OK, let me check the code again to see if that is what I expected 👍

netniV commented 6 years ago

Actually first, lets correct the SQL statement. I was surprised by the result so I thought I'd test it against my own data and realised I've missed off the group by statement.

mysql> select community, count(*) from automation_devices;
+-----------+----------+
| community | count(*) |
+-----------+----------+
|           |      542 |
+-----------+----------+
1 row in set (0.00 sec)

mysql> select community, count(*) from automation_devices group by community;
+-----------+----------+
| community | count(*) |
+-----------+----------+
|           |       83 |
| <name>    |      459 |
+-----------+----------+
2 rows in set (0.00 sec)

Try using that second SQL statement and let me know results. Also, if you know the name of the device that you are looking at as an example, replace <name> with what you are looking for

select id, hostname, community from automation_devices where hostname like '%<name>%'
g1augusto commented 6 years ago

To run this test I deleted one of the devices that was in CACTI, created a test automation network to discover it and when I ran it. I can see that the community is in the automation_devices table entry

image

Then I purged the discovery automation_devices table, I modified the automation discovery test adding the option : Automatically Add to Cacti and when I ran again the discovery I could see that the device added to cacti is missing the Community.

Seems that the issue is on the portion of code that creates a device in cacti

Also I can see the following logs into that specific poller logs :

02/Feb/2018 21:13:51 - CMDPHP PHP ERROR NOTICE Backtrace: (/poller_automation.php: 345 discoverDevices)(/poller_automation.php: 612 automation_add_device)(/lib/api_automation.php: 2656 CactiErrorHandler)(/lib/functions.php: 4482 cacti_debug_backtrace)

02/Feb/2018 21:13:51 - ERROR PHP NOTICE: Undefined index: community in file: /var/www/html/cacti/lib/api_automation.php on line: 2656
netniV commented 6 years ago

I looked at that section of code. But with your backtrace, this makes things a bit easier. It depends on whether $device is from the host table or the automation_devices table. I really think we should make a mod to rename the automation_devices table to be inline with every other table's snmp options and I'm hoping @cigamit would agree with that assessment.

netniV commented 6 years ago

The poller_automation.php is setting everything as snmp_community, so is automation_valid_snmp_device() in api_automation.php. The problem is that (also in api_automation.php) is automation_add_device that is using community not snmp_community.

Now, if the $device variable comes from row directly from automation_devices, that would have that column. However, poller_automation never sets it thus you get a blank version.

This furthers my resolve that we should be renaming that column to avoid conflict/confusion.

cigamit commented 6 years ago

Totally agree with the renaming. I've been trying to get some of the older plugins (especially thold) to use proper schema naming for ever. The update in 1.1.32 attempted to handle all the snmp stuff in the core Cacti (excluding username and password due to numerous user scripts using those columns directly). It appears however, that we missed this one in the automation table/files, which of course leads to errors.

As developers, we try to minimize variation so that we can get into the habit of assuming things when writing code, and then of course, as a consequence, we don't catch the fact that assumption is the mother of all screw ups until regression testing. And as a small open source project, we often times skip that part of the release process.

Either way, we are making great progress here.

cigamit commented 6 years ago

Resolved. Thanks for reporting.