New breeds for HP Procurve and ios15; unit tests for HP Procurve breed #225

igloo777 · 2018-08-14T05:16:20Z

Hello!
This is the first time when i write php code, so please, don't kick me too much :)

What in this pull request:

New breed for HP Procurve L2 switches (like 2810) with software ver. N.11.78 (tested) - hpprocurveN1178
- Live Ports - Done
- Live LLDP - Done
- 802.1Q management - Done
- Unit tests for hpprocurveN1178 functions with data samples - Done (HP Cli is slow, without unit tests debug consumed huge amount of time)
Limitations and notes:
- HP Procurve 802.1Q management tested without vlan 1 due to port mode "user: any mode"
- HP Procurve Live Ports doesn't show vlans due to the lack of information in CLI output (it's possible to do it in 2 connections, but requires complex logic) and very slow due to slow HP Procurve CLI
- HP Procurve Port's Show info set doesn't work (TODO in future)
- Requires HP software upgrade to N.11.78+
- New breed ios15 with new LLDP parser based on detailed LLDP output from 'show lldp neighbors detail'; also fixed an issue when remote port id value is mac address - used port description value in such case.
- hpprocurveN1178 and ios15 functions separated into dedicated files (to provide more readability) and included into deviceconfig.php via require_once

Any advises, comments and suggestions are welcome.

infrastation · 2018-08-14T08:43:03Z

Having the tests is good. Could you make sure PHP is indented with tabs please? Also the HP dictionary entry is missing, and it must have a key < 50000. Push more commits into the same branch and they will appear in the pull request.

igloo777 · 2018-08-15T07:02:24Z

Done.

HP breed added into dictionary.php
initial spaces converted into tabs by "unexpand" command

infrastation · 2018-08-17T08:31:26Z

wwwroot/inc/deviceconfig.php

+// functions for HP Procurve switches (N.11 OS)
+require_once 'breeds/hpprocurveN1178.php';
+// functions for Cisco IOS 15 switches
+require_once 'breeds/ios15.php';


make install-applib will not handle the new files if they are in a sub-directory.

Should i move new breed functions into deviceconfig.php or update Makefile to handle breed dir content?

Move them one level up to avoid a new directory please.

infrastation · 2018-08-17T08:37:48Z

tests/breed-hpprocurveN1178Test.php

