Skip to content
This repository has been archived by the owner on Sep 17, 2019. It is now read-only.

ping() method implementation is inconsistent #315

Open
ktbyers opened this issue Oct 7, 2017 · 2 comments
Open

ping() method implementation is inconsistent #315

ktbyers opened this issue Oct 7, 2017 · 2 comments

Comments

@ktbyers
Copy link
Contributor

ktbyers commented Oct 7, 2017

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
Copy link
Member

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
Copy link
Contributor Author

ktbyers commented Oct 9, 2017

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).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants