ZOOKEEPER-3188: Improve resilience to network

[symat]

This PR is the rebase of the previous pull request, so all the kudos should go to the original authors...

In ZOOKEEPER-3188 we add ability to specify several addresses for quorum operations. Also added reconnection attempts if connection to leader lost.

In this PR I rebased the changes on the current master, resolving some minor conflicts with:

• ZOOKEEPER-3296: Explicitly closing the sslsocket when it failed handshake to prevent issue where peers cannot join quorum
• ZOOKEEPER-3320: Leader election port stop listen when hostname unresolvable for some time
• ZOOKEEPER-3385: Add admin command to display leader
• ZOOKEEPER-3386: Add admin command to display voting view
• ZOOKEEPER-3398: Learner.connectToLeader() may take too long to time-out

I still want to test the feature manually (e.g. using docker containers with multiple virtual networks / interfaces). The steps to the manual test could be recorded in the google docs as well.

Also I think we could add a few more unit tests where we are using multiple addresses. The current tests are using a single address only.

Also the Zookeeper documentation needs to be changed (e.g. by a follow-up Jira?) to promote the new feature and the new config format (possibly including also the admin command documentation in relation with ZOOKEEPER-3386 and ZOOKEEPER-3461)

[symat]

I created a simple docker config with multiple virtual networks and managed to test the situation when some of the containers loose the access to one of the virtual networks. I uploaded the docker related scripts / configs here: https://github.com/symat/zookeeper-docker-test

During these manual tests I found some situations when the previous patch didn't work.

• the InitialMessage sent during the leader election contained only a single election address. If this address was not reachable by the recipient of the InitialMessage, then the connection was never successfully initiated. I changed the format of the InitialMessage to send all the election addresses and the other side will use only the one which is reachable.
• When an existing tcp connection to an electionAddress is broken, the server will try to send notification messages re-using the existing SendWorker threads. I would assume that the SendWorker.send() method should die when it tries to flush the output stream on the socket which destination is already unreachable. However, for some reason it doesn't die. (this could be investigated further) Anyway, I added a small logic for the connection initiation to verify if the existing destination in SendWorker is still reachable. If the destination is unreachable in the SendWorker thread, then we can gracefully finish it and during the next connection attempt we will choose a destination what is reachable. (this part I fixed in a second commit)

With these modifications I was able to test the following situation successfully:

1. starting a zookeeper with nodes, each server listening on two addresses (on two separate virtual networks)
2. waiting the initial leader election to happen
3. removing the current leader from the virtual network that is used by the others as destination
4. it took a few seconds until all the servers recognised the loss of connections, and in 5-10 seconds the connections were re-established and the new leader election finished

I will think how to unittest these features. (or should we crate some docker-based automated integration test?)

In the mean while I would appreciate a deep review of these changes, as I am quite new in the Zookeeper code...

[symat]

retest maven build

[symat]

retest maven build

[anmolnar]

Overall looks good to me, see my comments below.

[anmolnar]

Please avoid using asterisk imports. This a common coding practice across ZooKeeper.

[symat]

sure, thanks!

[anmolnar]

I believe that we don't need this magic here which is trying to recreate the original config file contents. The output of admin command doesn't need to match with the configuration, so you can just dump the contents of the internal representation (map) which I think more helpful for anyway reading this output.

[symat]

Yes, I agree, I will change it.

This admin command is about to show the internal view of the voting members (how the zookeeper server thinks who the voting members are and where do they listen). I wouldn't complicate this PR any further, but it might be a good idea to create a follow-up ticket to have some admin command showing if the given server can actually reach all the different ports of the other servers (and not only the voting members). It can help debugging network problems, showing if certain network interfaces on some servers are unreachable.

[symat]

I changed the format, this is how the voting_view admin command responds now:

{
  "current_config" : {
    "1" : {
      "server_addresses" : [ "/172.16.101.11:2888", "/172.16.102.11:2888" ],
      "election_addresses" : [ "/172.16.101.11:3888", "/172.16.102.11:3888" ],
      "client_address" : "/0.0.0.0:2181",
      "learner_type" : "participant"
    },
    "2" : {
      "server_addresses" : [ "/172.16.101.22:2888", "/172.16.102.22:2888" ],
      "election_addresses" : [ "/172.16.101.22:3888", "/172.16.102.22:3888" ],
      "client_address" : "/0.0.0.0:2181",
      "learner_type" : "participant"
    },
    "3" : {
      "server_addresses" : [ "/172.16.101.33:2888", "/172.16.102.33:2888" ],
      "election_addresses" : [ "/172.16.101.33:3888", "/172.16.102.33:3888" ],
      "client_address" : "/0.0.0.0:2181",
      "learner_type" : "participant"
    }
  },
  "command" : "voting_view",
  "error" : null
}

