feat: refactor cmpd's role definition #8416
Conversation
cjc7373
force-pushed
the
feature/refactor-role
branch
from
b18c91c
to
deaf5f1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8416 +/- ##
==========================================
- Coverage 61.22% 61.15% -0.07%
==========================================
Files 351 351
Lines 41854 41828 -26
==========================================
- Hits 25624 25581 -43
- Misses 13906 13921 +15
- Partials 2324 2326 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cjc7373
requested review from
free6om,
a team,
sophon-zt,
wangyelei,
leon-inf and
Y-Rookie
as code owners
cjc7373
force-pushed
the
feature/refactor-role
branch
from
752aefa
to
90b8f22
Compare
| // | ||
| // +kubebuilder:default=0 | ||
| // +optional | ||
| UpdatePriority int `json:"updatePriority"` |
| // | ||
| // +kubebuilder:default=false | ||
| // +optional | ||
| SwitchoverBeforeUpdate bool `json:"switchoverBeforeUpdate"` |
There was a problem hiding this comment.
maybe SwitchoverBeforeMaintenance or NeedSwitchover
There was a problem hiding this comment.
How about remove this option and decide within the switchover action whether a switchover is necessary based on the role of target pod?
There was a problem hiding this comment.
What you mean is to use lifecycleaction's TargetPodSelector and MatchingKey to select the role to perform switchover?
There was a problem hiding this comment.
If so, then this part of code can be removed. And there's only one place that needs role definition: instanceset controller uses roles to determine pod update sequence.
| // | ||
| // +kubebuilder:default=false | ||
| // +optional | ||
| ParticipatesInQuorum bool `json:"participatesInQuorum"` |
cjc7373
force-pushed
the
feature/refactor-role
branch
from
98bfd03
to
b0e9c70
Compare
cjc7373
force-pushed
the
feature/refactor-role
branch
from
ea9ddbd
to
12ffec8
Compare
| @@ -232,7 +232,16 @@ func (r *RestoreManager) DoPostReady(comp *component.SynthesizedComponent, | |||
| backupObj *dpv1alpha1.Backup) error { | |||
| jobActionLabels := constant.GetCompLabels(r.Cluster.Name, comp.Name) | |||
| if len(comp.Roles) > 0 { | |||
| jobActionLabels[instanceset.AccessModeLabelKey] = string(appsv1alpha1.ReadWrite) | |||
| // HACK: assume the role with highest priority to be writable | |||
There was a problem hiding this comment.
It's better to let the data-protection define a policy to select target pods explicitly.
There was a problem hiding this comment.
Maybe we can add a new field in actionset to indicate how to select pod in restore's postready step. But @wangyelei suggests for now we can just hack it.
|
|
||
| // getTargetRole returns the role on which the switchover is performed | ||
| // FIXME: the assumption that only one role supports switchover may change in the future | ||
| func getTargetRoleName(roles []appsv1.ReplicaRole) (string, error) { |
There was a problem hiding this comment.
We don't need to infer the target pods, just let the user specify the specific roles or pods.
fixes #8377