+	{
+	$data_file = 'breeds_data/breed-hpprocurveN1178-8021QConfigSamples.php';
+	if(is_file($data_file)) 
+		return include $data_file;


The array with the samples should be in the provider function, unless there is a good reason to do otherwise.

Also please put the new files in tests, this way express.sh will test them for syntax errors together with the existing files.

The first version was done as you commented above, but the file was huge and it was quite difficult to debug it (even when the array had one-line format). If you still recommends to place sample data into file breed-hpprocurveN1178Test.php, i will update it.

Same as above please (new directories have maintenance cost).

infrastation · 2018-08-17T08:48:59Z

tests/breed-hpprocurveN1178Test.php

+
+	public function testHP8021QConfParser ($test_data)
+	{
+	foreach ($test_data as $test => $test_array)


There is an implied foreach between the provider and the test function, just make sure the arrays are nested properly in the provider function and then the test function can be written to handle exactly one case, PHPUnit will take care of driving the process. See PureFunctionTest.php for examples. It looks like you could just add items to providerBinaryEquals() and avoid a custom test class at all.

A possible way to keep it readable in PureFunctionTest.php would be to have a new helper method like providerHP8021Q() and then to move the existing tests to providerOtherBinaryFunctions() and have providerBinaryEquals() return the full set with something like array_merge ($this->providerOtherBinaryFunctions(), $this->providerHP8021Q())

infrastation · 2018-08-17T08:54:07Z

tests/breed-hpprocurveN1178Test.php

+		$test_descr = $test ." " . $test_array['descr'];
+		$actual = hpprocurveN1178ReadInterfaceStatus ($input);
+		if (! $this->assertEquals($expected, $actual))
+			print_r("\n\tOK! ----> HP Show Interface Brief Parser test " . $test_descr);


When the arguments are not equal, assertEquals() throws instead of returning, so it is likely the message is never printed. Hence the test description could be just a comment (in the PHP code or in the input text).

OK, it is printed, but the if() is a no-op.

infrastation · 2018-08-17T08:58:10Z

tests/breed-hpprocurveN1178Test.php

+	 * @dataProvider providerHP8021QSamples
+	 */
+
+	public function testHPvlanMGMT ($test_data)


This test indeed requires a custom test method (but not a custom test class), so in theory this would work as a method of PureFunctionTest with its own provider function.

infrastation · 2018-08-17T09:17:16Z

tests/breed-hpprocurveN1178Test.php

+
+	// hpprocurveN1178Read8021QConfig
+	/**
+	 * @dataProvider providerHP8021QConfigSamples


The new method(s) should be annotated with @group small, otherwise the new tests are not run by express.sh during the automatic CI testing. You need to run specifically this test (or all tests, including heavy ones) to run this code:

$ phpunit --bootstrap bootstrap_v6v7.php breed-hpprocurveN1178Test.php PHPUnit 6.5.5 by Sebastian Bergmann and contributors. . OK! ----> HP 802.1Q Config Parser test case1 - parse interface brief. OK! ----> HP Show Interface Brief Parser test case1 . OK! ----> HP LLDP Parser test case1 - parse lldp data. OK! ----> HP Read MAC List Parser test case1 - all mac addresses OK! ----> HP Read MAC List Parser test case2 - mac addresses on port. 5 / 5 (100%) OK! ----> HP 8021Q test case1 - access -> access: port 1 vlan 200 -> 201 OK! ----> HP 8021Q test case2 - access -> trunk (+native): port 2 vlan 200 -> T200+201 OK! ----> HP 8021Q test case3 - access -> trunk (-native): port 3 vlan 200 -> T+200,201 OK! ----> HP 8021Q test case4 - trunk (+native) -> access: port 4 T200+201 -> access 200 (old native VID) OK! ----> HP 8021Q test case5 - trunk (+native) -> access: port 5 T200+201 -> access 201 (old tagged VID) OK! ----> HP 8021Q test case6 - trunk (+native) -> access: port 6 T200+201 -> access 202 (new VID) OK! ----> HP 8021Q test case7 - trunk (+native) -> trunk (+native): port 7 T200+201 -> T200+201-202 (add tagged vlan 202) OK! ----> HP 8021Q test case8 - trunk (+native) -> trunk (+native): port 7 T200+201-202 -> T200+201 (remove tagged vlan 202) OK! ----> HP 8021Q test case9 - trunk (+native) -> trunk (-native): port 8 T200+201 -> T+200-201 (keep old native 200 as tagged) OK! ----> HP 8021Q test case10 - trunk (+native) -> trunk (-native): port 9 T200+201 -> T+201 (remove old native 200 totally) OK! ----> HP 8021Q test case11 - trunk (+native) -> trunk (+native): port 10 T200+201 -> T200+202 (change tagged vlan to 202) OK! ----> HP 8021Q test case12 - trunk (-native) -> access: port 20 T+200-201 -> A202 OK! ----> HP 8021Q test case13 - trunk (-native) -> trunk (+native): port 21 T+200-201 -> T200+201 (add native vlan 200) OK! ----> HP 8021Q test case14 - trunk (-native) -> trunk (-native): port 22 T+200-201 -> T+201 (remove tagged vlan 200) OK! ----> HP 8021Q test case15 - trunk (-native) -> trunk (-native): port 23 T+200-201 -> T+200-202 (add tagged vlan 202) OK! ----> HP 8021Q test case16 - trunk (-native) -> trunk (-native): port 24 T+200-201 -> T+202 (change all vlans to new tagged VID) OK! ----> HP 8021Q test case17 - get8021q OK! ----> HP 8021Q test case18 - getlldpstatus OK! ----> HP 8021Q test case19 - getportstatus OK! ----> HP 8021Q test case20 - getmaclist OK! ----> HP 8021Q test case21 - getportmaclist Time: 164 ms, Memory: 10.00MB OK (5 tests, 26 assertions)

infrastation · 2018-08-17T09:42:53Z

Besides the comments above, there is still space for improvements in the PHP sections of the new test files. Let me know which of the discussed issues you would like to fix yourself and which you leave up to me, if any.

infrastation · 2018-08-18T12:47:08Z

For clarity, this is what I had in my mind:

--- a/tests/PureFunctionTest.php
+++ b/tests/PureFunctionTest.php
@@ -12,6 +12,8 @@ identical (assertSame) to the expected return value.
 */
 
 require_once '../wwwroot/inc/popup.php';
+require ('BreedHPPureProvider.php');
+// any other breed-specific files
 
 class PureFunctionTest extends RTTestCase
 {
@@ -124,6 +126,14 @@ class PureFunctionTest extends RTTestCase
  }
 
  public function providerUnaryEquals ()
+ {
+   $ret = self::baseUnaryEquals();
+   $ret = array_merge ($ret, breedHPUnaryEquals());
+   // any other breed-specific additions
+   return $ret;
+ }
+
+ private static function baseUnaryEquals ()
  {
    return array
    (

infrastation · 2018-08-28T13:55:37Z

The tests look much better now, thank you. One thing I would like to do before merging this work is to check if any of the new breed-specific functions are duplicates of the existing ones, please let me have some time to do that.

Before that happens I would ask you to check if the new code translates HP format (xxxxxx-xxxxxx) MAC addresses into the xx:xx:xx:xx:xx:xx form in the web-interface, because this works for all other formats. If it does not, git grep RE_L2 will highlight the currently implemented formats and you will see the required trivial changes.

The difference in mac_sort() behaviour between PHP 5 and PHP 7 is related to usort(), which arranges "equal" items in undefined (differently in different PHP versions) order, and mac_sort() only takes the VLAN ID into account, but not the MAC address. If mac_sort() was covered with tests, this would become visible long ago. In other words, you just made a pre-existing problem with RackTables code visible, so it is OK not to solve it right now.

igloo777 · 2018-08-29T09:42:06Z

Updated.
Umh.. ios12 show mac addresses in web interface in format 000c.2992.6403 - is it ok?

infrastation · 2018-08-29T13:25:46Z

wwwroot/inc/functions.php

@@ -38,6 +38,7 @@
 define ('RE_L2_IFCFG', '/^[0-9A-F]{2}(:[0-9A-F]{2}){5}$/'); // most ifconfigs
 define ('RE_L2_IFCFG_SUNOS', '/^[0-9A-F]{1,2}(:[0-9A-F]{1,2}){5}$/'); // SunOS ifconfig
 define ('RE_L2_CISCO', '/^[0-9A-F]{4}(\.[0-9A-F]{4}){2}$/');
+define ('RE_L2_HP', '/^[0-9A-Fa-z]{6}(-[0-9A-Fa-z]{6}){1}$/');


0-9A-Fa-f of course

even "0-9A-F" (see similar regexps, there is a strtoupper() somewhere)

Also {1} is a no-op, so the regexp is as simple as: /^[0-9A-F]{6}-[0-9A-F]{6}$/

infrastation · 2018-08-29T13:29:28Z

I did not realize the output was not consistent, it is up to you in which format to return it then. The important point is, the HP input format must be recognized (such that the user can copy and paste into the search input or port MAC address field), and the latest commit seems to implement that.

igloo777 · 2018-09-28T04:00:58Z

Hi Denis, how are you doing? Do you have something new about this pull request?

infrastation · 2018-10-14T17:44:27Z

Thank you for waiting. As discussed above, here is the remaining part of the review.

The function hpprocurveN1178SpotConfigText() does not exist, which means in theory this code will not work. Could you check if any changes are missing from the pull request?
ios15ReadCDPStatus() is identical to ios12ReadCDPStatus() hence IOS 15 can just use the latter directly through $breedfunc.
Same for ios15ReadMacList().
Same for ios15SpotConfigText().
The set of IOS 15 802.1Q functions (ios15ReadVLANConfig(), ios15ScanTopLevel(), ios15ReadSwitchPortList(), ios15PickSwitchportCommand(), ios15PickVLANCommand()) is almost identical to the respective IOS 12 functions. Besides $breedfunc indexing, the only changes are in two regexps for a variable amount of whitespace (which is backward compatible with IOS 12) and a removal of one isset() check, which should not actually be removed as far as I can tell (feel free to correct me). In other words, the same stands as above if the regexp changes go into the existing IOS 12 functions.
The only difference in ios15TranslatePushQueue() compared to ios12TranslatePushQueue() is for "getlldpstatus", hence the former should call the latter for all other cases (nxos4TranslatePushQueue() is a good example of this arrangement, as explained before).

To sum it up, the only new code definitely required for IOS 15 support would be ios15ReadLLDPStatus() as it is now and a much shorter version of ios15TranslatePushQueue(). HP functions look OK less some indentation (which can be fixed later) and the tagged/untagged VLAN workarounds, which I cannot validate without some hands-on experience with the actual switches, but for the first version that should be fine.

Please review and update which improvements you are willing to make (I can make the rest after merging).

igloo777 · 2018-10-17T10:14:19Z

Hi Denis! Thank you for comments!

I removed all duplicated functions for ios15 and changed indexing in $breedfunc (only ios15ReadLLDPStatus() and ios15TranslatePushQueue() has been kept)
Added hpprocurveN1178SpotConfigText()

infrastation · 2018-10-17T22:55:58Z

Rebased, squashed and merged, thank you!

As far as a draft commit in pull request #225 went, this is supposed to improve compatibility with IOS 15. [skip ci]

As far as a draft commit in pull request RackTables#225 went, this is supposed to improve compatibility with IOS 15. [skip ci]

infrastation reviewed Aug 17, 2018

View reviewed changes

infrastation reviewed Aug 29, 2018

View reviewed changes

new breeds for HP Procurve and ios15; unit tests for HP Procurve breed

82286a3

infrastation merged commit 82286a3 into RackTables:master Oct 17, 2018

infrastation added a commit that referenced this pull request Oct 17, 2018

relax whitespace in some IOS 12 regexps

39e56f2

As far as a draft commit in pull request #225 went, this is supposed to improve compatibility with IOS 15. [skip ci]

lrupp pushed a commit to lrupp/racktables that referenced this pull request Jul 5, 2019

relax whitespace in some IOS 12 regexps

ca9c1e5

As far as a draft commit in pull request RackTables#225 went, this is supposed to improve compatibility with IOS 15. [skip ci]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New breeds for HP Procurve and ios15; unit tests for HP Procurve breed #225

New breeds for HP Procurve and ios15; unit tests for HP Procurve breed #225

igloo777 commented Aug 14, 2018

infrastation commented Aug 14, 2018

igloo777 commented Aug 15, 2018

infrastation Aug 17, 2018

igloo777 Aug 17, 2018

infrastation Aug 17, 2018

infrastation Aug 17, 2018

infrastation Aug 17, 2018 •

edited

igloo777 Aug 17, 2018

infrastation Aug 17, 2018

infrastation Aug 17, 2018

infrastation Aug 17, 2018

infrastation Aug 17, 2018

infrastation Aug 17, 2018

infrastation Aug 17, 2018

infrastation Aug 17, 2018

infrastation commented Aug 17, 2018

infrastation commented Aug 18, 2018

infrastation commented Aug 28, 2018

igloo777 commented Aug 29, 2018

infrastation Aug 29, 2018

infrastation Aug 29, 2018

infrastation commented Aug 29, 2018

igloo777 commented Sep 28, 2018

infrastation commented Oct 14, 2018

igloo777 commented Oct 17, 2018

infrastation commented Oct 17, 2018

New breeds for HP Procurve and ios15; unit tests for HP Procurve breed #225

New breeds for HP Procurve and ios15; unit tests for HP Procurve breed #225

Conversation

igloo777 commented Aug 14, 2018

infrastation commented Aug 14, 2018

igloo777 commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infrastation Aug 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infrastation commented Aug 17, 2018

infrastation commented Aug 18, 2018

infrastation commented Aug 28, 2018

igloo777 commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infrastation commented Aug 29, 2018

igloo777 commented Sep 28, 2018

infrastation commented Oct 14, 2018

igloo777 commented Oct 17, 2018

infrastation commented Oct 17, 2018

infrastation Aug 17, 2018 •

edited