apecloud
[Improvement] Increasing the allowed range of Maxreplicas and setting both fields in replicasLimit to optional
Is your improvement request related to a problem? Please describe.
- the allowed range of Maxreplicas values is too small. The maximum replicasLimit in componentdefinition can be set to 128, which doesn't meet my needs.
- Setting both fields in replicasLimit to optional which are required in v1.0.0-alpha1 could give users a better experience
If this is a new function, please describe the motivation and goals.
（A clear and concise description of why you want to happen, link the design doc if possible）
like the below struct
// ReplicasLimit defines the valid range of number of replicas supported.
//
// +kubebuilder:validation:XValidation:rule="self.minReplicas >= 0 && self.maxReplicas <= 2147483647",message="the minimum and maximum limit of replicas should be in the range of [0, math.MaxInt32]"
// +kubebuilder:validation:XValidation:rule="self.minReplicas <= self.maxReplicas",message="the minimum replicas limit should be no greater than the maximum"
type ReplicasLimit struct {
// The minimum limit of replicas.
//
// +kubebuilder:default=1
// +optional
MinReplicas int32 `json:"minReplicas,omitempty"`
// The maximum limit of replicas.
//
// +kubebuilder:default=2147483647
// +optional
MaxReplicas int32 `json:"maxReplicas,omitempty"`
}
the fields in ReplicasLimit should not be required. If the user does not fill the maxReplicas or minReplicas, they should be the default value.
@ian-hui what's the requirements to change two fields as optional?
BTW, the upper limit should be set to a reasonable value, which means:
- it describes a common and realistic system
- it's within the design capacity of the KB supported
@leon-inf The consideration of this design mainly comes from the following points:
-
Setting them as optional is mainly to take into account that users may only want to modify one of the max or min ReplicasLimit and keep the other at the default value. For example, during use, I only want to change the minReplicasLimit to 0, and I do not consider modifying the maxReplicasLimit.
-
The upper limit mainly because I see there is a verification about replica and limit. It mentions that if replicasLimit is not filled in, the default value is [1, MaxInt32]
What I want to do is to use the default value if replicaLimit is not filled in. This default value should have the same semantics as here
In the design of replica limits API, if the developer wants to specify the limit, then he MUST explicitly specify both the min and max limits, so that he knows exactly what he is doing.
It can be considered that the replica limits and KB default (unlimited) replicas are two functions or options that are independent and mutually exclusive.
I still think
- For example, in this case, we only want to modify the minReplicasLimit 0, and we don’t actually want to consider the maximum replicas value.
- The default maxReplicaLimit value here needs a reference, so it is difficult to define whether it is 128 or 1000. So I still think because there is a defaultLimit, then I can set this value according to this defaultLimit.
I still think
- For example, in this case, we only want to modify the shrink to 0, and we don’t actually want to consider how to set the maximum value
- The default maxReplicaLimit value here needs a reference, so it is difficult to define whether it is 128 or 1000. So I still think because there is a defaultLimit, then I can set this value according to this defaultLimit.
That's because the default limits are implicit by design, its value will be adjusted within a reasonable range as requirements and system functions change, and it is not provided for external APIs to reference.
Setting maxReplicas to 2147483647 (math.MaxInt32) is not a reasonable choice, neither in the ReplicaLimits API nor as the default value in the KubeBlocks controller code. It is recommended to modify the maximum value in both places to a more sensible number, such as 16,384.
Setting maxReplicas to 2147483647 (math.MaxInt32) is not a reasonable choice, neither in the ReplicaLimits API nor as the default value in the KubeBlocks controller code. It is recommended to modify the maximum value in both places to a more sensible number, such as 16,384.
@weicao I agree with your opinion. How about we just set the max replicas limit at 16,384 within both the API and the controller. And I will proceed to update and resubmit the code accordingly.
Setting maxReplicas to 2147483647 (math.MaxInt32) is not a reasonable choice, neither in the ReplicaLimits API nor as the default value in the KubeBlocks controller code. It is recommended to modify the maximum value in both places to a more sensible number, such as 16,384.
@weicao I agree with your opinion. How about we just set the max replicas limit at 16,384 within both the API and the controller. And I will proceed to update and resubmit the code accordingly.
All right.
By the way, I suggest keeping both minReplicas and maxReplicas as required fields. When an addon developer needs to change one of them, it's better for them to explicitly specify the other threshold as well.
This way, an addon would have only two scenarios: either use the default pair of thresholds from the controller code for a "normal" number of replicas, or have specific replica count requirements and explicitly provide its own pair of thresholds.
For example, at Kuaishou, there might be clusters with 10,000 replicas. If a certain version of KubeBlocks were to change the default threshold to 5,000, it could produce unexpected side effects. Explicitly setting the upper limit in the addon would mitigate this risk.
Setting maxReplicas to 2147483647 (math.MaxInt32) is not a reasonable choice, neither in the ReplicaLimits API nor as the default value in the KubeBlocks controller code. It is recommended to modify the maximum value in both places to a more sensible number, such as 16,384.
@weicao I agree with your opinion. How about we just set the max replicas limit at 16,384 within both the API and the controller. And I will proceed to update and resubmit the code accordingly.
All right.
By the way, I suggest keeping both minReplicas and maxReplicas as required fields. When an addon developer needs to change one of them, it's better for them to explicitly specify the other threshold as well.
This way, an addon would have only two scenarios: either use the default pair of thresholds from the controller code for a "normal" number of replicas, or have specific replica count requirements and explicitly provide its own pair of thresholds.
For example, at Kuaishou, there might be clusters with 10,000 replicas. If a certain version of KubeBlocks were to change the default threshold to 5,000, it could produce unexpected side effects. Explicitly setting the upper limit in the addon would mitigate this risk.
I agree with your suggestion to require both minReplicas and maxReplicas as mandatory fields. Indeed, I was initially considering informing users of maximum and minimum values through annotations, but this might lead to issues you described: users may overlook the annotations, resulting in unclear replicasLimit thresholds, and potential problems when KubeBlocks needs to change default values.
All right, I'll go ahead and adjust the code right now and then resubmit the changes.
@ian-hui I think this issue can be closed because https://github.com/apecloud/kubeblocks/pull/7953 has already been merged.
