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

Refactor helper pacakges and fix spec validation #623

Merged
merged 5 commits into from
Aug 4, 2020
Merged

Refactor helper pacakges and fix spec validation #623

merged 5 commits into from
Aug 4, 2020

Conversation

AstroProfundis
Copy link
Contributor

@AstroProfundis AstroProfundis commented Jul 23, 2020 •
edited by lucklove
Loading

What problem does this PR solve?

The ports of monitoring agents are not checked with other existing clusters before deploying a new cluster.

What is changed and how it works?

  • Refactor the package prepare and clusterutils.Retry to make it less possible of having import cycle.
  • Refactor the pre-deploy cross cluster conflict checks to involve ports of monitoring agents and make them easier to test
  • Add test cases for cross cluster conflict checks

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Code changes

  • Has exported function/method change

Release note:

Support detecting port conflict of monitoring agents between different clusters

@AstroProfundis AstroProfundis added the type/bug-fix Categorizes PR as a bug-fix label Jul 23, 2020
@AstroProfundis AstroProfundis requested a review from lonng July 23, 2020 07:56
@AstroProfundis AstroProfundis self-assigned this Jul 23, 2020
@AstroProfundis AstroProfundis force-pushed the fix-validate branch 2 times, most recently from 032de9b to e8280f0 Compare July 23, 2020 07:58
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2020 •
edited
Loading

Codecov Report

Merging #623 into master will decrease coverage by 8.68%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #623      +/-   ##
==========================================
- Coverage   56.71%   48.03%   -8.69%     
==========================================
  Files         242      243       +1     
  Lines       17469    17510      +41     
==========================================
- Hits         9908     8411    -1497     
- Misses       6265     7966    +1701     
+ Partials     1296     1133     -163
Flag Coverage Δ
#coverage 48.03% <ø> (-8.69%) ⬇️

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

Impacted Files Coverage Δ
.../src/github.com/pingcap/tiup/components/dm/main.go 0.00% <0.00%> (-100.00%) ⬇️
...hub.com/pingcap/tiup/components/dm/spec/cluster.go 0.00% <0.00%> (-90.91%) ⬇️
...om/pingcap/tiup/pkg/cluster/task/update_dm_meta.go 0.00% <0.00%> (-90.48%) ⬇️
...ithub.com/pingcap/tiup/components/dm/spec/logic.go 0.00% <0.00%> (-79.57%) ⬇️
...cap/tiup/pkg/cluster/template/scripts/dm_worker.go 0.00% <0.00%> (-77.15%) ⬇️
...b.com/pingcap/tiup/pkg/cluster/task/update_meta.go 0.00% <0.00%> (-74.20%) ⬇️
...com/pingcap/tiup/components/dm/spec/topology_dm.go 0.00% <0.00%> (-73.78%) ⬇️
...cap/tiup/pkg/cluster/template/scripts/dm_master.go 0.00% <0.00%> (-71.43%) ⬇️
...com/pingcap/tiup/components/dm/command/scale_in.go 31.03% <0.00%> (-62.07%) ⬇️
...c/github.com/pingcap/tiup/pkg/cluster/api/dmapi.go 0.00% <0.00%> (-61.32%) ⬇️
... and 57 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 41fbacf...1620e96. Read the comment docs.

@AstroProfundis AstroProfundis added the category/usability Categorizes issue or PR as a usability enhancement. label Jul 23, 2020
lonng
lonng reviewed Jul 23, 2020
View reviewed changes
pkg/cluster/spec/instance.go Outdated Show resolved Hide resolved
@AstroProfundis AstroProfundis changed the title cluster/spec: add monitoring agents to used port and used dir lists cluster/prepare: check also ports of monitoring agents with other existing clusters Jul 24, 2020
@lucklove
Copy link
Member

Should we apply this logic in CheckClusterDirConflict?
We should check directory conflict, too.

@lonng
Copy link
Contributor

lonng commented Jul 27, 2020

Should we apply this logic in CheckClusterDirConflict?
We should check directory conflict, too.

I think we should.

@AstroProfundis
Copy link
Contributor Author

There is already checks for dir conflicts with hostDirAccessor

lonng
lonng reviewed Jul 30, 2020
View reviewed changes
Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

Please add some tests for this PR.

@AstroProfundis AstroProfundis changed the title cluster/prepare: check also ports of monitoring agents with other existing clusters Refactor helper pacakges and fix spec validation Jul 30, 2020
@AstroProfundis AstroProfundis added category/testing Categorizes issue or PR as a testing enhancement. type/enhancement Categorizes issue or PR as related to an enhancement. labels Jul 30, 2020
@AstroProfundis AstroProfundis force-pushed the fix-validate branch 3 times, most recently from 11557c8 to 0adcd03 Compare July 30, 2020 12:11
july2993
july2993 reviewed Jul 31, 2020
View reviewed changes
pkg/cluster/spec/spec_manager.go Show resolved Hide resolved
pkg/utils/retry.go Show resolved Hide resolved
@lucklove
Copy link
Member

Rest LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 3, 2020
@lucklove lucklove merged commit 90544b1 into pingcap:master Aug 4, 2020
@AstroProfundis AstroProfundis deleted the fix-validate branch August 4, 2020 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@july2993 july2993 july2993 left review comments

@lonng lonng lonng left review comments

@lucklove lucklove lucklove approved these changes

Assignees

@AstroProfundis AstroProfundis

Labels
category/testing Categorizes issue or PR as a testing enhancement. category/usability Categorizes issue or PR as a usability enhancement. status/LGT1 Indicates that a PR has LGTM 1. type/bug-fix Categorizes PR as a bug-fix type/enhancement Categorizes issue or PR as related to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AstroProfundis @codecov-commenter @lucklove @lonng @july2993 @ti-srebot