Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

HA: add advertise-addr for DM-worker #450

Merged
merged 25 commits into from
Jan 15, 2020

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Jan 11, 2020

What problem does this PR solve?

DM-worker may listen on some special address (like 0.0.0.0:8262) in a k8s pod, when registering it into the DM-master cluster, the DM-master may not access it with this special address.

What is changed and how it works?

  • add advertise-addr config item for DM-worker to handle client traffic.

Check List

Tests

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

Code changes

  • Has exported variable/fields change

Related changes

  • Need to update the documentation
  • Need to update the dm/dm-ansible

@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated status/PTAL This PR is ready for review. Add this label back after committing new changes type/feature New feature should-update-dm-ansible status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jan 11, 2020
…rtise

# Conflicts:
#	tests/_utils/run_dm_worker
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc csuzhangxc added status/LGT1 One reviewer already commented LGTM status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress status/LGT1 One reviewer already commented LGTM labels Jan 11, 2020
@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @lichunzhu PTAL

@csuzhangxc csuzhangxc changed the title Worker advertise *: add advertise-addr for DM-worker Jan 11, 2020
@csuzhangxc csuzhangxc changed the title *: add advertise-addr for DM-worker HA: add advertise-addr for DM-worker Jan 11, 2020
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC 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

@@ -25,7 +25,7 @@ PWD=$(pwd)
echo "[$(date)] <<<<<< START DM-WORKER on port $port, config: $conf >>>>>>"
cd $workdir
$binary -test.coverprofile="$TEST_DIR/cov.$TEST_NAME.worker.$port.$(date +"%s").out" DEVEL \
--worker-addr=:$port \
--worker-addr=0.0.0.0:$port \
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check the host? 0.0.0.0:$port will become the advertise-addr, but it can not be used by dm-master

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 addressed in 2024c7c.

  1. add the check for 0.0.0.0 as the advertise-addr
  2. add advertise-addr when running dm-worker in tests.

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jan 13, 2020
@lichunzhu
Copy link
Contributor

There same to be some problem with make check.

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

There same to be some problem with make check.

Yes, the problem was introduced when merging changes from for other PR, and I've updated in d6f8084.

Copy link
Contributor

@lichunzhu lichunzhu 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 lichunzhu added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Jan 14, 2020
@csuzhangxc csuzhangxc merged commit 255846f into pingcap:ha-dev Jan 15, 2020
@csuzhangxc csuzhangxc deleted the worker-advertise branch January 15, 2020 01:53
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@csuzhangxc csuzhangxc added already-update-docs The docs related to this PR already updated. Add this label once the docs are updated already-update-dm-ansible and removed needs-update-dm-ansible needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-update-docs The docs related to this PR already updated. Add this label once the docs are updated priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants