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: Fix test-commands test case failures. #172

Merged
merged 18 commits into from
Jan 4, 2022

Conversation

ossama-othman
Copy link
Member

@ossama-othman ossama-othman commented Oct 29, 2021

Correct several issues with the test-commands unit test:

  • Existing MPTCP address IDs are ignored.
    • The current test-commands unit test assumes that it can use hard-coded MPTCP address ID test values, but that could fail if the same address ID is currently in use by the in-kernel path manager. Leverage the mptcpd global MPTCP address ID manager to generate an ID suitable for the test since it attempts to stay in sync with the kernel.
  • The mptcpd_kpm_add_addr() test case always fails.
    • Fix mptcpd_kpm_add_addr() test failures by explicitly binding the test IPv4 address to a network interface (loopback).
  • mptcpd_kpm_flush_addrs() was called too early.
    • MPTCP addresses were flushed before get_addrs test case, causing the latter to fail due to all MPTCP addresses being purged from the in-kernel MPTCP path manager prior to retrieval of the test addresses.

@ossama-othman ossama-othman added the bug Something isn't working label Oct 29, 2021
@ossama-othman ossama-othman added this to the Q4-2021 milestone Oct 29, 2021
@ossama-othman ossama-othman self-assigned this Oct 29, 2021
@ossama-othman
Copy link
Member Author

PR #171 will be leveraged by this pull requested once it is merged to master.

@ossama-othman ossama-othman marked this pull request as draft October 29, 2021 18:33
@coveralls
Copy link

coveralls commented Oct 29, 2021

Pull Request Test Coverage Report for Build 1647641308

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 increased (+2.9%) to 50.922%

Totals Coverage Status
Change from base Build 1584597976: 2.9%
Covered Lines: 1077
Relevant Lines: 2115

💛 - Coveralls

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.

Just one grammar nit, I don't have any other suggestions right now. Will watch for updates w.r.t. #171

tests/test-commands.c Outdated Show resolved Hide resolved
@ossama-othman ossama-othman changed the title tests: Handle case MPTCP IDs exist in the kernel. tests: Handle MPTCP IDs existing in the kernel. Nov 2, 2021
@ossama-othman ossama-othman force-pushed the test-commands-fixes branch 2 times, most recently from f317b97 to 0f9111d Compare November 2, 2021 21:37
Ossama Othman and others added 11 commits January 2, 2022 13:59
The current test-commands unit test assumes that it can use hard-coded
MPTCP address ID test values, but that could fail if the same address
ID is currently in use by the in-kernel path manager.  Leverage the
mptcpd global MPTCP address ID manager to generate an ID suitable for
the test since it attempts to stay in sync with the kernel.
Co-authored-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Instead of relying on the appearance of the MPTCP generic netlink path
manager family, use the mptcpd path manager "pm_ready" callback
feature to determine when to run the test cases in the test-commands
unit test.  This ensures that mptcpd is fully initialized prior to
running the test cases.
The mptcpd_pm_ready() call in the test-commands unit test is no longer
necessary since the tests now are only run when the mptcpd path
manager is guaranteed to be fully initialized and ready.
Call flush_addrs after all kernel path management command test cases
are run to make sure resources, such as MPTCP address/IP mappings,
used between between test cases are valid.  Calling flush_addrs wipes
out all such mappings.
The mptcpd_kpm_add_addr() and mptcpd_kpm_get_addr() command tests will
fail if the test IP address is not bound to a network interface.  Set
up the test IP address on the loopback interface prior to the test
run, and tear it down afterward.  This requires that the test be run
with CAP_NET_ADMIN capability, such as running it through the root
user.

The previous behavior of ignoring and logging failures due to lack of
the CAP_NET_ADMIN capability is still preserved.
Ossama Othman added 6 commits January 2, 2022 14:25
Run the mptcpd test suite as root when generating code coverage
results so that mptcpd code paths that require that capability are
also exercised.
Run tests as both non-root and root user to help ensure code paths
that are triggered when insufficient and sufficient privileges
respectively exist are executed.
The GitHub "ubuntu-latest" environment does not support the "ip mptcp"
command/object.  Drop the call to it that reset the MPTCP limits to
0.
@ossama-othman ossama-othman changed the title tests: Handle MPTCP IDs existing in the kernel. tests: Fix test-commands test case failures. Jan 3, 2022
@ossama-othman ossama-othman marked this pull request as ready for review January 3, 2022 05:18
@ossama-othman
Copy link
Member Author

With these changes in place the full set of test cases in the test-commands unit test now pass when run as root or with the CAP_NET_ADMIN capability. Test cases that require CAP_NET_ADMIN but run as an unprivileged user still fail as expected. Failures in that cases are ignored.

The test suite was run with the following set of commands:

  • Unprivileged user
    • make check
  • root user
    • sudo make check
  • Unprivileged user with CAP_NET_ADMIN capability
    • sudo capsh --caps="cap_net_admin+eip cap_setpcap,cap_setuid,cap_setgid+ep" --keep=1 --user=$USER --addamb=cap_net_admin -- -c "make check"

There is one caveat to keep in mind when running make check or just the test-commands unit test after a privileged test run: the get_limits test case assumes that the subflows and add_addr_accepted MPTCP limits are always zero when not run as root, which is not true after the privileged test run, resulting in failure. MPTCP limits should be reset to zero between privileged and unprivileged test runs, e.g.:

$ sudo make check
$ sudo ip mptcp limits set subflows 0 add_addr_accepted 0
$ make check

Ideally, the test should retrieve and store existing MPTCP limits prior to running the set_limits test case, and then restore the original limits before process exit. Such a change should be done in a separate pull request since this patch set is already rather large.

@ossama-othman
Copy link
Member Author

I also tried to run tests in the mptcpd GitHub workflows as root but the test-commands unit test failed for some reason. Log messages for the test-commands unit test (e.g. tests/test-commands.log or tests/test-suite.log) wasn't readily available in the GitHub ubuntu-latest environment so I'll attempt enable such privileged test runs and dump those log files in the existing mptcpd GitHub workflows in a separate pull request.

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.

Looks good to me. Tried building/running with ELL 0.30 and recent master branch (although I modified the recent ELL to deal with the warnings, unrelated to this PR).

@ossama-othman ossama-othman merged commit a223869 into multipath-tcp:master Jan 4, 2022
@ossama-othman ossama-othman deleted the test-commands-fixes branch January 4, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants