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

Add storage_e2e_single_threaded test in bazel #23878

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

travisdowns
Copy link
Member

Adds the storage_e2e_single_threaded test to the bazel build, including prerequisite work, mostly adding libraries.

Fixes CORE-7649.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.2.x
  • v24.1.x
  • v23.3.x

Release Notes

  • none

@travisdowns travisdowns changed the title Td e2e test in bazel Add storage_e2e_single_threaded test in bazel Oct 22, 2024
@travisdowns
Copy link
Member Author

/dt

@@ -2356,11 +2356,11 @@ FIXTURE_TEST(changing_cleanup_policy_back_and_forth, storage_test_fixture) {
} while (disk_log->segments().back()->size_bytes() < size);
};
// add 2 log segments
add_segment(1_MiB, model::term_id(1));
Copy link
Member Author

Choose a reason for hiding this comment

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

unused param

@travisdowns travisdowns marked this pull request as ready for review October 22, 2024 13:59
@travisdowns
Copy link
Member Author

@dotnwat @rockwotj - this is the same as this other pull #23875 but I somehow managed to push a bad rebase there, dragging in a bunch of folks, so I've just re-opened this here.

Diff from that one is that clang-tidy is clean and I depend now on seastar_boost which means I don't need to pull in async.h or tmp_dir.h in a different way.

rockwotj
rockwotj previously approved these changes Oct 22, 2024
@travisdowns
Copy link
Member Author

The test itself is failing with a segfault in the bazel build. Looking into it (it ran for me locally but I was using a different config).

@dotnwat
Copy link
Member

dotnwat commented Oct 22, 2024

@travisdowns

The test itself is failing with a segfault in the bazel build

some tests fail if seastar is started with more than one cpu. in cmake i see -- -c1, so that might be the issue. do this in bazel with redpanda_cc_btest(..., cpu = 1, ...).

@travisdowns
Copy link
Member Author

some tests fail if seastar is started with more than one cpu. in cmake i see -- -c1, so that might be the issue. do this in bazel with redpanda_cc_btest(..., cpu = 1, ...).

Yeah I changed this locally to use cpu = 1. I'm not sure if that's related to the segfault but what is apparent is that this test creates several GB of output, which is parsed using bash & sed which make the test take at least an hour (I killed it after that), as you can see in this run also. So we need some fix for the XML test output to avoid this path in bazel.

To get this test in I can probably reduce the logging volume.

@travisdowns travisdowns marked this pull request as draft October 23, 2024 12:37
@travisdowns
Copy link
Member Author

I've shelving this for now given that this long-running test has a couple of issues making it unsuitable for integration ATM.

Add bazel test libraries needed for the storage e2e test:

 - common
 - storage_test_fixture
 - log_gap_analysis

Ref CORE-7649.
Fix compile warnings (as errors) which appear only in the bazel build,
prior to enabling this test in bazel.

Ref CORE-7649.
@travisdowns travisdowns force-pushed the td-e2e-test-in-bazel branch 2 times, most recently from a409406 to 91014fa Compare December 9, 2024 19:59
@travisdowns travisdowns marked this pull request as ready for review December 9, 2024 20:00
Add the storage_e2e_single_threaded test to the bazel build.

Ref CORE-7649.
@dotnwat
Copy link
Member

dotnwat commented Dec 9, 2024

needs

bazel run //tools:buildifier.fix

i wonder if there is value in running that linter at the end of CI so if you push and walk away you can still come back and see bigger ticket failures if they occur.

@rockwotj
Copy link
Contributor

rockwotj commented Dec 9, 2024

i wonder if there is value in running that linter at the end of CI so if you push and walk away you can still come back and see bigger ticket failures if they occur.

The other side is that it's nice we don't pay the cost of a full CI run if this stuff is broken, because for me I usually check to make sure the big ticket stuff works, but forget to format

@dotnwat
Copy link
Member

dotnwat commented Dec 9, 2024

I usually check to make sure the big ticket stuff works

that makes too much sense

@travisdowns
Copy link
Member Author

I think this one is ready to go in now that CORE-8013 is fixed, it works locally for me anyway!

Doing 100 (uncached) iterations just to make sure.

@travisdowns
Copy link
Member Author

The CMake test doesn't specify any specific amount of memory so I guess the test is getting the entire available memory of the host. This test seems to work fine with the btest amount of 1 MiB, so I left it that way in the bazel version (and this would allow faster test runs due to more parallelism).

@dotnwat
Copy link
Member

dotnwat commented Dec 9, 2024

The CMake test doesn't specify any specific amount of memory so I guess the test is getting the entire available memory of the host.

I think it should have been getting 1 GB (cmake_test.py:234)

@vbotbuildovich
Copy link
Collaborator

@travisdowns
Copy link
Member Author

I think it should have been getting 1 GB (cmake_test.py:234)

Good call, I forgot about the wrapper.

@travisdowns travisdowns merged commit 77e4e2a into redpanda-data:dev Dec 10, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants