Add ensemble relocation command which adheres to placement policy #2931

equanz · 2021-12-07T05:11:32Z

Motivation

Currently, we can't recover the ensemble which isn't adhering to placement policy directly.
The recover command or autorecovery process is one of the alternative approaches. Unfortunately, however, we can't.

For example, suppose

the bookie cluster has two racks( /region/rack1 , /region/rack2 )
target ledger has the ensemble ( [bookie1(/region/rack1), bookie2(/region/rack1)] )
the replicator uses RackawareEnsemblePlacementPolicy

When bookie1 or bookie2 goes down, then run autorecovery process.
If the bookie cluster has remained bookies which place to /region/rack1, the ledger recovers to the /region/rack1 bookie again.
https://github.com/apache/bookkeeper/blob/release-4.14.4/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java#L225-L246
https://github.com/apache/bookkeeper/blob/release-4.14.4/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java#L1055-L1103
https://github.com/apache/bookkeeper/blob/release-4.14.4/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L474-L481

Therefore, we can't recover the ensemble which isn't adhering to placement policy directly.
The recover command is similar to this behavior.

I want to add a relocation feature to adhere placement policy.

Changes

Add relocation feature to shell command.
So, we can use it synchronously instead of executing asynchronously like the autorecovery process.

In this specification, optimize the new ensemble arrangement to minify the number of bookies replaced.
Therefore, we should implement the new method that returns the new ensemble by policies.
First, I introduce this feature to RackawareEnsemblePlacementPolicy in this PR.

eolivelli

This is a great work!

I did a first pass, it is a big patch.
So we'll need more eyes

@Ghatage @dlg99 @merlimat @ivankelly

eolivelli · 2022-03-10T10:22:55Z

...-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java

