test(control-client): Adding emulator test for control client stall#4884
test(control-client): Adding emulator test for control client stall#4884raj-prince wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces integration tests to verify the experimental retry logic for the gRPC Control Client when encountering stalls. It adds several stall configuration files, a new test suite, and updates the emulator test runner to support HNS buckets. Additionally, other emulator tests are updated to disable HNS to avoid gRPC initialization issues with legacy proxy servers, and the gRPC proxy is updated to inject testbench instructions. The review feedback highlights two issues: a potential hazard where tearing down a failed test could attempt to kill PID 0, and a permission issue in the shell script where sudo was omitted from the docker commands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4884 +/- ##
==========================================
+ Coverage 83.84% 83.87% +0.03%
==========================================
Files 173 173
Lines 21083 21081 -2
==========================================
+ Hits 17676 17681 +5
+ Misses 2748 2743 -5
+ Partials 659 657 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enables the storage control client for localhost custom endpoints, allowing Hierarchical Namespace (HNS) features to be tested against the emulator. It introduces a new suite of integration tests to verify control client retry behavior under simulated operation stalls, updates existing emulator tests to disable HNS by default to prevent compatibility issues with legacy proxy servers, and refactors the gRPC proxy server to support bidirectional streaming and instruction injection. The reviewer identified a potential script failure in emulator_tests.sh under set -euo pipefail if grep returns no matches, and provided a robust suggestion to handle this case.
| TEST_TARGET=${TEST_TARGET:-"./tools/integration_tests/emulator_tests/..."} | ||
| # Run all emulator test packages in sequence to avoid high cpu usage. | ||
| # Run all other emulator tests with standard bucket | ||
| go test -v -p 1 -timeout 10m $(go list ${TEST_TARGET} | grep -v control_client_stall) --integrationTest --testbucket=${TEST_BUCKET:-test-bucket} "${args[@]}" |
There was a problem hiding this comment.
Under set -euo pipefail, if TEST_TARGET is set to only run the control_client_stall tests (e.g., ./tools/integration_tests/emulator_tests/control_client_stall/...), grep -v control_client_stall will find no matches and exit with status 1. This will cause the entire script to exit immediately with an error. We should handle this gracefully by using || true and checking if the package list is non-empty before running go test.
| TEST_TARGET=${TEST_TARGET:-"./tools/integration_tests/emulator_tests/..."} | |
| # Run all emulator test packages in sequence to avoid high cpu usage. | |
| # Run all other emulator tests with standard bucket | |
| go test -v -p 1 -timeout 10m $(go list ${TEST_TARGET} | grep -v control_client_stall) --integrationTest --testbucket=${TEST_BUCKET:-test-bucket} "${args[@]}" | |
| TEST_TARGET=${TEST_TARGET:-"./tools/integration_tests/emulator_tests/..."} | |
| # Run all emulator test packages in sequence to avoid high cpu usage. | |
| # Run all other emulator tests with standard bucket | |
| TEST_PKGS=$(go list ${TEST_TARGET} | grep -v control_client_stall || true) | |
| if [[ -n "$TEST_PKGS" ]]; then | |
| go test -v -p 1 -timeout 10m $TEST_PKGS --integrationTest --testbucket=${TEST_BUCKET:-test-bucket} "${args[@]}" | |
| fi |
|
Hi @meet2mky, @charith87, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
1 similar comment
|
Hi @meet2mky, @charith87, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
Description
This PR adds emulator-based integration tests to verify GCSFuse's retry-on-stall logic for Storage Control APIs (CreateFolder, GetFolder, DeleteFolder, RenameFolder, and GetStorageLayout).
Link to the issue in case of a bug fix.
Testing details
Any backward incompatible change? If so, please explain.