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

issue: 4231240 Revive gtest for doca_xlio_vNext #283

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

Conversation

tomerdbz
Copy link
Collaborator

The gtest suite doesn't run for doca_xlio_vNext.

while the fork related tests will fail
there's no real reason not to run the other tests.

Description

  1. Fixing assumptions in tests
  2. tracking latest stable doca as the old tag had a critical bug (crash) that our test suite caught.
  3. skipping fork tests.
What

gtest suite for doca_xlio_vNext.

Why ?

Justification for the PR. If there is existing issue/bug please reference.

How ?

It is optional but for complex PRs please provide information about the design,
architecture, approach, etc.

Change type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Check list

  • Code follows the style de facto guidelines of this project
  • Comments have been inserted in hard to understand places
  • Documentation has been updated (if necessary)
  • Test has been added (if possible)

@tomerdbz tomerdbz requested review from dpressle and galnoam December 31, 2024 12:34
@tomerdbz tomerdbz changed the title issue: 4231240 Revive gtest issue: 4231240 Revive gtest for doca_xlio_vNext Dec 31, 2024
@tomerdbz tomerdbz force-pushed the 4231240_gtest_for_doca branch 8 times, most recently from ac30a78 to 04caeeb Compare January 5, 2025 09:04
@tomerdbz tomerdbz force-pushed the 4231240_gtest_for_doca branch 5 times, most recently from 5bec7d0 to 7665a82 Compare January 9, 2025 12:39
@tomerdbz
Copy link
Collaborator Author

tomerdbz commented Jan 9, 2025

bot:retest

version[2] = PRJ_LIBRARY_RELEASE;
version[3] = PRJ_LIBRARY_REVISION;
data.ver = (PRJ_LIBRARY_MAJOR << 12) | (PRJ_LIBRARY_MINOR << 8) | (PRJ_LIBRARY_RELEASE << 4) |
PRJ_LIBRARY_REVISION;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please notice , original code gives 8 bits (1byte) per category (thus range of 0 to 255 each).
the new code gives only 4 bits per category ( thus range of 0-15).
our current library minor is 40 , thus requires 8 bits.
either keep the original (i think it's more readable, but you are the master), or increase the offsets to be 8 bits instead of 4

version[2] = PRJ_LIBRARY_RELEASE;
version[3] = PRJ_LIBRARY_REVISION;
m_data.ver = (PRJ_LIBRARY_MAJOR << 12) | (PRJ_LIBRARY_MINOR << 8) |
(PRJ_LIBRARY_RELEASE << 4) | PRJ_LIBRARY_REVISION;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please notice , original code gives 8 bits (1byte) per category (thus range of 0 to 255 each).
the new code gives only 4 bits per category ( thus range of 0-15).
our current library minor is 40 , thus requires 8 bits.
either keep the original (i think it's more readable, but you are the master), or increase the offsets to be 8 bits instead of 4


# Verify keep_alive IPv6
eval "${sudo_cmd} $timeout_exe env GTEST_TAP=2 LD_PRELOAD=$gtest_lib $gtest_app $gtest_opt_ipv6 --gtest_filter=keep_alive* --gtest_output=xml:${WORKSPACE}/${prefix}/test-keepalive_ipv6.xml"
eval "${sudo_cmd} $timeout_exe env GTEST_TAP=2 XLIO_MEM_ALLOC_TYPE=ANON XLIO_DOCA_RX=1 XLIO_DOCA_TX=1 LD_PRELOAD=$gtest_lib $gtest_app $gtest_opt --gtest_filter=keep_alive* --gtest_output=xml:${WORKSPACE}/${prefix}/test-keepalive_ipv4.xml"
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you mean to only run --gtest_filter=keep_alive* tests?
you have no intention of running the others?

@@ -29,6 +29,8 @@ function do_get_addrs()
echo $gtest_ip_list
}

gtest_ip_remote=$(ip -f inet addr show eth0 | awk '/inet / {print $2}' | cut -d/ -f1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why eth0? how to guarantee the existing of eth0?

Copy link
Collaborator

@dpressle dpressle Jan 13, 2025

Choose a reason for hiding this comment

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

This will always exist in our k8s environment, not sure about other places, we should probably make sure we are in k8s/Docker env before setting this up

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed according to dpressle comment :) Thanks!
"lets put this line only where we set ip addresses for net1/2 (which is also only true for k8s)"

Copy link
Collaborator

Choose a reason for hiding this comment

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

So i think you should move it somewhere after line 34 (the non manual run which is CI run) because its always true there

@tomerdbz tomerdbz force-pushed the 4231240_gtest_for_doca branch 3 times, most recently from 5c6acdc to 7dc9536 Compare January 16, 2025 14:07
@igor-ivanov
Copy link
Collaborator

@tomerdbz it seems that all gtests that do actual connection or data exchange are skipped. It looks as significant degradation in basic verification

@galnoam
Copy link
Collaborator

galnoam commented Jan 19, 2025

Hi @igor-ivanov, Currently DOCA do not support fork, this is why we suppress the server client test for now in gtest.

@@ -29,6 +29,8 @@ function do_get_addrs()
echo $gtest_ip_list
}

gtest_ip_remote=$(ip -f inet addr show eth0 | awk '/inet / {print $2}' | cut -d/ -f1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So i think you should move it somewhere after line 34 (the non manual run which is CI run) because its always true there

@igor-ivanov
Copy link
Collaborator

Hi @igor-ivanov, Currently DOCA do not support fork, this is why we suppress the server client test for now in gtest.

does XLIO support NGINX w/o fork?

@tomerdbz tomerdbz force-pushed the 4231240_gtest_for_doca branch from 7dc9536 to d3c1f77 Compare January 20, 2025 08:31
The gtest suite doesn't run for doca_xlio_vNext.

while the fork related tests will fail
there's no real reason not to run the other tests.

Signed-off-by: Tomer Cabouly <[email protected]>
@tomerdbz tomerdbz force-pushed the 4231240_gtest_for_doca branch from d3c1f77 to 798e814 Compare January 20, 2025 08:33
@galnoam
Copy link
Collaborator

galnoam commented Jan 21, 2025

Hi @igor-ivanov, Currently DOCA do not support fork, this is why we suppress the server client test for now in gtest.

does XLIO support NGINX w/o fork?

in NGINX we have a master process so grissik did a W/A to make it work.
but without a master process it's not possible.

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.

5 participants