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

Automatically disable p4tc tests when the necessary environment is missing #5177

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

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Mar 15, 2025

Do not add P4TC STF tests when certain components are missing. Currently this is just brctland clang-15, but there might be other things we can check for.

Also add some typing to the run-tc-test.py and fix some incorrect error output and handling.

@vbnogueira Could you give this a review?

@fruffy fruffy added p4tc Topics related to the P4-TC back end. On PRs, also triggers p4tc CI tests to run. run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run. run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. labels Mar 15, 2025
@fruffy fruffy force-pushed the fruffy/disable_tc_tests branch from 09febd1 to 763bfd5 Compare March 15, 2025 19:59
@fruffy fruffy marked this pull request as ready for review March 15, 2025 20:17
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

With the caveat I have no TC knowledge the changes look good to me, but I don't see how are the python changes related. I'll wait from someone from TC to take a look before I approve.

@fruffy
Copy link
Collaborator Author

fruffy commented Mar 17, 2025

With the caveat I have no TC knowledge the changes look good to me, but I don't see how are the python changes related. I'll wait from someone from TC to take a look before I approve.

The TC script had a problem where they caused exceptions to be thrown when a binary was missing. The Python changes handle these exceptions gracefully. Let me move them to a separate PR.

@fruffy fruffy force-pushed the fruffy/disable_tc_tests branch 2 times, most recently from c09ce43 to 371a3c1 Compare March 17, 2025 23:28
Copy link
Contributor

@vbnogueira vbnogueira left a comment

Choose a reason for hiding this comment

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

Unless I'm misunderstanding something, with this PR the p4tc compilation tests have stopped running even when the p4tc tag is on

@fruffy
Copy link
Collaborator Author

fruffy commented Mar 18, 2025

Unless I'm misunderstanding something, with this PR the p4tc compilation tests have stopped running even when the p4tc tag is on

Guess I was a bit too aggressive. Need to have two separate checks for the normal compilation tests and the STF ones.

@fruffy
Copy link
Collaborator Author

fruffy commented Mar 18, 2025

@vbnogueira I remember now why the tests were disabled. Could you look at the failures and tell me on what system or CI they are meant to be run?

@vbnogueira
Copy link
Contributor

vbnogueira commented Mar 18, 2025

@vbnogueira I remember now why the tests were disabled. Could you look at the failures and tell me on what system or CI they are meant to be run?

So the mac tests shouldn't run any of the p4tc compilation tests
The fedora one should work fine and the ubuntu 18.04 as well
If you look at the last PR, the p4tc tests ran succesfully (https://github.com/p4lang/p4c/pull/5172/checks)
Looking at the fedora fail and the escape issue seems very strange
It's even stranger given the fact that this line has been there since sosutha's original commit, I just moved it to another file
I looked at the changes you did to the python script, but couldn't identify any obvious culprit
Trying to figure the ubuntu issue as well

@vbnogueira
Copy link
Contributor

vbnogueira commented Mar 18, 2025

Trying to figure the ubuntu issue as well

It seems like the ubuntu-18 test was skipped on all the previous PRs, so we didn't catch this
The kernel they have in the ubuntu-18 image is quite old and struct __sk_buff doesn't have the tstamp field which was included in 2018
So we should avoid running the p4tc tests in ubuntu-18 as well

@fruffy
Copy link
Collaborator Author

fruffy commented Mar 18, 2025

So we should avoid running the p4tc tests in ubuntu-18 as well

Let me see how we can check for that. Ideally I want to get to a point where we do not have to add all these filters to ctest.

@vbnogueira
Copy link
Contributor

So we should avoid running the p4tc tests in ubuntu-18 as well

Let me see how we can check for that. Ideally I want to get to a point where we do not have to add all these filters to ctest.

For ubuntu, you can try checking whether the linux-libc-dev version is greater than 5.0
Dummy example in bash:

VERSION=$(dpkg-query -W -f='${Version}' linux-libc-dev)

if dpkg --compare-versions "$VERSION" gt 5.0; then
    echo "linux-libc-dev is greater than 5.0"
else
    echo "linux-libc-dev is not greater than 5.0"
fi

@fruffy fruffy force-pushed the fruffy/disable_tc_tests branch 2 times, most recently from eebd7bf to 2eb0cae Compare March 18, 2025 15:00
@fruffy
Copy link
Collaborator Author

fruffy commented Mar 18, 2025

It looks like tests are now running as expected and are also being filtered correctly. @vbnogueira let me know whether the CMake requirements and changes make sense to you.

Edit: Actually need to make sure to check the package correctly to filter the 18.04 tests.

@fruffy fruffy force-pushed the fruffy/disable_tc_tests branch 2 times, most recently from c93f968 to 3c1425a Compare March 18, 2025 17:08
@vbnogueira
Copy link
Contributor

It looks like tests are now running as expected and are also being filtered correctly. @vbnogueira let me know whether the CMake requirements and changes make sense to you.

I'll wait for the tests to finish to approve, but it looks ok to me

@fruffy
Copy link
Collaborator Author

fruffy commented Mar 18, 2025

I'll wait for the tests to finish to approve, but it looks ok to me

Working on some tweaks which are causing failures. Hope to resolve that soon.

The problem is when you run in Docker, the kernel version can be mismatched with the Linux headers, which is what I am running into here.

@fruffy fruffy force-pushed the fruffy/disable_tc_tests branch from 3c1425a to cbacc50 Compare March 18, 2025 17:22
@fruffy fruffy requested review from vbnogueira and vlstill March 18, 2025 19:50
set (SUPPORTS_KERNEL True)
endif()
include(CheckLinuxKernel)
check_minimum_kernel_version("4.15.0" SUPPORTS_KERNEL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor nit, shouldn't you also check the linux libc version as well here for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was thinking about this, yeah. But wasn't sure whether that could have unintended side effects. I think it is safe though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see
I don't think there would be any side effects, the check shouldn't cause any trouble
It's just that you could encounter the same issue in the future where the host's kernel version is supported, but the libc version in the container isn't for the ebpf backend as well
It should be a rare case, but could happen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a bunch of commit spam because I ran into an (unrelated) bug. The latest version should include the additional check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me

@fruffy fruffy force-pushed the fruffy/disable_tc_tests branch 2 times, most recently from c6a89f1 to 740bcce Compare March 18, 2025 23:50
@fruffy fruffy force-pushed the fruffy/disable_tc_tests branch 4 times, most recently from dc8ccf9 to 412f1db Compare March 19, 2025 02:15
@fruffy fruffy force-pushed the fruffy/disable_tc_tests branch from 412f1db to bafd232 Compare March 20, 2025 13:28
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. run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants