[GCS FT] Redis e2e cleanup check #2773
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Rueian <[email protected]>
Signed-off-by: Rueian <[email protected]>
rueian
commented
Jan 18, 2025
| t.Ctx(), | ||
| appsv1ac.Deployment("redis", namespace). | ||
| WithSpec(appsv1ac.DeploymentSpec(). |
There was a problem hiding this comment.
no need for us to use Deployment for Redis in tests. just use Pod instead.
ray-operator/test/e2e/support.go
Outdated
| if output = checkDBSize(); output == "0" { | ||
| return | ||
| } | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
Wait for the cleanup job.
This was referenced Jan 18, 2025
kevin85421
reviewed
Jan 20, 2025
ray-operator/test/e2e/support.go
Outdated
| @@ -213,4 +210,46 @@ func deployRedis(t Test, namespace string, password string) { | |||
| TestApplyOptions, | |||
| ) | |||
| assert.NoError(t.T(), err) | |||
|
|
|||
| checkDBSize := func() string { | |||
ray-operator/test/e2e/support.go
Outdated
|
|
||
| return strings.TrimSpace(stdout.String() + stderr.String()) | ||
| } | ||
| return func() { |
There was a problem hiding this comment.
Maybe only return checkDBSize and use Eventually in raycluster_gcsft_test.go?
| @@ -1230,7 +1230,7 @@ func (r *RayClusterReconciler) buildRedisCleanupJob(ctx context.Context, instanc | |||
| "redis_address = os.getenv('RAY_REDIS_ADDRESS', '').split(',')[0]; " + | |||
| "redis_address = redis_address if '://' in redis_address else 'redis://' + redis_address; " + | |||
| "parsed = urlparse(redis_address); " + | |||
| "sys.exit(1) if not cleanup_redis_storage(host=parsed.hostname, port=parsed.port, password=os.getenv('REDIS_PASSWORD', parsed.password), use_ssl=parsed.scheme=='rediss', storage_namespace=os.getenv('RAY_external_storage_namespace')) else None\"", | |||
| "sys.exit(1) if not cleanup_redis_storage(host=parsed.hostname, port=parsed.port, password=os.getenv('REDIS_PASSWORD', parsed.password or ''), use_ssl=parsed.scheme=='rediss', storage_namespace=os.getenv('RAY_external_storage_namespace')) else None\"", | |||
There was a problem hiding this comment.
would you mind explaining the change?
There was a problem hiding this comment.
Without the change, we could pass None to the password parameter and resulting in this error:
| @@ -81,7 +81,7 @@ func TestGcsFaultToleranceOptions(t *testing.T) { | |||
| g := NewWithT(t) | |||
| namespace := test.NewTestNamespace() | |||
|
|
|||
| deployRedis(test, namespace.Name, tc.redisPassword) | |||
| defer deployRedis(test, namespace.Name, tc.redisPassword)() | |||
There was a problem hiding this comment.
what will the stack trace look like if the check in defer fails? If it is not easy to read, maybe we can return the clean up function and explicitly call it with Eventually at the end of the test logic?
There was a problem hiding this comment.
This trace is the defer approach:
=== RUN TestGcsFaultToleranceAnnotations/GCS_FT_without_redis_password
Modified Ray Image to: rayproject/ray:2.40.0-aarch64 for ARM chips
raycluster_gcsft_test.go:211: Created RayCluster test-ns-72s65/raycluster-gcsft successfully
raycluster_gcsft_test.go:213: Waiting for RayCluster test-ns-72s65/raycluster-gcsft to become ready
raycluster_gcsft_test.go:217: Verifying environment variables on Head Pod
support.go:253: failed cleanup redis expect 0 but got: 3
test.go:110: Retrieving Pod Container test-ns-72s65/redis/redis logs
test.go:98: Creating ephemeral output directory as KUBERAY_TEST_OUTPUT_DIR env variable is unset
test.go:101: Output directory has been created at: /var/folders/sw/cyfhnvns2hv82r3fj1sgwsb00000gn/T/TestGcsFaultToleranceAnnotationsGCS_FT_without_redis_password2730163886/001
--- FAIL: TestGcsFaultToleranceAnnotations/GCS_FT_without_redis_password (48.50s)This one is the trace for ExecPodCmd + Eventually.
=== RUN TestGcsFaultToleranceAnnotations/GCS_FT_without_redis_password
Modified Ray Image to: rayproject/ray:2.40.0-aarch64 for ARM chips
raycluster_gcsft_test.go:214: Created RayCluster test-ns-s257l/raycluster-gcsft successfully
raycluster_gcsft_test.go:216: Waiting for RayCluster test-ns-s257l/raycluster-gcsft to become ready
raycluster_gcsft_test.go:220: Verifying environment variables on Head Pod
core.go:88: Executing command: [redis-cli --no-auth-warning DBSIZE]
core.go:101: Command stdout: 3
core.go:102: Command stderr:
...
core.go:88: Executing command: [redis-cli --no-auth-warning DBSIZE]
core.go:101: Command stdout: 3
core.go:102: Command stderr:
core.go:88: Executing command: [redis-cli --no-auth-warning DBSIZE]
core.go:101: Command stdout: 3
core.go:102: Command stderr:
raycluster_gcsft_test.go:236:
Timed out after 30.072s.
Expected
<string>: 3
to be equivalent to
<string>: 0
test.go:110: Retrieving Pod Container test-ns-s257l/redis/redis logs
test.go:98: Creating ephemeral output directory as KUBERAY_TEST_OUTPUT_DIR env variable is unset
test.go:101: Output directory has been created at: /var/folders/sw/cyfhnvns2hv82r3fj1sgwsb00000gn/T/TestGcsFaultToleranceAnnotationsGCS_FT_without_redis_password315262234/001
--- FAIL: TestGcsFaultToleranceAnnotations/GCS_FT_without_redis_password (51.40s)
rueian
force-pushed
the
redis-e2e-cleanup-check
branch
from
8b02924 to
b73254d
Compare
Signed-off-by: Rueian <[email protected]>
rueian
force-pushed
the
redis-e2e-cleanup-check
branch
from
b73254d to
8f9a030
Compare
kevin85421
approved these changes
Jan 20, 2025
win5923
pushed a commit
to win5923/kuberay
that referenced
this pull request
Jan 20, 2025
win5923
pushed a commit
to win5923/kuberay
that referenced
this pull request
Jan 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Follow-ups for #2696 (comment) and resolves #2764.
Related issue number
Checks