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: Expand mptcpd start/stop test. #194

Merged

Conversation

ossama-othman
Copy link
Member

Test mptcpd start and stop when MPTCP is both disabled and enabled.

Test mptcpd start and stop when MPTCP is both disabled and enabled.
@ossama-othman ossama-othman added the enhancement New feature or request label Dec 27, 2021
@ossama-othman ossama-othman added this to the Q4-2021 milestone Dec 27, 2021
@ossama-othman ossama-othman self-assigned this Dec 27, 2021
@coveralls
Copy link

coveralls commented Dec 27, 2021

Pull Request Test Coverage Report for Build 1625372049

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 48.038%

Totals Coverage Status
Change from base Build 1584597976: 0.0%
Covered Lines: 1016
Relevant Lines: 2115

💛 - Coveralls

Only run the individual enabled/disabled MPTCP test cases if the
"enabled" value can be set non-interactively.
@ossama-othman ossama-othman marked this pull request as draft December 27, 2021 01:32
@ossama-othman ossama-othman marked this pull request as ready for review December 27, 2021 04:33
Copy link
Member

@mjmartineau mjmartineau left a comment

Choose a reason for hiding this comment

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

Seems to be a bash syntax issue

# 2, depending on the kernel in use. For example, the
# multipath-tcp.org kernel "enabled" value could be 1 or 2. Use the
# old "enabled" value in case it is 2 instead of 1.
if [ $old_value -ne 0 ]
Copy link
Member

Choose a reason for hiding this comment

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

My system complains here:

tests/test-start-stop: line 23: [: too many arguments

Copy link
Member Author

@ossama-othman ossama-othman Jan 5, 2022

Choose a reason for hiding this comment

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

Fixed by commit ef905e7.

The regex didn't correctly handle multiple net.mptcp sysctl variables with enabled in them. In particular, the 5.13 kernel on my Ubuntu box only has net.mptcp.enabled whereas the 5.15 kernel, for example, also has a net.mptcp.checksum_enabled variable, resulting in multiple values being assigned to the $old_value shell variable.

The regular expression used to detect the MPTCP "enabled" sysctl
variable did not support the case where multiple net.mptcp sysctl
variables contain "enabled" in them.  For example recent upstream
kernels have net.mptcp.checksum_enabled and net.mptcp.enable sysctl
variable.  Both values would end up being placed in the to shell
variable used to store the old variable resulting in a syntax error
when performing an integer comparison, e.g. [ 0 1 -ne 0 ].

Simplify the regular expressions and leverage built-in pattern
matching in the sysctl command.
@ossama-othman
Copy link
Member Author

Expected content of tests/test-start-stop.log:

Interactive sudo required

Running non-interactive sudo check...
sudo: a password is required
Running test without root privileges
PASS test-start-stop (exit status: 0)

Non-interactive sudo allowed

Running non-interactive sudo check...
    succeeded
MPTCP is not enabled in the kernel.
Try 'sysctl -w net.mptcp.enabled=1'.
Required kernel MPTCP support not available.
TEST CASE: net.mptcp.enabled = 0 ...passed
TEST CASE: net.mptcp.enabled = 1 ...passed
PASS test-start-stop (exit status: 0)

Copy link
Member

@mjmartineau mjmartineau left a comment

Choose a reason for hiding this comment

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

Regex fix works for me, I see the expected output in both sudo and non-sudo cases.

@ossama-othman ossama-othman merged commit aa63d42 into multipath-tcp:master Jan 6, 2022
@ossama-othman ossama-othman deleted the expand-start-stop-test branch January 6, 2022 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants