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

First take at p4tc automated tests #5011

Merged
merged 2 commits into from
Mar 4, 2025

Conversation

vbnogueira
Copy link
Contributor

First take at implementing the automated testing code for tc backend. In a nutshell, what it's doing is expanding on the already existing run-tc-test.py script. The expansion consists of:

Compiling the generated C files to eBPF bytecode
Extracting a P4TC kernel image from github
Compiling P4TC's version of iproute2
Spawning a VM using virtme to boot the P4TC kernel
Executing the template script generated by the compiler
Loading the generated eBPF parser and control blocks binaries using a TC P4 filter
Loading the runtime rules specified in the samples directory (*.rules)
Parsing an STF in the samples directory detailing what packets to send/expect
Sending the packets using scapy
Verifying that the sent packets (and eventual received packets from the p4tc pipeline) and correct according to the STF file
Opening it as a draft PR because we still need to see how this works with github actions
Locally it's doing fine, however we never know

I created a separate directory for the tests that involve STFs testdata/p4tc_samples_stf/ to make it cleaner
However that might've been overkill, I can undo that if reviewers think it's best

@fruffy
Copy link
Collaborator

fruffy commented Nov 11, 2024

The installation might be missing gcc-multilib.

I recommend adding a TC-specific section here: https://github.com/p4lang/p4c/blob/main/tools/ci-build.sh#L170

@fruffy fruffy added the p4tc Topics related to the P4-TC back end. On PRs, also triggers p4tc CI tests to run. label Nov 11, 2024
@vbnogueira
Copy link
Contributor Author

The installation might be missing gcc-multilib.

I recommend adding a TC-specific section here: https://github.com/p4lang/p4c/blob/main/tools/ci-build.sh#L170

Thank you, will do

@vbnogueira vbnogueira force-pushed the automated_p4tc_tests branch 14 times, most recently from 12ebfd2 to e5ba246 Compare November 19, 2024 17:39
@vbnogueira vbnogueira force-pushed the automated_p4tc_tests branch 3 times, most recently from 80d0655 to c915f8e Compare November 27, 2024 17:49
@vbnogueira vbnogueira marked this pull request as ready for review November 27, 2024 19:18
@komaljai komaljai requested a review from fruffy November 28, 2024 14:09
Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Some initial comments

@vbnogueira vbnogueira force-pushed the automated_p4tc_tests branch 2 times, most recently from 148cd65 to 8b06b32 Compare December 10, 2024 14:21
Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Couple nits left.

@@ -26,7 +26,7 @@ sudo dnf install -y -q \
boost-test \
boost-thread \
ccache \
clang \
clang-15 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why clang-15 is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because ubuntu and fedora's clang versions are too old and generated eBPF code that was rejected by the eBPF verifier. clang-15 was the oldest version that worked with the kernel version we are using for the tests

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the TC back end check the clang version? It should either throw an error or disable itself if clang-15 is not available.

Copy link
Contributor Author

@vbnogueira vbnogueira Dec 18, 2024

Choose a reason for hiding this comment

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

I pushed a change for this yesterday, which checks whether we have at least clang-15 (anything above that should also work for the current p4tc kernel). However the tests are failing because they are not being able to clone p4c. Can you try to run them again? I don't think I have permission to do so.

@fruffy
Copy link
Collaborator

fruffy commented Dec 18, 2024

The code looks okay from my side, I just have final reservations on the CI process. The installation of the VM etc adds significant overhead to our CI. Maybe we should make this a separate workflow that is optional and only runs on code that touches p4tc parts or PRs tagged with p4tc.

