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

Add python3 virtual environment for docker-ptf #10599

Merged
merged 6 commits into from
Apr 26, 2022

Conversation

ZhaohuiS
Copy link
Contributor

@ZhaohuiS ZhaohuiS commented Apr 18, 2022

Why I did it

Migrate ptftests script to python3, in order to do an incremental migration, add python virtual environment firstly, install all required python packages in virtual env as well.
Then migrate ptftests scripts from python2 to python3 one by one avoid impacting non-changed scripts.

Signed-off-by: Zhaohui Sun [email protected]

How I did it

Add python3 virtual environment for docker-ptf.
Add submodule ptf-py3 and install patched ptf 0.9.3 into virtual environment as well, two ptf issues were reported here:
p4lang/ptf#173
p4lang/ptf#174

How to verify it

login docker-ptf container and check if there is a folder /root/env-python3/bin

  1. Connect to the docker-ptf container, take kvm testbed as example as below:
$ docker exec -ti ptf_vms6-1 bash
  1. Activate the virtual environment
$ source /root/env-python3/bin/activate
(env-python3) root@70d4f7e9dfcc:/# python
Python 3.7.3 (default, Jan 22 2021, 20:04:44) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.

>>> import ptf

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

wangxin
wangxin previously approved these changes Apr 21, 2022
Copy link
Contributor

@wangxin wangxin left a comment

Choose a reason for hiding this comment

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

LGTM. Just a few comments need improvements.


RUN python3 -m pip install --upgrade --ignore-installed pip

# Install all python modules from pypi. python-scapy is exception, ptf debian package requires python-scapy
Copy link
Contributor

Choose a reason for hiding this comment

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

In the comment, python-scapy -> python3-scapy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

RUN python3 -m pip install --upgrade --ignore-installed pip

# Install all python modules from pypi. python-scapy is exception, ptf debian package requires python-scapy
# TODO: Clean up this step
Copy link
Contributor

Choose a reason for hiding this comment

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

What to be cleaned up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What to be cleaned up?

Yes, remove this useless comment.

Copy link
Contributor

@Pterosaur Pterosaur left a comment

Choose a reason for hiding this comment

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

Should we combine ptf-py3 and ptf-py3.patch to one folder,ptf-py3, or combine the existing ptf with them to a same folder ptf? I think that will look better to code structure.

Please ignore this comment, because I saw other folders that follow this convention.

@ZhaohuiS
Copy link
Contributor Author

Yes, exactly, I just followed the same rule as scapy patch in this PR #10457

@ZhaohuiS
Copy link
Contributor Author

@lguohan @qiluo-msft @xumia Could you please review and approve this change? Thank you.

&& pip3 install supervisor \
&& pip3 install ipython==5.4.1 \
&& pip3 install Cython \
&& git clone https://github.com/sflow/sflowtool \
Copy link
Collaborator

Choose a reason for hiding this comment

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

python venv should be python, not sure if sflowtool, openbfdd, nanomsg has python part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python venv should be python, not sure if sflowtool, openbfdd, nanomsg has python part?

@lguohan You are right, they are not python part, checked existing test cases, they don't have to be in virtual environment. Remove this part in my latest commit.

The usage is as below.

/usr/local/bin/sflowtool
bfdd-beacon

@ZhaohuiS ZhaohuiS merged commit cc30771 into sonic-net:master Apr 26, 2022
ZhaohuiS added a commit to sonic-net/sonic-mgmt that referenced this pull request Apr 28, 2022
What is the motivation for this PR?
PR sonic-net/sonic-buildimage#10599 move the work directory to /root.
So gnxi is moved to /root. test_telemetry failed because of wrong file location.

How did you do it?
Update /gnxi/... to /root/gnxi/...

How did you verify/test it?
run tests/telemetry/test_telemetry.py

Signed-off-by: Zhaohui Sun <[email protected]>
wangxin pushed a commit to sonic-net/sonic-mgmt that referenced this pull request May 11, 2022
What is the motivation for this PR?
gnxi's location is updated from /gnxi to /root/gnxi in RP sonic-net/sonic-buildimage#10599.
But if test case run on old docker-ptf image which doesn't have this update, it will fail. Because it should still call /gnxi files.

