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

resolve #1293 #1382

Merged
merged 7 commits into from
Mar 8, 2021
Merged

resolve #1293 #1382

merged 7 commits into from
Mar 8, 2021

Conversation

minefuto
Copy link
Contributor

This is related to #1293.
I added support for logical interface counters of Junos.

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.

Good start @minefuto, thanks for having a go at this. I've requested some changes, as I wouldn't want to increase the complexity while the data is already available.

@@ -403,11 +403,18 @@ def get_interfaces_counters(self):
"""Return interfaces counters."""
query = junos_views.junos_iface_counter_table(self.device)
query.get()
query_logical = junos_views.junos_logical_iface_counter_table(self.device)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should make a separate request just for this: the logical interfaces are already present in the junos_iface_counter_table, we just need to extract them. For this we can use the nested tables junos-eznc offers, see an example here where a view is able to reference a sub-table. In the same way, we can have the logical interfaces nested under a sub-table, and avoid an additional RPC request.

@minefuto
Copy link
Contributor Author

minefuto commented Mar 5, 2021

Thanks, @mirceaulinic.
I think I was able to implement it as per your requests.
Sorry, but I have no confidence because I'm not an expert python/junos-eznc.
Could you please review it again?

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.

Hey @minefuto - thanks for making the changes. It is slightly different from what I had in mind, but I like this implementation too. 😄

@mirceaulinic mirceaulinic merged commit d733d47 into napalm-automation:develop Mar 8, 2021
@minefuto minefuto deleted the issue-1293 branch March 9, 2021 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants