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/import: read and set deploy_dir properly on import #704

Merged
merged 2 commits into from
Aug 24, 2020

Conversation

AstroProfundis
Copy link
Contributor

What problem does this PR solve?

The global deploy_dir is not set during the process of importing from ansible deployed cluster, however, as we detect and set deploy dir for each instance individually, this only affect monitoring agents (node_exporter and blackbox_exporter).

What is changed and how it works?

Read the value of deploy_dir in inventory.ini and set it to global.deploy_dir and monitored.deploy_dir global variables.

Note that as we read and set dirs for each instance individually, this change does not effect them.

Check List

Tests

  • Unit test

Code changes

  • Has persistent data change

Side effects
None

Related changes

  • Need to cherry-pick to the release branch

Release notes:

Set correct `deploy_dir` of monitoring agents when importing ansible deployed clusters

@AstroProfundis AstroProfundis added the type/bug-fix Categorizes PR as a bug-fix label Aug 21, 2020
@AstroProfundis AstroProfundis requested a review from lonng August 21, 2020 11:10
@AstroProfundis AstroProfundis self-assigned this Aug 21, 2020
@@ -59,6 +66,7 @@ monitoring_servers: []
`)

topo, err := yaml.Marshal(clsMeta.Topology)
fmt.Printf("Got initial topo:\n%s\n", topo)
Copy link
Contributor

Choose a reason for hiding this comment

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

For debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it won't output anything if the test passes.

pkg/cluster/spec/spec.go Outdated Show resolved Hide resolved
// subdirs of deploy_dir in global options
clsMeta.Topology.MonitoredOptions.DeployDir = clsMeta.Topology.GlobalOptions.DeployDir
clsMeta.Topology.MonitoredOptions.DataDir = filepath.Join(
clsMeta.Topology.MonitoredOptions.DeployDir,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there risk is the user doesn't set the global deploy_dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible for tidb-ansible, in that case the deploy process could not be finished.

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2020

Codecov Report

Merging #704 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   57.64%   57.65%   +0.01%     
==========================================
  Files         254      254              
  Lines       18433    18447      +14     
==========================================
+ Hits        10626    10636      +10     
- Misses       6410     6412       +2     
- Partials     1397     1399       +2     
Flag Coverage Δ
#coverage 57.65% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
go/src/github.com/pingcap/tiup/pkg/utils/ioutil.go 47.54% <0.00%> (-1.64%) ⬇️
...c/github.com/pingcap/tiup/pkg/cluster/api/pdapi.go 58.53% <0.00%> (-1.22%) ⬇️
...ub.com/pingcap/tiup/pkg/cluster/spec/prometheus.go 80.20% <0.00%> (+0.20%) ⬆️
...ap/tiup/pkg/cluster/template/scripts/prometheus.go 81.81% <0.00%> (+3.43%) ⬆️
...hub.com/pingcap/tiup/pkg/cluster/ansible/import.go 56.89% <0.00%> (+8.81%) ⬆️

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 48d4a55...20fdc0d. Read the comment docs.

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.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 24, 2020
@lonng lonng merged commit 190e9b1 into pingcap:master Aug 24, 2020
@AstroProfundis AstroProfundis deleted the fix-import-deploy-dir branch August 24, 2020 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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