How did you do it?
For avoiding this conflict, check gnxi path before test and set GNXI_PATH to correct value.
Add it in verify_telemetry_dockerimage autouse module fixture to make sure to set GNXI_PATH before test.

How did you verify/test it?
run telemetry/test_telemetry.py

Signed-off-by: Zhaohui Sun <[email protected]>
wangxin pushed a commit to sonic-net/sonic-mgmt that referenced this pull request May 11, 2022
What is the motivation for this PR?
PR sonic-net/sonic-buildimage#10599 move the work directory to /root.
So gnxi is moved to /root. test_telemetry failed because of wrong file location.

How did you do it?
Update /gnxi/... to /root/gnxi/...

How did you verify/test it?
run tests/telemetry/test_telemetry.py

Signed-off-by: Zhaohui Sun <[email protected]>
wangxin pushed a commit to sonic-net/sonic-mgmt that referenced this pull request May 11, 2022
What is the motivation for this PR?
gnxi's location is updated from /gnxi to /root/gnxi in RP sonic-net/sonic-buildimage#10599.
But if test case run on old docker-ptf image which doesn't have this update, it will fail. Because it should still call /gnxi files.

How did you do it?
For avoiding this conflict, check gnxi path before test and set GNXI_PATH to correct value.
Add it in verify_telemetry_dockerimage autouse module fixture to make sure to set GNXI_PATH before test.

How did you verify/test it?
run telemetry/test_telemetry.py

Signed-off-by: Zhaohui Sun <[email protected]>
liushilongbuaa pushed a commit to liushilongbuaa/sonic-buildimage that referenced this pull request Jun 20, 2022
Related work items: #49, #58, #107, sonic-net#247, sonic-net#249, sonic-net#277, sonic-net#593, sonic-net#597, sonic-net#1035, sonic-net#2130, sonic-net#2150, sonic-net#2165, sonic-net#2169, sonic-net#2178, sonic-net#2179, sonic-net#2187, sonic-net#2188, sonic-net#2191, sonic-net#2195, sonic-net#2197, sonic-net#2198, sonic-net#2200, sonic-net#2202, sonic-net#2206, sonic-net#2209, sonic-net#2211, sonic-net#2216, sonic-net#7909, sonic-net#8927, sonic-net#9681, sonic-net#9733, sonic-net#9746, sonic-net#9850, sonic-net#9967, sonic-net#10104, sonic-net#10152, sonic-net#10168, sonic-net#10228, sonic-net#10266, sonic-net#10288, sonic-net#10294, sonic-net#10313, sonic-net#10394, sonic-net#10403, sonic-net#10404, sonic-net#10421, sonic-net#10431, sonic-net#10437, sonic-net#10445, sonic-net#10457, sonic-net#10458, sonic-net#10465, sonic-net#10467, sonic-net#10469, sonic-net#10470, sonic-net#10474, sonic-net#10477, sonic-net#10478, sonic-net#10482, sonic-net#10485, sonic-net#10488, sonic-net#10489, sonic-net#10492, sonic-net#10494, sonic-net#10498, sonic-net#10501, sonic-net#10509, sonic-net#10512, sonic-net#10514, sonic-net#10516, sonic-net#10517, sonic-net#10523, sonic-net#10525, sonic-net#10531, sonic-net#10532, sonic-net#10538, sonic-net#10555, sonic-net#10557, sonic-net#10559, sonic-net#10561, sonic-net#10565, sonic-net#10572, sonic-net#10574, sonic-net#10576, sonic-net#10578, sonic-net#10581, sonic-net#10585, sonic-net#10587, sonic-net#10599, sonic-net#10607, sonic-net#10611, sonic-net#10616, sonic-net#10618, sonic-net#10619, sonic-net#10623, sonic-net#10624, sonic-net#10633, sonic-net#10646, sonic-net#10655, sonic-net#10660, sonic-net#10664, sonic-net#10680, sonic-net#10683
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.

4 participants