[seaweed] add tests for S3 buckets #1283
Conversation
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
WalkthroughA new end-to-end test for SeaweedFS Bucket resources was introduced, leveraging the BATS framework and the MinIO client. The root tenant configuration test script was updated to enable the "seaweedfs" component, adjust HelmRelease waits, and extend timeouts. The e2e-sandbox Docker image now includes the MinIO client ( Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as BATS Test Script
participant K8s as Kubernetes API
participant Helm as HelmRelease Controller
participant S3 as SeaweedFS S3
participant MC as MinIO Client
Tester->>K8s: Apply Bucket CR
K8s->>Helm: Reconcile Bucket, create HelmRelease
Tester->>K8s: Wait for HelmRelease, BucketClaims, BucketAccesses ready
Tester->>K8s: Get bucket credentials Secret
Tester->>S3: Port-forward S3 endpoint
Tester->>MC: Configure alias with credentials
MC->>S3: Upload credentials.json to bucket
MC->>S3: List uploaded file
MC->>S3: Remove uploaded file
Tester->>K8s: Delete Bucket CR
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @IvanHunters, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances our automated testing capabilities by introducing a comprehensive end-to-end test for SeaweedFS bucket functionality within our Kubernetes environments. It ensures that SeaweedFS can be properly provisioned and utilized, covering the full lifecycle of a bucket from creation to data operations and cleanup.
Highlights
- New End-to-End Test: I've added a new end-to-end test (
hack/e2e-apps/bucket.bats) specifically for SeaweedFS. This test automates the creation of a SeaweedFS bucket, verifies its readiness, retrieves credentials, performs S3 operations (upload, list, delete a file) using the MinIO client (mc), and finally cleans up the created bucket. - SeaweedFS Enablement in E2E Setup: The
hack/e2e-install-cozystack.batsscript has been updated to enable SeaweedFS for the root tenant during the e2e environment setup. This ensures that the SeaweedFS HelmRelease is properly deployed and ready before the new bucket tests are executed. - MinIO Client Integration: The
packages/core/testing/images/e2e-sandbox/Dockerfilenow includes the MinIO client (mc). This client is essential for the new SeaweedFS bucket test, as it's used to interact with the SeaweedFS S3-compatible endpoint to perform file operations.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This PR introduces an e2e test for SeaweedFS bucket functionality. The changes to enable SeaweedFS in the test environment and update the test image are correct. However, the new test script hack/e2e-apps/bucket.bats has several critical and high-severity issues. The S3 client commands are incorrect and won't test the intended functionality. Additionally, the script lacks proper resource cleanup, which can lead to flaky tests and resource leaks. I've provided specific suggestions to fix these issues and improve the test's robustness.
| mc alias set local https://localhost:8333 $ACCESS_KEY $SECRET_KEY --insecure | ||
|
|
||
| # Upload file to bucket | ||
| mc cp bucket-test-credentials.json $BUCKET_NAME/bucket-test-credentials.json |
There was a problem hiding this comment.
The mc cp command is missing the S3 alias in the destination path. Without the local/ prefix, mc will treat the destination as a local filesystem path instead of an S3 object path. This means the test is not actually uploading the file to SeaweedFS, which is a critical flaw in the test's logic.
mc cp bucket-test-credentials.json local/$BUCKET_NAME/bucket-test-credentials.json
| mc cp bucket-test-credentials.json $BUCKET_NAME/bucket-test-credentials.json | ||
|
|
||
| # Verify file was uploaded | ||
| mc ls $BUCKET_NAME/bucket-test-credentials.json |
There was a problem hiding this comment.
Similar to the mc cp command, the mc ls command is missing the local/ alias prefix. As a result, this command will not check for the file in the S3 bucket, and the verification step is ineffective.
mc ls local/$BUCKET_NAME/bucket-test-credentials.json
| mc ls $BUCKET_NAME/bucket-test-credentials.json | ||
|
|
||
| # Clean up uploaded file | ||
| mc rm $BUCKET_NAME/bucket-test-credentials.json |
There was a problem hiding this comment.
The mc rm command is also missing the local/ alias prefix. This means the test will not remove the file from the S3 bucket, failing to clean up the test object.
mc rm local/$BUCKET_NAME/bucket-test-credentials.json
| @test "Create and Verify Seeweedfs Bucket" { | ||
| # Create the bucket resource | ||
| name='test' | ||
| kubectl apply -f - <<EOF | ||
| apiVersion: apps.cozystack.io/v1alpha1 | ||
| kind: Bucket | ||
| metadata: | ||
| name: ${name} | ||
| namespace: tenant-test | ||
| spec: {} | ||
| EOF | ||
|
|
||
| # Wait for the bucket to be ready | ||
| kubectl -n tenant-test wait hr bucket-${name} --timeout=100s --for=condition=ready | ||
| kubectl -n tenant-test wait bucketclaims.objectstorage.k8s.io bucket-${name} --timeout=300s --for=jsonpath='{.status.bucketReady}' | ||
| kubectl -n tenant-test wait bucketaccesses.objectstorage.k8s.io bucket-${name} --timeout=300s --for=jsonpath='{.status.accessGranted}' | ||
|
|
||
| # Get and decode credentials | ||
| kubectl -n tenant-test get secret bucket-${name} -ojsonpath='{.data.BucketInfo}' | base64 -d > bucket-test-credentials.json | ||
|
|
||
| # Get credentials from the secret | ||
| ACCESS_KEY=$(jq -r '.spec.secretS3.accessKeyID' bucket-test-credentials.json) | ||
| SECRET_KEY=$(jq -r '.spec.secretS3.accessSecretKey' bucket-test-credentials.json) | ||
| BUCKET_NAME=$(jq -r '.spec.bucketName' bucket-test-credentials.json) | ||
|
|
||
| # Start port-forwarding | ||
| bash -c 'timeout 100s kubectl port-forward service/seaweedfs-s3 -n tenant-root 8333:8333 > /dev/null 2>&1 &' | ||
|
|
||
| # Wait for port-forward to be ready | ||
| timeout 30 sh -ec 'until nc -z localhost 8333; do sleep 1; done' | ||
|
|
||
| # Set up MinIO alias with error handling | ||
| mc alias set local https://localhost:8333 $ACCESS_KEY $SECRET_KEY --insecure | ||
|
|
||
| # Upload file to bucket | ||
| mc cp bucket-test-credentials.json $BUCKET_NAME/bucket-test-credentials.json | ||
|
|
||
| # Verify file was uploaded | ||
| mc ls $BUCKET_NAME/bucket-test-credentials.json | ||
|
|
||
| # Clean up uploaded file | ||
| mc rm $BUCKET_NAME/bucket-test-credentials.json | ||
|
|
||
| kubectl -n tenant-test delete bucket.apps.cozystack.io ${name} | ||
| } |
There was a problem hiding this comment.
This test creates several resources (a temporary file, a background process, and a Kubernetes resource) that are not guaranteed to be cleaned up if the test fails before its completion. This can lead to resource leaks and cause subsequent test runs to be flaky.
It is highly recommended to use a trap at the beginning of the test to ensure all resources are cleaned up upon exit, regardless of whether the test succeeds or fails. The cleanup logic should handle:
- Killing the
port-forwardbackground process. - Removing the temporary credentials file.
- Deleting the
bucket.apps.cozystack.ioresource (thekubectl deleteon line 46 should be moved into the trap).
| BUCKET_NAME=$(jq -r '.spec.bucketName' bucket-test-credentials.json) | ||
|
|
||
| # Start port-forwarding | ||
| bash -c 'timeout 100s kubectl port-forward service/seaweedfs-s3 -n tenant-root 8333:8333 > /dev/null 2>&1 &' |
There was a problem hiding this comment.
The ampersand (&) to background the port-forward command is inside the single-quoted string passed to bash -c. This causes the timeout process to be orphaned and disconnected from the test's process tree, making it difficult to manage and clean up.
The & should be placed outside the single quotes to correctly background the bash -c command. This allows the parent shell to capture its PID for proper cleanup.
bash -c 'timeout 100s kubectl port-forward service/seaweedfs-s3 -n tenant-root 8333:8333 > /dev/null 2>&1' &
|
|
||
| @test "Create and Verify Seeweedfs Bucket" { | ||
| # Create the bucket resource | ||
| name='test' |
There was a problem hiding this comment.
Using a static name like 'test' for the bucket can lead to conflicts if tests are run in parallel or if a previous test run failed to clean up its resources properly. To make the test more robust, it's better to use a unique name for each test run.
name="test-$(date +%s)-${RANDOM}"
| timeout 30 sh -ec 'until nc -z localhost 8333; do sleep 1; done' | ||
|
|
||
| # Set up MinIO alias with error handling | ||
| mc alias set local https://localhost:8333 $ACCESS_KEY $SECRET_KEY --insecure |
There was a problem hiding this comment.
The mc alias set command modifies the default configuration file in the user's home directory (~/.mc/config.json). If multiple tests run concurrently, they could overwrite each other's configuration, leading to race conditions and flaky tests.
To ensure test isolation, you should use the --config-dir flag with a temporary directory for all mc commands. This directory should be created at the start of the test and cleaned up in a trap.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
hack/e2e-apps/bucket.bats (7)
5-5: Use unique bucket name to avoid test conflicts.Using a static name like
'test'for the bucket can lead to conflicts if tests are run in parallel or if a previous test run failed to clean up its resources properly.
29-29: Fix backgrounding of port-forward command.The ampersand (
&) to background theport-forwardcommand is inside the single-quoted string, causing thetimeoutprocess to be orphaned and disconnected from the test's process tree.
35-35: Ensure test isolation for MinIO client configuration.The
mc alias setcommand modifies the default configuration file which could cause race conditions if multiple tests run concurrently.
38-38: Fix missing S3 alias in mc cp command.The
mc cpcommand is missing the S3 alias in the destination path. Without thelocal/prefix, the file will not be uploaded to SeaweedFS.
41-41: Fix missing S3 alias in mc ls command.The
mc lscommand is missing thelocal/alias prefix, so it will not check for the file in the S3 bucket.
44-44: Fix missing S3 alias in mc rm command.The
mc rmcommand is missing thelocal/alias prefix, so it will not remove the file from the S3 bucket.
3-47: Add proper resource cleanup with trap.The test creates several resources that are not guaranteed to be cleaned up if the test fails before completion, which can cause subsequent test runs to be flaky.
🧹 Nitpick comments (1)
hack/e2e-apps/bucket.bats (1)
3-3: Fix typo in test function name.The test function name contains a typo: "Seeweedfs" should be "SeaweedFS".
-@test "Create and Verify Seeweedfs Bucket" { +@test "Create and Verify SeaweedFS Bucket" {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hack/e2e-apps/bucket.bats(1 hunks)hack/e2e-install-cozystack.bats(1 hunks)packages/core/testing/images/e2e-sandbox/Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/core/testing/images/e2e-sandbox/Dockerfile (1)
Learnt from: NickVolynkin
PR: #1117
File: packages/apps/mysql/Makefile:8-8
Timestamp: 2025-06-26T04:29:24.830Z
Learning: The cozystack project uses yq v4+ on their CI runner, so yq v4 syntax (-o json --indent 4) is compatible and version checks are not needed.
🧬 Code Graph Analysis (1)
hack/e2e-apps/bucket.bats (1)
hack/e2e-apps/run-kubernetes.sh (1)
run_kubernetes_test(1-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install Cozystack
🔇 Additional comments (4)
hack/e2e-install-cozystack.bats (2)
126-126: LGTM! SeaweedFS component enabled correctly.The addition of
"seaweedfs": trueto the tenant specification is properly formatted and follows the established pattern for enabling components in the root tenant configuration.
128-129: LGTM! Proper wait configuration for SeaweedFS.The addition of
seaweedfsto the HelmRelease wait commands and the timeout increase to 4 minutes are appropriate changes that ensure SeaweedFS is fully deployed before proceeding with subsequent tests.packages/core/testing/images/e2e-sandbox/Dockerfile (1)
22-23: LGTM! MinIO client installation follows established patterns.The addition of the MinIO client (
mc) is properly implemented using the same pattern as other CLI tool installations in the Dockerfile. The use ofTARGETOSandTARGETARCHvariables ensures cross-platform compatibility, and the executable permissions are correctly set.hack/e2e-apps/bucket.bats (1)
15-18: Well-structured readiness checks.The test properly waits for the HelmRelease, BucketClaims, and BucketAccesses to be ready before proceeding, which is good practice for ensuring test reliability.
|
Successfully created backport PR for |
# Description Backport of #1283 to `release-0.34`.
What this PR does
Introduced automated end-to-end testing for SeaweedFS bucket creation and verification in Kubernetes environments.
Release note
Summary by CodeRabbit