Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Patch PVCs when storage request is increased #3096

Merged
merged 6 commits into from
Aug 18, 2020
Merged

Patch PVCs when storage request is increased #3096

merged 6 commits into from
Aug 18, 2020

Conversation

cofyc
Copy link
Contributor

@cofyc cofyc commented Aug 11, 2020 •
edited
Loading

What problem does this PR solve?

fixes #3004

What is changed and how does it work?

Check List

Tests

  • Unit test
  • E2E test
  • Stability test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has Go code change
  • Has CI related scripts change
  • Has Terraform scripts change

Side effects

  • Breaking backward compatibility

Related changes

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

Does this PR introduce a user-facing change?:

Support patching PVCs when the storage request is increased. Due to the limitation of the current statefulset, new PVCs will be created with the old storage request and resized later

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2020 •
edited
Loading

Codecov Report

Merging #3096 into master will increase coverage by 0.15%.
The diff coverage is 71.27%.

@@            Coverage Diff             @@
##           master    #3096      +/-   ##
==========================================
+ Coverage   42.98%   43.13%   +0.15%     
==========================================
  Files         155      156       +1     
  Lines       16830    16924      +94     
==========================================
+ Hits         7234     7301      +67     
- Misses       8983     8999      +16     
- Partials      613      624      +11
Flag Coverage Δ
#unittest 43.13% <71.27%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@cofyc cofyc changed the title Patch PVC when storage request is increased WIP: Patch PVC when storage request is increased Aug 12, 2020
@cofyc cofyc force-pushed the fix3004 branch 2 times, most recently from 1277571 to ee394d6 Compare August 13, 2020 05:31
@cofyc cofyc changed the title WIP: Patch PVC when storage request is increased Patch PVC when storage request is increased Aug 13, 2020
@cofyc
Copy link
Contributor Author

cofyc commented Aug 13, 2020

cc @mightyguava

@cofyc cofyc requested a review from weekface August 13, 2020 06:55
@cofyc cofyc changed the title Patch PVC when storage request is increased Patch PVCs when storage request is increased Aug 13, 2020
weekface
weekface previously approved these changes Aug 13, 2020
View reviewed changes
Copy link
Contributor

@weekface weekface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mightyguava
mightyguava previously approved these changes Aug 14, 2020
View reviewed changes
Copy link
Contributor

@mightyguava mightyguava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can/should the operator also monitor that the volume expansion is eventually applied? I guess it can only do that if ExpandInUsePersistentVolumes is enabled, which it probably can't check

pkg/manager/member/pvc_resizer_test.go
newPVCWithStorage("pd-0", label.PDLabelVal, "sc", "2Gi"),
},
wantPVCs: []*v1.PersistentVolumeClaim{
newPVCWithStorage("pd-0", label.PDLabelVal, "sc", "2Gi"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice test!

@ti-srebot
Copy link
Contributor

@mightyguava,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: k8s(slack).

1 similar comment
@ti-srebot
Copy link
Contributor

@mightyguava,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: k8s(slack).

@cofyc
Copy link
Contributor Author

cofyc commented Aug 14, 2020

can/should the operator also monitor that the volume expansion is eventually applied? I guess it can only do that if ExpandInUsePersistentVolumes is enabled, which it probably can't check

it can but the operator can only report some events, but users can watch the PVCs too. as for now, I think it's not necessary.

DanielZhangQD
DanielZhangQD previously approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

pkg/manager/member/pvc_resizer.go Outdated Show resolved Hide resolved
ti-srebot
ti-srebot previously approved these changes Aug 17, 2020
View reviewed changes
@cofyc @DanielZhangQD
Update pkg/manager/member/pvc_resizer.go
1157008 
Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>
@cofyc cofyc dismissed stale reviews from ti-srebot and DanielZhangQD via 1157008 August 17, 2020 05:52
@cofyc cofyc dismissed stale reviews from mightyguava and weekface via 1157008 August 17, 2020 05:52
DanielZhangQD
DanielZhangQD approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lichunzhu
Copy link
Contributor

/test pull-e2e-kind

weekface
weekface approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@weekface weekface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cofyc
Copy link
Contributor Author

cofyc commented Aug 18, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit ea320d2 into pingcap:master Aug 18, 2020
ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Aug 18, 2020
@cofyc @ti-srebot
cherry pick pingcap#3096 to release-1.1
09a01ef 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 18, 2020
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-1.1 in PR #3122

@cofyc cofyc mentioned this pull request Aug 18, 2020
Merged
DanielZhangQD added a commit that referenced this pull request Aug 18, 2020
@cofyc @DanielZhangQD
Automated cherry pick of #3096: Patch PVC when storage request is inc…
afd8599 
…reased (#3124)

* Patch PVC when storage request is increased

* shrinking is not supported

* Update pkg/manager/member/pvc_resizer.go

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

* resolve conflicts

Co-authored-by: DanielZhangQD <36026334+DanielZhangQD@users.noreply.github.com>
@sebgl sebgl mentioned this pull request Sep 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weekface weekface weekface approved these changes

@DanielZhangQD DanielZhangQD DanielZhangQD approved these changes

@mightyguava mightyguava mightyguava left review comments

@ti-srebot ti-srebot ti-srebot left review comments

@lichunzhu lichunzhu Awaiting requested review from lichunzhu

Assignees
No one assigned
Labels
CanMerge needs-cherry-pick-1.1 status/LGT2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Patch PVC when storage request is increased
7 participants
@cofyc @codecov-commenter @ti-srebot @lichunzhu @weekface @mightyguava @DanielZhangQD