Feature:support configuring PodManagementPolicy in TidbCluster CR #4211

mianhk · 2021-09-30T09:48:28Z

What problem does this PR solve?

Close: #3228

What is changed and how does it work?

Code changes

Has Go code change
Has CI related scripts change

Tests

Unit test
E2E test
Manual test
No code

Side effects

Breaking backward compatibility
Other side effects:

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release Notes

Please refer to Release Notes Language Style Guide before writing the release note.

Feature: support configuring PodManagementPolicy in TidbCluster CR

the TidbCluster

ti-chi-bot · 2021-09-30T09:48:29Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

csuzhangxc
july2993

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

codecov-commenter · 2021-09-30T09:58:58Z

Codecov Report

Merging #4211 (059a158) into master (26b6a1e) will increase coverage by 0.00%.
The diff coverage is 77.77%.

@@           Coverage Diff           @@
##           master    #4211   +/-   ##
=======================================
  Coverage   61.78%   61.79%           
=======================================
  Files         181      181           
  Lines       19273    19277    +4     
=======================================
+ Hits        11908    11912    +4     
  Misses       6217     6217           
  Partials     1148     1148

Flag	Coverage Δ
unittest	`61.79% <77.77%> (+<0.01%)`	⬆️

mianhk · 2021-09-30T09:52:11Z

pkg/apis/pingcap/v1alpha1/tidbcluster_component.go

+		policy = a.ComponentSpec.PodManagementPolicy
+	}
+	// unified podManagementPolicy check to avoid check everywhere
+	if len(policy) == 0 || policy == apps.OrderedReadyPodManagement {


I add the check of PodManagementPolicyType here to avoid check it everywhere, is this OK?

I think it's OK

mianhk · 2021-09-30T09:57:24Z

pkg/manager/member/dm_master_member_manager.go

@@ -696,7 +696,7 @@ func getNewMasterSetForDMCluster(dc *v1alpha1.DMCluster, cm *corev1.ConfigMap) (
 				},
 			},
 			ServiceName:         controller.DMMasterPeerMemberName(dcName),
-			PodManagementPolicy: apps.ParallelPodManagement,
+			PodManagementPolicy: apps.OrderedReadyPodManagement,


Not support configuring DM master&worker cause StatefulSetUpdateStrategy is not supported too.

tidb-operator/pkg/manager/member/dm_master_member_manager.go

Lines 700 to 704 in d4c238c

UpdateStrategy: apps.StatefulSetUpdateStrategy{

Type: apps.RollingUpdateStatefulSetStrategyType,

RollingUpdate: &apps.RollingUpdateStatefulSetStrategy{

Partition: pointer.Int32Ptr(dc.Spec.Master.Replicas + int32(failureReplicas) + deleteSlotsNumber),

}},

I'll add them if need.

I think it's needed, but you can added them later

OK, will add them in another PR.

examples/advanced/tidb-cluster.yaml

…k/tidb-operator into support-PodManagementPolicy-config

pkg/manager/member/pd_member_manager.go

…mentPolicy-config

…k/tidb-operator into support-PodManagementPolicy-config

july2993

rest LGTM

july2993 · 2021-11-02T02:55:09Z

pkg/apis/pingcap/v1alpha1/tidbcluster_component.go

+	// unified podManagementPolicy check to avoid check everywhere
+	if policy == apps.OrderedReadyPodManagement {
+		return apps.OrderedReadyPodManagement
+	}


some unknown value will be treated as ParallelPodManagement silently? at least log for unknown config item?

how about just passing the value to set in sts spec, make it error to let the user know setting wrong item?

Yes, Refer to StatefulSetUpdateStrategy：

tidb-operator/pkg/manager/member/pd_member_manager.go

Lines 759 to 767 in 48ae829

updateStrategy := apps.StatefulSetUpdateStrategy{}

if basePDSpec.StatefulSetUpdateStrategy() == apps.OnDeleteStatefulSetStrategyType {

updateStrategy.Type = apps.OnDeleteStatefulSetStrategyType

} else {

updateStrategy.Type = apps.RollingUpdateStatefulSetStrategyType

updateStrategy.RollingUpdate = &apps.RollingUpdateStatefulSetStrategy{

Partition: pointer.Int32Ptr(tc.PDStsDesiredReplicas() + deleteSlotsNumber),

}

}

