-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Helm modifiable permissionFix paths and command #7932
base: develop
Are you sure you want to change the base?
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes to the CVAT Helm chart focus on enhancing the initialization process for backend services by introducing a reusable template for setting permissions. This update replaces multiple manual definitions of permission-fixing init containers with a single, centralized template, improving maintainability and consistency across various deployment configurations. Changes
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- changelog.d/20240522_215910_jacky.lam_helm_init.md (1 hunks)
- helm-chart/templates/_helpers.tpl (1 hunks)
- helm-chart/templates/cvat_backend/server/deployment.yml (1 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)
Files skipped from review due to trivial changes (3)
- changelog.d/20240522_215910_jacky.lam_helm_init.md
- helm-chart/templates/cvat_backend/utils/deployment.yml
- helm-chart/values.yaml
Additional comments not posted (4)
helm-chart/templates/cvat_backend/server/deployment.yml (1)
88-88
: The inclusion ofcvat.backend.initContainers
template simplifies the init container configuration and enhances maintainability. Please ensure that this template is tested across different deployment scenarios to verify its impact.Verification successful
The
cvat.backend.initContainers
template is defined inhelm-chart/templates/_helpers.tpl
and is used in multiple deployment YAML files within the Helm chart. This confirms that the template inclusion is correctly referenced and defined.
- Definition found in:
helm-chart/templates/_helpers.tpl
- Usage found in:
helm-chart/templates/cvat_backend/server/deployment.yml
helm-chart/templates/cvat_backend/worker_import/deployment.yml
helm-chart/templates/cvat_backend/worker_annotation/deployment.yml
helm-chart/templates/cvat_backend/worker_export/deployment.yml
helm-chart/templates/cvat_backend/utils/deployment.yml
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the template inclusion works across different configurations. # Test: Deploy using different configurations and check for errors. echo "Deploying with multiple configurations to test new init container template..." # Example command, replace with actual deployment command kubectl apply -f deployment.ymlLength of output: 273
Script:
#!/bin/bash # Search for the definition of the `cvat.backend.initContainers` template in the Helm chart templates. ast-grep --lang yaml --pattern $'{{- define "cvat.backend.initContainers" }}' # Search for the usage of the `cvat.backend.initContainers` template in the Helm chart templates. rg '{{- include "cvat.backend.initContainers" . }}'Length of output: 363
Script:
#!/bin/bash # Search for the definition of the `cvat.backend.initContainers` template in the Helm chart templates. rg '\{\{- define "cvat\.backend\.initContainers" \}\}' # Search for the usage of the `cvat.backend.initContainers` template in the Helm chart templates. rg '\{\{- include "cvat\.backend\.initContainers" \. \}\}'Length of output: 117
Script:
#!/bin/bash # Search for any template definitions related to `initContainers` in the Helm chart templates. rg 'define.*initContainers' # Search for any template usages related to `initContainers` in the Helm chart templates. rg 'include.*initContainers'Length of output: 778
helm-chart/templates/cvat_backend/worker_export/deployment.yml (1)
87-87
: The inclusion ofcvat.backend.initContainers
template in the worker export deployment is consistent with other services, enhancing maintainability. Please ensure that this template is tested across different deployment scenarios to verify its impact.Verification successful
The
cvat.backend.initContainers
template is correctly defined inhelm-chart/templates/_helpers.tpl
and is consistently used across multiple deployment files, includingworker_annotation
,worker_import
,worker_export
,utils
, andserver
. This confirms that the template inclusion is properly integrated.
helm-chart/templates/_helpers.tpl
: Template definition.helm-chart/templates/cvat_backend/worker_annotation/deployment.yml
: Template usage.helm-chart/templates/cvat_backend/worker_import/deployment.yml
: Template usage.helm-chart/templates/cvat_backend/worker_export/deployment.yml
: Template usage.helm-chart/templates/cvat_backend/utils/deployment.yml
: Template usage.helm-chart/templates/cvat_backend/server/deployment.yml
: Template usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the template inclusion works across different configurations. # Test: Deploy using different configurations and check for errors. echo "Deploying with multiple configurations to test new init container template..." # Example command, replace with actual deployment command kubectl apply -f deployment.ymlLength of output: 273
Script:
#!/bin/bash # Description: Verify the presence and correctness of the `cvat.backend.initContainers` template in the Helm chart. # Search for the definition of the `cvat.backend.initContainers` template echo "Searching for the definition of the 'cvat.backend.initContainers' template..." rg --multiline --multiline-dotall 'define "cvat.backend.initContainers"' . # Search for the usage of the `cvat.backend.initContainers` template across the Helm chart files echo "Searching for the usage of the 'cvat.backend.initContainers' template..." rg --multiline --multiline-dotall '{{- include "cvat.backend.initContainers"' .Length of output: 688
Script:
#!/bin/bash # Description: Verify the presence and correctness of the `cvat.backend.initContainers` template in the Helm chart. # Search for the definition of the `cvat.backend.initContainers` template echo "Searching for the definition of the 'cvat.backend.initContainers' template..." rg --multiline --multiline-dotall 'define "cvat.backend.initContainers"' . # Search for the usage of the `cvat.backend.initContainers` template across the Helm chart files echo "Searching for the usage of the 'cvat.backend.initContainers' template..." rg --multiline --multiline-dotall '\{\{- include "cvat\.backend\.initContainers"' .Length of output: 1224
helm-chart/templates/cvat_backend/worker_import/deployment.yml (1)
87-87
: The inclusion ofcvat.backend.initContainers
template in the worker import deployment is consistent with other services, enhancing maintainability. Please ensure that this template is tested across different deployment scenarios to verify its impact.Verification successful
The
cvat.backend.initContainers
template is defined inhelm-chart/templates/_helpers.tpl
, confirming its presence in the Helm chart templates. This ensures that the inclusion in theworker_import/deployment.yml
file is valid and consistent with the rest of the configuration.
helm-chart/templates/_helpers.tpl
:{{- define "cvat.backend.initContainers" -}}
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the template inclusion works across different configurations. # Test: Deploy using different configurations and check for errors. echo "Deploying with multiple configurations to test new init container template..." # Example command, replace with actual deployment command kubectl apply -f deployment.ymlLength of output: 273
Script:
#!/bin/bash # Description: Render the Helm chart and inspect the inclusion of the initContainers template. # Render the Helm chart for the cvat_backend worker_import deployment helm template cvat . > rendered_output.yaml # Check for the inclusion of the initContainers template in the rendered output grep -A 10 "initContainers:" rendered_output.yamlLength of output: 144
Script:
#!/bin/bash # Description: Search for the cvat.backend.initContainers template definition in the Helm chart templates. # Find the template definition in the Helm chart templates grep -r "define \"cvat.backend.initContainers\"" helm-chart/templates/Length of output: 149
helm-chart/templates/cvat_backend/worker_annotation/deployment.yml (1)
87-87
: The inclusion ofcvat.backend.initContainers
template in the worker annotation deployment is consistent with other services, enhancing maintainability. Please ensure that this template is tested across different deployment scenarios to verify its impact.Verification successful
The
cvat.backend.initContainers
template is defined inhelm-chart/templates/_helpers.tpl
and is used in multiple deployment files, includinghelm-chart/templates/cvat_backend/worker_annotation/deployment.yml
. This confirms that the template is correctly defined and utilized across the codebase.
- Template Definition:
helm-chart/templates/_helpers.tpl
- Template Usages:
helm-chart/templates/cvat_backend/worker_import/deployment.yml
helm-chart/templates/cvat_backend/worker_annotation/deployment.yml
helm-chart/templates/cvat_backend/worker_export/deployment.yml
helm-chart/templates/cvat_backend/utils/deployment.yml
helm-chart/templates/cvat_backend/server/deployment.yml
The inclusion of the
cvat.backend.initContainers
template in the worker annotation deployment is consistent with other services, enhancing maintainability.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the template inclusion works across different configurations. # Test: Deploy using different configurations and check for errors. echo "Deploying with multiple configurations to test new init container template..." # Example command, replace with actual deployment command kubectl apply -f deployment.ymlLength of output: 273
Script:
#!/bin/bash # Description: Verify the definition and usage of the `cvat.backend.initContainers` template. # Search for the template definition echo "Searching for the definition of 'cvat.backend.initContainers' template..." ast-grep --lang yaml --pattern $'{{- define "cvat.backend.initContainers" }}' # Search for the usage of the template across the codebase echo "Searching for the usage of 'cvat.backend.initContainers' template..." rg '{{- include "cvat.backend.initContainers" . }}'Length of output: 679
Script:
#!/bin/bash # Description: Verify the definition and usage of the `cvat.backend.initContainers` template. # Search for the template definition echo "Searching for the definition of 'cvat.backend.initContainers' template..." grep -r '{{- define "cvat.backend.initContainers" }}' . # Search for the usage of the template across the codebase echo "Searching for the usage of 'cvat.backend.initContainers' template..." rg '\{\{- include "cvat\.backend\.initContainers" \. \}\}'Length of output: 434
Script:
#!/bin/bash # Description: Verify all template definitions and usages in the codebase. # Search for all template definitions echo "Searching for all template definitions..." grep -r '{{- define' . # Search for all template usages echo "Searching for all template usages..." rg '\{\{- include'Length of output: 7219
@jackylamhk Could you take a look at the failed test? |
03b864e
to
300bab9
Compare
Looks like I messed up the YAML indent of the volume mounts. Pushed a fix for it |
b3dfcf8
to
77d6505
Compare
Hi, what is the status of this PR review? This PR looks good to me. I am also against issues like this where a volume created from a config is read-only, and the init container tries to change its permissions. Current solution is patching the helm chart though our forked repo |
|
Motivation and context
Not all volumes support changing permissions of files/directories; by allowing administrators to specify the sub-directories to update permissions allows greater flexibility for deployments.
For example, an admin may choose to mount
/data
with an S3 backend but the other directories under EBS/EFS, the initContainer will fail because the S3 CSI driver will not allow changing permissions.How has this been tested?
Checklist
develop
branch(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
Refactor
Chores