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

tests: also run Join tests with Checksum support #1

Open
wants to merge 1 commit into
base: latest
Choose a base branch
from

Conversation

matttbe
Copy link
Member

@matttbe matttbe commented Oct 27, 2021

This needs to be covered by the CI.

Only downside: it takes ~25 minutes with the public CI and a debug
kernel.

cc: @mjmartineau

entrypoint.sh Outdated
@@ -451,6 +452,10 @@ run_mptcp_connect_mmap() {
_run_selftest_one_tap "${mptcp_connect_mmap_tap}" ./mptcp_connect.sh -m mmap
}

run_mptcp_join_csum() {
_run_selftest_one_tap "${mptcp_join_csum_tap}" ./mptcp_join.sh -C
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_run_selftest_one_tap "${mptcp_join_csum_tap}" ./mptcp_join.sh -C
_run_selftest_one_tap "${mptcp_join_csum_tap}" ./mptcp_join.sh -fsabpkC

This runs in about 1/4 the time on my system.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mjmartineau Thank you for the suggestion! (and sorry for the delay!)

That seems to add 3-4 minutes on a slow environment.
But what worry me is more to maintain a list of acceptable options. I mean: if we modify mptcp_join.sh and maintain this list of tests there, that seems to be a more valuable solution (or better: some tests are ran twice in a normal situation: with an without checksum).

Or maybe enough to run mptcp_connect.sh a second time with checksum support? That seems to cover quite a few scenarios. For the ones with multiple paths, we have mptcp_join.sh that already cover some tests with checksum enabled.
In other words: drop this commit and only take yours?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's important to get both the mptcp_connect.sh coverage in #2 and (at least) some of the multiple-subflow cases in mptcp_join.sh. Modifying mptcp_join.sh would be fine with me, that way the checksum code would get exercised more by individual developers and other CI systems like kbuild.

mjmartineau added a commit to mjmartineau/mptcp-upstream-virtme-docker that referenced this pull request Oct 27, 2021
Much like multipath-tcp#1, run the connect tests an extra time with the -C flag in
order to exercise the checksum code in more scenarios.

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
To add more coverage.

We already have some in mptcp_join.sh.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
@matttbe
Copy link
Member Author

matttbe commented Dec 5, 2022

@mjmartineau with some delay, I'm reviving this PR: do you think we still need more tests to run with csum enabled?

Now that tests are running on 4 instances, we have a bit more time. Sadly, not enough to run all mptcp_join.sh tests with checksum enabled.

But do you think it makes sense to run mptcp_connect.sh with csum support? Or is it not really needed because this is already covered by the tests in mptcp_join.sh (with multiple subflows)? Running these tests takes ~10 min in debug mode on the public CI, so it is not "cheap".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants