Fix rack awareness cache expiration data race

[michaeljmarshall]

Motivation

The BookieRackAffinityMapping class relies on a metadata cache that expires entries after 10 minutes. When an entry expires, the next call to BookieRackAffinityMapping#getRack returns an incomplete future (because the entry expired) and the TopologyAwareEnsemblePlacementPolicy (bookkeeper class) stores the bookie's network location as default-rack.

It is trivial to reproduce the issue. Start a Pulsar cluster, define a rack topology, wait for at least 10 minutes, kill one of the bookies that is not in the default-rack, and observe the broker logs as the bookie comes back. The broker will log that the bookie is a member of the default-rack. When bookkeeperClientEnforceMinNumRacksPerWriteQuorum is enabled in the broker, this bug becomes a blocking issue where the only way to resolve the bad state is to restart the broker (or to restart the bookie assuming the broker still has the right mapping in the cache).

This PR changes the design of the BookieRackAffinityMapping by removing cache expiration. When the broker starts up, it will discover the mapping from zookeeper and store that mapping until the broker observes an update from a ZK watch.

Modifications

• Rely on an indefinitely cached rack mapping in BookieRackAffinityMapping, instead of relying on a metadata cache, which is defined to have an entry expiration.
• Eagerly resolve the bookie mapping. This was removed in Bugfix: Fix rackaware placement policy init error #12097, but now that fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver bookkeeper#2788 is merged and available in the bookkeeper client, we can safely resolve the addresses early.
• Add synchronized keyword to all relevant methods that modify mutable state from multiple threads. Based on my reading of the code, there is not a risk for deadlock with this change. Making these methods synchronized also prevents certain races that could negatively affect bookie network location resolution. The only potential problem is that this synchronization could block a zk callback thread briefly. Because the operations in these methods do not contain any blocking io (other than on initialization), I view blocking a zk thread as unlikely.
• Remove the volatile keyword for two maps that are now only updated within synchronized blocks.
• Move the registerListener call to before getting the value from zookeeper. This ensures that an update is not missed in the very short time between getting the value and registering the listener. Because the method is synchronized, the event will properly be observed after the original initialization.
• Update a test to use Awaitility to account for the asynchronous nature of metadata store notifications.
• Move the rackawarePolicy null check to later in the sequence to make tests pass. Note that we always use a rackawarePolicy, so this is a trivial change.

Verifying this change

This change is covered by existing tests. Note that the original bug is challenging to reproduce in a unit test because the bug relies on cache expiration, which is hard coded at 10 minutes in the MetadataCacheImpl. By removing any chance for cache expiration, we remove the possibility for this bug.

Additional Context

Here are sample logs from a reproduction of the issue:

2022-07-27T15:20:55,352+0000 [main-EventThread] INFO  org.apache.bookkeeper.net.NetworkTopologyImpl - Removing a node: /az1/pulsar-bookkeeper-3.pulsar-bookkeeper.michael-test.svc.cluster.local:3181
2022-07-27T15:20:55,353+0000 [main-EventThread] INFO  org.apache.bookkeeper.net.NetworkTopologyImpl - Removing a node: /az1/pulsar-bookkeeper-3.pulsar-bookkeeper.michael-test.svc.cluster.local:3181
2022-07-27T15:20:59,310+0000 [main-EventThread] WARN  org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy - Failed to resolve network location for pulsar-bookkeeper-3.pulsar-bookkeeper.michael-test.svc.cluster.local, using default rack for it : /default-rack.
2022-07-27T15:20:59,310+0000 [main-EventThread] INFO  org.apache.bookkeeper.net.NetworkTopologyImpl - Adding a new node: /default-rack/pulsar-bookkeeper-3.pulsar-bookkeeper.michael-test.svc.cluster.local:3181
2022-07-27T15:20:59,311+0000 [main-EventThread] WARN  org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy - Failed to resolve network location for pulsar-bookkeeper-3.pulsar-bookkeeper.michael-test.svc.cluster.local, using default rack for it : /default-rack.
2022-07-27T15:20:59,311+0000 [main-EventThread] INFO  org.apache.bookkeeper.net.NetworkTopologyImpl - Adding a new node: /default-rack/pulsar-bookkeeper-3.pulsar-bookkeeper.michael-test.svc.cluster.local:3181

Alternative Solution

An alternative solution is to add a callback to the metadata store's result when the future is not complete. The callback would trigger the logic in the BookieRackAffinityMapping#handleUpdates. While this change would be smaller in terms of lines of code touched, I view it as suboptimal because it necessarily leads to misclassification of bookies as members of the default-rack, which is both confusing to users and could lead to temporary errors.

Does this pull request potentially affect one of the following parts:

This PR does not introduce any breaking changes. It might not easily get cherry picked to older release branches.

Documentation

• doc-not-needed

Docs are not needed because this is just an internal bug fix.

[michaeljmarshall]

@merlimat - one of the core assumptions for this PR is that zk notifications won't get missed. Initially, I thought that was a valid assumption, but now I am thinking that might not be true in the event that a zk connection is dropped and an update to the /bookies zk node is performed while the broker is disconnected. Will you take a look and let me know if my design is valid? Thanks.

[michaeljmarshall]

Given the documentation here, https://zookeeper.apache.org/doc/r3.8.0/zookeeperProgrammers.html, I think we're likely fine, though I'm not sure how a crashed zk will affect watches and changes made while disconnected.

Here is the relevant section from the above docs:

Watches are maintained locally at the ZooKeeper server to which the client is connected. This allows watches to be lightweight to set, maintain, and dispatch. When a client connects to a new server, the watch will be triggered for any session events. Watches will not be received while disconnected from a server. When a client reconnects, any previously registered watches will be reregistered and triggered if needed. In general this all occurs transparently. There is one case where a watch may be missed: a watch for the existence of a znode not yet created will be missed if the znode is created and deleted while disconnected.

[lhotari]

LGTM. Good catch @michaeljmarshall

[eolivelli]

LGTM

[BewareMyPower]

Could you cherry-pick this PR (or open an independent PR) to branch-2.8? It relies on #14708 but when I cherry-picked #14708, there were still many conflicts.

[michaeljmarshall]

@BewareMyPower - yes, I will take care of cherry picking this PR.

[mattisonchao]

Hi @michaeljmarshall
Could you help cherry-pick this PR to branch-2.9? thanks a lot!!!

[congbobo184]

Hi @michaeljmarshall
Could you help cherry-pick this PR to branch-2.9? thanks

[congbobo184]

@michaeljmarshall hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.

[hangc0276]

This issue is introduced by #12841, which was only released in branch-2.10+. Does this Pr only need to be cherry-picked to branch-2.10? @michaeljmarshall

[michaeljmarshall]

@hangc0276 - I haven't verified the other releases, but I'm not able to cherry pick it right now. We can probably just drop the older release lines since the conditions that lead to this bug are unlikely in any production system.