refactor(greptimedb-standalone): Refactor the object storage configuration to make it simplify #178
Conversation
…ation to make it simplify
|
Warning Rate limit exceeded@daviderli614 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request includes updates to the Helm charts for both Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
charts/greptimedb-standalone/templates/configmap.yaml (2)
Line range hint
1-1: Document configuration precedence in values.yaml.Since the configuration is composed from two sources (
.Values.configTomland the object storage helper), it would be helpful to document:
- The order of configuration application
- Which settings take precedence in case of conflicts
- Example configurations showing the interaction between user-provided and generated configs
Also applies to: 11-12
11-12: Consider adding TOML validation.To prevent runtime issues, consider adding validation to ensure the combined configuration produces valid TOML syntax.
Would you like me to provide an implementation for TOML validation in the template?
charts/greptimedb-standalone/templates/secret.yaml (2)
17-18: Fix indentation in the GCS block.The indentation of the GCS block is inconsistent with other provider blocks.
Apply this diff to fix the indentation:
{{ else if .Values.objectStorage.gcs }} - GREPTIMEDB_STANDALONE__STORAGE__CREDENTIAL: {{ .Values.objectStorage.credentials.serviceAccountKey }} + GREPTIMEDB_STANDALONE__STORAGE__CREDENTIAL: {{ .Values.objectStorage.credentials.serviceAccountKey }}
Line range hint
1-22: Add template documentation for better maintainability.Consider adding a comment block at the beginning of the template to explain:
- The purpose of this secret
- The relationship with object storage configuration
- The conditions under which it gets created
Add this documentation at the start of the file:
{{/* This template creates a Kubernetes Secret for object storage credentials when: 1. Object storage is enabled (.Values.objectStorage) 2. Credentials are provided (.Values.objectStorage.credentials) 3. No existing secret is specified (.Values.objectStorage.credentials.existingSecretName) The secret contains provider-specific credentials for S3, OSS, or GCS, while other storage configuration is managed through _helpers.tpl. */}}charts/greptimedb-standalone/templates/_helpers.tpl (1)
64-84: Consider adding provider-specific validation templates.To improve maintainability and validation, consider extracting provider-specific logic into separate template functions.
Create separate templates for each provider:
{{- define "greptimedb-standalone.s3Config" -}} {{- $config := .Values.objectStorage.s3 }} {{- if and $config.bucket $config.endpoint $config.region }} type = "S3" bucket = "{{ $config.bucket }}" endpoint = "{{ $config.endpoint }}" region = "{{ $config.region }}" {{- if $config.root }} root = "{{ $config.root }}" {{- end }} {{- end }} {{- end }}Then update the main template to use these provider-specific templates:
{{- define "greptimedb-standalone.objectStorageConfig" -}} [storage] {{- if .Values.objectStorage.s3 }} {{ include "greptimedb-standalone.s3Config" . | indent 2 }} {{- else if .Values.objectStorage.oss }} {{ include "greptimedb-standalone.ossConfig" . | indent 2 }} {{- else if .Values.objectStorage.gcs }} {{ include "greptimedb-standalone.gcsConfig" . | indent 2 }} {{- end }} {{- end }}This approach:
- Makes it easier to maintain provider-specific logic
- Allows for better validation per provider
- Improves code reusability
charts/greptimedb-standalone/README.md (1)
75-75: Documentation needs enhancement for GCS supportWhile the configuration now includes GCS support, the documentation should be enhanced to:
- Document GCS-specific configuration parameters
- Provide an example of GCS configuration similar to the existing AWS S3 example
- Include migration notes for users updating from previous versions
Here's a suggested addition to include after the AWS S3 example:
+ **Use Google Cloud Storage as backend storage** + ```console + helm upgrade --install greptimedb-standalone greptime/greptimedb-standalone \ + --set objectStorage.credentials.keyID="your-gcs-key-id" \ + --set objectStorage.credentials.secretKey="your-gcs-secret-key" \ + --set objectStorage.gcs.bucket="your-bucket-name" \ + --set objectStorage.gcs.scope="your-gcs-scope" \ + --set objectStorage.gcs.endpoint="gcs-endpoint" \ + --set objectStorage.gcs.root="root-directory-of-data" \ + -n default + ```charts/greptimedb-standalone/values.yaml (3)
45-54: Enhance documentation for credential configuration.The credential configuration looks good, but let's improve the documentation for better clarity:
- For GCP credentials, specify that the service account key should be the entire JSON key file content, not just the private key.
- For the existing secret, document the expected key names within the secret.
Consider updating the comments like this:
- # GCP cloudProvider serviceAccountKey JSON-formatted base64 value + # GCP cloudProvider serviceAccountKey: The entire service account JSON key file content in base64 format + # Generate it using: cat service-account.json | base64 -w 0 - # Set the existing secret to get the key's of cloudProvider + # Name of existing secret containing cloud provider credentials + # Expected keys in secret: + # - For AWS/AliCloud: 'accessKeyId' and 'secretAccessKey' + # - For GCP: 'serviceAccountKey'
73-81: Enhance GCS configuration with examples and validation.The GCS configuration structure is good, but consider these improvements:
- Add example values to demonstrate proper configuration
- Document which fields are required vs optional
- Include common scope values for different use cases
Consider updating the GCS configuration like this:
# configure to use gcs storage gcs: {} - # bucket: "bucket-name" - # scope: "" # example: "https://www.googleapis.com/auth/devstorage.read_write" + # # Required: GCS bucket name + # bucket: "my-greptimedb-bucket" + # + # # Required: OAuth 2.0 scope for GCS access + # # Common values: + # # - https://www.googleapis.com/auth/devstorage.read_write (Full access) + # # - https://www.googleapis.com/auth/devstorage.read_only (Read-only access) + # scope: "https://www.googleapis.com/auth/devstorage.read_write" # # The data directory in gcs will be: 'gcs://<bucket>/<root>/data/...'. - # root: "mycluster" + # # Optional: Root path prefix (default: greptimedb-standalone) + # root: "greptimedb-standalone" + # + # # Optional: GCS endpoint (default: https://storage.googleapis.com) + # # Only change this if using a custom endpoint # endpoint: "https://storage.googleapis.com"
Line range hint
45-81: Consider documenting migration steps for existing users.The refactoring of object storage configuration is well-structured and improves maintainability. However, since this changes how credentials are managed, existing users might need guidance on migrating their configurations.
Consider adding migration documentation that covers:
- How to move existing credentials to the new structure
- Steps to create and use external secrets
- Backward compatibility considerations
- Examples of migrating from the old to new configuration format
Would you like me to help draft the migration guide?
charts/greptimedb-cluster/values.yaml (2)
605-613: LGTM! Well-structured credentials configuration.The credentials configuration properly handles sensitive information through Kubernetes secrets and provides flexibility with
existingSecretName.Consider adding comments to clarify:
- The format of the
serviceAccountKey(JSON key file content in base64)- The required IAM permissions for each cloud provider
Line range hint
605-638: Consider enhancing monitoring configuration documentation.The monitoring configuration includes good defaults, but could benefit from:
- Example pipeline configuration for logs collection
- Documentation on how to tune Vector resource limits for different scales
- Guidelines for adjusting the Prometheus scraping interval based on cluster size
Add a comment block above the
logsCollectionsection with an example pipeline configuration:# Example pipeline configuration: # logsCollection: # pipeline: # data: | # [sources.log_files] # type = "file" # include = ["/data/greptimedb/logs/*.log"] # # [transforms.parse_logs] # type = "remap" # inputs = ["log_files"] # # [sinks.greptimedb] # type = "greptimedb" # inputs = ["parse_logs"] # endpoint = "http://localhost:4000"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- charts/greptimedb-cluster/Chart.yaml (1 hunks)
- charts/greptimedb-cluster/README.md (1 hunks)
- charts/greptimedb-cluster/values.yaml (2 hunks)
- charts/greptimedb-standalone/Chart.yaml (1 hunks)
- charts/greptimedb-standalone/README.md (2 hunks)
- charts/greptimedb-standalone/templates/_helpers.tpl (1 hunks)
- charts/greptimedb-standalone/templates/configmap.yaml (1 hunks)
- charts/greptimedb-standalone/templates/secret.yaml (2 hunks)
- charts/greptimedb-standalone/templates/statefulset.yaml (1 hunks)
- charts/greptimedb-standalone/values.yaml (2 hunks)
✅ Files skipped from review due to trivial changes (3)
- charts/greptimedb-cluster/Chart.yaml
- charts/greptimedb-cluster/README.md
- charts/greptimedb-standalone/Chart.yaml
🔇 Additional comments (5)
charts/greptimedb-standalone/templates/configmap.yaml (1)
12-12: Verify object storage configuration template output.The object storage configuration is appended after the user-provided config. Please ensure:
- The helper template generates valid TOML syntax
- There are no conflicting sections between user config and generated config
✅ Verification successful
Object storage configuration template is valid and safe to use
The helper template
greptimedb-standalone.objectStorageConfigis well-designed and generates valid TOML syntax:
- It only generates the
[storage]section when object storage is configured (s3/oss/gcs)- The template handles all required fields (type, bucket, root) and provider-specific fields (endpoint, region, scope)
- The values.yaml shows no conflicting storage sections with user-provided config
- The indentation (4 spaces) matches the ConfigMap structure
The template safely handles object storage configuration without risk of duplicate or conflicting sections.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the helper template implementation and verify TOML structure # Look for the helper template definition rg -A 10 "objectStorageConfig" charts/greptimedb-standalone/templates/_helpers.tpl # Check for potential duplicate sections in values.yaml rg "^\[.*storage.*\]" charts/greptimedb-standalone/values.yamlLength of output: 1029
Script:
#!/bin/bash # Let's check the complete helper template definition and values.yaml for storage configuration # Get the complete helper template cat charts/greptimedb-standalone/templates/_helpers.tpl # Check the values.yaml for any storage configuration cat charts/greptimedb-standalone/values.yaml # Check if there are any example values or documentation fd -g "*values*.yaml" -x cat {}Length of output: 37678
charts/greptimedb-standalone/templates/secret.yaml (1)
3-3: LGTM: Good addition of existing secret support!The new conditional check for
existingSecretNamefollows Helm best practices by allowing users to either create a new secret or use an existing one.charts/greptimedb-standalone/templates/_helpers.tpl (1)
64-84: Verify the impact on existing deployments.The refactoring of object storage configuration might affect existing deployments.
Let's verify the usage and potential impact:
✅ Verification successful
Let's gather more specific information about the object storage configuration format and its usage.
Let's check how this template is being used in the deployment configuration.
Let's check one final thing - the configuration map setup where this template might be used.
The object storage configuration changes are safe for existing deployments
This is a new feature addition that:
- Adds a new helper template
objectStorageConfigthat generates storage configuration- Only gets included in the ConfigMap when
configTomlis enabled- Uses values from the existing
objectStoragestructure in values.yaml- Maintains backward compatibility as it's purely additive and optional
The changes don't modify any existing behavior and will only take effect when explicitly configured through values. The implementation properly handles all three storage providers (S3, OSS, GCS) while maintaining the same configuration structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing object storage configurations in values files echo "Checking for object storage configurations in values files:" rg -l 'objectStorage:' charts/ # Check for potential uses of the old configuration format echo "Checking for potential uses of old configuration format:" rg -l 'storage:' charts/Length of output: 939
Script:
#!/bin/bash # Check the values.yaml for object storage configuration structure echo "=== Checking values.yaml for object storage configuration ===" cat charts/greptimedb-standalone/values.yaml | grep -A 20 "objectStorage:" # Check if there are any existing deployments using the old format echo -e "\n=== Checking for existing object storage configuration patterns ===" rg "type\s*=\s*[\"'](?:S3|Oss|Gcs)[\"']" charts/ -A 5 # Check the changes in this PR echo -e "\n=== Checking git diff for object storage changes ===" git diff HEAD^1 -- charts/greptimedb-standalone/templates/_helpers.tplLength of output: 3061
Script:
#!/bin/bash # Check where the new template is being used echo "=== Checking usage of objectStorageConfig template ===" rg "template.*greptimedb-standalone\.objectStorageConfig" charts/ # Check the statefulset configuration echo -e "\n=== Checking StatefulSet configuration ===" cat charts/greptimedb-standalone/templates/statefulset.yaml # Check if there are any existing deployments using different storage configuration echo -e "\n=== Checking for other storage configurations ===" rg "\[storage\]" charts/ -B 2 -A 5Length of output: 7005
Script:
#!/bin/bash # Check for configmap template echo "=== Checking ConfigMap template ===" cat charts/greptimedb-standalone/templates/configmap.yaml 2>/dev/null || echo "ConfigMap template not found" # Check for any other files that might use this configuration echo -e "\n=== Checking for other files that might reference the config ===" rg "greptimedb-standalone\.objectStorageConfig" charts/ --type yaml --type tomlLength of output: 852
charts/greptimedb-standalone/README.md (1)
5-5: LGTM: Version bump aligns with changesThe version increment from 0.1.24 to 0.1.25 appropriately reflects the backward-compatible changes being made to the object storage configuration.
charts/greptimedb-cluster/values.yaml (1)
638-638: Verify the stability of the debug pod image tag.The image tag
20240905-67eaa147appears to be a development build. Consider:
- Using a stable release tag for production environments
- Documenting the process for updating this tag
Let's verify if this is the latest stable tag:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
charts/greptimedb-standalone/values.yaml (2)
1-1: Remove unnecessary empty line at the start of the file.The static analysis tool indicates there are too many blank lines at the start of the file.
- image:🧰 Tools
🪛 GitHub Check: chart-test
[failure] 1-1:
1:1 [empty-lines] too many blank lines (1 > 0)
45-59: Enhance credential configuration documentation.While the configuration options are clear, it would be helpful to add:
- The expected format of the secret when using
existingSecretName- Example of how to create the secret for each cloud provider
Consider adding comments like:
# Example secret format for AWS: # apiVersion: v1 # kind: Secret # metadata: # name: object-storage-creds # type: Opaque # data: # accessKeyId: <base64-encoded-value> # secretAccessKey: <base64-encoded-value>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- charts/greptimedb-standalone/templates/secret.yaml (1 hunks)
- charts/greptimedb-standalone/values.yaml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/greptimedb-standalone/templates/secret.yaml
🧰 Additional context used
🪛 GitHub Check: chart-test
charts/greptimedb-standalone/values.yaml
[failure] 1-1:
1:1 [empty-lines] too many blank lines (1 > 0)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
charts/greptimedb-standalone/templates/_helpers.tpl (1)
64-65: Add GoTemplate documentation for the new function.Consider adding documentation comments explaining the purpose, parameters, and usage of this template function, following the style of other functions in this file.
+{{/* +Generate object storage configuration in TOML format. + +This template function generates storage configuration based on the provided values +for S3, OSS, or GCS storage providers. It outputs a TOML-formatted configuration +block that can be used in GreptimeDB's configuration. +*/}} {{- define "greptimedb-standalone.objectStorageConfig" -}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- charts/greptimedb-standalone/templates/_helpers.tpl (1 hunks)
🔇 Additional comments (1)
charts/greptimedb-standalone/templates/_helpers.tpl (1)
64-109: Verify template usage and backward compatibility.Since this is a refactored configuration template, we should verify its integration and compatibility.
✅ Verification successful
Let me gather more information about the configuration usage and changes.
Let me check for any historical changes to understand if this is a breaking change.
Template refactoring maintains backward compatibility
The changes to the object storage configuration template are safe and maintain backward compatibility:
- The template is only used in
configmap.yamlfor generating the storage configuration- The refactoring improves readability by using variables while preserving the same configuration structure
- All storage providers (S3, OSS, GCS) and their parameters remain unchanged
- The values.yaml shows consistent evolution of the configuration schema
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new template and potential breaking changes # Test 1: Find all files that use this template echo "Files using the object storage template:" rg "greptimedb-standalone\.objectStorageConfig" charts/ # Test 2: Check for any existing storage configurations that might be affected echo -e "\nExisting storage configurations:" rg -A 5 "\[storage\]" charts/Length of output: 957
Script:
#!/bin/bash # Check the configmap.yaml file to understand how the template is used echo "Content of configmap.yaml:" cat charts/greptimedb-standalone/templates/configmap.yaml # Check values.yaml for default configurations echo -e "\nDefault values in values.yaml:" cat charts/greptimedb-standalone/values.yaml | grep -A 20 "objectStorage:"Length of output: 1352
Script:
#!/bin/bash # Check git history for the helper template and values echo "History of _helpers.tpl:" git log -p charts/greptimedb-standalone/templates/_helpers.tpl | grep -A 10 "objectStorageConfig" echo -e "\nHistory of values.yaml:" git log -p charts/greptimedb-standalone/values.yaml | grep -A 10 "objectStorage:"Length of output: 3683
Keep the keyID、secretkey information of the object storage secret, move other configurations to
_helpers.tplfile, and configure it to GreptimeDB in the form of a toml file.Add google cloud storage configurations.
Wait me to test...
Summary by CodeRabbit
New Features
greptimedb-clusterandgreptimedb-standaloneto version0.2.23and0.1.25, respectively.greptimedb-cluster.greptimedb-standalone, including new placeholders for GCS settings.Bug Fixes
greptimedb-standalone.Documentation