+        if (!excludeBookies.contains(currentNode) && predicate.apply(currentNode, ensemble)) {
+            if (ensemble.addNode(currentNode)) {
+                // add the candidate to exclude set
+                excludeBookies.add(currentNode);


we should not modify the excludeBookies Set passed as argument, please create a copy in the beginning of the method
otherwise this method will have unwanted side effects

modify the excludeBookies to notify exclude bookie to called method like below.
https://github.com/apache/bookkeeper/blob/release-4.14.4/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L671-L675

However, as you said, it might have unwanted side effects. Therefore, I'll modify to put to excludeBookies at called method.

diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java index d9167c321..fa3e59cd6 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java @@ -1131,6 +1131,13 @@ public class RackawareEnsemblePlacementPolicyImpl extends TopologyAwareEnsembleP prevNode = replaceToAdherePlacementPolicyInternal( curRack, excludeNodes, ensemble, ensemble, provisionalEnsembleNodes, i, ensembleSize, minNumRacksPerWriteQuorumForThisEnsemble); + // got a good candidate + if (ensemble.addNode(prevNode)) { + // add the candidate to exclude set + excludeNodes.add(prevNode); + } else { + throw new BKNotEnoughBookiesException(); + } // replace to newer node provisionalEnsembleNodes.set(i, prevNode); } catch (BKNotEnoughBookiesException e) { @@ -1159,10 +1166,6 @@ public class RackawareEnsemblePlacementPolicyImpl extends TopologyAwareEnsembleP final BookieNode currentNode = provisionalEnsembleNodes.get(ensembleIndex); // if the current bookie could be applied to the ensemble, apply it to minify the number of bookies replaced if (!excludeBookies.contains(currentNode) && predicate.apply(currentNode, ensemble)) { - if (ensemble.addNode(currentNode)) { - // add the candidate to exclude set - excludeBookies.add(currentNode); - } return currentNode; } @@ -1234,11 +1237,6 @@ public class RackawareEnsemblePlacementPolicyImpl extends TopologyAwareEnsembleP continue; } BookieNode bn = (BookieNode) n; - // got a good candidate - if (ensemble.addNode(bn)) { - // add the candidate to exclude set - excludeBookies.add(bn); - } return bn; } }

eolivelli · 2022-03-10T10:24:14Z

...eeper-server/src/test/java/org/apache/bookkeeper/client/CorrectEnsemblePlacementCmdTest.java

+        writableBookies.add(addr9.toBookieId());
+
+        // add bookie node to resolver
+        StaticDNSResolver.reset();


we should do this also in a "After" method, in order to ensure that we do not pollute the test environment

equanz · 2022-03-23T09:09:16Z

I've created the issue about the failing OWASP Dependency Check.
#3134

…olicyInternal

equanz · 2022-03-30T11:19:53Z

@eolivelli Addressed your first comments. PTAL.
(Also rebase to current master to follow #3140 and #3149 .)

eolivelli · 2022-05-19T05:44:22Z

...n/java/org/apache/bookkeeper/tools/cli/commands/bookies/CorrectEnsemblePlacementCommand.java

+        final LedgerUnderreplicationManager lum = lmf.newLedgerUnderreplicationManager();
+
+        final List<Long> targetLedgers =
+                flags.ledgerIds.stream().parallel().distinct().filter(ledgerId -> {


What's the reason for 'parallel'?
This loop is doing metadata operations, you can kill the zookeeper if you don't throttle properly the requests.

What's the reason for 'parallel'?

It was no strong reason.

This loop is doing metadata operations, you can kill the zookeeper if you don't throttle properly the requests.

I missed that point. I'll remove it.

eolivelli · 2022-05-19T05:44:58Z

...n/java/org/apache/bookkeeper/tools/cli/commands/bookies/CorrectEnsemblePlacementCommand.java

+        final NavigableSet<Pair<Long, Long>> failedTargets = new ConcurrentSkipListSet<>();
+        final CountDownLatch latch = new CountDownLatch(targetLedgers.size());
+        for (long ledgerId : targetLedgers) {
+            if (!flags.dryRun) {


What about logging the ledger id here?

eolivelli · 2022-05-19T05:46:30Z

...n/java/org/apache/bookkeeper/tools/cli/commands/bookies/CorrectEnsemblePlacementCommand.java

+                    continue;
+                }
+            }
+            admin.asyncOpenLedger(ledgerId, (rc, lh, ctx) -> {


We should move all this block into some BookKeeperAdmin method.
It will be easier to write tests and also to use this feature programmatically

equanz · 2022-05-24T06:48:35Z

rerun failure checks

equanz · 2022-05-24T08:20:44Z

@eolivelli Addressed your comments. PTAL.

equanz · 2022-07-13T04:11:40Z

@eolivelli ping

dlg99 · 2022-07-20T17:31:17Z

I have rather generic comments.

If the bookie cluster has remained bookies which place to /region/rack1, the ledger recovers to the /region/rack1 bookie again.

As I understand, there is enforceMinNumRacksPerWriteQuorum option to prevent that (and enforceMinNumFaultDomainsForWrite, enforceStrictZoneawarePlacement for other placement policies)

As I understand, Auditor's placementPolicyCheck just detects the problem. Maybe it makes sense to make Auditor (optionally) put the ledgers with bad placement for re-replication and make AutoRecovery handle that?
In this case the CLI command is already existing triggeraudit.

Another note is that the test only covers rack-aware policy. What happens in case of the region-aware or zone-aware policies?

dlg99 · 2022-07-20T18:28:34Z

Also: #3359

equanz · 2022-07-21T08:43:54Z

Also: #3359

Thank you for sharing. I'll check it first.

horizonzy · 2022-08-04T13:16:42Z

...eper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicy.java

    }

+    @Override
+    public PlacementResult<List<BookieId>> replaceToAdherePlacementPolicy(


In some case, the replace policy will replace more bookie. There are two racks.(rack1, rack2).
rack1: bookie1,bookie2,bookie3,bookie4
rack2: bookie5,bookie6,bookie7,bookie8
E:6 WQ:2 AQ:2. The ensemble is (5,6,1,7,2,8). (rack2,rack2,rack1,rack2,rack1,rack2), only replace the first bookie5 to bookie3, it adhere the placement policy. But now the implements didn't replace the firstly, it will replace from second element(bookie6), it will replace more bookie.

You're right. We can't "minimize" replacement bookies in some cases by the current approach.

Maybe we can list the kinds of replace result, and pick the "minimize" result. We can pick the different start index to replace. such as start with index0, start with index1, .... Finally, we pick the "minimize" replace result.

Maybe we can list the kinds of replace result, and pick the "minimize" result.

Yeah.
I think, alternatively, if the minimization is too costly for calculation, reduce as much as possible in a realistic amount of time like the current approach.

horizonzy

I notice that the implements didn’t handle the bookie which already shutdown. The shutdown bookie will be as the defaultRack, can we replace it?

equanz · 2022-08-05T02:00:32Z

@dlg99 #2931 (comment)

Sorry for the late reply. I've checked #3359 (review) .
The approach (using autorecovery) sounds better. If it was merged and still has the above issue, I'll rebase to follow #3359 interfaces.

As I understand, there is enforceMinNumRacksPerWriteQuorum option to prevent that (and enforceMinNumFaultDomainsForWrite, enforceStrictZoneawarePlacement for other placement policies)

In my understanding, if we use enforceMinNumRacksPerWriteQuorum , we can't recover quorum temporally for redundancy when a rack goes down. I want to avoid it.

As I understand, Auditor's placementPolicyCheck just detects the problem. Maybe it makes sense to make Auditor (optionally) put the ledgers with bad placement for re-replication and make AutoRecovery handle that?
In this case the CLI command is already existing triggeraudit.

I think so too.
I thought the next step is asynchronous relocation, like the auto-recovery feature (mentioned in here). However, it was implemented in #3359 .

Another note is that the test only covers rack-aware policy. What happens in the case of the region-aware or zone-aware policies?

When using non-Rackaware, it throws an Exception. I'll add the test.
https://github.com/equanz/bookkeeper/blob/62970b3122fa1f8266763f0d3f6115a066fcabe9/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/EnsemblePlacementPolicy.java#L460-L467

horizonzy · 2022-08-05T03:06:29Z

I want to use the EnsemblePlacementPolicy#replaceToAdherePlacementPolicy in #3359, it's more graceful, and I will enhance it in some cases.

equanz · 2022-08-05T05:00:44Z

@horizonzy

I notice that the implements didn’t handle the bookie which already shutdown. The shutdown bookie will be as the defaultRack, can we replace it?

Currently, maybe, we can't do it. This feature expects all of the bookies were already recovered to a normal state.
However, your point is correct.

I want to use the EnsemblePlacementPolicy#replaceToAdherePlacementPolicy in #3359, it's more graceful, and I will enhance it in some cases.

It's okay.
Just confirm, do you mean "bring this feature to #3359 and improve it myself"?
Or improve it in this PR?

horizonzy · 2022-08-05T05:28:14Z

Improve it both #3359 and this pr. I think the improvement is common.

equanz · 2022-08-05T09:39:58Z

I notice that the implements didn’t handle the bookie which already shutdown. The shutdown bookie will be as the defaultRack, can we replace it?

Currently, maybe, we can't do it. This feature expects all of the bookies were already recovered to a normal state.
However, your point is correct.

I've checked the current code and found handing. If the enforceMinNumRacksPerWriteQuorum is true and the default rack name is /default-region/default-rack then handle in below.
https://github.com/equanz/bookkeeper/blob/62970b3122fa1f8266763f0d3f6115a066fcabe9/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L1089-L1090

If the default rack name is /default-rack, then we can't add it.
https://github.com/equanz/bookkeeper/blob/62970b3122fa1f8266763f0d3f6115a066fcabe9/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L138
https://github.com/equanz/bookkeeper/blob/62970b3122fa1f8266763f0d3f6115a066fcabe9/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/NetworkTopologyImpl.java#L416-L420

However, it is not expected of me. I'll modify to add to excludeNodes regardless of enforceMinNumRacksPerWriteQuorum value.

Moreover, EnsemblePlacementPolicy#isEnsembleAdheringToPlacementPolicy has a similar process like below.
https://github.com/equanz/bookkeeper/blob/62970b3122fa1f8266763f0d3f6115a066fcabe9/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L1043

horizonzy · 2022-08-05T10:10:08Z

I notice that the implements didn’t handle the bookie which already shutdown. The shutdown bookie will be as the defaultRack, can we replace it?

If the default rack name is /default-rack, then we can't add it.
https://github.com/equanz/bookkeeper/blob/62970b3122fa1f8266763f0d3f6115a066fcabe9/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/NetworkTopologyImpl.java#L416-L420

I means the defaultRack is not literal, I means the defaultRack is /default-region/default-rack. 😂

horizonzy · 2022-08-05T10:11:44Z

However, it is not expected of me. I'll modify to add to excludeNodes regardless of enforceMinNumRacksPerWriteQuorum value.

Yes, If we already knows which bookie shutdown, I can add it to excludeNodes to ignore it.

equanz · 2022-08-05T11:10:09Z

In my understanding, currently, defaultRack of Rackaware is /default-rack. So we should consider about it if use it.
https://github.com/equanz/bookkeeper/blob/62970b3122fa1f8266763f0d3f6115a066fcabe9/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L138
https://github.com/equanz/bookkeeper/blob/62970b3122fa1f8266763f0d3f6115a066fcabe9/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L234-L248

However, it is not expected of me. I'll modify to add to excludeNodes regardless of enforceMinNumRacksPerWriteQuorum value.

Yes, If we already knows which bookie shutdown, I can add it to excludeNodes to ignore it.

Could you tell me more about shutdown? I think the resolver doesn't return /default-region/default-rack just call TopologyAwareEnsemblePlacementPolicy#handleBookiesThatJoined.

horizonzy · 2022-08-06T10:14:24Z

In my understanding, currently, defaultRack of Rackaware is /default-rack. So we should consider about it if use it.

Yes, you are right. If the test case, we changed the default rack to /defualt-region/default-rack.

repp.withDefaultRack(NetworkTopology.DEFAULT_REGION_AND_RACK);

If we didn't change it, the default rack is /default-rack.

StevenLuMT · 2022-08-24T08:14:41Z

fix old workflow,please see #3455 for detail

equanz · 2023-04-05T01:44:11Z

Part of the code are ported to #3359 and it was merged.
So, I'll close this PR.

equanz force-pushed the add_relocate_placement_command_pr branch 2 times, most recently from cb41b1e to e3d3d00 Compare December 7, 2021 05:29

eolivelli requested changes Mar 10, 2022

View reviewed changes

equanz force-pushed the add_relocate_placement_command_pr branch from e3d3d00 to 6bd16cf Compare March 17, 2022 07:57

equanz force-pushed the add_relocate_placement_command_pr branch from 6bd16cf to 5dc71e2 Compare March 28, 2022 04:54

equanz added 3 commits March 30, 2022 17:59

feat: add relocate placement command

151171b

test: add test for relocate placement feature

4ab4f85

refactor: modify addNode call to outside of replaceToAdherePlacementP…

e6e4deb

…olicyInternal

equanz force-pushed the add_relocate_placement_command_pr branch from 5dc71e2 to e6e4deb Compare March 30, 2022 09:00

equanz requested a review from eolivelli April 21, 2022 04:59

eolivelli requested changes May 19, 2022

View reviewed changes

equanz force-pushed the add_relocate_placement_command_pr branch from d49826f to 1555e54 Compare May 24, 2022 06:03

fix: move relocation procecure to BookKeeperAdmin

62970b3

equanz force-pushed the add_relocate_placement_command_pr branch from 1555e54 to 62970b3 Compare May 24, 2022 06:04

equanz requested a review from eolivelli July 13, 2022 04:11

hangc0276 requested review from Ghatage, dlg99, hangc0276, hezhangjian, ivankelly, merlimat and nicoloboschi July 25, 2022 01:56

hangc0276 requested a review from zymap July 25, 2022 01:56

hangc0276 assigned equanz Jul 25, 2022

hangc0276 added type/feature area/autorecovery labels Jul 25, 2022

hangc0276 modified the milestones: 4.16.0, 4.17.0 Jul 25, 2022

equanz mentioned this pull request Aug 4, 2022

Feature: auto recover support repaired not adhering placement ledger #3359

Merged

horizonzy reviewed Aug 4, 2022

View reviewed changes

ivankelly removed their request for review March 28, 2023 08:37

equanz closed this Apr 5, 2023

equanz deleted the add_relocate_placement_command_pr branch April 5, 2023 01:44

Add ensemble relocation command which adheres to placement policy #2931

Add ensemble relocation command which adheres to placement policy #2931

Uh oh!

Conversation

equanz commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equanz commented Mar 23, 2022

Uh oh!

equanz commented Mar 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equanz commented May 24, 2022

Uh oh!

equanz commented May 24, 2022

Uh oh!

equanz commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlg99 commented Jul 20, 2022

Uh oh!

dlg99 commented Jul 20, 2022

Uh oh!

equanz commented Jul 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equanz Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

equanz Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

horizonzy left a comment

Choose a reason for hiding this comment

Uh oh!

equanz commented Aug 5, 2022

Uh oh!

horizonzy commented Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

equanz commented Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horizonzy commented Aug 5, 2022

Uh oh!

equanz commented Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horizonzy commented Aug 5, 2022

Uh oh!

horizonzy commented Aug 5, 2022

Uh oh!

equanz commented Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

horizonzy commented Aug 6, 2022

Uh oh!

equanz commented Dec 7, 2021 •

edited

Loading

equanz commented Jul 13, 2022 •

edited

Loading

equanz Aug 4, 2022 •

edited

Loading

equanz Aug 5, 2022 •

edited

Loading

horizonzy commented Aug 5, 2022 •

edited

Loading

equanz commented Aug 5, 2022 •

edited

Loading

equanz commented Aug 5, 2022 •

edited

Loading

equanz commented Aug 5, 2022 •

edited

Loading