Refactor daemonset controller and fetch upstream codebase #883
Conversation
|
/cc @jetmuffin @evertrain |
|
@FillZpp: GitHub didn't allow me to request PR reviews from the following users: jetmuffin, evertrain. Note that only openkruise members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
Codecov Report
@@ Coverage Diff @@
## master #883 +/- ##
==========================================
+ Coverage 47.89% 49.17% +1.28%
==========================================
Files 119 119
Lines 11235 11078 -157
==========================================
+ Hits 5381 5448 +67
+ Misses 5037 4790 -247
- Partials 817 840 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
FillZpp
force-pushed
the
optimize-daemonset-controller
branch
from
a5e1137 to
bb16e7f
Compare
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
FillZpp
force-pushed
the
optimize-daemonset-controller
branch
2 times, most recently
from
effbe38 to
9de17f0
Compare
| case "", appsv1alpha1.StandardRollingUpdateType: | ||
| case appsv1alpha1.InplaceRollingUpdateType: | ||
| if hasSurge || !hasUnavailable { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("maxUnavailable"), "cannot be 0 for InPlaceIfPossible")) |
There was a problem hiding this comment.
no need to check hasSurge since the switch clause above had already made the check.
| allErrs = append(allErrs, field.Required(fldPath.Child("maxUnavailable"), "cannot be 0 for InPlaceIfPossible")) | ||
| } | ||
| case appsv1alpha1.DeprecatedSurgingRollingUpdateType: | ||
| if !hasSurge || hasUnavailable { |
There was a problem hiding this comment.
no need to check hasUnavailable since the switch clause above had already made the check.
| } | ||
| } | ||
| } | ||
|
|
||
| return requeueDuration.Get(), nil | ||
| updateSatisfied, unsatisfiedDuration, updateDirtyPods := dsc.updateExpectations.SatisfiedExpectations(dsKey, hash) |
There was a problem hiding this comment.
is it possible to move the following code block out of this function to make this function serve only a single purpose
There was a problem hiding this comment.
If so, the two place may all have to getNodesToDaemonPods(ds), which means the pod list will be called twice.
FillZpp
force-pushed
the
optimize-daemonset-controller
branch
from
9de17f0 to
a250128
Compare
Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
FillZpp
force-pushed
the
optimize-daemonset-controller
branch
from
a250128 to
80d2f86
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FillZpp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/remove-approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: furykerry The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: FillZpp FillZpp.pub@gmail.com
Ⅰ. Describe what this PR does
Refactor daemonset controller and fetch upstream release-1.22 codebase.
Ⅱ. Does this pull request fix one issue?
fixes #873
Ⅲ. Special notes for reviews
Main changes:
Surgingtype is deprecated, just use theStandardinstead. Now you can choosemaxUnavailableormaxSurgeinStandardtype, but only the former one can be used withInPlaceIfPossibletype.inPlaceUpdatePodsbefore delete the pods to update.ignore-not-readyorignore-unscheduableany more. I don't think they are necessary. But if you need them, just leave your comments below.