feat(pvc): support modify ebs #4683

liubog2008 · 2022-08-23T08:14:29Z

What problem does this PR solve?

Sync volume status when syncing tc status
Support to config env for the operator when install by helm
Add a feature gate "VolumeModifying" to enable the volume modifying, default to disable.

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.

support modify ebs

ti-chi-bot · 2022-08-23T08:14:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

KanShiori
csuzhangxc

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 · 2022-08-23T08:28:10Z

Codecov Report

Merging #4683 (05d53ca) into master (7c188d4) will increase coverage by 9.37%.
The diff coverage is 58.93%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4683      +/-   ##
==========================================
+ Coverage   62.13%   71.51%   +9.37%     
==========================================
  Files         197      210      +13     
  Lines       21917    25492    +3575     
==========================================
+ Hits        13619    18230    +4611     
+ Misses       7053     5954    -1099     
- Partials     1245     1308      +63

Flag	Coverage Δ
e2e	`58.51% <33.26%> (?)`
unittest	`61.56% <49.10%> (-0.57%)`	⬇️

csuzhangxc · 2022-08-23T08:24:36Z

pkg/manager/volumes/delegation/interface.go

+
+type VolumeModifier interface {
+	MinWaitDuration() time.Duration
+	// ModifyVolume modifies the underlay volume of pvc to match the args of storageclass


it's better to explain the bool return value here.

pkg/manager/volumes/delegation/aws/ebs_modifier.go

csuzhangxc · 2022-08-31T07:38:27Z

pkg/manager/volumes/delegation/aws/ebs_modifier.go

+}
+
+func (m *EBSModifier) setArgsFromPV(v *Volume, pv *corev1.PersistentVolume) error {
+	v.VolumeId = pv.Spec.CSI.VolumeHandle


is it needed to check pv.Spec.CSI != nil and pv.Spec.CSI.VolumeHandle != "" here?

csuzhangxc · 2022-08-31T07:55:08Z

pkg/manager/volumes/delegation/aws/ebs_modifier.go

+
+func (m *EBSModifier) setArgsFromStorageClass(v *Volume, sc *storagev1.StorageClass) error {
+	if sc == nil {
+		return nil


When will sc be nil, and what's the expect behaviour?

csuzhangxc · 2022-08-31T08:08:02Z

pkg/manager/volumes/delegation/aws/mock.go

+				m.vs[i] = Volume{
+					VolumeId:   *param.VolumeId,
+					Size:       param.Size,
+					IOPS:       param.Iops,
+					Throughput: param.Throughput,
+					Type:       param.VolumeType,
+				}


csuzhangxc · 2022-08-31T09:10:40Z

pkg/manager/volumes/phase.go

+	return isPVCStatusMatched(pvc, scName, size)
+}
+
+func isPVCStatusMatched(pvc *corev1.PersistentVolumeClaim, scName, size string) bool {


any better name? the returned value is changed not matched. the same for isPVCSpecMatched in pod_vol_modifier.go

csuzhangxc · 2022-08-31T09:18:33Z

pkg/manager/volumes/pod_vol_modifier.go

+}
+
+func (p *podVolModifier) Modify(tc *v1alpha1.TidbCluster, pod *corev1.Pod, expected []DesiredVolume, shouldEvictLeader bool) (bool, error) {
+	ctx := context.TODO()


pass ctx in as a parameter?

csuzhangxc · 2022-08-31T09:41:48Z

pkg/manager/volumes/pod_vol_modifier.go

+func (p *podVolModifier) modifyVolume(ctx context.Context, vol *ActualVolume) (bool, error) {
+	m := p.getVolumeModifier(vol.Desired.StorageClass)
+	if m == nil {
+		// skip modifying volume by delegation.VolumeModifier


It's not an invalid path. Size of some volumes can be modified even though delegation.VolumeModifier is not implemented.

pkg/controller/dependences.go

KanShiori · 2022-09-14T03:07:32Z

pkg/manager/member/tikv_upgrader.go

-		return fmt.Errorf("upgradeTiKVPod: failed to get pods %s for cluster %s/%s, error: %s", upgradePodName, ns, tcName, err)
+		return fmt.Errorf("upgradeTiKVPod: failed to get pod %s for tc %s/%s, error: %s", upgradePodName, ns, tcName, err)
 	}

-	storeID, err := TiKVStoreIDFromStatus(tc, upgradePodName)
+	done, err := u.evictLeaderBeforeUpgrade(tc, upgradePod)
 	if err != nil {
-		if err == ErrNotFoundStoreID {
-			return controller.RequeueErrorf("tidbcluster: [%s/%s] no store status found for tikv pod: [%s]", ns, tcName, upgradePodName)
-		}
-		return err
+		return fmt.Errorf("upgradeTiKVPod: failed to evict leader of pod %s for tc %s/%s, error: %s", upgradePodName, ns, tcName, err)
 	}
-
-	_, evicting := upgradePod.Annotations[EvictLeaderBeginTime]
-	if !evicting {
-		return u.beginEvictLeader(tc, storeID, upgradePod)
+	if !done {
+		return controller.RequeueErrorf("upgradeTiKVPod: evicting leader of pod %s for tc %s/%s", upgradePodName, ns, tcName)
 	}

-	if u.readyToUpgrade(upgradePod, tc) {
-		mngerutils.SetUpgradePartition(newSet, ordinal)
-		return nil


have moved into the func evictLeaderBeforeUpgrade

liubog2008 · 2022-09-19T02:00:55Z

/test pull-e2e-kind-serial

liubog2008 · 2022-09-19T05:44:10Z

/test pull-e2e-kind

liubog2008 · 2022-09-19T08:47:40Z

/test pull-e2e-kind

liubog2008 · 2022-09-20T06:44:18Z

/test pull-e2e-kind-serial

liubog2008 · 2022-09-20T06:44:27Z

/test pull-e2e-kind-tngm

KanShiori · 2022-09-21T07:18:02Z

pkg/manager/volumes/phase.go

+	VolumePhaseUnknown VolumePhase = iota
+	// 1. isPVCRevisionChanged: false
+	// 2. needModify: true
+	// 3. waitForNextTime: true
+	VolumePhasePending
+	// 1. isPVCRevisionChanged: false
+	// 2. needModify: true
+	// 3. waitForNextTime: false
+	VolumePhasePreparing
+	// 1. isPVCRevisionChanged: true
+	// 2. needModify: true/false
+	// 3. waitForNextTime: true/false
+	VolumePhaseModifying
+	// 1. isPVCRevisionChanged: false
+	// 2. needModify: false
+	// 3. waitForNextTime: true/false
+	VolumePhaseModified
+
+	VolumePhaseCannotModify


How about adding some comments to describe every phase?

KanShiori · 2022-09-21T07:24:38Z

pkg/manager/volumes/pvc_modifier.go

+	annoKeyPVCSpecRevision     = "spec.tidb.pingcap.com/revision"
+	annoKeyPVCSpecStorageClass = "spec.tidb.pingcap.com/storage-class"
+	annoKeyPVCSpecStorageSize  = "spec.tidb.pingcap.com/storage-size"
+
+	annoKeyPVCStatusRevision     = "status.tidb.pingcap.com/revision"
+	annoKeyPVCStatusStorageClass = "status.tidb.pingcap.com/storage-class"
+	annoKeyPVCStatusStorageSize  = "status.tidb.pingcap.com/storage-size"
+
+	annoKeyPVCLastTransitionTimestamp = "status.tidb.pingcap.com/last-transition-timestamp"


how about adding some comments to describe every annotation?

KanShiori · 2022-09-21T07:26:44Z

pkg/manager/volumes/pvc_modifier.go

+	sort.Slice(pods, func(i, k int) bool {
+		a, b := pods[i].Name, pods[k].Name
+		if len(a) != len(b) {
+			return len(a) < len(b)
+		}
+		return a < b
+	})


how about adding comments to describe the reason for the sorting?

Co-authored-by: Shiori <sihao.yu@pingcap.com>

csuzhangxc · 2022-09-21T09:25:01Z

/test pull-e2e-kind-br

KanShiori · 2022-09-21T09:25:05Z

/test pull-e2e-kind-br

csuzhangxc · 2022-09-21T10:12:42Z

/test pull-e2e-kind-tikv-scale-simultaneously

csuzhangxc · 2022-09-21T11:29:21Z

/test pull-e2e-kind

csuzhangxc · 2022-09-21T11:29:31Z

/test pull-e2e-kind-basic

csuzhangxc · 2022-09-21T11:30:29Z

/merge

ti-chi-bot · 2022-09-21T11:30:32Z

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

Commit hash: 05d53ca

csuzhangxc · 2022-09-21T12:21:25Z

/test pull-e2e-kind-tikv-scale-simultaneously

liubog2008 · 2022-09-21T12:58:05Z

/test pull-e2e-kind-basic

csuzhangxc · 2022-09-21T13:38:08Z

/test pull-e2e-kind

csuzhangxc · 2022-09-21T14:25:47Z

/test pull-e2e-kind-basic

csuzhangxc · 2022-09-21T23:41:46Z

/test pull-e2e-kind

csuzhangxc · 2022-09-21T23:42:06Z

/test pull-e2e-kind-basic

coderplay · 2022-10-18T18:22:19Z

docs/api-references/docs.md

+</td>
+<td>
+<em>(Optional)</em>
+<p>ModifiedStorageClass is the modified storage calss of the volume.</p>


incorrect description?

coderplay · 2022-10-18T18:23:56Z

docs/api-references/docs.md

 <p>CurrentCapacity is the current capacity of the volume.
 If any volume is resizing, it is the capacity before resizing.
 If all volumes are resized, it is the resized capacity and same as desired capacity.</p>
 </td>
 </tr>
 <tr>
 <td>
+<code>modifiedCapacity</code></br>


The terms "resizedCapacity" and "modifiedCapacity" are confusingly named.

ti-chi-bot requested review from DanielZhangQD and july2993 August 23, 2022 08:14

liubog2008 marked this pull request as draft August 23, 2022 08:15

csuzhangxc reviewed Aug 23, 2022

View reviewed changes

ti-chi-bot added the needs-rebase label Aug 31, 2022

csuzhangxc reviewed Aug 31, 2022

View reviewed changes

liubog2008 marked this pull request as ready for review September 13, 2022 06:23

liubog2008 changed the title ~~wip: support modify ebs~~ feat(pvc): support modify ebs Sep 13, 2022

liubog2008 force-pushed the support-modify-ebs branch from 7db6167 to 5ef3e70 Compare September 13, 2022 06:40

ti-chi-bot removed the needs-rebase label Sep 13, 2022

KanShiori reviewed Sep 14, 2022

View reviewed changes

liubog2008 force-pushed the support-modify-ebs branch from b9a9f54 to 44dcf3a Compare September 15, 2022 00:55

liubog2008 force-pushed the support-modify-ebs branch from 47af3f4 to 21b3c70 Compare September 19, 2022 03:50

liubog2008 force-pushed the support-modify-ebs branch from 21b3c70 to 949fe88 Compare September 19, 2022 07:08

liubog2008 force-pushed the support-modify-ebs branch from 949fe88 to f84df33 Compare September 20, 2022 03:25

liubog2008 force-pushed the support-modify-ebs branch 2 times, most recently from 062efd3 to 77f741e Compare September 21, 2022 06:34

KanShiori reviewed Sep 21, 2022

View reviewed changes

liubog2008 and others added 2 commits September 21, 2022 16:21

feat(volume): support modify ebs.

1f3cb8e

Co-authored-by: Shiori <sihao.yu@pingcap.com>

fix tidb debug container

05d53ca

liubog2008 force-pushed the support-modify-ebs branch from 77f741e to 05d53ca Compare September 21, 2022 08:22

KanShiori mentioned this pull request Sep 21, 2022

Release v1.4.0-alpha.1 #4714

Closed

6 tasks

csuzhangxc approved these changes Sep 21, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 label Sep 21, 2022

KanShiori approved these changes Sep 21, 2022

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Sep 21, 2022

ti-chi-bot added the status/can-merge label Sep 21, 2022

ti-chi-bot merged commit 755dff3 into pingcap:master Sep 22, 2022

KanShiori mentioned this pull request Oct 10, 2022

Release v1.3.9 #4735

Closed

6 tasks

coderplay reviewed Oct 18, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pvc): support modify ebs #4683

feat(pvc): support modify ebs #4683

liubog2008 commented Aug 23, 2022 •

edited by KanShiori

Loading

ti-chi-bot commented Aug 23, 2022 •

edited

Loading

codecov-commenter commented Aug 23, 2022 •

edited

Loading

csuzhangxc Aug 23, 2022

csuzhangxc Aug 31, 2022

csuzhangxc Aug 31, 2022

csuzhangxc Aug 31, 2022

csuzhangxc Aug 31, 2022

csuzhangxc Aug 31, 2022

csuzhangxc Aug 31, 2022

liubog2008 Sep 15, 2022

KanShiori Sep 14, 2022

liubog2008 commented Sep 19, 2022

liubog2008 commented Sep 19, 2022

liubog2008 commented Sep 19, 2022

liubog2008 commented Sep 20, 2022

liubog2008 commented Sep 20, 2022

KanShiori Sep 21, 2022

KanShiori Sep 21, 2022

KanShiori Sep 21, 2022

csuzhangxc commented Sep 21, 2022

KanShiori commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

ti-chi-bot commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

liubog2008 commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

coderplay Oct 18, 2022

coderplay Oct 18, 2022

feat(pvc): support modify ebs #4683

feat(pvc): support modify ebs #4683

Conversation

liubog2008 commented Aug 23, 2022 • edited by KanShiori 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 Aug 23, 2022 • edited Loading

codecov-commenter commented Aug 23, 2022 • 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

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

liubog2008 commented Sep 19, 2022

liubog2008 commented Sep 19, 2022

liubog2008 commented Sep 19, 2022

liubog2008 commented Sep 20, 2022

liubog2008 commented Sep 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc commented Sep 21, 2022

KanShiori commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

ti-chi-bot commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

liubog2008 commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

csuzhangxc commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liubog2008 commented Aug 23, 2022 •

edited by KanShiori

Loading

ti-chi-bot commented Aug 23, 2022 •

edited

Loading

codecov-commenter commented Aug 23, 2022 •

edited

Loading