Fix PD --peer-urls flag to use listen_host variable

[fln]

What problem does this PR solve?

This allows us to use server hostname instead of IP address. Without
this change if PD service is declared using hostname it fails to start,
reporting an error that it can not bind to given interface.

Metadata variable listen_host was introduced in #495 and is already used for
the --client-urls flag.

This should resolve #337 and partially implement #691 .

What is changed and how it works?

This commit updates run_pd.sh template to use listen_host for
--peer-urls flag instead of host.

Check List

Tests

• No code

Side effects

• Breaking backward compatibility

If existing installations relied on host field to be used as address for accepting peer connections this change could be treated as breaking change. Default value of listen_host is 0.0.0.0 so it should not break existing clusters.

Some might want to use different listen addresses for peer and client connections. However I think this would add unnecessary complexity and not worth supporting.

Related changes

• Need to update the documentation

Not sure where metadata file is properly documented. So far I have used source code to understand what is expected in the metadata.yaml file.

Release notes:

Support using hostname instead of IP for PD servers in host section.

[CLAassistant]

All committers have signed the CLA.

[codecov-io]

Codecov Report

Merging #941 (220429b) into master (bc0270c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #941   +/-   ##
=======================================
  Coverage   55.66%   55.67%           
=======================================
  Files         263      263           
  Lines       19392    19396    +4     
=======================================
+ Hits        10795    10799    +4     
  Misses       6880     6880           
  Partials     1717     1717           

Flag	Coverage Δ
cluster	43.38% <100.00%> (+<0.01%)	⬆️
dm	23.92% <28.57%> (-0.01%)	⬇️
integrate	49.90% <100.00%> (+<0.01%)	⬆️
playground	20.21% <ø> (ø)
tiup	16.92% <100.00%> (ø)
unittest	23.03% <28.57%> (-0.01%)	⬇️

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

Impacted Files	Coverage Δ
pkg/cluster/embed/autogen_pkger.go	100.00% <100.00%> (ø)
pkg/cluster/task/tls.go	65.38% <100.00%> (+2.88%)	⬆️

---

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 bc0270c...220429b. Read the comment docs.

[9547]

@fln Could you please add some test case, such as the case of #337

[fln]

@9547 Thanks for feedback and suggestions.

Updated the tests as requested. First attempt was to create a separate test just for host name support. It turned out the integration tests are quite convoluted and performs some tricks to actually work with IP addresses which are no longer necessary if hostnames are used.

In the end all IP addresses in tests were replaced with docker hostnames n1 - n5. One could argue that it is good idea to use hostnames as a default since we have a support for it already. It will also prevent us from breaking hostname support in the future. Furthermore it is more likely that code which works with hostnames will work with IP addresses without modifications, which is not true in the opposite direction.

Key changes that were made since initial commit:

• Discovered that run_pd_scale.sh.tpl is used for scaling it needed same template change that was done in the initial commit. This made refactoring the tests worthwhile and saved us tripping on it at the later stage.
• Made a small change to the automated TLS certificate generator, to generate correct certificates if hostname are used - IP and hostname is goes to different x509 sections.
• Made all test topology files stable. Removed sed invocations that used to create actual topology files from the topology templates. Running tests no longer creates temporary topology files (it was kind of annoying as docker running with root created temp files owned by root). In the process file full_scale_in_tidb.yaml had to be split into two versions, apparently this file was being mutated in the middle of the tests.

[9547]

Thanks, but could you please split this pr into two or three

• this pr: fix the pd url
• refact tests: use hostname instead of ip for common tests, but keep some ip cases
• fix the x509 issue

thanks again.

[fln]

PRs are split. Just noticed that I have overlooked the request to have mixed IP and host name tests.

Mixing host names and IP addresses in tests is not that trivial.

If we mix host names and IP address in a single topology - tiup-cluster will treat them as different hosts and complain about ports being already in use by external services.

Leaving some tests with host names while other test with IP addresses also causes issue because full.yaml is shared between scale and cmd tests.

Is there any chance full test suite switch to host names would be accepted 😄 ?

Alternatively would a simple test of just building a minimal cluster and tearing it down using only IP addresses suffice? My opinion is that if we have code that works with host names it should with high confidence work with IP addresses as well without explicit testing.

[ti-chi-bot]

@fln: PR needs rebase.

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 kubernetes/test-infra repository.

[fln]

Closing this PR, it is replaced by:

• Support host names in TLS certificates #948
• Fix PD --peer-urls flag to use listen_host variable #949
• Switch to host names in integration tests #950