[anmolnar]

Perfect!

[anmolnar]

Instead of self.getQuorumAddress(), you should print address, because that's the actual address that you're trying to bind.

[symat]

thanks!

[anmolnar]

As discussed offline, we probably need 2 things here:

• Increment PROTOCOL_VERSION, because the patch introduces new InitialMessage format,
• Use JSON format and add Jackson JSON serialiser to enable forward compatibility for future changes.

[symat]

referring to our second offline discussion:

• I incremented the PROTOCOL_VERSION
• did not do the JSON format modification, as it makes the future code complicated to always ensure both forward and backward compatibility of the election protocol during the rolling upgrades. The current solution (simply failing if the protocol versions mismatch) is more simple and still working just fine: as the servers are restarted one-by-one, the nodes with the old protocol version and the nodes with the new protocol version will form two partitions, but any given time only one partition will have the quorum.

[anmolnar]

@symat I think your 2 new bugfixes are correct, we might just want to improve the format of InitialMessage and convert it to JSON.

@tdunning PTAL.

[symat]

retest maven build

[anmolnar]

+1 lgtm

[enixon]

a couple of comments from a lazy scan

[enixon]

if learnerMaster is null here then you'll get a NPE on the learnerMaster.addr.getReachableOrOne() above.

[symat]

thanks, nice catch!
(actually beside this, we also got a NoSuchElementException if learnerMaster.addr is empty)
I will fix this in the next commit

[enixon]

should there be a timeout here?

[symat]

The code basically now starting listener threads on all local addresses, then waiting until all the listeners are dead, and then simply start over again (assuming no stop was requested by Leader.shutdown() or no unexpected failure happened in the Listener threads).

I don't think we need a timeout here... ideally we want to wait forever :)

[symat]

