add basic STP/RSTP support #2690

Merged
merged 5 commits into from Jan 10, 2016

Projects

None yet

8 participants

@vitalisator
Contributor

first try to close #2673

  • discover / polling from ieee802.1D MIB (STP RSTP only)
  • database upgrade for STP Bridges (without ports)
  • STP device tab with basic STP information (without ports)

that works for me, but maybe need some checks and improvements

@vitalisator vitalisator add basic STP/RSTP support
d12c780
@paulgear
Member
paulgear commented Jan 6, 2016

@vitalisator The issue is that 085.sql was merged in another PR, so you need to move it to 086.sql. Also, our schema update code is a bit limited, and only supports one statement per line. So you need to combine those three statements into single lines.

So what you need to do is:

cd sql-schema
git mv 085.sql 086.sql
# make the required changes to 086.sql
git commit -m"Update schema to 086.sql" .

That should eliminate any need for a full rebase.

@paulgear paulgear and 1 other commented on an outdated diff Jan 6, 2016
html/includes/print-stp.inc.php
@@ -0,0 +1,29 @@
+<?php
+
+$stp_raw = dbFetchRow('SELECT * FROM `stp` WHERE `device_id` = ?', array($device['device_id']));
+$stp = array (
+ 'Root bridge' => ($stp_raw['rootBridge'] == 1) ? 'Yes' : 'No',
@paulgear
paulgear Jan 6, 2016 Member

It would be nice to visually align the => in this section.

@vitalisator
vitalisator Jan 7, 2016 Contributor

Paulgear, thank you for hint!

@vitalisator vitalisator formatting
e807a69
@SaaldjorMike SaaldjorMike commented on an outdated diff Jan 7, 2016
sql-schema/086.sql
@@ -0,0 +1 @@
+CREATE TABLE IF NOT EXISTS `stp` (`stp_id` int(11) NOT NULL, `device_id` int(11) NOT NULL, `rootBridge` tinyint(1) NOT NULL, `bridgeAddress` varchar(32) NOT NULL, `protocolSpecification` varchar(16) NOT NULL, `priority` mediumint(9) NOT NULL, `timeSinceTopologyChange` varchar(32) NOT NULL, `topChanges` mediumint(9) NOT NULL, `designatedRoot` varchar(32) NOT NULL, `rootCost` mediumint(9) NOT NULL, `rootPort` mediumint(9) NOT NULL, `maxAge` mediumint(9) NOT NULL, `helloTime` mediumint(9) NOT NULL, `holdTime` mediumint(9) NOT NULL, `forwardDelay` mediumint(9) NOT NULL, `bridgeMaxAge` smallint(6) NOT NULL, `bridgeHelloTime` smallint(6) NOT NULL, `bridgeForwardDelay` smallint(6) NOT NULL) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8; ALTER TABLE `stp` ADD PRIMARY KEY (`stp_id`), ADD KEY `stp_host` (`device_id`); ALTER TABLE `stp` MODIFY `stp_id` int(11) NOT NULL AUTO_INCREMENT,AUTO_INCREMENT=1;
@SaaldjorMike
SaaldjorMike Jan 7, 2016 Member

This line contains 3 statements (CREATE, 2x ALTER), each of which should be on its own line.

@vitalisator vitalisator Update 086.sql
4223a15
@laf
Member
laf commented Jan 7, 2016

👍 from me. It's not broken CI and As this is a new feature, if we see issues we can fix on the go.

When we merge, I suggest an email out to the list + twitter post to let people know.

@f0o f0o and 1 other commented on an outdated diff Jan 8, 2016
html/pages/device.inc.php
@@ -229,6 +229,14 @@
</a>
</li>');
+ if (@dbFetchCell("SELECT COUNT(stp_id) FROM stp WHERE device_id = '".$device['device_id']."'") > '0') {
@f0o
f0o Jan 8, 2016 Member

Instead of count just select 1 - will take some load away from the sql if the table gets too populated

@vitalisator
vitalisator Jan 8, 2016 Contributor

this should do not count all the id entries, because of "WHERE device_id =" statement in the query.
As I know the COUNT function is called after WHERE, but you can correct me if I'm wrong (I'm not the SQL guru)

@f0o
f0o Jan 8, 2016 Member

Well it boils down to:
Does stp have more than 1 entry?

If so, the count is more excessive than just a boolean check whether something is there or not.

On 8 January 2016 10:52:39 CET, Vitali Kari notifications@github.com wrote:

@@ -229,6 +229,14 @@

');

  •    if (@dbFetchCell("SELECT COUNT(stp_id) FROM stp WHERE
    
    device_id = '".$device['device_id']."'") > '0') {

this should do not count all the id entries, because of "WHERE
device_id =" statement in the query.
As I know the COUNT function is called after WHERE, but you can correct
me if I'm wrong (I'm not the SQL guru)


Reply to this email directly or view it on GitHub:
https://github.com/librenms/librenms/pull/2690/files#r49172512

@vitalisator
vitalisator Jan 8, 2016 Contributor

I don't think that we would have more then one STP/RSTP instance per device, maybe in VRF.
I can change this to boolean to save some ms.

@vitalisator vitalisator Update device.inc.php
e20f114
@laf laf added the Schema label Jan 10, 2016
@laf laf merged commit d3132a1 into librenms:master Jan 10, 2016

2 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer 1653 Issues, 19 Patches
Details
@markuspachali

Just as a short feedback:
For me it is working for Cisco Catalyst and HP ProCurve Switches. There is just one "problem". Cisco IOS does not support normal STP and RSTP, they have implemented their own PVST and RPVST (spanning-tree per VLAN) which results in the following output:

Root bridge No
Bridge address (MAC)    0015f9bf5d80
Protocol specification  unknown
Priority (0-61440)  32769
Time since topology change  13m 38s
Topology changes    9088
Designated root (MAC)   00083025b189
Root cost   20003
Root port   520
Max age (s) 20
Hello time (s)  2
Hold time (s)   1
Forward delay (s)   15
Bridge max age (s)  20
Bridge hello time (s)   2
Bridge forward delay (s)    15
@PENorris

Is there a global disable for this feature? I think it might be causing some performance issues in my Cisco PVST environment.

@SaaldjorMike
Member

@PENorris Yes, you can disable the modules by inserting these into your config.php file:

$config['poller_modules']['stp'] = 0;
$config['discovery_modules']['stp'] = 0;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment