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.

Sign up for GitHub

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

Jump to bottom

Initial version for nxos_ssh get_optics #1215

Merged
merged 10 commits into from
Jul 16, 2020
Merged

Initial version for nxos_ssh get_optics #1215

merged 10 commits into from
Jul 16, 2020

Conversation

ExaneServerTeam
Copy link
Contributor

Implements nxos_ssh get_optics function

  • | json does not work, we have to parse the Terminal
  • Extracts current / input / ouput metrics
  • Also retrieves Gib data
  • Maps Gbic type to Generic YANG types (Reference the Gbics We own so far)

@ExaneServerTeam
Initial version for nxos_ssh get_optics
be90c15 
Implements nxos_ssh get_optics function

* | json does not work, we have to parse the Terminal
* Extracts current / input / ouput metrics
* Also retrieves Gib data
* Maps Gbic type to Generic YANG types (Reference the Gbics We own so far)
@coveralls
Copy link

coveralls commented May 14, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 0.0% when pulling ac35ddf on ExaneServerTeam:feature/nxos_ssh_get_optics into 6529ad1 on napalm-automation:develop.

@ExaneServerTeam
Copy link
Contributor Author

Hello
You will find an implementation of Nexus get_optics. I tries to map the openconfig Yand format.
Sadly as stated, the json pipe did not work on my plateform.

Vendor Type is pretty useful but I do not know where to place it in the Json file (See
conntector_type_map for samples). I you have any idea on this matter it would be welcome.

mirceaulinic
mirceaulinic reviewed May 14, 2020
View reviewed changes
Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good on the surface, we'll ask you to provide some test data as well.

@ktbyers what was the approach for the nxos/nxos-ssh, do we want feature parity between them, or we look at them as separate drivers?

napalm/nxos_ssh/nxos_ssh.py Outdated

port_detail["state"] = state
optics_detail[port] = port_detail
# print(port_detail)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this line please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spurious print statements have been removed with the last commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with a sample test

@mirceaulinic mirceaulinic added this to the 3.1.0 milestone May 14, 2020
mirceaulinic
mirceaulinic previously approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Will defer to @ktbyers if he has any comments or suggestions.

@mirceaulinic mirceaulinic requested a review from ktbyers May 14, 2020 14:36
mirceaulinic
mirceaulinic reviewed May 14, 2020
View reviewed changes
napalm/nxos_ssh/nxos_ssh.py
if "transceiver is not present" in port_ts:
continue
if "transceiver is not applicable" in port_ts:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a personal curiosity... what does "transceiver is not applicable" mean in this context? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirceaulinic the not applicable state applies to the ports related to the HP C7000 Enclosure B22 switches.

@ExaneServerTeam
Copy link
Contributor Author

@mirceaulinic & @ktbyers: A welcome addition would be to have the cisco type of Gbic set in the Json / Dict. Sample are 1000base-LH, 1000base-SX, 10Gbase-LR ...
Do you have any idea where it could be set ? I have tried to find a place in the YANG Transceiver Tree but can't really find out a good place . May be in one of those two descriptions ?

+--rw physical-channels
          +--rw channel* [index]
             +--rw index     -> ../config/index
             +--rw config
             |  +--rw index?                 uint16
             |  +--rw description?           string
             |  +--rw tx-laser?              boolean
             |  +--rw target-output-power?   decimal64
             +--ro state
                +--ro index?                 uint16
                +--ro description?           string

@mirceaulinic mirceaulinic modified the milestones: 3.2.0, 3.1.0 Jul 13, 2020
mirceaulinic
mirceaulinic approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. As always, thanks again for your contribution @ExaneNetworkTeam.
I don't suppose you have access to any Nexus switches to implement the counter-part API-based get_optics (in the "main" nxos driver)?

@mirceaulinic mirceaulinic merged commit a2743ea into napalm-automation:develop Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirceaulinic mirceaulinic mirceaulinic approved these changes

@ktbyers ktbyers Awaiting requested review from ktbyers

Assignees
No one assigned
Labels
method implementation nxos-ssh
Projects
None yet
Milestone
3.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ExaneServerTeam @coveralls @mirceaulinic