-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add suport for specifying percentage of pods for RollingUpdate #36
Conversation
@@ -103,7 +104,7 @@ type TAppUpdateStrategy struct { | |||
// Template is the rolling update template name | |||
Template string `json:"template,omitempty"` | |||
// MaxUnavailable is the max unavailable number when tapp is rolling update, default is 1. | |||
MaxUnavailable *int32 `json:"maxUnavailable,omitempty"` | |||
MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` |
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.
能兼容处理已经创建的tapp对象吗?
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.
测试已创建的tapp更新没问题
pkg/tapp/controller.go
Outdated
@@ -823,7 +824,11 @@ func (c *Controller) transformPodActions(tapp *tappv1.TApp, podActions map[strin | |||
|
|||
maxUnavailable := tappv1.DefaultMaxUnavailable | |||
if tapp.Spec.UpdateStrategy.MaxUnavailable != nil { | |||
maxUnavailable = int(*tapp.Spec.UpdateStrategy.MaxUnavailable) | |||
var err error | |||
maxUnavailable, err = intstr.GetValueFromIntOrPercent(tapp.Spec.UpdateStrategy.MaxUnavailable, len(rollingUpdateIds), 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.
这里len(rollingUpdateIds)
改成desiredRunningPods.Len()
会不会更合理
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.
百分比应该是指定要滚动更新的template的pod的,也就是rollingupdate的。换成desiredRunningpod会变成所有pod的
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.
结合现有的测试样例看了一下,其他template的更新会影响到滚动更新的template的数量,所以还是换成了desiredRunningpod。
var err error | ||
maxUnavailable, err = intstr.GetValueFromIntOrPercent(tapp.Spec.UpdateStrategy.MaxUnavailable, len(rollingUpdateIds), true) | ||
if err != nil { | ||
klog.Errorf("invalid value for MaxUnavailable: %v", err) |
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.
如果出错了,return err直接返回?
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.
add return
b5535b9
to
74843d1
Compare
@qianjun1993 没问题了,加一个测试用例? |
追加了测试用例 |
LGTM |
Add suport for specifying percentage of pods for RollingUpdate