fix: avoid silent rejection to invalid podManagementPolicy
#4650
Conversation
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
hoyhbx
changed the title
podManagementPolicy
| } | ||
| klog.Warningf("PodManagementPolicy \"%s\" is invalid. Fall back to use \"Parallel\" instead\n", policy) |
There was a problem hiding this comment.
Usually, we do not add log in this file, as you can see, no klog imported in this file, each function will be called repeatedly during each sync which will flood too many logs, what about adding descriptions in types.go to describe this behavior clearly.
There was a problem hiding this comment.
@DanielZhangQD Thank you for your reply. Yes, I think it makes sense not to put the log in this file. But I suppose we can add an enum for kubebuilder in types.go to prevent invalid input. I updated the code in the latest commit.
| @@ -118,6 +118,8 @@ func (a *componentAccessorImpl) PodManagementPolicy() apps.PodManagementPolicyTy | |||
| // unified podManagementPolicy check to avoid check everywhere | |||
| if policy == apps.OrderedReadyPodManagement { | |||
| return apps.OrderedReadyPodManagement | |||
| } else if policy == apps.ParallelPodManagement { | |||
There was a problem hiding this comment.
This is not necessary, right？
There was a problem hiding this comment.
Yes, if we use enum then this is not necessary. I have removed it in the latest commit.
|
@hoyhbx Please rebase this PR, thanks! |
hoyhbx
force-pushed
the
FixPodManagementPolicy
branch
from
b5ca120 to
5b41895
Compare
hoyhbx
force-pushed
the
FixPodManagementPolicy
branch
from
5b41895 to
5c7e1a4
Compare
…tor into FixPodManagementPolicy
Codecov Report
@@ Coverage Diff @@
## master #4650 +/- ##
=======================================
Coverage 62.64% 62.65%
=======================================
Files 186 186
Lines 20853 20853
=======================================
+ Hits 13064 13066 +2
+ Misses 6592 6591 -1
+ Partials 1197 1196 -1
|
@DanielZhangQD It's done now. |
| @@ -324,6 +324,7 @@ type TidbClusterSpec struct { | |||
| StatefulSetUpdateStrategy apps.StatefulSetUpdateStrategyType `json:"statefulSetUpdateStrategy,omitempty"` | |||
|
|
|||
| // PodManagementPolicy of TiDB cluster StatefulSets | |||
| // +kubebuilder:validation:Enum=OrderedReady;Parallel | |||
There was a problem hiding this comment.
Does this work if this field is not set explicitly?
There was a problem hiding this comment.
@DanielZhangQD Do you mean we need a default value for this field? If so, what should be the default value?
If we want a default value when the field is not set explicitly, we can also specify the default value through this kubebuilder tag
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What problem does this PR solve?
This PR solves the problem mentioned in #4649. Basically, we believe an invalid
podManagementPolicyshould not be silently replaced by some default values without explicitly telling the user that the input is invalid.What is changed and how does it work?
We added a warning to explicitly tell the user that
podManagementis invalid and the operator will useParallelinstead.Code changes
Tests
Side effects
Related changes
Release Notes
Please refer to Release Notes Language Style Guide before writing the release note.