Use existing pvc with default storage #8564
Conversation
WalkthroughThe pull request introduces several updates to the Helm chart for the CVAT application. The chart version is incremented from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes
User->>Helm: Deploy CVAT Chart
Helm->>Kubernetes: Create PersistentVolumeClaim
alt defaultStorage enabled
Kubernetes->>Kubernetes: Check existing claim name
alt existing claim not provided
Kubernetes->>Kubernetes: Define new PersistentVolumeClaim
end
end
Helm->>Kubernetes: Deploy other resources
Kubernetes-->>User: Deployment complete
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: 0
🧹 Outside diff range and nitpick comments (8)
helm-chart/templates/cvat_backend/storage.yml (2)
Line range hint
2-13: LGTM: Well-structured PVC metadata with a suggestion.The inner conditional and PVC metadata are well-structured:
- The condition allows for using an existing PVC or creating a new one.
- The PVC name uses the release name for uniqueness.
- The
helm.sh/resource-policy: keepannotation prevents accidental deletion.- Labels are properly set, including those from a common template.
Consider adding a comment explaining the purpose of the
helm.sh/resource-policy: keepannotation for better maintainability.🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
Line range hint
14-27: LGTM: Flexible PVC spec with a suggestion for improvement.The PVC spec is well-structured and flexible:
- Access modes are conditionally set, defaulting to ReadWriteMany if not specified.
- Storage class name is optional, good for compatibility across different Kubernetes environments.
- Storage size is configurable, essential for various deployment scenarios.
Consider adding a default storage size in case
.Values.cvat.backend.defaultStorage.sizeis not set. This could prevent potential errors if the value is accidentally omitted. For example:storage: {{ .Values.cvat.backend.defaultStorage.size | default "10Gi" }}helm-chart/Chart.yaml (1)
19-19: LGTM. Don't forget to update the changelog and documentation.The chart version increment from 0.14.0 to 0.14.1 is appropriate for the changes described in the PR. This minor version bump aligns with semantic versioning principles for non-breaking changes.
Please ensure you complete the following items from your PR checklist:
- Create a changelog fragment
- Update documentation if necessary
- Add tests if applicable
- Link any related issues
- Increment versions of any affected npm packages
These steps are crucial for maintaining a well-documented and traceable project history.
helm-chart/templates/cvat_backend/utils/deployment.yml (1)
130-130: Approve the change with a minor suggestion for improved readability.The modification to use
defaultfunction for theclaimNameis a good improvement. It allows for more flexibility in specifying an existing PVC while maintaining backward compatibility.To slightly improve readability, consider breaking the line into multiple lines:
claimName: {{- default (printf "%s-backend-data" .Release.Name) .Values.cvat.backend.defaultStorage.existingClaimName | quote }}This change maintains the functionality while making the line easier to read and maintain.
helm-chart/templates/cvat_backend/worker_export/deployment.yml (1)
134-134: Approved: Enhanced flexibility for PVC claim name.This change improves the configurability of the deployment by allowing users to specify an existing claim name through
.Values.cvat.backend.defaultStorage.existingClaimName. If not specified, it falls back to the previous behavior of using{{ .Release.Name }}-backend-data.Consider adding a check for empty string to avoid potential issues:
claimName: {{ default (printf "%s-backend-data" .Release.Name) (trimAll " " .Values.cvat.backend.defaultStorage.existingClaimName) | quote}}This ensures that if
.Values.cvat.backend.defaultStorage.existingClaimNameis set to an empty string or only whitespace, the default value is still used.helm-chart/templates/cvat_backend/server/deployment.yml (1)
146-146: Approve change with a suggestion for improved readabilityThe modification to use
defaultfunction for theclaimNameis a good improvement. It allows for more flexibility in specifying an existing PVC while maintaining backwards compatibility.However, the line is quite long. Consider breaking it into multiple lines for better readability:
claimName: {{- default (printf "%s-backend-data" .Release.Name) .Values.cvat.backend.defaultStorage.existingClaimName | quote }}Also, ensure that the
values.yamlfile and relevant documentation are updated to reflect the newexistingClaimNameoption undercvat.backend.defaultStorage.helm-chart/values.yaml (2)
Line range hint
148-152: LGTM. Consider updating documentation for new storage options.The addition of the
defaultStorageconfiguration for CVAT is a good improvement for storage management. The default size of 20Gi seems reasonable, but it might be worth mentioning in the documentation that this can be adjusted based on specific use cases.Also, the commented out lines (
existingClaimName,storageClassName,accessModes) suggest additional configuration options. Consider documenting these options in the chart's README or values file comments to guide users on how they can be utilized.
Line range hint
307-311: LGTM. Consider maintaining consistency in commenting style.The addition of the
defaultStorageconfiguration for kvrocks is a good improvement, aligning with the changes made for CVAT. The larger default size of 100Gi is appropriate for a data storage component.For consistency, consider using the same commenting style as in the CVAT section. For example:
defaultStorage: enabled: true # existingClaimName: "" # storageClassName: default # accessModes: # - ReadWriteOnce size: 100GiThis would make it easier for users to understand and configure these options consistently across different components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- helm-chart/Chart.yaml (1 hunks)
- helm-chart/templates/cvat_backend/server/deployment.yml (1 hunks)
- helm-chart/templates/cvat_backend/storage.yml (2 hunks)
- helm-chart/templates/cvat_backend/utils/deployment.yml (1 hunks)
- helm-chart/templates/cvat_backend/worker_annotation/deployment.yml (1 hunks)
- helm-chart/templates/cvat_backend/worker_export/deployment.yml (1 hunks)
- helm-chart/templates/cvat_backend/worker_import/deployment.yml (1 hunks)
- helm-chart/values.yaml (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
helm-chart/templates/cvat_backend/storage.yml (2)
1-1: LGTM: Conditional resource creation is a good practice.The outer conditional block allows for optional creation of the PersistentVolumeClaim based on the
.Values.cvat.backend.defaultStorage.enabledflag. This is a good practice in Helm charts as it provides flexibility in resource management.Also applies to: 28-28
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
Line range hint
1-28: Overall, excellent improvements to storage configurability.This change significantly enhances the flexibility of the CVAT backend storage configuration:
- It allows for optional PVC creation.
- It supports using an existing PVC or creating a new one.
- It provides customizable access modes, storage class, and size.
These improvements will make it easier to deploy CVAT in various Kubernetes environments with different storage requirements.
To ensure these changes are properly reflected in the Helm chart's documentation, please run the following command:
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
helm-chart/templates/cvat_backend/worker_import/deployment.yml (2)
133-133: Excellent enhancement to PVC configuration flexibility!This change improves the configurability of the persistent volume claim for the backend data. By using
defaultfunction with.Values.cvat.backend.defaultStorage.existingClaimName, it allows users to specify an existing claim name in their Helm values while maintaining backward compatibility with the original behavior. This aligns well with the PR objective of using existing PVC with default storage.The use of the
quotefunction ensures proper YAML formatting, which is a good practice.
Line range hint
1-146: Overall, this change enhances the flexibility of the CVAT backend worker import deployment.The modification to the persistent volume claim configuration allows for better customization without breaking existing setups. This change is well-aligned with the PR objectives and follows Helm chart best practices. No issues or concerns were identified in this file.
helm-chart/templates/cvat_backend/worker_annotation/deployment.yml (1)
133-133: LGTM! Verify consistency across the chart.The change to use
{{ default (printf "%s-backend-data" .Release.Name) .Values.cvat.backend.defaultStorage.existingClaimName | quote}}for theclaimNameis a good improvement. It allows for more flexibility in configuration while maintaining backwards compatibility.To ensure consistency, please run the following script to check if similar changes have been applied to other deployment files in the chart:
This will help identify any deployment files that might need similar updates for consistency.
✅ Verification successful
Claim Name Consistently Updated Across Deployment Files
All deployment files have been reviewed and the
claimNameis consistently set to{{ default (printf "%s-backend-data" .Release.Name) .Values.cvat.backend.defaultStorage.existingClaimName | quote}}across all relevant deployment configurations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent use of claimName across deployment files # Test: Search for claimName in deployment files rg --type yaml 'claimName:' helm-chart/templatesLength of output: 1047
helm-chart/values.yaml (1)
Line range hint
1-577: Overall, the changes improve storage management and configuration flexibility.The modifications to
helm-chart/values.yamlsuccessfully introducedefaultStorageconfigurations for both CVAT and kvrocks components. These changes align with the PR objective of using existing PVC with default storage and enhance the flexibility of the Helm chart.Key points:
- CVAT now has a default storage of 20Gi, while kvrocks has 100Gi.
- Both configurations include commented-out options for further customization.
- The changes are consistent and do not introduce conflicts with existing configurations.
These improvements will likely make it easier for users to deploy and manage storage for CVAT installations. Consider updating the chart's documentation to reflect these new configuration options and provide guidance on when and how to customize them.
Issue #8563
Motivation and context
How has this been tested?
Deployed with custom helm chart
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
defaultStorageconfigurations for CVAT and KVROCKS, enhancing storage management capabilities.Improvements
PersistentVolumeClaimto improve storage configurability.Bug Fixes