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

Try testing with Scapy version updated to 2.5.0 #212

Merged
merged 28 commits into from
Feb 18, 2025
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
58fca24
Try testing with Scapy version updated to 2.5.0
jafingerhut Feb 16, 2025
ff3345b
Changes attempting to get build.yml workflow working on Ubuntu 24.04
jafingerhut Feb 16, 2025
f77941f
More changes to attempt to get tests passing on Ubuntu 24.04 ...
jafingerhut Feb 16, 2025
e55f231
Make cmake builds more verbose
jafingerhut Feb 16, 2025
4dc21c7
Make nanomsg install the same as what I use in my p4-guide script
jafingerhut Feb 16, 2025
f4838fc
A little more debug in the nanomsg install
jafingerhut Feb 16, 2025
7762e30
Extra 'pip --verbose list' commands to see where packages are installed
jafingerhut Feb 16, 2025
7602f72
More tweaks
jafingerhut Feb 16, 2025
15ecb61
Do not upgrade scapy to version later than 2.5.0
jafingerhut Feb 16, 2025
7924ffa
Disable --verbose for nanomsg cmake install
jafingerhut Feb 16, 2025
997d4f5
Install scapy version 2.5.0
jafingerhut Feb 16, 2025
df9c770
More debug info
jafingerhut Feb 16, 2025
78a6d6b
More setting of PYTHONPATH in sudo commands where needed
jafingerhut Feb 16, 2025
affca33
More tweaks to try to get all tests to pass
jafingerhut Feb 16, 2025
5755a87
Automate detection of Python major & minor version
jafingerhut Feb 16, 2025
0413aa4
More tweaking
jafingerhut Feb 16, 2025
4ebea59
Remove some debug commands that seem excessive now
jafingerhut Feb 16, 2025
462c175
Try testing on multiple Ubuntu versions
jafingerhut Feb 16, 2025
cbdab40
Also run test on multiple Ubuntu versions
jafingerhut Feb 16, 2025
cdfadb7
Trim versions to just Ubuntu 22.04 and latest
jafingerhut Feb 16, 2025
e73fa13
Add a second version of Scapy to the test matrix
jafingerhut Feb 16, 2025
c557643
Maybe a fix for dangling CI test
jafingerhut Feb 16, 2025
cce5f72
Address review comments
jafingerhut Feb 18, 2025
e6b4fd3
Merge remote-tracking branch 'up/main' into try-scapy-2.5.0
jafingerhut Feb 18, 2025
75bfc78
Try sudo -E for running Python code as root
jafingerhut Feb 18, 2025
89d920c
Try sudo open --preserve-env for CI test scripts
jafingerhut Feb 18, 2025
c138cca
Reverting previous change, which led to failing CI tests
jafingerhut Feb 18, 2025
c8ab50f
Fix typo
jafingerhut Feb 18, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,41 +11,53 @@ on:

jobs:
verify-python:
strategy:
matrix:
os: [ubuntu-22.04, ubuntu-latest]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use ubuntu-24.04 here instead to avoid failures once this version changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At least one argument used for this in p4lang/p4runtime repo is: when ubuntu-latest changes in 2 years, if it fails, we'll get reminded and update this.

If that argument doesn't work for you, I am happy to change it, as when 26.04 is released, someone might remember to do a sweep of repos to update these kinds of things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changes ubuntu-latest to ubuntu-24.04 in commit 23

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah fixing the version avoids surprises or gaps (for example, when they update the runner to ubuntu-26.04 and now we are missing 24.04 without even knowing).

scapy_version: [2.5.0, 2.6.1]
# Don't abort other runs when one of them fails, to ease debugging.
fail-fast: false

name: Python code verification (src)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this linting or static analysis. Minor comment.

I updated the CI to require these tests now.

runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v4
- name: Install dependencies
run: |
make set-dev
python -m pip install scapy==2.4.5
python -m pip install scapy==${{ matrix.scapy_version }}
- name: Verify code (python w/black)
run: |
make format-check
- name: Run pytests (utests/tests)
run: |
make test
test:
runs-on: ubuntu-latest
strategy:
matrix:
os: [ubuntu-22.04, ubuntu-latest]
scapy_version: [2.5.0, 2.6.1]
# Don't abort other runs when one of them fails, to ease debugging.
fail-fast: false

name: run ptf unit tests
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v4

- name: Dependencies
run: |
sudo apt-get install cmake libffi-dev ethtool python3-dev
# Install the latest pip version (not python3-pip) as older versions
# seem to have issues when installing from source.
curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py
sudo python3 get-pip.py
sudo python3 -m pip install --upgrade setuptools nose2 scapy wheel
python3 -m pip install --upgrade setuptools nose2 wheel
python -m pip install scapy==${{ matrix.scapy_version }}

bash CI/install-nanomsg.sh
sudo ldconfig
bash CI/install-nnpy.sh

- name: Install
run: |
sudo python3 -m pip install .
python3 -m pip install .
ptf --version

- name: Before_script
Expand All @@ -54,6 +66,6 @@ jobs:

- name: Script
run: |
pip --verbose list
python3 CI/check-nnpy.py
./CI/run_tests.sh
sudo python3 /usr/local/bin/nose2 utests.tests.test
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI for reviewers - these tests are still run, but I moved the command that runs them into the ./CI/run_tests.sh script, for easier control of additional command line options.

3 changes: 3 additions & 0 deletions CI/install-nanomsg.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
THIS_DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
source $THIS_DIR/common.sh

set -x
cmake --version
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the motivation for this output?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

debugging CI script behavior.