pkg/apis/pingcap/v1alpha1/tidbcluster_component.go

july2993 · 2021-11-02T03:46:02Z

examples/advanced/tidb-cluster.yaml

+
+  ## PodManagementPolicy default Parallel.
+  ## Ref: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#pod-management-policies
+  podManagementPolicy: Parallel


Do we need to change the default value as OrderedReady?
@DanielZhangQD

We need to keep the current behavior for the default configuration.

ok, for pump it is not keep the current behavior for the default configuration, it does not set the value before

DanielZhangQD · 2021-11-09T03:14:07Z

/run-all-tests

DanielZhangQD · 2021-11-09T05:38:56Z

/test pull-e2e-kind

DanielZhangQD · 2021-11-09T05:55:12Z

pkg/manager/member/pump_member_manager.go

@@ -470,6 +470,7 @@ func getNewPumpStatefulSet(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap) (*app

 			Template:             podTemplate,
 			VolumeClaimTemplates: volumeClaims,
+			PodManagementPolicy:  spec.PodManagementPolicy(),


The podManagementPolicy is not set before as @july2993 has said in another comment, so the default value is OrderedReady for pump, and this field is not allowed to update after the sts is created, so we have to set to OrderedReady for pump if it's not set explicitly.

pkg/manager/member/pump_member_manager.go

Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>

pkg/manager/member/pump_member_manager.go

DanielZhangQD

LGTM

DanielZhangQD · 2021-11-09T09:12:14Z

/run-all-tests

DanielZhangQD · 2021-11-09T09:12:49Z

/merge

ti-chi-bot · 2021-11-09T09:12:52Z

This pull request has been accepted and is ready to merge.

Commit hash: eb41a1b

DanielZhangQD · 2021-11-09T10:17:05Z

/run-all-tests

DanielZhangQD · 2021-11-09T13:09:13Z

/test pull-e2e-kind

DanielZhangQD · 2021-11-10T03:29:05Z

/test pull-e2e-kind

feature:support configuring PodManagementPolicy in

1496938

the TidbCluster

ti-chi-bot requested review from csuzhangxc and DanielZhangQD September 30, 2021 09:48

mianhk changed the title ~~[WIP]Feature:support configuring PodManagementPolicy in TidbCluster CR~~ Feature:support configuring PodManagementPolicy in TidbCluster CR Oct 9, 2021

Merge branch 'master' into support-PodManagementPolicy-config

bcd18ce

mianhk commented Oct 9, 2021

View reviewed changes

csuzhangxc reviewed Oct 9, 2021

View reviewed changes

examples/advanced/tidb-cluster.yaml Outdated Show resolved Hide resolved

mianhk added 2 commits October 9, 2021 17:24

fix: set default PodManagementPolicy to Parallel

f5e934c

Merge branch 'support-PodManagementPolicy-config' of github.com:mianh…

ab044dd

…k/tidb-operator into support-PodManagementPolicy-config

csuzhangxc approved these changes Oct 9, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 label Oct 9, 2021

Merge branch 'master' into support-PodManagementPolicy-config

f2b44f6

DanielZhangQD reviewed Oct 14, 2021

View reviewed changes

pkg/manager/member/pd_member_manager.go Show resolved Hide resolved

mianhk added 3 commits October 25, 2021 22:47

Merge remote-tracking branch 'upstream/master' into support-PodManage…

009ab1f

…mentPolicy-config

update: PodManagementPolicy for pump

7df0757

Merge branch 'support-PodManagementPolicy-config' of github.com:mianh…

5b677b9

…k/tidb-operator into support-PodManagementPolicy-config

csuzhangxc approved these changes Oct 26, 2021

View reviewed changes

july2993 reviewed Nov 2, 2021

View reviewed changes

optimize code

b4fbbaa

july2993 approved these changes Nov 3, 2021

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Nov 3, 2021

Merge branch 'master' into support-PodManagementPolicy-config

82244b5

DanielZhangQD reviewed Nov 9, 2021

View reviewed changes

DanielZhangQD and others added 2 commits November 9, 2021 13:59

Update tidb-cluster.yaml

e345dac

fix: Compatible pump default podManagementPolicy

fb767dc

DanielZhangQD reviewed Nov 9, 2021

View reviewed changes

pkg/manager/member/pump_member_manager.go Outdated Show resolved Hide resolved

Update pkg/manager/member/pump_member_manager.go

41ff7dd

Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>

DanielZhangQD reviewed Nov 9, 2021

View reviewed changes

pkg/manager/member/pump_member_manager.go Outdated Show resolved Hide resolved

Update pkg/manager/member/pump_member_manager.go

eb41a1b

DanielZhangQD approved these changes Nov 9, 2021

View reviewed changes

ti-chi-bot added the status/can-merge label Nov 9, 2021

Merge branch 'master' into support-PodManagementPolicy-config

059a158

DanielZhangQD merged commit afa5a3d into pingcap:master Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature:support configuring PodManagementPolicy in TidbCluster CR #4211

Feature:support configuring PodManagementPolicy in TidbCluster CR #4211

mianhk commented Sep 30, 2021 •

edited

Loading

ti-chi-bot commented Sep 30, 2021 •

edited

Loading

codecov-commenter commented Sep 30, 2021 •

edited

Loading

mianhk Sep 30, 2021

csuzhangxc Oct 9, 2021

mianhk Sep 30, 2021

csuzhangxc Oct 9, 2021

mianhk Oct 9, 2021

july2993 left a comment

july2993 Nov 2, 2021

july2993 Nov 2, 2021

mianhk Nov 2, 2021

july2993 Nov 2, 2021

DanielZhangQD Nov 3, 2021

july2993 Nov 3, 2021

DanielZhangQD commented Nov 9, 2021

DanielZhangQD commented Nov 9, 2021

DanielZhangQD Nov 9, 2021

DanielZhangQD left a comment

DanielZhangQD commented Nov 9, 2021

DanielZhangQD commented Nov 9, 2021

ti-chi-bot commented Nov 9, 2021

DanielZhangQD commented Nov 9, 2021

DanielZhangQD commented Nov 9, 2021

DanielZhangQD commented Nov 10, 2021

	UpdateStrategy: apps.StatefulSetUpdateStrategy{
	Type: apps.RollingUpdateStatefulSetStrategyType,
	RollingUpdate: &apps.RollingUpdateStatefulSetStrategy{
	Partition: pointer.Int32Ptr(dc.Spec.Master.Replicas + int32(failureReplicas) + deleteSlotsNumber),
	}},

	updateStrategy := apps.StatefulSetUpdateStrategy{}
	if basePDSpec.StatefulSetUpdateStrategy() == apps.OnDeleteStatefulSetStrategyType {
	updateStrategy.Type = apps.OnDeleteStatefulSetStrategyType
	} else {
	updateStrategy.Type = apps.RollingUpdateStatefulSetStrategyType
	updateStrategy.RollingUpdate = &apps.RollingUpdateStatefulSetStrategy{
	Partition: pointer.Int32Ptr(tc.PDStsDesiredReplicas() + deleteSlotsNumber),
	}
	}

Feature:support configuring PodManagementPolicy in TidbCluster CR #4211

Feature:support configuring PodManagementPolicy in TidbCluster CR #4211

Conversation

mianhk commented Sep 30, 2021 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Code changes

Tests

Side effects

Related changes

Release Notes

ti-chi-bot commented Sep 30, 2021 • edited Loading

codecov-commenter commented Sep 30, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

july2993 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielZhangQD commented Nov 9, 2021

DanielZhangQD commented Nov 9, 2021

Choose a reason for hiding this comment

DanielZhangQD left a comment

Choose a reason for hiding this comment

DanielZhangQD commented Nov 9, 2021

DanielZhangQD commented Nov 9, 2021

ti-chi-bot commented Nov 9, 2021

DanielZhangQD commented Nov 9, 2021

DanielZhangQD commented Nov 9, 2021

DanielZhangQD commented Nov 10, 2021

mianhk commented Sep 30, 2021 •

edited

Loading

ti-chi-bot commented Sep 30, 2021 •

edited

Loading

codecov-commenter commented Sep 30, 2021 •

edited

Loading