feat: Supports upper-layer modification of the InstanceSet's UpdateStrategy #7939
Conversation
YTGhost
requested review from
nayutah,
ldming,
heng4fun,
free6om,
wangyelei,
Y-Rookie and
weicao
as code owners
YTGhost
force-pushed
the
feat-updateStrategy
branch
from
e4d3405
to
4cddc13
Compare
apecloud-bot
added
pre-approve
YTGhost
force-pushed
the
feat-updateStrategy
branch
from
4cddc13
to
7821276
Compare
apecloud-bot
added
pre-approve
| // Partition are updated. All pods from ordinal Partition-1 to 0 remain untouched. | ||
| // This is helpful in being able to do a canary based deployment. The default value is 0. | ||
| // +optional | ||
| Partition *int32 `json:"partition,omitempty"` |
There was a problem hiding this comment.
Do we need partition? @free6om
After introducing instance templates, the naming of Pods managed by an InstanceSet follows this pattern: "comp-0, comp-1, comp-tpl0-0, comp-tpl1-0, comp-tpl1-1"
Unlike before, the Pods no longer have a linear ordinal numbering scheme. This makes specifying a partition much more challenging.
There was a problem hiding this comment.
Yes, I feel the same way. Currently, the partition feature allows Pods to be updated in a rolling update based on dictionary order from largest to smallest, but it seems there is no way to perform a rolling update on a specific Template at the moment.
There was a problem hiding this comment.
How we do a RollingUpdate then?
| // That means if there is any unavailable pod in the range 0 to Replicas-1, | ||
| // it will be counted towards MaxUnavailable. | ||
| // +optional | ||
| MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` |
There was a problem hiding this comment.
The term 'maxUnavailable' is misleading. In the original StatefulSet, updates always involved restarting pods, and any update to a pod would cause it to become unavailable. Therefore, controlling the number of maxUnavailable pods represented the level of concurrency.
However, in InstanceSet, if we continue to use 'maxUnavailable', strictly speaking, all non-restarting updates (i.e., those that don't cause the pod to become unavailable) would not be controlled by this parameter.
There was a problem hiding this comment.
Why should we control the non-restarting updates as they are taking effect instantly after the Pods being updated? Do all the non-restarting updates in one reconciliation loop seems no difference with doing them in several reconciliation loops.
There was a problem hiding this comment.
Because 'non-restarting update' is also a change in production environment, it should be able to be controlled by a gradual upgrade strategy. For example, it's reasonable to allow users to make changes to a small number of replicas first, and after verification, gradually roll out to more replicas.
e.g. A 'non-restarting update' might involve parameter modifications, which could potentially lead to issues such as performance degradation. Or, it might be an IP whitelist change, which could inadvertently block legitimate traffic from apps.
There was a problem hiding this comment.
To summarize, incorrect 'non-restarting updates' can potentially harm business continuity. Therefore, users may require a gradual update process. This is something we need to consider.
There was a problem hiding this comment.
@weicao I think, for the 'non-restarting update,' it seems that we don't have a way to automate the control. How about we use partition to manually control it instead?
There was a problem hiding this comment.
@weicao I think, for the 'non-restarting update,' it seems that we don't have a way to automate the control. How about we use
partitionto manually control it instead?
When you have multiple instance templates, how do you plan to use partitions?
There was a problem hiding this comment.
What I mean is, say if you have 3 templates and a total of 10 replicas, when you set the partition to 7 (upgrading the 8th, 9th, and 10th replicas), it becomes difficult to determine which templates these 8th, 9th, and 10th replicas belong to. This makes it challenging to verify whether the updated replicas meet the expected outcomes.
There was a problem hiding this comment.
What I mean is, say if you have 3 templates and a total of 10 replicas, when you set the partition to 7 (upgrading the 8th, 9th, and 10th replicas), it becomes difficult to determine which templates these 8th, 9th, and 10th replicas belong to. This makes it challenging to verify whether the updated replicas meet the expected outcomes.
@weicao We have also considered this issue. However, based on our current requirements, we do not need to specify a separate partition for gray upgrades in multiple templates. Nevertheless, I understand the need for multi-template partitioning, and we can discuss and design a solution for this.
There was a problem hiding this comment.
Currently, our requirement is only for the partition to be able to perform a global update in alphabetical order.
| // | ||
| // +kubebuilder:validation:Enum={Serial,BestEffortParallel,Parallel} | ||
| // +optional | ||
| MemberUpdateStrategy *MemberUpdateStrategy `json:"memberUpdateStrategy,omitempty"` |
There was a problem hiding this comment.
'memberUpdateStrategy' and 'maxUnavailable' are not orthogonal.
- When 'memberUpdateStrategy' is set to 'serial', 'maxUnavailable' has no effect, right?
- When 'memberUpdateStrategy' is set to 'bestEffortParallel', the concurrency is calculated based on quorum, so 'maxUnavailable' should not have an effect either, right?
- Therefore, does 'maxUnavailable' only take effect when 'memberUpdateStrategy' is set to 'parallel'?
There was a problem hiding this comment.
Yes, but MaxUnavailable means no more than a total number of Pods defined by MaxAvailable should be unavailable. It's upper bound.
There was a problem hiding this comment.
so 'maxUnavailable' takes effect when 'memberUpdateStrategy' is set to 'bestEffortParallel' and 'parallel'?
YTGhost
force-pushed
the
feat-updateStrategy
branch
from
7821276
to
c33642b
Compare
apecloud-bot
added
pre-approve
YTGhost
force-pushed
the
feat-updateStrategy
branch
from
c33642b
to
f84bd1e
Compare
apecloud-bot
added
pre-approve
YTGhost
force-pushed
the
feat-updateStrategy
branch
from
f84bd1e
to
06e5a75
Compare
YTGhost
force-pushed
the
feat-updateStrategy
branch
from
06e5a75
to
044523d
Compare
apecloud-bot
added
pre-approve
…rategy Signed-off-by: Liang Deng <[email protected]>
YTGhost
force-pushed
the
feat-updateStrategy
branch
from
044523d
to
e1de8d6
Compare
apecloud-bot
added
pre-approve
resolve #7913