Let me think about how to introduce this.

Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Approving to unblock things. We should consider putting the TC tests in a separate (nightly) workflow that is also triggered when code in the tc folder changes or when a PR is tagged with p4tc. We have a similar workflow for the sanitizer for example (https://github.com/p4lang/p4c/blob/main/.github/workflows/ci-ubuntu-20-sanitizer-nightly.yml)

@vbnogueira
Copy link
Contributor Author

Approving to unblock things. We should consider putting the TC tests in a separate (nightly) workflow that is also triggered when code in the tc folder changes or when a PR is tagged with p4tc. We have a similar workflow for the sanitizer for example (https://github.com/p4lang/p4c/blob/main/.github/workflows/ci-ubuntu-20-sanitizer-nightly.yml)

Sorry for the delay, we had a busy couple of weeks
Will get into that as soon as possible

@vbnogueira vbnogueira force-pushed the automated_p4tc_tests branch 2 times, most recently from 7dc45ec to 736c84a Compare February 26, 2025 01:26
@vbnogueira
Copy link
Contributor Author

@fruffy please take another look when you have some time

@fruffy
Copy link
Collaborator

fruffy commented Feb 27, 2025

@fruffy please take another look when you have some time

Any major change from the version I approved or was this just the rebase?

@vbnogueira
Copy link
Contributor Author

@fruffy please take another look when you have some time

Any major change from the version I approved or was this just the rebase?

The biggest changes are running the more heavy tests only on PRs that target P4TC, and stopping these same tests from being run in other circumstances

Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

@fruffy please take another look when you have some time

Any major change from the version I approved or was this just the rebase?

The biggest changes are running the more heavy tests only on PRs that target P4TC, and stopping these same tests from being run in other circumstances

Great! And it looks like it works well. Approving again.

@@ -58,7 +58,7 @@ jobs:
- name: Run tests (MacOS)
run: |
source ~/.bash_profile
ctest --output-on-failure --schedule-random -E "bpf|ubpf|testgen|smith"
ctest --output-on-failure --schedule-random -E "bpf|ubpf|testgen|smith|p4tc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is best to disable these tests when some conditions are not met (eg CMake detects some runner is missing or the OS is not supported). But we can do this in a separate PR.

First take at implementing the automated testing code for tc backend.
In a nutshell, what it's doing is expanding on the already existing
run-tc-test.py script. The expansion consists of:

- Compiling the generated C files to eBPF bytecode
- Extracting a P4TC kernel image from github
- Compiling P4TC's version of iproute2
- Spawning a VM using virtme to boot the P4TC kernel
- Executing the template script generated by the compiler
- Loading the generated eBPF parser and control blocks binaries using a
  TC P4 filter
- Parsing an STF in the samples directory detailing what packets to
  send/expect and what runtime rules to load
- Loading any specified runtime rules
- Sending the packets using scapy
- Verifying that the sent packets (and eventual received packets from
  the p4tc pipeline) and correct according to the STF file

The commands are sent to the VM using ssh through a bridge that connects
the host to the VM
After the test is finished both the bridge and the VM are destroyed

We also added an example (arp_responder) to exercise the testing framework

Signed-off-by: Victor Nogueira <[email protected]>
Signed-off-by: Victor Nogueira <[email protected]>
@vbnogueira vbnogueira force-pushed the automated_p4tc_tests branch from ea08315 to f1db84f Compare March 3, 2025 15:55
@fruffy
Copy link
Collaborator

fruffy commented Mar 3, 2025

@vbnogueira lmk when you want this merged.

@vbnogueira
Copy link
Contributor Author

@vbnogueira lmk when you want this merged.

If it won't cause any issues, the sooner the better

@jafingerhut
Copy link
Contributor

I am fine if you want to merge these right away, as is.

I will point out that the portions of these tests that use ubuntu-20.04 will start failing intermittently during March, 2025, and deterministically on April 1, 2025.

#5155

It seems reasonable to me to remove uses of Ubuntu 20.04 in a later PR, after this one is merged.

@jafingerhut
Copy link
Contributor

I am fine if you want to merge these right away, as is.

I will point out that the portions of these tests that use ubuntu-20.04 will start failing intermittently during March, 2025, and deterministically on April 1, 2025.

#5155

It seems reasonable to me to remove uses of Ubuntu 20.04 in a later PR, after this one is merged.

I am also happy if this PR is merged first, and I will update #5156 if necessary.

@jafingerhut
Copy link
Contributor

Going ahead and merging this, given the approval and the submitter's desire to have it merged.

@jafingerhut jafingerhut added this pull request to the merge queue Mar 4, 2025
Merged via the queue into p4lang:main with commit f67ab33 Mar 4, 2025
21 checks passed
Vineet1101 pushed a commit to Vineet1101/p4c that referenced this pull request Mar 15, 2025
Signed-off-by: Victor Nogueira <[email protected]>
Signed-off-by: Vineet1101 <[email protected]>
Vineet1101 pushed a commit to Vineet1101/p4c that referenced this pull request Mar 15, 2025
Signed-off-by: Victor Nogueira <[email protected]>
Signed-off-by: Vineet1101 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4tc Topics related to the P4-TC back end. On PRs, also triggers p4tc CI tests to run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants