Issue 6258 - Mitigate race condition in paged_results_test.py #6433

abeisemb · 2024-12-04T16:25:56Z

The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order.

This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search().

Helps fix: #6258

Reviewed by: @droideck , @progier389 (Thanks!)

abeisemb · 2024-12-04T18:16:43Z

Amended commit to move a helper function inside the test function for readability.

progier389

LGTM
FYI: rather than using indexes to extract the string you can also use directly the regular expression module. Here is an use example:

import re

str="xxxx op=123 conn=567 RESULT xxx"
result = re.search(r'(op=\d+ conn=\d+)', str)
print(result.group(1))

vashirov · 2024-12-05T08:21:05Z

I agree with @progier389, this should be a regex.
Minor nit: in the commit message instead of "Helps fix" please use "Fixes:", see https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests

abeisemb · 2024-12-05T12:59:36Z

Amended commit to correct the error in the message and replace the string index function with the simpler regex solution. Full test runtime for paged_results_test.py comes in at 6 minutes 46 seconds.

progier389 · 2024-12-05T14:27:07Z

Looks good.

abeisemb · 2024-12-05T16:23:10Z

Added missing re import.

droideck

Besides that, LGTM!

droideck · 2024-12-05T23:07:03Z

dirsrvtests/tests/suites/paged_results/paged_results_test.py

@@ -1126,6 +1127,8 @@ def test_multi_suffix_search(topology_st, create_user, new_suffixes):
        topology_st.standalone.restart(timeout=10)

        access_log_lines = topology_st.standalone.ds_access_log.match('.*pr_cookie=.*')
+        # Sort access_log_lines by op number to mitigate race condition effects. 
+        access_log_lines.sort(key=lambda x: int(re.search(r"op=(.*?) RESULT", x).group(1)))


Minor nitpick, but I think we should be as specific as possible to avoid any weird issues:

Suggested change

access_log_lines.sort(key=lambda x: int(re.search(r"op=(.*?) RESULT", x).group(1)))

access_log_lines.sort(key=lambda x: int(re.search(r"op=(\d+) RESULT", x).group(1)))

Good catch! The commit is amended now with that change.

Full test runtime comes in at 7 minutes 2 seconds.

The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Fixes: 389ds#6258

@droideck

…6433) The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: 389ds#6258 Reviewed by: @droideck , @progier389 (Thanks!)

@droideck

…6433) The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: 389ds#6258 Reviewed by: @droideck , @progier389 (Thanks!)

@droideck

…6433) The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: 389ds#6258 Reviewed by: @droideck , @progier389 (Thanks!)

@droideck

…6433) The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: 389ds#6258 Reviewed by: @droideck , @progier389 (Thanks!)

@droideck

…6433) The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: 389ds#6258 Reviewed by: @droideck , @progier389 (Thanks!)

@droideck

…6433) The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: 389ds#6258 Reviewed by: @droideck , @progier389 (Thanks!)

@droideck

The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: #6258 Reviewed by: @droideck , @progier389 (Thanks!) Co-authored-by: Anuar Beisembayev <111912342+abeisemb@users.noreply.github.com>

@droideck

The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: #6258 Reviewed by: @droideck , @progier389 (Thanks!) Co-authored-by: Anuar Beisembayev <111912342+abeisemb@users.noreply.github.com>

@droideck

The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: #6258 Reviewed by: @droideck , @progier389 (Thanks!) Co-authored-by: Anuar Beisembayev <111912342+abeisemb@users.noreply.github.com>

@droideck

The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: #6258 Reviewed by: @droideck , @progier389 (Thanks!) Co-authored-by: Anuar Beisembayev <111912342+abeisemb@users.noreply.github.com>

@droideck

The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: #6258 Reviewed by: @droideck , @progier389 (Thanks!) Co-authored-by: Anuar Beisembayev <111912342+abeisemb@users.noreply.github.com>

@droideck

The regression test dirsrvtests/tests/suites/paged_results/paged_results_test.py::test_multi_suffix_search has a race condition causing it to fail due to multiple queries potentially writing their logs out of chronological order. This failure is mitigated by sorting the retrieved access_log_lines by their "op" value. This ensures the log lines are in chronological order, as expected by the assertions at the end of test_multi_suffix_search(). Helps fix: #6258 Reviewed by: @droideck , @progier389 (Thanks!) Co-authored-by: Anuar Beisembayev <111912342+abeisemb@users.noreply.github.com>

abeisemb marked this pull request as draft December 4, 2024 18:02

abeisemb force-pushed the sort-logs-multi-suffix-test branch from dadec79 to f7c7fa8 Compare December 4, 2024 18:16

abeisemb marked this pull request as ready for review December 4, 2024 18:16

progier389 approved these changes Dec 4, 2024

View reviewed changes

abeisemb force-pushed the sort-logs-multi-suffix-test branch from f7c7fa8 to e901e1b Compare December 5, 2024 12:58

abeisemb force-pushed the sort-logs-multi-suffix-test branch from e901e1b to a5cc7f2 Compare December 5, 2024 16:22

droideck approved these changes Dec 5, 2024

View reviewed changes

abeisemb force-pushed the sort-logs-multi-suffix-test branch from a5cc7f2 to 2848e3e Compare December 6, 2024 20:11

progier389 merged commit c1c46c1 into 389ds:main Dec 9, 2024
197 of 199 checks passed

mmatsuya mentioned this pull request Feb 4, 2025

Issue 6258 - Add sleep for ramdom failure in two tests #6261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Issue 6258 - Mitigate race condition in paged_results_test.py #6433

Issue 6258 - Mitigate race condition in paged_results_test.py #6433

Uh oh!

abeisemb commented Dec 4, 2024 •

edited by progier389

Loading

Uh oh!

abeisemb commented Dec 4, 2024

Uh oh!

progier389 left a comment

Uh oh!

vashirov commented Dec 5, 2024

Uh oh!

abeisemb commented Dec 5, 2024 •

edited

Loading

Uh oh!

progier389 commented Dec 5, 2024

Uh oh!

abeisemb commented Dec 5, 2024

Uh oh!

droideck left a comment

Uh oh!

droideck Dec 5, 2024

Uh oh!

abeisemb Dec 6, 2024

Uh oh!

abeisemb Dec 6, 2024

Uh oh!

Uh oh!

Uh oh!

	access_log_lines.sort(key=lambda x: int(re.search(r"op=(.*?) RESULT", x).group(1)))
	access_log_lines.sort(key=lambda x: int(re.search(r"op=(\d+) RESULT", x).group(1)))

Issue 6258 - Mitigate race condition in paged_results_test.py #6433

Issue 6258 - Mitigate race condition in paged_results_test.py #6433

Uh oh!

Conversation

abeisemb commented Dec 4, 2024 • edited by progier389 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abeisemb commented Dec 4, 2024

Uh oh!

progier389 left a comment

Choose a reason for hiding this comment

Uh oh!

vashirov commented Dec 5, 2024

Uh oh!

abeisemb commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

progier389 commented Dec 5, 2024

Uh oh!

abeisemb commented Dec 5, 2024

Uh oh!

droideck left a comment

Choose a reason for hiding this comment

Uh oh!

droideck Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

abeisemb Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

abeisemb Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abeisemb commented Dec 4, 2024 •

edited by progier389

Loading

abeisemb commented Dec 5, 2024 •

edited

Loading