Include yorkie-mongodb to yorkie-cluster Helm Chart #1031
Conversation
WalkthroughThe changes in this pull request primarily involve updates to the Helm chart for the Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 yamllint
🔇 Additional comments (4)
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: 2
🧹 Outside diff range and nitpick comments (9)
build/charts/yorkie-cluster/templates/yorkie/role.yaml (3)
2-10: LGTM: Well-defined Role with appropriate permissionsThe Role "db-status-reader" is well-defined with appropriate read-only permissions for jobs in the batch API group. This aligns with the principle of least privilege and is likely used to check the status of database-related jobs.
Consider adding a comment to explain the purpose of this Role, for example:
kind: Role metadata: namespace: {{ index .Values "yorkie-mongodb" "namespace" }} name: db-status-reader # This role allows reading the status of database-related jobs
14-26: LGTM: Correct RoleBinding configurationThe RoleBinding "read-db-status" correctly associates the "db-status-reader" Role with the "yorkie-db-service-account" ServiceAccount. The use of different namespaces for the RoleBinding and ServiceAccount aligns with the PR objectives, allowing the Yorkie cluster to access MongoDB status across namespaces.
Consider adding a comment to explain the cross-namespace binding, for example:
kind: RoleBinding metadata: name: read-db-status namespace: {{ index .Values "yorkie-mongodb" "namespace" }} # This RoleBinding allows the Yorkie cluster to access MongoDB status across namespaces
30-34: LGTM: ServiceAccount correctly definedThe ServiceAccount "yorkie-db-service-account" is correctly defined and scoped to the Yorkie cluster namespace. This setup is consistent with the RoleBinding configuration and will allow the Yorkie cluster to access MongoDB status.
Consider adding a comment to explain the purpose of this ServiceAccount, for example:
kind: ServiceAccount metadata: name: yorkie-db-service-account namespace: {{ .Values.yorkie.namespace }} # This ServiceAccount is used by the Yorkie cluster to access MongoDB statusbuild/charts/yorkie-cluster/values.yaml (2)
74-81: LGTM! Consider adding more configuration options and documentation.The new
yorkie-mongodbconfiguration section aligns well with the PR objectives. It provides essential settings for integrating MongoDB as a subchart. However, consider the following suggestions to enhance flexibility and usability:
- Add comments explaining each configuration option, especially the
sharded.enabledflag.- Consider adding more configuration options that users might need to customize, such as authentication settings, resource limits, or storage configurations.
- Include a reference to the
yorkie-mongodbchart documentation for more advanced settings.Here's an example of how you could enhance the configuration:
# Configuration for yorkie-mongodb yorkie-mongodb: # Name of the MongoDB deployment name: mongodb # Namespace where MongoDB will be deployed namespace: mongodb # Port on which MongoDB will listen port: 27017 # Kubernetes cluster domain clusterDomain: cluster.local # Sharded cluster configuration sharded: # Enable sharded cluster setup enabled: false # Add more configuration options as needed, e.g.: # auth: # enabled: false # rootPassword: "" # resources: # limits: # cpu: 1 # memory: 1Gi # For more advanced settings, refer to the yorkie-mongodb chart documentation: # https://github.com/yorkie-team/yorkie/tree/main/build/charts/yorkie-mongodb🧰 Tools
🪛 yamllint
[error] 81-81: no new line character at the end of file
(new-line-at-end-of-file)
81-81: Add a newline at the end of the file.To adhere to best practices and prevent potential issues with some tools, please add a newline character at the end of the file.
Apply this change to the last line of the file:
sharded: enabled: false +🧰 Tools
🪛 yamllint
[error] 81-81: no new line character at the end of file
(new-line-at-end-of-file)
build/charts/yorkie-cluster/README.md (1)
Line range hint
1-85: Overall, excellent improvements to the installation instructions.The changes made to this README file successfully address the PR objectives and the concerns raised in issue #819. The installation process for the yorkie-cluster has been simplified, and the integration of yorkie-mongodb as a subchart is now clearly reflected in the instructions.
To further enhance the documentation:
Consider adding a brief explanation of what the
yorkie-mongodb.sharded.enabled=trueoption does, perhaps in a comment or a note below the installation command. This would help users understand the implications of enabling sharding during installation.build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1)
89-89: Add a newline character at the end of the file.To adhere to best practices and resolve the linter warning, please add a newline character at the end of the file.
Apply this change:
{{ toYaml .Values.yorkie.resources | nindent 12 }} +🧰 Tools
🪛 yamllint
[error] 89-89: no new line character at the end of file
(new-line-at-end-of-file)
build/charts/yorkie-cluster/charts/yorkie-mongodb/values.yaml (2)
176-176: Approved: Explicit namespace override for mongos service.The addition of
namespaceOverride: mongodbfor themongosservice is a good practice. It ensures that themongospods are deployed in the correct namespace, addressing the namespace-related issues mentioned in the PR objectives.For consistency, consider adding similar
namespaceOverridefields to theconfigsvrandshardsvrsections if they should also be deployed in themongodbnamespace. If the current setup is intentional, it might be helpful to add a comment explaining why onlymongoshas this override.
Line range hint
1-238: Overall configuration looks good, with room for future enhancements.The configuration for the MongoDB sharded cluster is comprehensive and well-structured. The addition of the
namespaceOverridefor themongosservice addresses the namespace-related issues mentioned in the PR objectives.Some suggestions for future improvements:
- Consider enabling and configuring the commented-out probe sections (liveness, readiness, and startup probes) for better health monitoring of the MongoDB components.
- Review the resource configurations (currently empty for some components) to ensure optimal performance in production environments.
- Evaluate if the metrics configuration (currently enabled but with
podMonitordisabled) meets your monitoring requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
build/charts/yorkie-cluster/Chart.lockis excluded by!**/*.lockbuild/charts/yorkie-cluster/charts/yorkie-mongodb/Chart.lockis excluded by!**/*.lockbuild/charts/yorkie-mongodb/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
- .gitignore (1 hunks)
- build/charts/yorkie-cluster/Chart.yaml (1 hunks)
- build/charts/yorkie-cluster/README.md (1 hunks)
- build/charts/yorkie-cluster/charts/yorkie-mongodb/Chart.yaml (1 hunks)
- build/charts/yorkie-cluster/charts/yorkie-mongodb/README.md (1 hunks)
- build/charts/yorkie-cluster/charts/yorkie-mongodb/values.yaml (1 hunks)
- build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (2 hunks)
- build/charts/yorkie-cluster/templates/yorkie/role.yaml (1 hunks)
- build/charts/yorkie-cluster/values.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- build/charts/yorkie-cluster/charts/yorkie-mongodb/README.md
🧰 Additional context used
🪛 yamllint
build/charts/yorkie-cluster/templates/yorkie/deployment.yaml
[error] 89-89: no new line character at the end of file
(new-line-at-end-of-file)
build/charts/yorkie-cluster/templates/yorkie/role.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
build/charts/yorkie-cluster/values.yaml
[error] 81-81: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (9)
build/charts/yorkie-cluster/charts/yorkie-mongodb/Chart.yaml (1)
18-18: Verify compatibility with the updated mongodb-sharded version.The mongodb-sharded dependency has been updated from version 7.2.2 to 9.0.1, which is a significant version jump. This change aligns with the PR objectives.
To ensure compatibility and proper functioning:
- Verify that this version update is intentional and aligns with the project's requirements.
- Check the changelog of mongodb-sharded between versions 7.2.2 and 9.0.1 for any breaking changes or new features that might affect the yorkie-mongodb chart.
- Ensure that all other components in the yorkie-cluster are compatible with this new version.
- Update any relevant documentation or README files to reflect this version change.
Please review the results of these checks and make any necessary adjustments to ensure smooth integration of the new mongodb-sharded version.
build/charts/yorkie-cluster/Chart.yaml (2)
Line range hint
1-22: LGTM! Changes align with PR objectives.The addition of the yorkie-mongodb dependency in the Chart.yaml file successfully addresses the main objective of this PR. The dependency is correctly specified with an appropriate condition for enabling the sharded setup. The chart and app versions are consistent, and the overall formatting of the file is correct.
18-21: LGTM! Consider verifying the yorkie-mongodb version.The addition of the yorkie-mongodb dependency aligns well with the PR objectives. The condition for enabling it based on the sharded setup is appropriate. However, it's worth double-checking if 0.4.13 is the latest stable version of yorkie-mongodb.
To verify the latest version of yorkie-mongodb, you can run:
build/charts/yorkie-cluster/templates/yorkie/role.yaml (3)
1-1: LGTM: Conditional creation of RBAC resourcesThe conditional statement ensures that the RBAC resources are only created when sharding is enabled for yorkie-mongodb. This is a good practice to avoid creating unnecessary resources and aligns well with the PR objectives.
Also applies to: 35-35
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
1-1: Ignore yamllint error: False positive due to Helm template directiveThe yamllint error reported for line 1 appears to be a false positive. The
{{- if ... }}syntax is valid for Helm templates but may not be recognized by yamllint. This error can be safely ignored as it doesn't affect the functionality of the Helm chart.🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
1-35: Overall assessment: Well-structured RBAC configuration for Yorkie clusterThis file provides a well-structured and appropriate RBAC configuration for the Yorkie cluster to access MongoDB status. Key points:
- Conditional creation of resources based on MongoDB sharding configuration.
- Proper Role definition with least privilege principle.
- Correct RoleBinding setup allowing cross-namespace access.
- Appropriate ServiceAccount definition.
The implementation aligns well with the PR objectives and demonstrates good use of Kubernetes RBAC and Helm templating. The minor suggestions for adding comments will further improve the maintainability of the chart.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
.gitignore (1)
74-74: LGTM: Gitignore updated to reflect new chart structureThis change correctly updates the
.gitignorefile to ignore theyorkie-mongodbcharts within theyorkie-clusterdirectory. This aligns with the PR objective of includingyorkie-mongodbas a subchart ofyorkie-cluster, reflecting the new directory structure for the integrated charts.build/charts/yorkie-cluster/README.md (1)
29-34: Excellent addition of the MongoDB namespace creation step.The inclusion of the
kubectl create namespace yorkiecommand is a crucial improvement. It ensures that the necessary namespace exists before the chart installation, which is essential for the integration of yorkie-mongodb as a subchart. This change directly addresses the objective of simplifying the installation process as mentioned in issue #819.build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1)
89-89: LGTM: Proper use of Helm templating for resource configuration.The use of
{{ toYaml .Values.yorkie.resources | nindent 12 }}is a correct and flexible way to include resource configurations from the values file. This approach allows for easy customization of resource limits and requests.🧰 Tools
🪛 yamllint
[error] 89-89: no new line character at the end of file
(new-line-at-end-of-file)
| helm install [RELEASE_NAME] yorkie-team/yorkie-cluster --set=yorkie-mongodb.sharded.enable | ||
| d=true |
There was a problem hiding this comment.
Great addition of the sharding option, but there's a small typo.
The inclusion of the yorkie-mongodb.sharded.enabled=true option in the Helm install command is an excellent improvement. It allows users to easily enable sharding during installation, which aligns with the PR objectives.
However, there's a typo in the command that needs to be fixed:
Please correct the typo by applying this change:
-helm install [RELEASE_NAME] yorkie-team/yorkie-cluster --set=yorkie-mongodb.sharded.enable
-d=true
+helm install [RELEASE_NAME] yorkie-team/yorkie-cluster --set=yorkie-mongodb.sharded.enabled=trueThis will ensure that users can copy-paste the command without encountering errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| helm install [RELEASE_NAME] yorkie-team/yorkie-cluster --set=yorkie-mongodb.sharded.enable | |
| d=true | |
| helm install [RELEASE_NAME] yorkie-team/yorkie-cluster --set=yorkie-mongodb.sharded.enabled=true |
| {{ if index .Values "yorkie-mongodb" "sharded" "enabled"}} | ||
| initContainers: | ||
| - name: wait-db | ||
| image: bitnami/kubectl:latest | ||
| command: | ||
| - /bin/sh | ||
| - -c | ||
| - | | ||
| echo "Wait db start" | ||
| while true; do | ||
| job_status=$(kubectl get job -n $NAMESPACE $JOB_NAME -o jsonpath='{.status.conditions[?(@.type=="Complete")].status}') | ||
| if [ "$job_status" = "True" ]; then | ||
| echo "Job completed" | ||
| break | ||
| fi | ||
| echo "Waiting for job to complete..." | ||
| sleep 5 | ||
| done | ||
| env: | ||
| - name: NAMESPACE | ||
| value: "{{ index .Values "yorkie-mongodb" "namespace" }}" | ||
| - name: JOB_NAME | ||
| value: "{{ index .Values "yorkie-mongodb" "name" }}-setup-job" | ||
| volumeMounts: | ||
| - name: kubeconfig | ||
| mountPath: /root/.kube | ||
| subPath: config | ||
| volumes: | ||
| - name: kubeconfig | ||
| emptyDir: { } | ||
| serviceAccountName: yorkie-db-service-account | ||
| {{ end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve initContainer configuration for better reliability and security.
The addition of an initContainer to wait for the MongoDB setup job is a good practice. However, there are a few points to consider:
- Instead of using
bitnami/kubectl:latest, specify a fixed version to ensure consistency across deployments. - Consider adding a timeout mechanism to the while loop to prevent infinite waiting if the job fails.
- The use of a dedicated service account (
yorkie-db-service-account) is good for security. Ensure this service account has the minimum required permissions.
Here's a suggested improvement for the command:
command:
- /bin/sh
- -c
- |
echo "Waiting for db to start"
timeout=300
start_time=$(date +%s)
while true; do
job_status=$(kubectl get job -n $NAMESPACE $JOB_NAME -o jsonpath='{.status.conditions[?(@.type=="Complete")].status}')
if [ "$job_status" = "True" ]; then
echo "Job completed successfully"
exit 0
fi
current_time=$(date +%s)
elapsed_time=$((current_time - start_time))
if [ $elapsed_time -ge $timeout ]; then
echo "Timeout waiting for job to complete"
exit 1
fi
echo "Waiting for job to complete... (${elapsed_time}s elapsed)"
sleep 5
doneThis modification adds a 5-minute timeout and provides more informative logging.
There was a problem hiding this comment.
Thank you for your contribution!
I'm very happy to see you have also made contribution to bitnami to finish this PR!
I have one thing to suggest, which is to provide options for the users to select whether to install with standalone mongodb, or sharded mongodb? The reason for this is to allow users to provision lightweight resources through standalone mongodb.
We had standalone mongodb in the past, and you can refer it to this PR #769.
Also, I have left some comments regarding wording and syntax specifications.
| kind: Role | ||
| metadata: | ||
| namespace: {{ index .Values "yorkie-mongodb" "namespace" }} | ||
| name: db-status-reader |
There was a problem hiding this comment.
How about changing the name more specifically like:
db-status-readertodb-job-status-readerread-db-statustodb-provision-job-status-readeryorkie-db-service-accounttoyorkie-db-provision-job-status-reader
to clarify the purpose of the resources?
- refactor typo and wording - set sharded=true to default option - remove yorkie-mongodb dependency condition for standalone mode
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
build/charts/yorkie-cluster/templates/yorkie/role.yaml (1)
1-10: LGTM with a minor suggestion for improved readability.The Role definition looks good. The permissions are appropriate for reading job statuses, and the name has been updated as per the previous review comment.
Consider adding quotes around the
enabledvalue in the conditional statement for better readability:{{- if index .Values "yorkie-mongodb" "sharded" "enabled" }}This change doesn't affect functionality but improves consistency with other Helm templates.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
build/charts/yorkie-cluster/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
- build/charts/yorkie-cluster/Chart.yaml (1 hunks)
- build/charts/yorkie-cluster/README.md (1 hunks)
- build/charts/yorkie-cluster/templates/yorkie/deployment.yaml (1 hunks)
- build/charts/yorkie-cluster/templates/yorkie/role.yaml (1 hunks)
- build/charts/yorkie-cluster/values.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- build/charts/yorkie-cluster/Chart.yaml
- build/charts/yorkie-cluster/README.md
- build/charts/yorkie-cluster/templates/yorkie/deployment.yaml
- build/charts/yorkie-cluster/values.yaml
🧰 Additional context used
🪛 yamllint
build/charts/yorkie-cluster/templates/yorkie/role.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (1)
build/charts/yorkie-cluster/templates/yorkie/role.yaml (1)
1-35: Improve readability with newlines and ignore false positive.The overall structure of the file is correct, but we can improve its readability.
- Add newlines between the different resource definitions:
- After line 11 (end of Role definition)
- After line 27 (end of RoleBinding definition)
This change will improve the readability of the file.
- Regarding the yamllint error:
This is a false positive due to the Helm template syntax (specifically the[error] 1-1: syntax error: expected the node content, but found '-'{{-at the beginning of the file). We can safely ignore this error as it doesn't affect the functionality of the template.🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: RoleBinding | ||
| metadata: | ||
| name: read-db-status | ||
| namespace: {{ index .Values "yorkie-mongodb" "namespace" }} | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: yorkie-db-service-account | ||
| namespace: {{ .Values.yorkie.namespace }} | ||
| roleRef: | ||
| kind: Role | ||
| name: db-provisioning-job-status-reader | ||
| apiGroup: rbac.authorization.k8s.io |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating the RoleBinding and ServiceAccount names for clarity.
The RoleBinding configuration looks correct, but we can improve the naming to better reflect its purpose.
Consider the following name changes:
- For the RoleBinding (line 17):
name: db-provision-job-status-reader- For the ServiceAccount (line 21):
name: yorkie-db-provision-job-status-readerThese changes align with the previous review suggestions and make the purpose of these resources more explicit.
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: yorkie-db-service-account | ||
| namespace: {{ .Values.yorkie.namespace }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update ServiceAccount name for consistency.
The ServiceAccount definition is correct, but let's ensure consistency with the naming convention suggested earlier.
Change the ServiceAccount name to match the suggestion in the RoleBinding:
name: yorkie-db-provision-job-status-readerThis change will make the purpose of the ServiceAccount clearer and maintain consistency across the RBAC resources.
This commit includes yorkie-mongodb to yorkie-cluster Helm chart as subchart to fully provide Yorkie cluster feature including DB layer when installing yorkie-cluster Helm chart.
Include yorkie-mongodb to yorkie-cluster Helm chart as subchart to fully provide Yorkie cluster feature including DB layer when installing yorkie-cluster Helm chart.
What this PR does / why we need it:
The
yorkie-mongodbchart was placed as a subchart of theyorkie-clusterchart.Therefore, when
yorkie-clusteris installed,yorkie-mongodbis also installed.There was a problem where it was difficult to use the
mongodb-shardedchart as a subchart because the namespace was not applied properly, and I fixed this. (related PR)After confirming the modified PR merge and
mongodb-shardedversion upgrade, themongodb-shardedversion used byyorkie-mongodbwas also upgraded. (7.2.2 -> 9.0.1)When creating a deployment in
yorkie-cluster, I used initContainer to detect thatyorkie-mongodbwas created successfully, and setyorkie-clusterpods to be created after that.Which issue(s) this PR fixes:
Fixes #819
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
yorkie-mongodbwith version0.4.13.yorkie-mongodbin the values file.initContainersto ensure the database is ready before application startup.Documentation
yorkie-clusterandyorkie-mongodb.yorkie-mongodbHelm chart.Bug Fixes
values.yamlfile for proper YAML formatting.