Skip to content

Modify CI tests to use bf-pktpy module, with no scapy, except for EBPF backend tests #5145

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

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Modify CI tests to use bf-pktpy module, with no scapy, except for EBPF backend tests #5145

Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
83fe9fa
Try testing p4c using bf-pktpy module, and without scapy installed
jafingerhut Feb 23, 2025
f33526a
Install bf-pktpy module from new public git repo
jafingerhut Feb 23, 2025
39aeab2
Changes to hopefully enable last 2 failing tests to pass
jafingerhut Feb 23, 2025
16b81ad
Allow multiple possible versions of Python module six
jafingerhut Feb 23, 2025
a9914fa
Remove mentions of scapy where it is no longer required.
jafingerhut Feb 24, 2025
f00659a
Force the use of scapy in module ptf.packet for EBPF tests
jafingerhut Feb 24, 2025
08d3bc5
Remove debug steps that verify what Python packages are ..
jafingerhut Feb 24, 2025
0e2adff
Try installing bf-pktpy from a subdirectory of open-p4studio repo
jafingerhut Feb 24, 2025
fc52efa
Try different syntax in requirements.txt for module in subdir
jafingerhut Feb 24, 2025
d7ab59f
Try 3 for correct requirements.txt syntax for subdir in repo
jafingerhut Feb 24, 2025
917521b
Fourth time is a charm?
jafingerhut Feb 24, 2025
7effb81
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut Mar 4, 2025
4663b50
Replace unnecessary use of Scapy with ptf.pcap_writer in p4tc tests
jafingerhut Mar 5, 2025
639e63c
Merge remote-tracking branch 'up/main' into replace-scapy-with-apache…
jafingerhut Mar 5, 2025
5aafcf8
Use black to reformat modified Python source file
jafingerhut Mar 5, 2025
2e17ebe
More changes to appease Python black
jafingerhut Mar 5, 2025
ab8c527
Merge branch 'replace-scapy-with-apache-code-in-p4tc-tests' into test…
jafingerhut Mar 5, 2025
8b184e1
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut Mar 6, 2025
49b3f5f
Modify new test program send_packet use ptf but not (directly) scapy
jafingerhut Mar 6, 2025
0a79fb1
Try different syntax for requirements.txt
jafingerhut Mar 6, 2025
652d722
Use a personal repo for bf-pktpy module source for now
jafingerhut Mar 6, 2025
63c4d63
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut Mar 14, 2025
a519c93
Update version of ptf code in requirements.txt
jafingerhut Mar 14, 2025
40ade0c
Remove separate bf_pktpy in requirements.txt, replace with version in…
jafingerhut Mar 14, 2025
53bcaa8
Use fork of ptf
jafingerhut Mar 14, 2025
7695ada
Remove packages on which bf_pktpy depends from requirements.txt
jafingerhut Mar 14, 2025
c550eb3
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut Mar 14, 2025
17ce595
Update ptf to latest released version after recent PR merge
jafingerhut Mar 14, 2025
28a6c26
Try a change in CI to see if EBPF tests using ptf with bf_pktpy passes
jafingerhut Mar 20, 2025
71dcbae
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut Mar 20, 2025
d039511
Move scapy install into ci-build.sh script, out of CI yml file
jafingerhut Mar 20, 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
2 changes: 1 addition & 1 deletion .github/workflows/ci-p4tools.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,5 @@ jobs:

- name: Run tests (Ubuntu 22.04)
# Need to use sudo for the eBPF kernel tests.
run: sudo -E ctest -R "testgen|smith" --output-on-failure --schedule-random
run: sudo -E PTF_PACKET_MANIPULATION_MODULE="bf_pktpy.ptf.packet_pktpy" ctest -R "testgen|smith" --output-on-failure --schedule-random
Copy link
Collaborator

@fruffy fruffy Mar 18, 2025

Choose a reason for hiding this comment

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

Not a big fan of this approach, let's wait until bf_pktpy is default?

Also just noticed, but packet_pktpy appears to be a redundant name.

Copy link
Contributor Author

@jafingerhut jafingerhut Mar 18, 2025

Choose a reason for hiding this comment

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

Regarding making bf_pktpy the default choice for the ptf package: ptf has been around for 10 years, it is used in SONiC testing, and I have no idea how many other places it might be used now. My guess is that changing the default now is going to cause others pain for years to come, and us in the form of issues.

Is there an approach that is not based on environment variables that you are a fan of, that you can think of?

Copy link
Collaborator

@fruffy fruffy Mar 18, 2025

Choose a reason for hiding this comment

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

Can we confirm that this is actually the case? If the behavior of PTF is equivalent with bf_pktpy then downstream repositories should not notice any issues. If not, there is a bug in bf_pktpy. In general, I'd assume these repositories probably pull in scapy separately, like P4C did. Or they pin their PTF version.

Copy link
Contributor Author

@jafingerhut jafingerhut Mar 19, 2025

Choose a reason for hiding this comment

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

