ixs / napalm-procurve

HP ProCurve Driver for NAPALM automation frontend
Apache License 2.0
21 stars 15 forks source link

procurve telnet functionality #1

Closed pieterdejaeghere closed 6 years ago

pieterdejaeghere commented 6 years ago

I've built on your code to add telnet functionality, improved python3 support and mac address table functionality. If you're interested, you can pull it in.

Thanks, Pieter

ixs commented 6 years ago

Thank you very much for your contribution. Lemme have a look at the code. Generally very happy to merge.

pieterdejaeghere commented 6 years ago

The mac-address table functionality isn't easy to mock as it's not one command that ends up parsed to one output. As procurve doesn't list the vlan_id in "show mac". This means I first get the list of vlans and then end up getting all their mac addresses individually.

Not sure how to get a decent test going without reworking your entire test framework or without removing vlan_id functionality.

ixs commented 6 years ago

The test framework supports multiple commands. Have a look at test_get_lldp_neighbors_detail which does exactly what your code does, first fetch a general list using "show lldp info remote device" and then it gets details such as "show lldp info remote device ethernet 1".

But don't worry about it, I already mocked the right data for the testing framework on my local branch...

But on another note, I just checked the command used. You use show vlan, but as far as I can see on my test switches, "show vlans" is the right command. Can you please double check that this is indeed the case on your device as well? If your device is different, please give product and os version.

pieterdejaeghere commented 6 years ago

you are right, it's "show vlans" on my model too.

ixs commented 6 years ago

Hi @pieterdejaeghere,

I was just in the process of merging your PR and did notice a parsing problem with your mac table code:

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../lib/python2.7/site-packages/napalm_base/test/getters.py:345: in test_get_mac_address_table
    get_mac_address_table = self.device.get_mac_address_table()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <conftest.PatchedProcurveDriver object at 0x1106e9f90>

    def get_mac_address_table(self):
        # Get mac table information
        # The implementation is rather poor, as procurve doesn't list the vlan in the general list, this
        # code first fetches the vlan list, then queries the mac address table for each vlan

        mac_table = []

        command = 'show vlans'
        output = self._send_command(command)

        if 'Invalid input' in output:
            raise ValueError("Command not supported by network device")

        try:
            output = re.split(r'^  -----.*$', output,
                              flags=re.M)[1].strip()
        except IndexError:
            return []

        for line in output.splitlines():
            if len(line.split("|")) == 2 and len(line.split("|")[0].split()) >= 2:
                vlan_id = int(line.split("|")[0].split()[0])
            elif len(line.split("|")) == 2:
                raise ValueError("Unexpected output from: {}".format(line.split("|")[0].split()))
            else:
>               raise ValueError("Unexpected output from: {}".format(line.split("|")))
E               ValueError: Unexpected output from: [u'1              DEFAULT_VLAN Port-based   No    No']

napalm_procurve/procurve.py:692: ValueError
====================================================================== 2 failed, 19 passed, 21 skipped in 1.70 seconds ======================================================================

It seems you are splitting items on a pipe symbol but the procurve devices I have access to do not generate a pipe symbol.

Could you please paste me some sample output here so that I understand what is going on? Out of curiosity, what kind of device do you have?

Thanks.

ixs commented 6 years ago

Closing pull request for now, code that works has been comitted as master.

pieterdejaeghere commented 6 years ago

you are right, it needed spaces as separator instead of a pipe.

I do however still have some weird breakage in that same function from time to time. It might have to do with the re-assignment of output (line 689), can you rename that inner variable?

Same thing for "line" at line 700.

ixs commented 6 years ago

@pieterdejaeghere Can you try adding adding the following code to your script and then see what the output is from the device when the weird breakage happens?

I am pretty sure it is not a problem with the reuse of the the output and line parameters but instead is something that goes wrong parsing the procurve output. I saw that once during my testing but was not able to reproduce.

import logging
logging.basicConfig(filename='test.log', level=logging.DEBUG)
logger = logging.getLogger("netmiko")

Edit: On second thought, maybe you are right with the inner vs outer loop.

Do you have a reproducer for a problem? could you try if it goes away if you rename the vars?

pieterdejaeghere commented 6 years ago

You're right, problem does not disappear with renamed vars (even though its a good idea to fix them)

It are empty lines leaking in in the middle of the vlan list, probably because of netmiko. It happens on my 2626 in about 50% of cases. So a check should be done done at line 686 to see if the line can actually be split before splitting it.

ixs commented 6 years ago

Do you have your debug output still around? Could you please paste it here as code with passwords removed?

That way I can understand what is going on.

pieterdejaeghere commented 6 years ago

I looked at my pycharm debugger, the output variable of the show vlans command had a couple of empty lines sitting in the middle of the output (so vlan x, newline, vlan y, newline, newline, newline, vlan z, newline)

ixs commented 6 years ago

@pieterdejaeghere I pushed a new version which wraps the split in a try/except block.

This should cover the case of unparsable newlines/empty lines. Tests are passing for this.

Would be interesting to see if this also works for you.