napalm-automation / napalm-base

Apache License 2.0
33 stars 48 forks source link

ping() method implementation is inconsistent #315

Open ktbyers opened 6 years ago

ktbyers commented 6 years ago

This is definition of the ping() method in napalm-base:

            {
                'success': {
                    'probes_sent': 5,
                    'packet_loss': 0,
                    'rtt_min': 72.158,
                    'rtt_max': 72.433,
                    'rtt_avg': 72.268,
                    'rtt_stddev': 0.094,
                    'results': [
                        {
                            'ip_address': u'1.1.1.1',
                            'rtt': 72.248
                        },
                        {
                            'ip_address': '2.2.2.2',
                            'rtt': 72.299
                        }
                    ]
                }
            }
            OR
            {
                'error': 'unknown host 8.8.8.8.8'
            }

napalm-ios, napalm-nxos (SSH), napalm-eos are not implemented this way on the error case.

Also I don't think it is the right data structure. I propose we change it to the following:

{
                    'status': 'success'      (or 'error')
                    'message':  'for returning error messages or other info'
                    'probes_sent': 5,
                    'packet_loss': 0,
                    'rtt_min': 72.158,
                    'rtt_max': 72.433,
                    'rtt_avg': 72.268,
                    'rtt_stddev': 0.094,
                    'results': [
                        {
                            'ip_address': u'1.1.1.1',
                            'rtt': 72.248
                        },
                        {
                            'ip_address': '1.1.1.1',
                            'rtt': 72.299
                        }
                    ]
}

We also should clearly define what 'error' means i.e. all probes dropped; one probe dropped, complete failure to ping (like a DNS failure).

mirceaulinic commented 6 years ago

Yes, I agree on the structure you propose @ktbyers. I think we should start discussing where the merged napalm will be hosted, open issues for the wishlist, and solve them after the merge. This one certainly is one of the things we should solve in a later version, with much advance notification.

ktbyers commented 6 years ago

Since the current data structure is totally inconsistent between platforms...the advance notification probably shouldn't be that long (as it is already totally messed up).