Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement get_interfaces_ip #24

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

DavidVentura
Copy link
Contributor

This implements #23 ; get_interfaces_ip
Signed-off-by: DavidVentura [email protected]

@bewing
Copy link
Collaborator

bewing commented Feb 16, 2022

Looks like there's a linting error -- can you blacken your change and amend your commit?

https://github.com/psf/black

@DavidVentura DavidVentura force-pushed the develop branch 2 times, most recently from 71b99d4 to 94ba753 Compare February 16, 2022 18:26
@DavidVentura
Copy link
Contributor Author

Whoops, now linted

@bewing
Copy link
Collaborator

bewing commented Feb 16, 2022

Thank you for this additional feature! However, you've highlighted what are probably some poor decisions I've made in the past regarding interface key naming.

Currently, get_interfaces returns data with keys like et1 and ma1. This proposed function uses keys like Ma1.

Ideally, we'd have the same key refer to the same interface across all the functions. The NX-OS/IOS drivers uses napalm.base.helpers.canonical_interface_name, but even that approach has some issues for us:

In [1]: from napalm.base.helpers import abbreviated_interface_name,
   ...: canonical_interface_name

In [2]: canonical_interface_name("Ma1")
Out[2]: 'Management1'

In [3]: canonical_interface_name("et1")
Out[3]: 'Ethernet1'

In [4]: canonical_interface_name("ma1")
Out[4]: 'ma1'

In [5]: abbreviated_interface_name("Management1")
Out[5]: 'Ma1'

In [6]: abbreviated_interface_name("Ethernet1")
Out[6]: 'Et1'

Note that ma1 doesn't get expanded to Management1 as one would expect.

At minimum, get_interfaces_ip should be able to return the same keys as get_interfaces, but there's no real good way to do that off the top of my head. We may have to bite the bullet and create a breaking change to the older getters to switch to using canonical names. I'm willing to hear other thoughts, though!

@DavidVentura
Copy link
Contributor Author

I agree on both counts: get_interfaces_ip's keys should be a subset of get_interfaces's keys; and also both should be canonicalized. However those two things can be done separately.

If you want to avoid breaking back-compat right now; I see two options:

  1. Assume interface names are always lower case (reasonable, see below), force the new function to return lower case (should get_interfaces pseudo break back-compat and also be forced lowercase?)
  2. Have get_interfaces_ip call get_interfaces to match key-casing (seems pretty bad, end up executing 3 commands to ensure consistency)

I think lower-casing is reasonable; I have a variety of devices:

OS Version

      1 "0.24.0"
      1 "0.35.1"
      2 "0.26.1"
      2 "0.26.2"
      8 "0.22.2"
     37 "0.30.0"

Model:

      1 "MetaConnect 48"
      1 "MetaMux 48 with K-Series Plus"
      2 "MetaConnect 16"
     12 "MetaApp 32 with K-Series Plus"
     35 "MetaMux 48 with L-Series"

and they all return lower-case interfaces.

Otherwise, break compatibility to ensure these two functions will always return compatible keys

@bewing
Copy link
Collaborator

bewing commented Feb 24, 2022

Sorry for the delay on this. Reviewed with some other users as well, and the consensus is to go with all lowercase keys for now, and if/when we do a major version upgrade in napalm_mos, switch to canonical. This may/may not happen at all given the move towards EOS with the newer devices.

Can you update your changeset to reflect?

Signed-off-by: DavidVentura <[email protected]>
@bewing
Copy link
Collaborator

bewing commented Mar 3, 2022

Thank you for contributing!

@bewing bewing merged commit fa74f90 into napalm-automation-community:develop Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants