Use MockS3Server instead of seaweedfs for ctest. #12412
Conversation
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
saintstack
force-pushed
the
mocks3_4_seaweedfs
branch
from
4782eae to
061b2c3
Compare
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
|
Hmmm. Looks like I need to add more background functionality before I can land this one (restore is missing metadata in the backup/restore ctest). |
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
fdbbackup/tests/s3_backup_test.sh
Outdated
|
|
||
| # Clean up encryption key file | ||
| if [[ -n "${ENCRYPTION_KEY_FILE:-}" ]] && [[ -f "${ENCRYPTION_KEY_FILE}" ]]; then | ||
| echo "Removing encryption key file: ${ENCRYPTION_KEY_FILE}" |
There was a problem hiding this comment.
I personally would just run this with set -x, and delete these echo messages as you'll have the full command history
There was a problem hiding this comment.
set -x is everything vs a bit of status on how the script is progressing (the latter -- which needs a bit of cleanup I'll admit).
There was a problem hiding this comment.
There's an element of personal preference here but at some point 10-15 years back I switched to always using -x (and -e) very specifically to avoid all of the random little if / then failure checks and logging needed in test code. It is a bit verbose/noisy obviously but you quickly learn to read thru it. Reading a bit of messy output is far less of a problem then test bugs that fail to check for intermediate failures, or that omit details of arguments when something fails. IMHO these simple features make bash better for orchestrating test cases as compared to say perl or python.
There was a problem hiding this comment.
Smile. Thats a different approach for sure (200 lines of output -- too much -- vs 2000 with -x).
There was a problem hiding this comment.
It's not everybody's cup of tea (I made the teams working in my areas deal with this because I had created the gtest C++ ==> shell script layering and used it in a couple major functional areas in the system; it literally involved invoking bash based tests from the C++ "unit test framework" so there is an abstraction layering inversion going on). Almost always one only cares about the last few lines if there is a test failure so in practice the verbose output was not a problem. Try it if you want. I am not pushing super hard here, I mean I don't want to push unconventional approaches too hard in areas that I'm not directly working on.
There was a problem hiding this comment.
No harm. I do end up doing 'bash -x ./s3client.sh...' alot... The change over would require thinking about this set of interacting scripts differently.
There was a problem hiding this comment.
If we stick with the echo command and not using set -x (I don't have a strong preference) we should change echo to log, correct?
fdbclient/tests/mocks3_README.md
Outdated
|
|
||
| Three tests now use MockS3Server as the default fallback when real S3 is not available: | ||
|
|
||
| 1. **s3client_test.sh** - S3 client functionality tests |
There was a problem hiding this comment.
If we have simulation based testing, do we also need non-simulation, small scale testing like what the shell scripts seem to be doing? If there is a good reason to continue to have both can you put a note in the doc to explain this? If what we are doing in simulation is a superset of what the sh-based ctests are doing, I am not opposed to deleting the latter.
There was a problem hiding this comment.
The simulation doesn't yet cover what these ctests do. These hokey ctests have helped evolve the s3 work. The ctests also run against actual s3 if available which should help keep us honest. Will add doc. as to why ctest.
Thanks for the review.
|
Made this draft till passes CI. |
|
Here is for the PR I'm about to push 20251014-225456-stack_replace_seaweed-3cb9c1409c5f53e3 compressed=True data_size=38906370 duration=5088769 ended=100000 fail_fast=10 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=2:34:51 sanity=False started=100000 stopped=20251015-012947 submitted=20251014-225456 timeout=5400 username=stack_replace_seaweed |
saintstack
force-pushed
the
mocks3_4_seaweedfs
branch
from
061b2c3 to
f0c3192
Compare
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Avoid download of seaweedfs and the 25second startup. Preserve the try-another-port if expected is occupied. Preserve use of s3 if available.
fdbbackup/tests/s3_backup_test.sh
Outdated
|
|
||
| # Clean up encryption key file | ||
| if [[ -n "${ENCRYPTION_KEY_FILE:-}" ]] && [[ -f "${ENCRYPTION_KEY_FILE}" ]]; then | ||
| echo "Removing encryption key file: ${ENCRYPTION_KEY_FILE}" |
There was a problem hiding this comment.
If we stick with the echo command and not using set -x (I don't have a strong preference) we should change echo to log, correct?
fdbclient/tests/mocks3_fixture.sh
Outdated
| @@ -0,0 +1,145 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
| #!/bin/bash | |
| #!/usr/bin/env bash |
There was a problem hiding this comment.
Yes on the log. Fixed. And then I changed all /bin/bash to /usr/bin/env bash (I've no strong opinion on this)
fdbclient/tests/mocks3_fixture.sh
Outdated
| local port=$start_port | ||
|
|
||
| for attempt in $(seq 1 $max_attempts); do | ||
| # Check if port is available using netstat (faster than starting server) |
There was a problem hiding this comment.
This comment and the command above find_available_port are conflicting. One says # Find available port by actually trying to start the server the other says Check if port is available using netstat (faster than starting server) and based on the code the latter is the correct one.
There was a problem hiding this comment.
Fixed (went back to just starting the server and catching any bind errors ... )
fdbclient/tests/mocks3_fixture.sh
Outdated
| fi | ||
|
|
||
| # Try next port | ||
| port=$((port + 1)) |
There was a problem hiding this comment.
In theory adding some randomness could reduce the potential for conflicts even more.
There was a problem hiding this comment.
Yeah. Maybe later if conflict is frequent.
| @@ -0,0 +1,145 @@ | |||
| #!/bin/bash | |||
|
|
|||
| # Mock S3 Server fixture for ctests | |||
There was a problem hiding this comment.
For scripts I prefer if they have a few examples how to start them.
| BUILD_DIR="$build_dir_param" | ||
| if [ -z "$BUILD_DIR" ]; then | ||
| # Find the build directory - look for fdbserver binary | ||
| for dir in "../build_output" "../../build_output" "../../../build_output" "./build_output" "."; do |
There was a problem hiding this comment.
Could but a bit afraid of finding someone else's build_output. Was thinking look in well-known locations.
| return 1 | ||
| fi | ||
|
|
||
| if [ ! -f "$BUILD_DIR/bin/fdbserver" ]; then |
There was a problem hiding this comment.
| if [ ! -f "$BUILD_DIR/bin/fdbserver" ]; then | |
| if [ ! -x "$BUILD_DIR/bin/fdbserver" ]; then |
I assume we want to execute the fdbserver binary?
fdbclient/tests/mocks3_fixture.sh
Outdated
| # Find an available port | ||
| if ! MOCKS3_PORT=$(find_available_port $MOCKS3_PORT); then | ||
| echo "ERROR: Could not find available port" | ||
| return 1 |
There was a problem hiding this comment.
Shouldn't this be a continue? Otherwise we are not retrying to start the mock server?
There was a problem hiding this comment.
This code removed when I undid use of netstat.
fdbclient/tests/mocks3_fixture.sh
Outdated
| echo "blobstore://mocks3:mocksecret@${MOCKS3_HOST}:${MOCKS3_PORT}" | ||
| } | ||
|
|
||
| # Provide compatibility functions with same names as seaweedfs_fixture.sh |
There was a problem hiding this comment.
Do we need this? Is the intention to have AWS S3, seaweedFS and the MockS3 implementation as candidates for testing?
There was a problem hiding this comment.
Removed the method. User can just use start. Intention is to use mocks3 but if s3 is available, use it. No more use of seaweedfs by ctest.
fdbclient/tests/mocks3_fixture.sh
Outdated
| export -f shutdown_mocks3 | ||
| export -f get_mocks3_url | ||
| export -f run_mocks3 | ||
| export -f get_weed_url No newline at end of file |
|
Thanks for the nice review @johscheuer . Yes, the scripts are getting large and probably time to consider using something else (though I'm probably the wrong person to ask -- I don't mind bash'ing it when python or go are alternatives). |
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
|
Let me do in a follow-on. There are a few references left-over but they are mostly commenting that seaweed doesn't support a s3 feature. In follow-up will make sure mocks3 does support the missing feature and then remove the seaweed comment. |
|
Some minor/non-critical comments for you to address at your discretion. This stuff seems likely to have some potential for startup races on slow machines and I'd like some evidence that the chosen constants for retry polling are robust. Is there a way to run X,000 (say 1,000 or 5,000 or 10,000) instances of test cases that use this infra? I.e. in addition to the generic j100k here? |
On the one machine? I could write a script. |
4 participants
Avoid download of seaweedfs and the 25second startup.
Preserve the try-another-port if expected is occupied.
Preserve use of s3 if available.
Based on #12410 so will wait on its commit...
20251002-173847-stack_mocks3_4_seaweed-f9c3183628946df4 compressed=True data_size=41606796 duration=5668765 ended=100000 fail=1 fail_fast=10 max_runs=100000 pass=99999 priority=100 remaining=0 runtime=0:57:28 sanity=False started=100000 stopped=20251002-183615 submitted=20251002-173847 timeout=5400 username=stack_mocks3_4_seaweed