fix: Cannot set default values in greptimedb cluster #173
Conversation
WalkthroughThe updates involve version upgrades for tools and dependencies, changes to type annotations for port variables, renaming fields in structs for clarity, and modifications to function signatures to enhance specificity. Additionally, enhancements to the Custom Resource Definition (CRD) structure and minor adjustments to build constraints are included. Overall, these changes aim to improve clarity, maintainability, and compatibility within the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant CRD
User->>Controller: Create or update resource
Controller->>CRD: Validate and set defaults
alt New Resource
Controller->>CRD: Initialize default values
end
Controller->>CRD: Update status
CRD-->>Controller: Acknowledge update
Controller-->>User: Confirm resource status
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (12)
- Makefile (1 hunks)
- apis/v1alpha1/defaulting.go (9 hunks)
- apis/v1alpha1/greptimedbstandalone_types.go (1 hunks)
- apis/v1alpha1/zz_generated.deepcopy.go (1 hunks)
- cmd/operator/app/command.go (2 hunks)
- config/crd/resources/greptime.io_greptimedbstandalones.yaml (51 hunks)
- config/rbac/role.yaml (1 hunks)
- controllers/greptimedbcluster/controller.go (1 hunks)
- controllers/greptimedbcluster/deployers/common.go (1 hunks)
- controllers/greptimedbstandalone/controller.go (1 hunks)
- controllers/greptimedbstandalone/deployer.go (3 hunks)
- go.mod (1 hunks)
Files skipped from review due to trivial changes (4)
- apis/v1alpha1/greptimedbstandalone_types.go
- apis/v1alpha1/zz_generated.deepcopy.go
- config/rbac/role.yaml
- go.mod
Additional comments not posted (11)
cmd/operator/app/command.go (1)
73-75: Refactor to use structured metrics configuration.The change from a simple string for
MetricsBindAddressto usingmetricsserver.OptionsforMetricsenhances configuration flexibility and maintainability. This structured approach allows for future extensions in metrics configuration.The code changes are approved.
controllers/greptimedbcluster/deployers/common.go (1)
109-109: Refinement in function parameter type forUpdateStatus.Changing the parameter type from
client.UpdateOptiontoclient.SubResourceUpdateOptionin theUpdateStatusfunction makes the update process more specific to sub-resources, which likely enhances the control over how updates are handled.The code changes are approved.
Makefile (1)
190-191: Update to tool versions in the Makefile.Updating
KUSTOMIZE_VERSIONtov5.3.0andCONTROLLER_TOOLS_VERSIONtov0.14.0ensures that the project uses the latest versions of these tools, which can include important features, bug fixes, or performance improvements.The version updates are approved.
apis/v1alpha1/defaulting.go (2)
33-37: Type Annotation Changes ApprovedThe explicit type annotations for port variables (
int32) enhance type safety and clarity, aligning with best practices for specifying data types in Go. This change ensures that port-related values are consistently treated across the codebase, reducing the risk of type mismatches.
87-87: Correct Usage of Typed Ports in Function CallsThe replacement of
intstr.FromIntwithintstr.FromInt32across various function calls is a necessary adjustment following the type annotation changes. This ensures that the correct type (int32) is passed to functions expecting integer values, which is crucial for maintaining type consistency and avoiding runtime errors.Also applies to: 93-97, 134-134, 141-144, 178-179, 200-201, 213-213, 251-251, 265-265, 279-279
controllers/greptimedbstandalone/deployer.go (1)
360-360: Field Renaming Consistency ApprovedRenaming the field from
HTTPServicePorttoHTTPPortenhances clarity and uniformity across the codebase. The changes are correctly reflected in the methods that define and utilize the HTTP port, ensuring that the functionality remains intact while improving the readability and maintainability of the code.Also applies to: 385-385, 406-406
controllers/greptimedbstandalone/controller.go (1)
357-357: Function Signature Update ApprovedChanging the parameter type in the
UpdateStatusfunction fromclient.UpdateOptiontoclient.SubResourceUpdateOptionis a targeted refinement that likely enhances the specificity of the update process, focusing on sub-resource updates. This change is appropriate given the context of updating the status of resources, which often involves sub-resource modifications.controllers/greptimedbcluster/controller.go (1)
141-144: Unconditional default setting enhances robustness.The change to call
SetDefaults()unconditionally is a positive improvement, ensuring that default values are always set regardless of the cluster's phase. This approach enhances the robustness of the reconciliation process and prevents issues related to uninitialized values.The implementation and error handling are correctly done.
config/crd/resources/greptime.io_greptimedbstandalones.yaml (3)
6-6: Version bump in controller-gen should be verified for compatibility.The update to
controller-gen.kubebuilder.io/version: v0.14.0is noted. While this is likely to bring enhancements, it's important to verify that this version is compatible with other tools and libraries used in the project.Please ensure compatibility with the project's dependencies.
433-445: Addition ofresizePolicyandrestartPolicyenhances resource configurability.The new fields
resizePolicyandrestartPolicyallow for detailed specifications of resource resizing and restart behaviors, enhancing the CRD's functionality. The use ofx-kubernetes-list-type: atomicfor these fields ensures that updates are treated as atomic operations, which is appropriate for these types of configurations.The implementation of these fields is correctly done and follows Kubernetes best practices.
448-459: Structured handling of resource claims through theclaimsfield.The addition of the
claimsfield underresourcesallows for a more structured and manageable approach to resource claims. The use ofx-kubernetes-list-type: mapwithx-kubernetes-list-map-keys: nameensures that updates to the claims are handled correctly, with each claim uniquely identified by its name.This addition is well-implemented and enhances the CRD's capabilities in managing resources.
|
@daviderli614 It's not a best practice to fix a bug and upgrade the dependencies simultaneously, which is not about the bug itself. Can you please make the PR into two standalone PRs:
|
| @@ -187,8 +187,8 @@ ENVTEST ?= $(LOCALBIN)/setup-envtest | |||
| GOLANGCI_LINT ?= $(LOCALBIN)/golangci-lint | |||
|
|
|||
| ## Tool Versions | |||
| KUSTOMIZE_VERSION ?= v4.5.3 | |||
| CONTROLLER_TOOLS_VERSION ?= v0.9.0 | |||
| KUSTOMIZE_VERSION ?= v5.3.0 | |||
There was a problem hiding this comment.
Please don't bump the unrelated dependencies at the same time.
| defaultMySQLPort = 4002 | ||
| defaultPostgreSQLPort = 4003 | ||
| defaultMetaRPCPort = 3002 | ||
| defaultHTTPPort int32 = 4000 |
There was a problem hiding this comment.
If we don't specify the type, the default type will be int.
There was a problem hiding this comment.
I think it's an unrelated modification of the bug.
| @@ -32,7 +32,7 @@ type GreptimeDBStandaloneSpec struct { | |||
| TLS *TLSSpec `json:"tls,omitempty"` | |||
|
|
|||
| // +optional | |||
| HTTPServicePort int32 `json:"httpPort,omitempty"` | |||
| HTTPPort int32 `json:"httpPort,omitempty"` | |||
There was a problem hiding this comment.
The schema is already httpPort, but the variable name is still HTTPServicePort. It's not a bug that affects functionality.
| HealthProbeBindAddress: o.HealthProbeAddr, | ||
| LeaderElection: o.EnableLeaderElection, | ||
| LeaderElectionID: leaderElectionID, | ||
| Metrics: metricsserver.Options{ |
There was a problem hiding this comment.
I think it's an unrelated modification of the bug.
| // Means the cluster is just created. | ||
| if len(cluster.Status.ClusterPhase) == 0 { | ||
| klog.Infof("Start to create the cluster '%s/%s'", cluster.Namespace, cluster.Name) | ||
|
|
There was a problem hiding this comment.
Can you describe the bug and how to reproduce it in the PR? You can also create an issue. Reverting the original logic without any description is not a good practice.
|
I will split this PR. |
if len(cluster.Status.ClusterPhase) == 0:Summary by CodeRabbit
greptimedbstandalones, adding new properties such asresizePolicy,restartPolicy, andclaimsfor improved resource management.