-
Notifications
You must be signed in to change notification settings - Fork 473
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.
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
jafingerhut
wants to merge
31
commits into
p4lang:main
Choose a base branch
from
jafingerhut:test-with-bf-pktpy-and-no-scapy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all 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 f33526a
Install bf-pktpy module from new public git repo
jafingerhut 39aeab2
Changes to hopefully enable last 2 failing tests to pass
jafingerhut 16b81ad
Allow multiple possible versions of Python module six
jafingerhut a9914fa
Remove mentions of scapy where it is no longer required.
jafingerhut f00659a
Force the use of scapy in module ptf.packet for EBPF tests
jafingerhut 08d3bc5
Remove debug steps that verify what Python packages are ..
jafingerhut 0e2adff
Try installing bf-pktpy from a subdirectory of open-p4studio repo
jafingerhut fc52efa
Try different syntax in requirements.txt for module in subdir
jafingerhut d7ab59f
Try 3 for correct requirements.txt syntax for subdir in repo
jafingerhut 917521b
Fourth time is a charm?
jafingerhut 7effb81
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut 4663b50
Replace unnecessary use of Scapy with ptf.pcap_writer in p4tc tests
jafingerhut 639e63c
Merge remote-tracking branch 'up/main' into replace-scapy-with-apache…
jafingerhut 5aafcf8
Use black to reformat modified Python source file
jafingerhut 2e17ebe
More changes to appease Python black
jafingerhut ab8c527
Merge branch 'replace-scapy-with-apache-code-in-p4tc-tests' into test…
jafingerhut 8b184e1
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut 49b3f5f
Modify new test program send_packet use ptf but not (directly) scapy
jafingerhut 0a79fb1
Try different syntax for requirements.txt
jafingerhut 652d722
Use a personal repo for bf-pktpy module source for now
jafingerhut 63c4d63
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut a519c93
Update version of ptf code in requirements.txt
jafingerhut 40ade0c
Remove separate bf_pktpy in requirements.txt, replace with version in…
jafingerhut 53bcaa8
Use fork of ptf
jafingerhut 7695ada
Remove packages on which bf_pktpy depends from requirements.txt
jafingerhut c550eb3
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut 17ce595
Update ptf to latest released version after recent PR merge
jafingerhut 28a6c26
Try a change in CI to see if EBPF tests using ptf with bf_pktpy passes
jafingerhut 71dcbae
Merge remote-tracking branch 'up/main' into test-with-bf-pktpy-and-no…
jafingerhut d039511
Move scapy install into ci-build.sh script, out of CI yml file
jafingerhut File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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