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.

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

cluster: fix parsing of paths if there are redundant slashes in it #1068

Merged
merged 4 commits into from
Jan 18, 2021

Conversation

AstroProfundis
Copy link
Contributor

@AstroProfundis AstroProfundis commented Jan 14, 2021

What problem does this PR solve?

If an imported cluster has deploy dir ending with /, and sub dirs as <deploy-dir>//sub, the result of CountDir() may not be accurate, and could results to delete wrong paths on scale-in.

One of the case is when:

  • The deploy dir is /foo/, and
  • The log dir is /foo//log, and
  • There is and only is 1 instance with only 1 sub directory under deploy dir (e.g., tidb with log dir under deploy dir) deployed on host 1.2.3.4, and
  • There are other instances without log dir and data dir (e.g., grafana) deployed on the same host

The deploy dir will be deleted when scale-in the grafana instance, so that the tidb instance is also broken.

What is changed and how it works?

  1. Not deleting deploy dir for imported instance
  2. Trim redundant slashes from the path when parsing meta file

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
  • Has persistent data change

Related changes

  • Need to cherry-pick to the release branch

Release notes:

cluster: fix parsing of paths if there are redundant slashes in it

@AstroProfundis AstroProfundis added type/bug-fix Categorizes PR as a bug-fix category/stability Categorizes issue or PR as a stability enhancement. labels Jan 14, 2021
@AstroProfundis AstroProfundis added this to the v1.3.2 milestone Jan 14, 2021
@AstroProfundis AstroProfundis self-assigned this Jan 14, 2021
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 14, 2021
@ti-chi-bot ti-chi-bot requested a review from lonng January 14, 2021 08:39
@codecov-io
Copy link

codecov-io commented Jan 14, 2021

Codecov Report

Merging #1068 (1d270e0) into master (3b6ac73) will decrease coverage by 16.84%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1068       +/-   ##
===========================================
- Coverage   55.95%   39.10%   -16.85%     
===========================================
  Files         282      281        -1     
  Lines       19898    19898               
===========================================
- Hits        11133     7781     -3352     
- Misses       7040    10874     +3834     
+ Partials     1725     1243      -482     
Flag Coverage Δ
cluster ?
dm 22.20% <100.00%> (-1.85%) ⬇️
integrate 30.23% <100.00%> (-20.02%) ⬇️
playground 20.29% <ø> (ø)
tiup 16.48% <0.00%> (-0.01%) ⬇️
unittest 22.27% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
pkg/cluster/operation/destroy.go 26.44% <100.00%> (-30.17%) ⬇️
pkg/cluster/spec/util.go 33.33% <100.00%> (-48.07%) ⬇️
components/cluster/main.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/cluster/template/config/config.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/cluster/template/scripts/scripts.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/cluster/spec/bindversion.go 0.00% <0.00%> (-83.34%) ⬇️
pkg/cluster/task/update_meta.go 0.00% <0.00%> (-80.00%) ⬇️
pkg/cluster/template/scripts/tiflash.go 0.00% <0.00%> (-79.60%) ⬇️
pkg/cluster/spec/cdc.go 3.44% <0.00%> (-79.32%) ⬇️
pkg/cluster/manager/reload.go 0.00% <0.00%> (-77.56%) ⬇️
... and 121 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b6ac73...1d270e0. Read the comment docs.

@lucklove
Copy link
Member

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 18, 2021
@lucklove
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 3a42a5c

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 18, 2021
@ti-chi-bot
Copy link
Member

@AstroProfundis: Your PR has out-of-dated, I have automatically updated it for you.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/ti-community-prow repository.

@ti-chi-bot ti-chi-bot merged commit 4ece39a into pingcap:master Jan 18, 2021
@AstroProfundis AstroProfundis deleted the fix-abs branch January 19, 2021 03:36
mianhk pushed a commit to mianhk/tiup that referenced this pull request Jan 21, 2021
…ingcap#1068)

* cluster: not deleting deploy dir for imported instance

* cluster/spec: trim slashes in paths

Co-authored-by: Ti Chi Robot <71242396+ti-chi-bot@users.noreply.github.com>
lucklove pushed a commit that referenced this pull request Jan 28, 2021
…1068)

* cluster: not deleting deploy dir for imported instance

* cluster/spec: trim slashes in paths

Co-authored-by: Ti Chi Robot <71242396+ti-chi-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/stability Categorizes issue or PR as a stability enhancement. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/bug-fix Categorizes PR as a bug-fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants