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

[sflow] Fix port_index_mapper.py script; Convert to Python 3 #5800

Merged
merged 2 commits into from
Nov 6, 2020
Merged

[sflow] Fix port_index_mapper.py script; Convert to Python 3 #5800

merged 2 commits into from
Nov 6, 2020

Conversation

GarrickHe
Copy link
Contributor

@GarrickHe GarrickHe commented Nov 4, 2020

- Why I did it
A memory issue was discovered during system test for scaling. The issue is documented here.

https://docs.pyroute2.org/ipdb.html

One of the major issues with IPDB is its memory footprint. It proved not to be suitable for environments with thousands of routes or neighbours. Being a design issue, it could not be fixed, so a new module was started, NDB, that aims to replace IPDB. IPDB is still more feature rich, but NDB is already more fast and stable.

- How I did it

  • Rewrote the port_index_mapper.py script to use dB events.
  • Convert to Python 3

- How to verify it
Enabled the sflow container and verified the PORT_INDEX_TABLE was populated properly

- Which release branch to backport (provide reason below if selected)

  • 201811
  • [] 201911
  • 202006

- Description for the changelog

  • Refactored port-index-mapper script due to memory scaling issue
    of the IPDB library.

Signed-off-by: Garrick He [email protected]

* Refactored port-index-mapper script due to memory scaling issue
  of the IPDB library.

Signed-off-by: Garrick He <[email protected]>
@jleveque
Copy link
Contributor

jleveque commented Nov 4, 2020

@GarrickHe: Thanks for the fixes. Python 2 is no longer supported, and we are migrating all Python code to Python 3. I have a pull request open to convert this script to Python 3 and use the logger from sonic-py common. However, it seems you are more familiar with this script than I, so you would be able to test it more thoroughly. Would you mind also converting this script to Python 3 and use the logger from sonic-py common in this PR? If so, I will close my other PR.

@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2020

This pull request fixes 1 alert when merging 783664b into 215ce13 - view on LGTM.com

fixed alerts:

  • 1 for Unreachable code

@GarrickHe
Copy link
Contributor Author

@jleveque - Sure. I'll change it to python 3

@GarrickHe
Copy link
Contributor Author

@jleveque - I changed to python3 but the script is no longer populating the dB. Is there a way I can show you my changes?

@jleveque
Copy link
Contributor

jleveque commented Nov 4, 2020

@GarrickHe: Are you testing against the latest master HEAD? This PR was merged yesterday. If missing, it might be part of your problem?

#5757

If you can confirm missing this commit is not the cause, I guess you could push your new changes to this PR and we can review them.

@GarrickHe
Copy link
Contributor Author

@jleveque - nevermind. looks like python3 got another library to convert ifname to ifindex. I just had to use that one instead.

* Update the port_index_mapper.py to use Python3

Signed-off-by: Garrick He <[email protected]>
@GarrickHe
Copy link
Contributor Author

@jleveque - I updated the script and retested. It works and the PORT_INDEX_TABLE in stateDB shows the proper information.

@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2020

This pull request fixes 1 alert when merging ef2d83e into 215ce13 - view on LGTM.com

fixed alerts:

  • 1 for Unreachable code

@jleveque jleveque changed the title [sflow] Fix port-mapper script [sflow] Fix port_index_mapper.py script; Convert to Python 3 Nov 5, 2020
@jleveque
Copy link
Contributor

jleveque commented Nov 5, 2020

@padmanarayana: Please review

@jleveque jleveque merged commit 27a911f into sonic-net:master Nov 6, 2020
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
…et#5800)

**- Why I did it**
A memory issue was discovered during system test for scaling. The issue is documented here: https://docs.pyroute2.org/ipdb.html

> One of the major issues with IPDB is its memory footprint. It proved not to be suitable for environments with thousands of routes or neighbours. Being a design issue, it could not be fixed, so a new module was started, NDB, that aims to replace IPDB. IPDB is still more feature rich, but NDB is already more fast and stable.

**- How I did it**
- Rewrote the port_index_mapper.py script to use dB events.
- Convert to Python 3
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