-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat:pika-master-slave-cluster in kb #2903
base: unstable
Are you sure you want to change the base?
feat:pika-master-slave-cluster in kb #2903
Conversation
…ead of script to bind master-slave relation
…Warning: This feature still has bugs waiting to be fixed)
…but still retaining)
WalkthroughThe changes encompass the addition of Helm charts and configuration files for managing Pika clusters within Kubernetes. Key updates include installation and uninstallation commands for Pika clusters, the introduction of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes
User->>Helm: Install Pika Cluster
Helm->>Kubernetes: Deploy Pika Resources
Kubernetes->>Kubernetes: Create ConfigMap, Role, RoleBinding, ServiceAccount
Kubernetes->>Kubernetes: Create Pika Cluster
User->>Helm: Uninstall Pika Cluster
Helm->>Kubernetes: Remove Pika Resources
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: 15
Outside diff range and nitpick comments (8)
tools/kubeblocks_helm/pika-master-slave/templates/componentversion-pika.yaml (1)
1-18
: Excellent work on defining the ComponentVersion CRD!The CRD structure is logically correct and follows the Kubernetes API conventions. The compatibility rules and releases sections provide a clear way to manage and track the Pika component versions. The use of Helm templating for the Docker image ensures flexibility.
Just a few minor formatting issues to address:
- {{- include "pika.labels" . | nindent 4 }} + labels: + {{- include "pika.labels" . | nindent 4 }} spec: compatibilityRules: - compDefs: - - pika + - pika - releases: + releases: - {{ .Chart.AppVersion }} releases: - name: {{ .Chart.AppVersion }} changes: serviceVersion: {{ .Chart.AppVersion }} images: pika: {{ include "pika.image" . }} +Tools
yamllint
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 8 but found 6
(indentation)
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
tools/kubeblocks_helm/pika-master-slave-cluster/templates/cluster.yaml (1)
11-11
: Adjustnindent
value for proper alignment oftopologyKeys
The
nindent 6
in line 11 may not correctly align thetopologyKeys
content underaffinity
. Changing it tonindent 8
ensures proper indentation in the generated YAML.Apply the following diff:
- topologyKeys: {{ . | toYaml | nindent 6 }} + topologyKeys: {{ . | toYaml | nindent 8 }}Tools
yamllint
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
tools/kubeblocks_helm/pika-master-slave/templates/componentdefinition-pika.yaml (3)
45-45
: Correct typo in commentThe comment at line 45 contains the word "Ergodic," which seems incorrect in this context. It should be "Iterate" to accurately describe the action.
Apply this diff to correct the comment:
45- # Ergodic name and ip array + # Iterate over name and ip array
73-73
: Remove trailing spacesLine 73 contains trailing spaces, which can cause issues with some linters and version control systems.
Apply this diff to remove the trailing spaces:
73- mountPath: /data + mountPath: /data
Tools
yamllint
[error] 73-73: trailing spaces
(trailing-spaces)
98-98
: Add newline at end of fileThe file does not end with a newline character, which is a common convention and can prevent potential issues with some tools.
Apply this diff to add a newline at the end of the file:
98 - "/etc/pika/pika.conf" +
Tools
yamllint
[error] 98-98: no new line character at the end of file
(new-line-at-end-of-file)
tools/kubeblocks_helm/pika-master-slave/config/pika-config.tpl (3)
537-552
: Provide Clear Examples forrename-command
UsageThe commented section for
rename-command
can be enhanced with clearer examples to help users understand how to safely rename commands.Would you like me to provide updated examples that demonstrate the correct usage of
rename-command
, ensuring consistency and preventing potential misconfigurations?
225-226
: Unused Parametercompact-interval
The
compact-interval
parameter is commented out without a value. If this parameter is intended to be used, consider providing a default value or removing it if it's unnecessary.-#compact-interval :
192-193
: Uncommentnetwork-interface
If NeededThe
network-interface
setting is commented out. If you need to bind Pika to a specific network interface, uncomment this line and specify the appropriate interface.-#network-interface : eth1 +network-interface : eth0 # Adjust 'eth0' to your desired network interface
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- tools/kubeblocks_helm/README.md (2 hunks)
- tools/kubeblocks_helm/pika-master-slave-cluster/.helmignore (1 hunks)
- tools/kubeblocks_helm/pika-master-slave-cluster/Chart.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave-cluster/templates/_helpers.tpl (1 hunks)
- tools/kubeblocks_helm/pika-master-slave-cluster/templates/cluster.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave-cluster/templates/role.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave-cluster/templates/rolebinding.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave-cluster/templates/serviceaccount.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave-cluster/values.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/.helmignore (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/Chart.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/config/pika-config.tpl (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/templates/_helpers.tpl (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/templates/clusterdefinition.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/templates/componentdefinition-pika.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/templates/componentversion-pika.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/templates/configmap.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/templates/script.yaml (1 hunks)
- tools/kubeblocks_helm/pika-master-slave/values.yaml (1 hunks)
Files skipped from review due to trivial changes (4)
- tools/kubeblocks_helm/pika-master-slave-cluster/.helmignore
- tools/kubeblocks_helm/pika-master-slave-cluster/Chart.yaml
- tools/kubeblocks_helm/pika-master-slave/.helmignore
- tools/kubeblocks_helm/pika-master-slave/Chart.yaml
Additional context used
yamllint
tools/kubeblocks_helm/pika-master-slave-cluster/templates/cluster.yaml
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[error] 10-10: syntax error: expected the node content, but found '-'
(syntax)
tools/kubeblocks_helm/pika-master-slave/templates/clusterdefinition.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
tools/kubeblocks_helm/pika-master-slave/templates/componentdefinition-pika.yaml
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
[error] 73-73: trailing spaces
(trailing-spaces)
[error] 98-98: no new line character at the end of file
(new-line-at-end-of-file)
tools/kubeblocks_helm/pika-master-slave/templates/componentversion-pika.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 8 but found 6
(indentation)
[error] 11-11: trailing spaces
(trailing-spaces)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
tools/kubeblocks_helm/pika-master-slave/templates/configmap.yaml
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
tools/kubeblocks_helm/pika-master-slave/templates/script.yaml
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
Additional comments not posted (49)
tools/kubeblocks_helm/pika-master-slave-cluster/templates/serviceaccount.yaml (1)
1-7
: LGTM!The ServiceAccount resource is correctly defined with the necessary fields. The use of Helm template functions for generating the name, namespace, and labels is a good practice that enhances the deployment's configurability and maintainability.
tools/kubeblocks_helm/pika-master-slave/templates/script.yaml (1)
1-10
: LGTM!The ConfigMap resource is well-structured and follows best practices. It effectively manages the
admin.sh
script for the Pika master-slave setup.Key points:
- The metadata section correctly sets the name, namespace, and labels for the ConfigMap.
- The data section retrieves the script content from the specified file path.
- The script content is properly indented for YAML formatting.
Regarding the yamllint error on line 7, it is a false positive. The syntax
{{- include "pika.labels" . | nindent 4 }}
is valid in Helm templates and does not require any changes.Tools
yamllint
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
tools/kubeblocks_helm/pika-master-slave/templates/configmap.yaml (1)
1-10
: LGTM!The ConfigMap resource is well-structured and serves its intended purpose of storing configuration data for the Pika master-slave setup. The metadata and data sections are correctly defined, and the use of the
pika-config.tpl
template file allows for flexible configuration management.Tools
yamllint
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
tools/kubeblocks_helm/pika-master-slave-cluster/templates/role.yaml (1)
1-14
: LGTM!The Kubernetes Role is correctly defined with the following observations:
- The
apiVersion
andkind
fields are accurately specified for a Role resource.- The
metadata
section correctly sets the Role name and namespace using template functions, which allows for dynamic naming based on the cluster.- The
rules
section appropriately grants thecreate
verb on theevents
resource in the core API group, enabling the application to create Kubernetes events for logging and monitoring purposes.- The labels are set using a template function, which is a good practice for maintaining consistency across resources.
The permissions are properly scoped to a specific namespace and resource, ensuring the principle of least privilege.
tools/kubeblocks_helm/pika-master-slave/values.yaml (4)
1-2
: LGTM!The Pika version is specified correctly.
3-12
: LGTM!The Docker image specifications for Pika and Redis are correct:
- The registry is set to docker.io.
- The repository is set to pikadb/pika for Pika and redis for Redis.
- The tag matches the Pika version specified in the
pika
section.- The pull policy is set to IfNotPresent.
13-17
: LGTM!The
roleProbe
configuration for Pika looks good:
- The
failureThreshold
is set to 2, which is a reasonable value for marking the container as unhealthy after consecutive failures.- The
periodSeconds
is set to 1, ensuring frequent health checks.- The
timeoutSeconds
is set to 1, providing a reasonable timeout for the health check response.
18-20
: LGTM!The configuration variables are set correctly:
- The
nameOverride
andfullnameOverride
are empty strings, allowing the default naming conventions to be used.- The
clusterDomain
is set to ".cluster.local", which is a common default value for Kubernetes clusters.tools/kubeblocks_helm/pika-master-slave-cluster/templates/rolebinding.yaml (1)
1-15
: LGTM! The RoleBinding is properly configured.The
rolebinding.yaml
file correctly defines a Kubernetes RoleBinding resource that associates a specific Role with a ServiceAccount within the designated namespace. The use of Helm template functions for dynamic generation of the RoleBinding name, labels, and other values ensures consistency and ease of management across different deployments.Properly configured RoleBindings are essential for enforcing security and access control policies within the Kubernetes cluster. They ensure that ServiceAccounts have the necessary permissions to perform their intended functions while adhering to the principle of least privilege.
It's crucial to regularly review and audit RoleBindings to maintain a secure and well-managed Kubernetes environment. Incorrect or overly permissive RoleBindings can lead to security vulnerabilities and unauthorized access to cluster resources.
Great job on properly structuring and configuring this RoleBinding!
tools/kubeblocks_helm/pika-master-slave-cluster/values.yaml (17)
5-5
: LGTM!The
nameOverride
parameter is correctly set to an empty string, allowing the default release name to be used.
6-6
: LGTM!The
fullnameOverride
parameter is correctly set to an empty string, allowing the default naming convention to be used for the Helm release.
8-8
: LGTM!The
slaveCount
parameter is set to 1, indicating that a single slave instance will be deployed by default in the Pika cluster.
10-10
: LGTM!The
terminationPolicy
parameter is set to "Delete", ensuring that the Pika cluster resources will be deleted when the cluster is terminated.
12-12
: LGTM!The
clusterVersionOverride
parameter is correctly set to an empty string, allowing the default cluster version to be used.
14-15
: LGTM!The
monitor.enabled
parameter is set to false, disabling monitoring for the Pika cluster by default.
17-18
: LGTM!The
switchPolicy.type
parameter is set to "Noop", indicating that no operational changes will occur under this policy for the Pika cluster.
25-28
: LGTM!The
resources.pikaGroup.limits.cpu
parameter is set to "500m", specifying a CPU limit of 0.5 cores for the Pika group.
25-28
: LGTM!The
resources.pikaGroup.limits.memory
parameter is set to "3Gi", specifying a memory limit of 3 gigabytes for the Pika group.
29-31
: LGTM!The
resources.pikaGroup.requests.cpu
parameter is set to "500m", specifying a CPU request of 0.5 cores for the Pika group.
29-31
: LGTM!The
resources.pikaGroup.requests.memory
parameter is set to "1Gi", specifying a memory request of 1 gigabyte for the Pika group.
33-34
: LGTM!The
persistence.enabled
parameter is set to true, enabling persistence for the Pika cluster by default.
35-37
: LGTM!The
persistence.pikaData.storageClassName
parameter is correctly set to an empty string, allowing the default storage class to be used for Pika data persistence.
35-37
: LGTM!The
persistence.pikaData.size
parameter is set to "10Gi", specifying a size of 10 gigabytes for the persistent volume used for Pika data.
39-40
: LGTM!The
topologyKeys
parameter is set to ["kubernetes.io/hostname"], indicating that pods will be scheduled based on the Kubernetes node hostname.
42-45
: LGTM!The
tolerations
parameter is set to an empty array, indicating that no specific tolerations are defined for the Pika cluster pods.
51-52
: LGTM!The
serviceAccount.name
parameter is correctly set to an empty string, allowing the default service account to be used by the Pika cluster components.tools/kubeblocks_helm/pika-master-slave-cluster/templates/_helpers.tpl (7)
1-6
: LGTM!The
pika-cluster.name
helper function is implemented correctly. It follows the best practice of allowing the chart name to be overridden using thenameOverride
value, truncates the name to 63 characters to comply with Kubernetes naming constraints, and removes any trailing dash for cleanliness and consistency.
8-24
: LGTM!The
pika-cluster.fullname
helper function is implemented correctly. It follows the best practice of allowing the fully qualified app name to be overridden using thefullnameOverride
value, intelligently handles the case where the release name already contains the chart name, truncates the name to 63 characters to comply with Kubernetes naming constraints, and removes any trailing dash for cleanliness and consistency.
26-31
: LGTM!The
pika-cluster.chart
helper function is implemented correctly. It provides a convenient way to format the chart name and version into a single string, replaces '+' characters with underscores to ensure compatibility with Kubernetes label value constraints, truncates the string to 63 characters to comply with Kubernetes naming constraints, and removes any trailing dash for cleanliness and consistency.
33-43
: LGTM!The
pika-cluster.labels
helper function is implemented correctly. It provides a centralized way to generate common labels for Kubernetes resources, including the chart name and version, selector labels, app version (if available), and the service managing the release. This helps in identifying and managing the resources effectively.
45-51
: LGTM!The
pika-cluster.selectorLabels
helper function is implemented correctly. It provides a centralized way to generate selector labels for Kubernetes resources, including the app name and instance name (release name). These labels are used by Kubernetes services and other resources to select the appropriate pods and resources, enabling effective management and communication within the cluster.
53-55
: LGTM!The
clustername
helper function is implemented correctly. It provides a simple and convenient way to retrieve the fully qualified name of the cluster by reusing thepika-cluster.fullname
helper function. This ensures consistency with the naming convention and improves code readability and maintainability.
57-62
: LGTM!The
pika-cluster.serviceAccountName
helper function is implemented correctly. It generates a default service account name that includes the cluster name (using theclustername
helper function) prefixed with "kb-", which helps in identifying its relation to the knowledge base (kb) component. The function also allows the service account name to be overridden by a value from the values file, providing flexibility for custom configurations. Using theclustername
helper function ensures consistency with the naming convention of the cluster.tools/kubeblocks_helm/README.md (3)
36-40
: LGTM!The uninstallation commands for the Pika cluster are correct and follow the standard Helm syntax. The order of uninstallation is also correct, ensuring a clean removal of the cluster and its associated resources.
55-73
: LGTM!The installation instructions for the Pika Master/Slave group are well-documented and easy to follow. The Helm commands are correct and adhere to the standard syntax. The inclusion of instructions for connecting to the cluster using port forwarding enhances the usability of the documentation.
75-79
: LGTM!The uninstallation commands for the Pika Master/Slave cluster are correct and follow the standard Helm syntax. The order of uninstallation is also correct, ensuring a clean removal of the cluster and its associated resources.
tools/kubeblocks_helm/pika-master-slave/templates/_helpers.tpl (9)
4-6
: LGTM!The
pika.name
function correctly generates a chart name, allowing for an optional override and ensuring compliance with Kubernetes naming conventions.
13-24
: LGTM!The
pika.fullname
function correctly generates a fully qualified app name, allowing for user overrides, incorporating the release name, and ensuring compliance with Kubernetes naming conventions.
29-31
: LGTM!The
pika.chart
function correctly formats the chart name and version into a single string, replacing invalid characters and ensuring the result is within 63 characters.
36-43
: LGTM!The
pika.labels
function correctly generates a set of common labels for Kubernetes resources, including chart information, selector labels, app version (if available), and the managing service.
48-51
: LGTM!The
pika.selectorLabels
function correctly generates labels for selecting resources, including the application name and instance name.
56-58
: LGTM!The
pika.image
function correctly defines the image path for the application, allowing for registry and repository customization and using the provided tag.
60-62
: LGTM!The
pika.imagePullPolicy
function correctly defines the image pull policy for the application, using the provided value or defaulting to"IfNotPresent"
.
64-66
: LGTM!The
redis.image
function correctly defines the image path for Redis, allowing for registry customization and using the provided repository.
68-70
: Verify the Redis image pull policy configuration.The
redis.imagePullPolicy
function uses.Values.image.pika.pullPolicy
to determine the image pull policy for Redis. However, this seems inconsistent as it's using the Pika configuration for Redis.Please verify if this is the intended behavior or if a separate
.Values.image.redis.pullPolicy
should be used for Redis-specific configuration.tools/kubeblocks_helm/pika-master-slave-cluster/templates/cluster.yaml (1)
22-30
: Verify the existence ofpikaGroup
resource specificationsThe resources block at lines 22-30 relies on
$.Values.resources.pikaGroup
. Ensure that this value is defined in yourvalues.yaml
to prevent rendering issues.Run the following script to check if
pikaGroup
is defined:Verification successful
pikaGroup
resource specifications verified in values.yamlThe
$.Values.resources.pikaGroup
referenced in the cluster.yaml template is correctly defined in the values.yaml file. It includes both 'limits' and 'requests' for CPU and memory, matching the structure expected by the template. The resource specifications are as follows:
- Limits: 500m CPU, 3Gi memory
- Requests: 500m CPU, 1Gi memory
These values appear to be reasonable defaults for a Pika instance. No issues were found with the resource definitions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'pikaGroup' is defined in values files. # Test: Search for 'pikaGroup' definition. Expect: At least one occurrence. rg --type yaml 'pikaGroup' values.yamlLength of output: 90
Script:
#!/bin/bash # Description: Locate values.yaml and check for pikaGroup resource definitions # Search for values.yaml in the chart directory and its subdirectories echo "Searching for values.yaml:" fd -t f values.yaml tools/kubeblocks_helm/pika-master-slave-cluster # If values.yaml is not found, search for any YAML files that might contain resource definitions echo -e "\nSearching for YAML files with resource definitions:" rg --type yaml 'resources:' tools/kubeblocks_helm/pika-master-slave-cluster # Check chart.yaml for any clues about value files or dependencies echo -e "\nChecking chart.yaml:" cat tools/kubeblocks_helm/pika-master-slave-cluster/Chart.yaml # Examine cluster.yaml for any comments or alternative value references echo -e "\nExamining cluster.yaml for alternative value references:" rg '(values|Values)\.resources' tools/kubeblocks_helm/pika-master-slave-cluster/templates/cluster.yamlLength of output: 1664
Script:
#!/bin/bash # Description: Examine values.yaml for pikaGroup resource definitions # Display the relevant section of values.yaml echo "Examining values.yaml for pikaGroup resource definitions:" sed -n '/^resources:/,/^[a-z]/p' tools/kubeblocks_helm/pika-master-slave-cluster/values.yaml # Specifically search for pikaGroup within the resources section echo -e "\nSearching for pikaGroup in resources section:" sed -n '/^resources:/,/^[a-z]/p' tools/kubeblocks_helm/pika-master-slave-cluster/values.yaml | grep -A6 pikaGroup # Count occurrences of cpu and memory under pikaGroup (if found) echo -e "\nCounting occurrences of cpu and memory under pikaGroup:" sed -n '/^resources:/,/^[a-z]/p' tools/kubeblocks_helm/pika-master-slave-cluster/values.yaml | sed -n '/pikaGroup/,/^[a-z]/p' | grep -c 'cpu\|memory'Length of output: 1324
tools/kubeblocks_helm/pika-master-slave/config/pika-config.tpl (3)
469-471
: Clarifycache-type
Parameter UsageEnsure that the listed cache types match the supported data structures and consider providing guidance on how to configure this parameter.
Confirm that all the specified cache types (
string
,set
,zset
,list
,hash
,bit
) are supported and that users understand how to customize this list based on their caching needs.
359-361
: Verifyslotmigrate
SettingThe
slotmigrate
parameter is set tono
. If slot migration is required for your deployment, consider setting this toyes
and ensure that you have reloaded slot keys as per the instructions.Confirm whether slot migration is necessary for your use case and update the setting accordingly.
489-505
: Review Cache Memory PoliciesThe cache configuration settings define important behaviors for memory management:
cache-maxmemory
is set to10737418240
(10GB). Ensure this value aligns with the available system memory to prevent potential OOM (Out of Memory) issues.cache-maxmemory-policy
is set to1
, which corresponds toallkeys-lru
. Verify that this eviction policy suits your workload.cache-maxmemory-samples
andcache-lfu-decay-time
parameters influence the accuracy of the eviction policy. Adjust these values if necessary to balance performance and memory utilization.Review these settings to confirm they meet the requirements of your deployment environment.
name: pika-master-slave-conf-template | ||
namespace: {{ .Release.Namespace }} | ||
labels: | ||
{{- include "pika.labels" . | nindent 4 }} |
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.
Fix the syntax error flagged by yamllint.
The static analysis tool yamllint has flagged a syntax error on line 7. The error message suggests that the node content was expected, but '-' was found instead.
To fix the syntax error, remove the '-' character from line 7:
- {{- include "pika.labels" . | nindent 4 }}
+ {{ include "pika.labels" . | nindent 4 }}
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.
{{- include "pika.labels" . | nindent 4 }} | |
{{ include "pika.labels" . | nindent 4 }} |
Tools
yamllint
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
apiVersion: apps.kubeblocks.io/v1alpha1 | ||
kind: ClusterDefinition | ||
metadata: | ||
name: pika | ||
labels: | ||
{{- include "pika.labels" . | nindent 4 }} | ||
spec: | ||
topologies: | ||
- name: master-slave-cluster | ||
components: | ||
- name: master-slave | ||
compDef: pika | ||
default: true |
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.
The ClusterDefinition
resource is well-structured and aligns with the PR objectives.
The resource follows the Kubernetes CRD conventions and defines a master-slave topology for the Pika cluster, enabling the desired operational capabilities within a Kubernetes environment.
The use of the include
template function for generating labels is a good practice and allows for consistent labeling across resources.
The default: true
flag simplifies deployment for users who may not need to specify alternative configurations.
However, there is a syntax error on line 6 that needs to be addressed. The {{-
syntax is incorrect. To fix the issue, remove the -
character:
- {{- include "pika.labels" . | nindent 4 }}
+ {{ include "pika.labels" . | nindent 4 }}
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.
apiVersion: apps.kubeblocks.io/v1alpha1 | |
kind: ClusterDefinition | |
metadata: | |
name: pika | |
labels: | |
{{- include "pika.labels" . | nindent 4 }} | |
spec: | |
topologies: | |
- name: master-slave-cluster | |
components: | |
- name: master-slave | |
compDef: pika | |
default: true | |
apiVersion: apps.kubeblocks.io/v1alpha1 | |
kind: ClusterDefinition | |
metadata: | |
name: pika | |
labels: | |
{{ include "pika.labels" . | nindent 4 }} | |
spec: | |
topologies: | |
- name: master-slave-cluster | |
components: | |
- name: master-slave | |
compDef: pika | |
default: true |
Tools
yamllint
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
{{- with $.Values.tolerations }} | ||
tolerations: {{ . | toYaml | nindent 4 }} | ||
{{- end }} |
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.
Fix indentation issue in the tolerations
section
The tolerations
section at lines 13-15 has incorrect indentation, which could cause YAML parsing errors. The {{- with $.Values.tolerations }}
block should be correctly aligned under spec
.
Apply the following diff to correct the indentation:
- {{- with $.Values.tolerations }}
- tolerations: {{ . | toYaml | nindent 4 }}
- {{- end }}
+{{- with $.Values.tolerations }}
+ tolerations: {{ . | toYaml | nindent 4 }}
+{{- end }}
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.
{{- with $.Values.tolerations }} | |
tolerations: {{ . | toYaml | nindent 4 }} | |
{{- end }} | |
{{- with $.Values.tolerations }} | |
tolerations: {{ . | toYaml | nindent 4 }} | |
{{- end }} |
{{- if $.Values.persistence.enabled }} | ||
volumeClaimTemplates: | ||
{{- with $.Values.persistence.pikaData }} | ||
- name: data # ref componentDefinition.containers.volumeMounts.name | ||
spec: | ||
storageClassName: {{ .storageClassName }} | ||
accessModes: | ||
- ReadWriteOnce | ||
resources: | ||
requests: | ||
storage: {{ .size }} | ||
{{- end }} | ||
{{- end }} |
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.
Correct indentation in the volumeClaimTemplates
section
The volumeClaimTemplates
section at lines 31-43 may have incorrect indentation, which could cause YAML parsing errors. The Helm template directives and YAML keys need proper alignment.
Apply the following diff to correct the indentation:
{{- if $.Values.persistence.enabled }}
- volumeClaimTemplates:
+ volumeClaimTemplates:
{{- with $.Values.persistence.pikaData }}
- - name: data # ref componentDefinition.containers.volumeMounts.name
+ - name: data # ref componentDefinition.containers.volumeMounts.name
spec:
storageClassName: {{ .storageClassName }}
accessModes:
- ReadWriteOnce
resources:
requests:
storage: {{ .size }}
{{- end }}
{{- end }}
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.
{{- if $.Values.persistence.enabled }} | |
volumeClaimTemplates: | |
{{- with $.Values.persistence.pikaData }} | |
- name: data # ref componentDefinition.containers.volumeMounts.name | |
spec: | |
storageClassName: {{ .storageClassName }} | |
accessModes: | |
- ReadWriteOnce | |
resources: | |
requests: | |
storage: {{ .size }} | |
{{- end }} | |
{{- end }} | |
{{- if $.Values.persistence.enabled }} | |
volumeClaimTemplates: | |
{{- with $.Values.persistence.pikaData }} | |
- name: data # ref componentDefinition.containers.volumeMounts.name | |
spec: | |
storageClassName: {{ .storageClassName }} | |
accessModes: | |
- ReadWriteOnce | |
resources: | |
requests: | |
storage: {{ .size }} | |
{{- end }} | |
{{- end }} |
- name: pika | ||
componentDef: pika # Ref componentdefinition.name | ||
enabledLogs: {{ $.Values.enabledLogs | toJson | indent 4 }} | ||
replicas: {{ add (int $.Values.slaveCount) 1 | default 2 }} |
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.
Ensure default value for replicas
works as intended
At line 20, the expression for replicas
may not provide the correct default when $.Values.slaveCount
is undefined. The current default 2
is applied after the addition, which might not be the intended logic.
Consider updating the expression to apply the default to $.Values.slaveCount
before the addition:
- replicas: {{ add (int $.Values.slaveCount) 1 | default 2 }}
+ replicas: {{ add (int (default 1 $.Values.slaveCount)) 1 }}
This ensures that if slaveCount
is not set, it defaults to 1
, resulting in replicas: 2
.
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.
replicas: {{ add (int $.Values.slaveCount) 1 | default 2 }} | |
replicas: {{ add (int (default 1 $.Values.slaveCount)) 1 }} |
#######################################################################E####### | ||
|
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.
Typographical Error in Comment Divider
Similarly, the comment divider on line 401 contains an extra 'E'. Consistency in comment formatting enhances readability.
Apply this diff to correct the issue:
-#######################################################################E#######
+###############################################################################
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.
#######################################################################E####### | |
############################################################################### | |
# connects to a master to request replication. | ||
# [NOTICE] The value of this parameter must match the "requirepass" setting on the master. | ||
masterauth : |
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.
Use Inclusive Terminology in Comments
The use of "slave" and "master" terminology is considered outdated and non-inclusive. It's recommended to replace them with "replica" and "primary" respectively.
Apply this diff to update the terminology:
-# Password for replication verify, used for authentication when a slave
-# connects to a master to request replication.
-# [NOTICE] The value of this parameter must match the "requirepass" setting on the master.
+# Password for replication verification, used for authentication when a replica
+# connects to a primary to request replication.
+# [NOTICE] The value of this parameter must match the "requirepass" setting on the primary.
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.
# connects to a master to request replication. | |
# [NOTICE] The value of this parameter must match the "requirepass" setting on the master. | |
masterauth : | |
# Password for replication verification, used for authentication when a replica | |
# connects to a primary to request replication. | |
# [NOTICE] The value of this parameter must match the "requirepass" setting on the primary. | |
masterauth : |
#######################################################################E####### | ||
|
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.
Typographical Error in Comment Divider
The comment divider on line 417 also has an extra 'E'. Correcting this ensures uniformity across the file.
Apply this diff:
-#######################################################################E#######
+###############################################################################
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.
#######################################################################E####### | |
############################################################################### | |
# Master's run-id | ||
# master-run-id : | ||
|
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.
Update Parameter Names for Inclusive Language
In line with modern terminology, consider renaming parameters to use "primary" and "replica".
Apply this diff:
-# Master's run-id
-# master-run-id :
+# Primary's run-id
+# primary-run-id :
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.
# Master's run-id | |
# master-run-id : | |
# Primary's run-id | |
# primary-run-id : | |
#slaveof : master-ip:master-port | ||
|
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.
Update Configuration Parameter for Inclusive Language
The slaveof
parameter can be renamed to replicaof
to reflect inclusive terminology.
Apply this diff:
-#slaveof : master-ip:master-port
+#replicaof : primary-ip:primary-port
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.
#slaveof : master-ip:master-port | |
#replicaof : primary-ip:primary-port | |
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.
空文件就不要留了
- "/pika/bin/pika" | ||
args: | ||
- "-c" | ||
- "/etc/pika/pika.conf" |
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.
复制了 pika.conf ,但没有使用
templateRef: pika-master-slave-conf-template | ||
namespace: {{ .Release.Namespace }} | ||
volumeName: config | ||
scripts: |
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.
不使用 admin.sh 等 script,相关的未使用的配置清理掉比较好
mountPath: /etc/pika | ||
- name: data | ||
mountPath: /data | ||
- name: script |
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.
same issue
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.
same issue
@@ -0,0 +1,43 @@ | |||
apiVersion: apps.kubeblocks.io/v1alpha1 |
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.
api version 应该有 beta1 了,可以用新的
|
||
version: 0.9.0 | ||
|
||
appVersion: "3.5.3" |
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.
用3.5.5吧
@@ -0,0 +1,20 @@ | |||
pika: | |||
version: v3.5.3 |
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.
用 3.5.5
pika: | ||
registry: docker.io | ||
repository: pikadb/pika | ||
tag: 3.5.3 |
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.
用3.5.5
# If the pod number is greater than 0 | ||
if (( pod_number > 0 )); then | ||
pod0_ip=${pod_ip_array[0]} | ||
echo "exec:redis-cli -h ${pod_ip} -p 9221 slaveof ${pod0_ip}:9221" |
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.
slaveof 的时候不要用ip,用 hostname,k8s 中 ip 是易失资源。
#2884
Support pika-master-slave mode in kb
In this mode, KB will evokes a job, mounts the redis image, traverses the environment variables
KB_CLUSTER_COMPONENT_POD_NAME_LIST
andKB_CLUSTER_COMPONENT_POD_IP_LIST
to retrieve all of the pod_ip andpod_name of the pika-master-slave component, determine the master,slave pod based on the pod_name, and then execute
the redis-cli slaveof command to bind
Summary by CodeRabbit
New Features
Documentation
Chores
.helmignore
files to streamline Helm package builds by excluding unnecessary files.