Can we confirm whether this will cause other projects pain for years to come, and us in the form of issues? I mean, sure, we can just change the default and find out in the next 2-3 years :-). The thing about a long-public project is that often you do not even know who all of its users are.

Copy link
Collaborator

@fruffy fruffy Mar 19, 2025

Choose a reason for hiding this comment

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

The thing about a long-public project is that often you do not even know who all of its users are.

That is the exact problem to me. We do not have paying customers and users are not providing feedback so we are dealing with an effectively imaginary user base. I rather move on and make changes that are beneficial for the organization and keep complexity low instead of locking us down. We do not have the resources to be backwards compatible.

I also think it is common (good) practice to pin your dependencies, if a project always uses the latest version of software we provide, that's on them. We can change the major version of PTF to signal that it is a breaking change.

Copy link
Contributor Author

@jafingerhut jafingerhut Mar 20, 2025

Choose a reason for hiding this comment

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

OK. Let us imagine for a moment that we do change the default packet manipulation module in the ptf package to bf_pktpy.

In a recent commit today, I tried out running the EBPF tests using ptf with bf_pktpy as the packet manipulation module. It fails, raising exceptions from bf_pktpy about attributes and/or methods that the tests are trying to use not being present.

I am personally not interested in tracking these down and attempting to update bf_pktpy, not even to see how much work it might be. If we want EBPF backend tests to keep working without potentially significant time spent changing them, the most straightforward way is to use scapy as ptf's packet manipulation module for those tests. The way the tests are run right now, that can be done with extra command line options to the ptf command.

I am guessing that there will be other ptf users that will similarly want to continue using Scapy as its packet manipulation module, because of such issues.

Are you OK with recommending to those other people that they use the environment variable PTF_PACKET_MANIPULATION_MODULE="ptf.packet_scapy" to continue using Scapy as their packet manipulation module, if they want to do so?

If not, then I am currently at a loss for how to proceed.

Copy link
Collaborator

@fruffy fruffy Mar 20, 2025

Choose a reason for hiding this comment

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

Are you OK with recommending to those other people that they use the environment variable PTF_PACKET_MANIPULATION_MODULE="ptf.packet_scapy" to continue using Scapy as their packet manipulation module, if they want to do so?

Yes, seems reasonable to me. Like other Python projects we can print a big deprecation warning for a couple months that we are going to switch and then eventually switch with a major PTF release. This gives users enough of a headsup. Does that work?

Copy link
Contributor Author

@jafingerhut jafingerhut Mar 20, 2025

Choose a reason for hiding this comment

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

Sure. And we can look into what it takes to cut a minor release with the current code soon-ish, just before making the backwards-compatible change.

Copy link
Contributor Author

@jafingerhut jafingerhut Mar 20, 2025

Choose a reason for hiding this comment

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

Issue on ptf repo to track this planned change of default: p4lang/ptf#222

working-directory: ./build
1 change: 1 addition & 0 deletions .github/workflows/ci-ptf.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ jobs:
- name: Build (Ubuntu 20.04)
run: |
tools/ci-build.sh
sudo pip3 install scapy==2.5.0
Copy link
Collaborator

@fruffy fruffy Mar 18, 2025

Choose a reason for hiding this comment

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

This is the wrong location, the ci build script should have a section for these kinds of tests (PTF_PSA?)

Copy link
Contributor Author

@jafingerhut jafingerhut Mar 20, 2025

Choose a reason for hiding this comment

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

Still to be run through CI tests, but in commit 31 I removed this command from ci-ptf.yml and added it to a bash function named install_ptf_ebpf_test_deps in ci-build.sh


- name: Run PTF tests for eBPF backend (Ubuntu 20.04)
run: sudo -E ./test.sh
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/ci-test-debian.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ jobs:

- name: Run tests (Ubuntu 22.04)
# Need to use sudo for the eBPF kernel tests and need to avoid p4tc_stf tests.
run: sudo -E ctest --output-on-failure --schedule-random -E "p4tc_samples_stf|p4tc_cleanup|p4tc_setup"
run: sudo -E PTF_PACKET_MANIPULATION_MODULE="bf_pktpy.ptf.packet_pktpy" ctest --output-on-failure --schedule-random -E "p4tc_samples_stf|p4tc_cleanup|p4tc_setup"
working-directory: ./build
if: matrix.unity == 'ON' && matrix.gtest == 'ON'

Expand Down Expand Up @@ -90,5 +90,5 @@ jobs:
tools/ci-build.sh