I merged with master (had a lot's of conflicts because of the checkstyle related changes), and also fixed the issue found by @enixon. I also did a manual test with docker before committing.

[symat]

I merged with master again, because of minor conflicts with ZOOKEEPER-3448: Introduce MessageTracker to assist debug leader and leaner connectivity issues.
I executed all unit tests and also did a manual test with docker before pushing the merge commit.

[hanm]

nice work on rebase and doing cluster level testing on the patch

a few things I'd like to discuss:

• What's the impact on dynamic reconfiguration - what's allowed / working and what's not allowed / not working when reconfiguring server's address now with this feature? Once this is concluded, some unit tests and cluster level testing would be good. I think @enixon also asked about this in Zookeeper-3188: Improve resilience to network #730.

• Impact on upgrading / downgrading: this was discussed on the JIRA, should be tested at cluster level and document it on upgrade FAQ wiki and/or zookeeper document.

• Impact on quorum TLS/kerberos feature: this was also discussed on JIRA, but not concluded yet.

• Unit test coverage: I think the current unit tests don't cover well the new code path when there is multiple addresses configured. We have quorum manager test, leader election tests, and so on; but those tests are still executed with a single address configured (even though the interfaces changed to multiple address). Would be good to upgrade relevant tests and make sure we have a relevant good coverage on networking code path with multiple addresses configured.

My current evaluation of the patch is the risk is fairly low if we merge this, as long as we still stick to single address configuration. Though I'd like to conclude the above discussions so we know what we will be getting into if something goes wrong.

[eolivelli]

I did a pass on the patch.
Great work, I left a few little comments

I feel we need to setup some kind of integration tests to ensure that a rolling upgrade of an existing cluster from at least 3.5 is working properly.

We must also add docs about this feature.

[eolivelli]

catching NullPointerException is a code and usually a bad practice.
please remove this case

catching InterruptedException requires to reset Thread#interrupt flag, please fix or explain

[symat]

Thanks, I agree... this one needs to be refactored. I will remove the atomic reference and do a simple parallel stream filtering in the getReachableAddress(). Please review after I push the new commit.

[eolivelli]

@symat why are you talking about parallelStream ? we are not talking about hundreds of addresses, using more that one thread won't help and maybe it could be more expensive

[symat]

well, we are using parallel streams only in two places, where IO operations happen (one for hostname resolution, one for checking if the address is reachable). I agree that we don't have hundreds of addresses, but even with a few addresses, parallelizing can mean that we can find a reachable address for a remote Quorum Server a few seconds quicker.

I agree that it is not a huge deal... but it is a trivial optimization in the code and I don't think it would be unsafe in any ways.

I cleaned up this part in my last commit. If you still think it is unnecessary and would be better to use simple streams, then we can go for it. (You are right that in the rare case of a network error that few seconds will not matter much)

[symat]

Thanks for all of you for the comments and review so far!

I extended a few exiting test classes (e.g. LearnerTest, CnxManagerTest) with multiple-address related cases, and added a new Test class (QuorumPeerMainMultiAddressTest) that contains tests for:

• starting servers with multiple addresses
• starting servers with some unreachable addresses
• testing dynamic reconfiguration (both adding or removing addresses; both incremental and non-incremental reconfigs)
• test all the above also with IPv6

An important change: due to the way how we represent the server configs during dynamic reconfig, I had to change the separator character from comma to pipe. So the valid config format now looks like:

server.1=127.0.0.1:1111:2222|some.host.com:3333:4444|other.host.com:5555:6666;7777

regarding the impact on dynamic reconfig:

• we have now a few basic tests for dynamic reconfig, based on these I consider no risks here at the moment @hanm: what do you think? are the tests in QuorumPeerMainMultiAddressTest enough for covering basic dynamic reconfig use-cases?
• Some remarks might be good to document regarding dynamic reconfig and multi-address:
  • removing addresses / ports which are used currently can cause a new leader election
  • Also it is a good practice to make sure all the ZK servers are connected when we issue a dynamic reconfigure. (an offline ZK server might be unable to re-join to the quorum if the other servers reconfigure their ports / addresses)
  • I actually saw during the tests, that simply adding new addresses for servers with incremental re-config does not cause a new leader election (although I haven't created an automated test explicitly forcing this). I am not sure if it is worth to add a new test for this, as dynamic reconfigs are relatively rare events during production use I guess.

I try to summarize the open discussions / tasks to see how much work is required to close this issue:

• we should add the new config format to the documentation
• we should extend the dynamic reconfig documentation with the comments above
• we should test (for now I propose only to try out manually) and document:
  • the impact on quorum TLS / kerberos feature
  • the impact on rolling upgrades
• and of course we need to resolve any code related conversation what would be still open (e.g. on parallel streams with @eolivelli )

I think if the tasks above are concluded, then it is safe to merge the PR. What do you think?

The automated integration tests would be nice to have definitely. These test cases I have in mind:

• Automated integration test for rolling upgrades (e.g. from 3.5.6 to master) without changing the config
• Automated integration test for rolling upgrades (e.g. from 3.5.6 to master) with changing the config to use multiple addresses during the upgrade
• Automated integration test for simulating the case of network failure during the use of multiple addresses
  • hint: use docker with multiple network interfaces (see: https://github.com/symat/zookeeper-docker-test )
  • I am doing this test manually after each of my commits

I would cover these integration tests in follow-up jira ticket(s?), as these seems to be not trivial to automatize. What do you think?

[tdunning]

On Tue, Oct 8, 2019 at 6:49 AM Mate Szalay-Beko ***@***.***> wrote: *Thanks for all of you for the comments and review so far!*

Nice work to you as well.

... An important change: due to the way how we represent the server configs during dynamic reconfig, I had to change the separator character from comma to pipe.

This is the predictable penalty of an ad hoc syntax that isn't consistent everywhere. Good catch.

- - ...I actually saw during the tests, that simply adding new addresses for servers with incremental re-config does not cause a new leader election (although I haven't created an automated test explicitly forcing this). I am not sure if it is worth to add a new test for this, as dynamic reconfigs are relatively rare events during production use I guess. This feels nice, but it is actually a little worrisome in that it seems to

indicate that the multi connect stuff is more clever than I would expect.

*I try to summarize the open discussions / tasks* to see how much work is required to close this issue: - we should add the new config format to the documentation - we should extend the dynamic reconfig documentation with the comments above - we should test (for now I propose only to try out manually) and document: - the impact on quorum TLS / kerberos feature - the impact on rolling upgrades - and of course we need to resolve any code related conversation what would be still open (e.g. on parallel streams with @eolivelli <https://github.com/eolivelli> )

I think that this is a good summary.

*I think if the tasks above are concluded, then it is safe to merge the PR.* What do you think?

I am only half informed on this, but your summary seems persuasive.

The automated integration tests would be nice to have definitely. These test cases I have in mind: - Automated integration test for rolling upgrades (e.g. from 3.5.6 to master) without changing the config - Automated integration test for rolling upgrades (e.g. from 3.5.6 to master) with changing the config to use multiple addresses during the upgrade - Automated integration test for simulating the case of network failure during the use of multiple addresses - hint: use docker with multiple network interfaces (see: https://github.com/symat/zookeeper-docker-test ) - I am doing this test manually after each of my commits *I would cover these integration tests in follow-up jira ticket(s?), as these seems to be not trivial to automatize.* What do you think?

Without an advanced test framework that can create and control containers, I think that these kinds of tests are difficult to create and maintain. To me, the key question is how much value would these automated tests actually provide. I see very high value in a documented procedure and outcome for a manual version of these tests, but I am not sure I see a high value for even repeating these manual tests again. How likely is it that subsequent changes will cause these tests to fail? Is that probability high enough to justify the considerable cost of automation?

[eolivelli]

Sounds a great plan.

I am fine with parallelStream for dns resolutions.

For integration tests we have to do it in a separate ticket, not blocker for merging this patch

[anmolnar]

@symat I experienced something weird with this patch. Tried the following:
I'm having 2 network interfaces in my Mac: wifi + cable, connected with 2 different IPs. Created the following config:

server.1=172.30.64.161:3181:4181|172.30.65.130:3181:4181
server.2=172.30.64.161:3182:4182|172.30.65.130:3182:4182
server.3=172.30.64.161:3183:4183|172.30.65.130:3183:4183

At the beginning both interfaces were up. When I started ZK quorum, it connected successfully and quorum was up within a second. When I disabled the cable interface (pulling the cable out), the nodes started to communicate on the other interface.

After that I swapped the interfaces (cable plugged in, wifi disabled) and nodes went back to looking state and were not able to form a quorum again. Not even after a restart!

Maybe this is not a real life scenario which this patch should be prepared for, but I'm not sure.

[tdunning]

On Tue, Oct 8, 2019 at 8:45 AM Andor Molnár ***@***.***> wrote: *...*

@symat <https://github.com/symat> I experienced something weird with this

patch. Tried the following: I'm having 2 network interfaces in my Mac: wifi + cable, connected with 2 different IPs. Created the following config: server.1=172.30.64.161:3181:4181|172.30.65.130:3181:4181 server.2=172.30.64.161:3182:4182|172.30.65.130:3182:4182 server.3=172.30.64.161:3183:4183|172.30.65.130:3183:4183 At the beginning both interfaces were up. When I started ZK quorum, it connected successfully and quorum was up within a second. When I disabled the cable interface (pulling the cable out), the nodes started to communicate on the other interface. After that I swapped the interfaces (cable plugged in, wifi disabled) and nodes went back to looking state and were not able to form a quorum again. Not even after a restart! Maybe this is not a real life scenario which this patch should be prepared for, but I'm not sure.

This is absolutely a real-life scenario and it should work. If I were doing this, I would be suspicious that something about what I was seeing wasn't really working the way I thought. In the zeroth scenario (both interfaces live), how do you verify which link is being used? What traffic is observed on the wifi side even when it is ostensibly not used? In your second scenario (wifi-disabled), can you ssh from one node to another?

[anmolnar]

I think this patch is in a good shape currently. The issue we found (as said) not a big deal if a single node is failing and I believe it's better not to touch that part of the code just because of this patch.

@eolivelli Are you still holding your -1?

[eolivelli]

Still I can't find documentation for this patch.
I think it is better to commit is now that is it in good shape.

But @symat o @anmolnar can you schedule to update the docs.
It will be kind of blocker for the release, this is an important change and we must document it properly

[anmolnar]

Forgot to mention that @symat is updating the docs, so hopefully we'll have it soon.

[symat]

thanks @anmolnar and @eolivelli for checking the PR!

In the last few commits:

• I added unit tests to show that using multiple addresses works with QuorumSSL and also with Kerberos (and digest) authentication
• I fixed a small issue that caused multiple leader elections when the active ethernet interface was failing on the current leader (in this specific error case we have quicker recovery now)
• I also added documentation (added some explanation and examples for the Admin Guide and for the Dynamic Reconfig page)

I also think that it is safe to merge this PR now. Some parts of the recovery still can be optimized later, but I think it is stable now and also backward compatible.

[symat]

retest maven build

[asf-ci]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1664/

[anmolnar]

Merged to master. Thanks @symat and everybody!

[aishwaryasoni1991]

Can we backport the fix to 3.5.8? I am having some trouble with zookeeper 3.6.1 https://issues.apache.org/jira/browse/ZOOKEEPER-3828 And this fix is the only reason I need 3.6.1, for now.

[symat]

Sorry @aishwaryasoni1991, this is a major new feature (also causing some internal protocol changes), our policy is that we don't backport these with bugfix releases to make sure the 3.5 remains stable.

[eolivelli]

What problem you have on 3.6.1?
Please ask on user@zookkeeper.apache.org
We will try to help or fix your problem

[aishwaryasoni1991]

@symat @eolivelli Thank you for responding.
This is the issue I have reported (also experienced and voted by other community members)
https://issues.apache.org/jira/browse/ZOOKEEPER-3828
Initially, I requested for backporting as I only needed 3.6.1 for this fix and I didn't face any issues (like 3828) with 3.5.5. But as per the comments on the ticket, one member experienced the issue in 3.5.5. So now I am not sure how to proceed. ZOOKEEPER-3838 is actually a blocker for us.
I won't corrupt this ticket by commenting about some other ticket, so I will tag you guys on the ticket. Mate looked at it so he is a bit familiar with the issue.