# nanomsg is very confusing in how it manages SOVERSION vs VERSION, but this
# should be okay... (5.0.0 is the SOVERSION)
check_lib libnanomsg libnanomsg.so.5.0.0
Expand Down
6 changes: 3 additions & 3 deletions CI/install-nnpy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
set -e
git clone https://github.com/nanomsg/nnpy.git
cd nnpy
sudo python3 -m pip install cffi
sudo python3 -m pip install --upgrade cffi
sudo python3 -m pip install .
python3 -m pip install cffi
python3 -m pip install --upgrade cffi
python3 -m pip install .
cd ..
5 changes: 5 additions & 0 deletions CI/python-major-minor-version.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/usr/bin/env python

import sys

print(f"{sys.version_info.major}.{sys.version_info.minor}")
27 changes: 20 additions & 7 deletions CI/run_tests.sh
Original file line number Diff line number Diff line change
@@ -1,23 +1,36 @@
#!/usr/bin/env bash

sudo python ptf_nn/ptf_nn_agent.py \
set -x

THIS_SCRIPT_FILE_MAYBE_RELATIVE="$0"
THIS_SCRIPT_DIR_MAYBE_RELATIVE="${THIS_SCRIPT_FILE_MAYBE_RELATIVE%/*}"
THIS_SCRIPT_DIR_ABSOLUTE=`readlink -f "${THIS_SCRIPT_DIR_MAYBE_RELATIVE}"`

MAJOR_MINOR_VERSION=`${THIS_SCRIPT_DIR_ABSOLUTE}/python-major-minor-version.py`
PPATH="$HOME/.local/lib/python${MAJOR_MINOR_VERSION}/site-packages"

sudo PATH=${PATH} PYTHONPATH=${PPATH} python3 ptf_nn/ptf_nn_agent.py \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these path hacks necessary btw?

Maybe we should consider setting up virtualenv or poetry if Ubuntu24.04 requires it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I guess because of the use of sudo.

Although sudo -E env PATH="$PATH" should be enough here. If the Python environment is set up correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you set up virtualenv as the normal user, then you pick a directory where that is installed, and using it typically either requires, or recommends, first running source $MYVENV/bin/activate, which might do other things, but perhaps the most crucial is setting the environment variable VIRTUAL_ENV.

If you know a way to run commands as root that don't do one of (a) pass the environment variable values on to the root user, as shown in my current sudo commands, or (b) somehow do source $MYVENV/bin/activate ; <other command I really want to run as root>, I'm interested to learn what that is.

I don't know of any way in which virtual environments somehow avoid this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And if I recall correctly, doing what I am doing now, where the root user uses a normal user's lib/python* directory, isn't merely read-only, but the root user can also write .pyc files and __pycache__ directories that the normal user then does not have permission to modify later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a way around this unless you disable sudo's path protection: https://unix.stackexchange.com/a/151188

I have found sudo -E env PATH="$PATH" the least invasive approach

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

commit 25 uses sudo -E env PATH="$PATH" ... with success. Thanks for the suggestion.

Copy link
Contributor

@fruffy fruffy Feb 18, 2025

Choose a reason for hiding this comment

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

https://unix.stackexchange.com/a/480764

Might even remove the need to use any dollar signs. But I have not tried that command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Trying that variation with commit 26 to see if it passes the tests.

At some point, we have to stop tweaking and use something that works :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It failed. Reverted that change in commit 27, back to what it was in commit 25.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was mostly interested because of p4lang/p4c#5047
Some of the lessons here I would like to apply to that PR.

--device-socket 0@tcp://127.0.0.1:10001 -i 0-1@veth0 \
&>/dev/null &
&

sleep 5

sudo python ptf_nn/ptf_nn_agent.py \
sudo PATH=${PATH} PYTHONPATH=${PPATH} python3 ptf_nn/ptf_nn_agent.py \
--device-socket 1@tcp://127.0.0.1:10002 -i 1-1@veth3 \
&>/dev/null &
&

sleep 5

sudo python ptf_nn/ptf_nn_test_bridge.py -ifrom veth1 -ito veth2 \
&>/dev/null &
sudo PATH=${PATH} PYTHONPATH=${PPATH} python3 ptf_nn/ptf_nn_test_bridge.py -ifrom veth1 -ito veth2 \
&

sleep 5

sudo ptf --test-dir ptf_nn/ptf_nn_test \
env
Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious?

Copy link
Collaborator Author

@jafingerhut jafingerhut Feb 18, 2025

Choose a reason for hiding this comment

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

Removed in commit 28. I don't know how I got that in there.


sudo PATH=${PATH} PYTHONPATH=${PPATH} `which ptf` --test-dir ptf_nn/ptf_nn_test \
--device-socket 0-{0-64}@tcp://127.0.0.1:10001 \
--device-socket 1-{0-64}@tcp://127.0.0.1:10002 \
--platform nn

sudo PATH=${PATH} PYTHONPATH=${PPATH} python3 `which nose2` utests.tests.test
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,15 @@ More information about Black, you find at
The following software is required to run PTF:

* Python 3.x
* Scapy 2.4.5 (unless you provide another packet manipulation module)
* Scapy 2.5.0 (unless you provide another packet manipulation module)
* pypcap (optional - VLAN tests will fail without this)
* tcpdump (optional - Scapy will complain if it's missing)

Root/sudo privilege is required on the host, in order to run `ptf`.

The default packet manipulator tool for `ptf` is `Scapy`. To install it use:
```text
pip install scapy==2.4.5
pip install scapy==2.5.0
```

To enable VLAN tests, you need to install `pypcap`:
Expand Down