- name: Run tests (Ubuntu 22.04)
run: ctest --output-on-failure --schedule-random -R tofino
run: PTF_PACKET_MANIPULATION_MODULE="bf_pktpy.ptf.packet_pktpy" ctest --output-on-failure --schedule-random -R tofino
working-directory: ./build
2 changes: 1 addition & 1 deletion .github/workflows/ci-test-fedora.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,5 @@ jobs:
- name: Run p4c tests (Fedora Linux)
run: |
# Avoid running p4tc stf tests for now
export PATH="$HOME/.local/bin:$PATH"; ctest --output-on-failure --schedule-random -E "p4tc_samples_stf|p4tc_cleanup|p4tc_setup"
export PATH="$HOME/.local/bin:$PATH"; export PTF_PACKET_MANIPULATION_MODULE="bf_pktpy.ptf.packet_pktpy"; ctest --output-on-failure --schedule-random -E "p4tc_samples_stf|p4tc_cleanup|p4tc_setup"
working-directory: ./build
4 changes: 2 additions & 2 deletions .github/workflows/ci-test-mac.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ jobs:
- name: Run tests (MacOS)
run: |
source ~/.bash_profile
ctest --output-on-failure --schedule-random -E "bpf|ubpf|testgen|smith|p4tc"
PTF_PACKET_MANIPULATION_MODULE="bf_pktpy.ptf.packet_pktpy" ctest --output-on-failure --schedule-random -E "bpf|ubpf|testgen|smith|p4tc"
working-directory: ./build

# Build and test p4c on MacOS 13 on x86.
Expand Down Expand Up @@ -105,5 +105,5 @@ jobs:
- name: Run tests (MacOS)
run: |
source ~/.bash_profile
ctest --output-on-failure --schedule-random -E "bpf|ubpf|testgen|smith|p4tc"
PTF_PACKET_MANIPULATION_MODULE="bf_pktpy.ptf.packet_pktpy" ctest --output-on-failure --schedule-random -E "bpf|ubpf|testgen|smith|p4tc"
working-directory: ./build
4 changes: 0 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,10 +276,6 @@ find_python_module (re REQUIRED)
# other packages
find_package (Doxygen)
find_package (BMV2)
# If we have found simple switch or psa switch or pna_nic, we also need scapy.
if(SIMPLE_SWITCH OR PSA_SWITCH OR PNA_NIC)
find_python_module (scapy REQUIRED)
endif()
# enable CTest
enable_testing ()

Expand Down
2 changes: 0 additions & 2 deletions backends/bmv2/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ To run and test this back-end you need some additional tools:
- the BMv2 behavioral model itself. Installation instructions are available [here](https://github.com/p4lang/behavioral-model#installing-bmv2). You may need to update your
dynamic libraries after installing bmv2: `sudo ldconfig`

- the Python scapy library `sudo pip3 install scapy`

# Unsupported P4_16 language features

Here are some unsupported features we are aware of. We will update this list as
Expand Down
3 changes: 2 additions & 1 deletion backends/ebpf/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@ Additionally, the eBPF compiler test suite has the following python dependencies

- The python ply package to parse .stf testing files.

- The python scapy package to read and write pcap files.
- The python scapy package, to easily construct test packets with
selected sequences of headers.

You can install these using:
```
Expand Down
1 change: 1 addition & 0 deletions backends/ebpf/tests/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ for xdp_enabled in "${XDP[@]}" ; do
TEST_PARAMS+=";xdp='$xdp_enabled';xdp2tc='$xdp2tc_mode'"
# Start tests
ptf \
--packet-manipulation-module ptf.packet_scapy \
--test-dir ptf/ \
--test-params="$TEST_PARAMS" \
--interface 0@s1-eth0 --interface 1@s1-eth1 --interface 2@s1-eth2 --interface 3@s1-eth3 \
Expand Down
18 changes: 15 additions & 3 deletions backends/tc/runtime/send_packet
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,22 @@
#!/usr/bin/env python3
# Copyright 2025 Intel Corporation
# SPDX-License-Identifier: GPL-2.0-only
# Reason-GPL: imports-scapy
# SPDX-License-Identifier: Apache-2.0
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import argparse
from scapy.all import sendp, rdpcap
import ptf
from ptf.pcap_writer import rdpcap
from ptf.packet import sendp

PARSER = argparse.ArgumentParser()
PARSER.add_argument("pcap_file", help="PCAP file to send")
Expand Down
3 changes: 1 addition & 2 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@
pyroute2==0.7.3
ply==3.11
# Version 0.10.0
ptf @ git+https://github.com/p4lang/ptf@d016cdfe99f2d609cc9c7fd7f8c414b56d5b3c5c
ptf @ git+https://github.com/p4lang/ptf@e29ef7ec0bd34d4ceef88016383e53238ab5383c
# FIXME: We should figure out a way to synchronize P4Runtime versions across CMake and Python.
# This is the same commit hash as defined in the top-level CMakelists.txt
p4runtime @ git+https://github.com/p4lang/p4runtime@ec4eb5ef70dbcbcbf2f8357a4b2b8c2f218845a5#subdirectory=py
scapy==2.5.0
clang-format==18.1.0
isort==5.13.2; python_version > '3.6'
black==24.3.0; python_version > '3.6'
Expand Down
1 change: 1 addition & 0 deletions tools/install_fedora_deps.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ sudo dnf install -y -q \
pkg-config \
procps-ng \
python3 \
python3-devel \
python3-pip \
python3-thrift \
readline-devel \
Expand Down
Loading