support v3 discovery to bootstrap a new etcd cluster #13635
Conversation
|
The test failures should be caused by unstable test cases, because all test passed in another PR, see ahrtr#1 |
ahrtr
force-pushed
the
v3_discovery
branch
3 times, most recently
from
a143acd to
a2174c2
Compare
|
@serathius Resolved all your comments, PTAL. Thanks! |
|
@serathius Thanks for the useful comments, and I resolved all of them. |
| maxExpoentialRetries = uint(8) | ||
| ) | ||
|
|
||
| type DiscoveryConfig struct { |
There was a problem hiding this comment.
This config is part of embed.Config, As so we should annotate its fields with json. Can you replace flags names with json annotations?
There was a problem hiding this comment.
Independently how we structure the flags (the other comment),
I think we should structure this configuration to be explicit what's just client config,
and what's discovery specific (in this case - nothing).
How about structuring this protos/json as follows (ie. grouping client stuff in the client: submessage.
type ClientConfig struct { // Ideally in `client/v3/...`
Endpoints string[] `json:"endpoints"`
DialTimeout time.Duration `json:"dial-timeout"`
...
}
type DiscoveryConfig struct {
Client client `json:"client"`
}
There was a problem hiding this comment.
Yes, it's a good point. Actually previously @serathius already raised a similar comment.
Since it may have some impacts on the clientv3 and also the etcdctl tool, so I'd like to resolve this comment in a separate PR.
There is already a clientconfig, but it's in etcdctl instead of clientv3. So we may need some minor refactor. So I prefer to doing it in a separate PR.
What do you think? @ptabor @serathius @spzala
server/etcdmain/help.go
Outdated
| --discovery '' | ||
| Discovery URL used to bootstrap the cluster. | ||
| --discovery-dial-timeout |
There was a problem hiding this comment.
It would be way nicer if we had a single json describing any etcd client configuration:
--discovery-conn-json='{url: ... dial-timeout: ...}'
this would keep the configuration up-to date with all the changes we introduce to the client without introducing a lot of custom flags.
There was a problem hiding this comment.
Yes, I agree that it's extensible, but it looks a little strange and may not be easy to use. Do you mean to get all the new fields (see below) included in a single json?
--discovery-dial-timeout
V3 discovery: dial timeout for client connections.
--discovery-request-timeout
V3 discovery: timeout for discovery requests (excluding dial timeout).
--discovery-keepalive-time
V3 discovery: keepalive time for client connections.
--discovery-keepalive-timeout
V3 discovery: keepalive timeout for client connections.
--discovery-insecure-transport 'false'
V3 discovery: disable transport security for client connections.
--discovery-insecure-skip-tls-verify
V3 discovery: skip server certificate verification (CAUTION: this option should be enabled only for testing purposes).
--discovery-cert
V3 discovery: identify secure client using this TLS certificate file.
--discovery-key
V3 discovery: identify secure client using this TLS key file.
--discovery-cacert
V3 discovery: verify certificates of TLS-enabled secure servers using this CA bundle.
--discovery-user
V3 discovery: username[:password] for authentication (prompt if password is not supplied).
--discovery-password
V3 discovery: password for authentication (if this option is used, --user option shouldn't include password).
There was a problem hiding this comment.
I think that we might want to invest into making configuration more reusable, like extracting and reusing common client configuration struct. This is however independent on the decision on introducing hybrid flag like --discovery-conn-json that combine both command line and JSON interface.
Personally I would prefer not to go in this direction as passing JSON into command line commands is very error prone.
There was a problem hiding this comment.
I like the idea, I will try to fix it in a separate PR. Thanks @serathius
I think that we might want to invest into making configuration more reusable, like extracting and reusing common client configuration struct.
There was a problem hiding this comment.
SGTM.
Even if we want to keep the flags separate and top-level, I think we should I'm at sharing the logic with:
Lines 66 to 80 in bf0b1a7
such tag all the flags can be imported as:
DeclareV3ClientFlags(rootCmd.PersistentFlags(), &someContext.Client, "discovery", "discoveryV3: ");
But I'm fine with doing this in separate PR. Just it's good to align the naming.
|
@ahrtr @serathius sorry didn't get to review it earlier, but doing it today. Thanks! |
ahrtr
force-pushed
the
v3_discovery
branch
2 times, most recently
from
e359364 to
539c263
Compare
|
Resolved the comment in help.go. PTAL @spzala Thanks. |
|
Just rebased this PR. |
|
Just rebased this PR. cc @ptabor |
ahrtr
force-pushed
the
v3_discovery
branch
2 times, most recently
from
8478fc7 to
8ca3ea6
Compare
|
Resolved all your comments except the following two items:
Since the changes to resolve above two comments might have some impacts on the existing clientv3 and etcdctl, so I prefer to resolving them in a separate PR. WDYT? @ptabor @serathius @spzala |
|
SGTM |
It's the first PR for issues/13624. I will raise separate PRs to add test (including unit test and
integration or e2etests) and update documentation.It follows the same workflow as the existing v2discovery.
Each etcd instance in the bootstrapped cluster communicates with the discovery service(anther etcd instance or cluster) using v3client. It's similar to etcdctl communicating with etcd server, so basically most of the global flags(such as --cert, --key, etc.) for etcdctl can be used here as well.
cc @serathius @ptabor @spzala