Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs #876

Closed
wants to merge 1 commit into from
Closed

Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs #876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions bookkeeper-server/conf/bk_server.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ journalDirectory=/tmp/bk-txn
# log files are deleted after GC.
# isForceGCAllowWhenNoSpace=false

# True if the bookie should double check readMetadata prior to gc
# verifyMetadataOnGC=false

#############################################################################
## TLS settings
#############################################################################
Expand Down
55 changes: 35 additions & 20 deletions ...per-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@
import java.util.NavigableSet;
import java.util.Set;
import java.util.SortedMap;
import java.util.TreeSet;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Semaphore;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.bookkeeper.client.BKException;
import org.apache.bookkeeper.client.LedgerMetadata;
import org.apache.bookkeeper.conf.ServerConfiguration;
Expand Down Expand Up @@ -76,6 +78,7 @@ public class ScanAndCompareGarbageCollector implements GarbageCollector{
private final long gcOverReplicatedLedgerIntervalMillis;
private long lastOverReplicatedLedgerGcTimeMillis;
private final String zkLedgersRootPath;
private final boolean verifyMetadataOnGc;

public ScanAndCompareGarbageCollector(LedgerManager ledgerManager, CompactableLedgerStorage ledgerStorage,
ServerConfiguration conf) throws IOException {
Expand All @@ -91,6 +94,7 @@ public ScanAndCompareGarbageCollector(LedgerManager ledgerManager, CompactableLe
this.zkLedgersRootPath = conf.getZkLedgersRootPath();
LOG.info("Over Replicated Ledger Deletion : enabled=" + enableGcOverReplicatedLedger + ", interval="
+ gcOverReplicatedLedgerIntervalMillis);
verifyMetadataOnGc = conf.getVerifyMetadataOnGC();
}

@Override
Expand All @@ -106,18 +110,6 @@ public void gc(GarbageCleaner garbageCleaner) {
NavigableSet<Long> bkActiveLedgers = Sets.newTreeSet(ledgerStorage.getActiveLedgersInRange(0,
Long.MAX_VALUE));

// Iterate over all the ledger on the metadata store
LedgerRangeIterator ledgerRangeIterator = ledgerManager.getLedgerRanges();

if (!ledgerRangeIterator.hasNext()) {
// Empty global active ledgers, need to remove all local active ledgers.
for (long ledgerId : bkActiveLedgers) {
garbageCleaner.clean(ledgerId);
}
}

long lastEnd = -1;

long curTime = MathUtils.now();
boolean checkOverreplicatedLedgers = (enableGcOverReplicatedLedger && curTime
- lastOverReplicatedLedgerGcTimeMillis > gcOverReplicatedLedgerIntervalMillis);
Expand All @@ -134,27 +126,50 @@ public void gc(GarbageCleaner garbageCleaner) {
lastOverReplicatedLedgerGcTimeMillis = MathUtils.now();
}

while (ledgerRangeIterator.hasNext()) {
LedgerRange lRange = ledgerRangeIterator.next();

Long start = lastEnd + 1;
Long end = lRange.end();
if (!ledgerRangeIterator.hasNext()) {
// Iterate over all the ledger on the metadata store
LedgerRangeIterator ledgerRangeIterator = ledgerManager.getLedgerRanges();
Set<Long> ledgersInMetadata = null;
long start;
long end = -1;
boolean done = false;
while (!done) {
start = end + 1;
if (ledgerRangeIterator.hasNext()) {
LedgerRange lRange = ledgerRangeIterator.next();
ledgersInMetadata = lRange.getLedgers();
end = lRange.end();
} else {
ledgersInMetadata = new TreeSet<>();
end = Long.MAX_VALUE;
done = true;
}

Iterable<Long> subBkActiveLedgers = bkActiveLedgers.subSet(start, true, end, true);

Set<Long> ledgersInMetadata = lRange.getLedgers();
if (LOG.isDebugEnabled()) {
LOG.debug("Active in metadata {}, Active in bookie {}", ledgersInMetadata, subBkActiveLedgers);
}
for (Long bkLid : subBkActiveLedgers) {
if (!ledgersInMetadata.contains(bkLid)) {
if (verifyMetadataOnGc) {
CountDownLatch latch = new CountDownLatch(1);
final AtomicInteger metaRC = new AtomicInteger(0);
ledgerManager.readLedgerMetadata(bkLid, (int rc, LedgerMetadata x) -> {
metaRC.set(rc);
latch.countDown();
});
latch.await();
if (metaRC.get() != BKException.Code.NoSuchLedgerExistsException) {
LOG.warn(
"Ledger {} Missing in metadata list, but ledgerManager returned rc: {}.",
bkLid,
metaRC.get());
continue;
}
}
garbageCleaner.clean(bkLid);
}
}
lastEnd = end;
}
} catch (Throwable t) {
// ignore exception, collecting garbage next time
Expand Down
20 changes: 20 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public class ServerConfiguration extends AbstractConfiguration<ServerConfigurati
protected static final String IS_FORCE_GC_ALLOW_WHEN_NO_SPACE = "isForceGCAllowWhenNoSpace";
protected static final String GC_OVERREPLICATED_LEDGER_WAIT_TIME = "gcOverreplicatedLedgerWaitTime";
protected static final String USE_TRANSACTIONAL_COMPACTION = "useTransactionalCompaction";
protected static final String VERIFY_METADATA_ON_GC = "verifyMetadataOnGC";
// Sync Parameters
protected static final String FLUSH_INTERVAL = "flushInterval";
protected static final String FLUSH_ENTRYLOG_INTERVAL_BYTES = "flushEntrylogBytes";
Expand Down Expand Up @@ -309,6 +310,25 @@ public ServerConfiguration setUseTransactionalCompaction(boolean useTransactiona
return this;
}

/**
* Get whether the bookie is configured to double check prior to gc.
*
* @return use transactional compaction
*/
public boolean getVerifyMetadataOnGC() {
return this.getBoolean(VERIFY_METADATA_ON_GC, false);
}

/**
* Set whether the bookie is configured to double check prior to gc.
* @param verifyMetadataOnGC
* @return server configuration
*/
public ServerConfiguration setVerifyMetadataOnGc(boolean verifyMetadataOnGC) {
this.setProperty(VERIFY_METADATA_ON_GC, verifyMetadataOnGC);
return this;
}

/**
* Get flush interval. Default value is 10 second. It isn't useful to decrease
* this value, since ledger storage only checkpoints when an entry logger file
Expand Down
226 changes: 225 additions & 1 deletion bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import java.util.TreeSet;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.bookkeeper.bookie.BookieException;
import org.apache.bookkeeper.bookie.CheckpointSource;
Expand Down Expand Up @@ -139,7 +140,7 @@ public void operationComplete(int rc2, Void result) {
}
});
assertTrue(latch.await(10, TimeUnit.SECONDS));
assertEquals("Remove should have succeeded", 0, rc.get());
assertEquals("Remove should have succeeded for ledgerId: " + ledgerId, 0, rc.get());
}

@Test
Expand Down Expand Up @@ -315,6 +316,229 @@ public void clean(long ledgerId) {
assertEquals("Should have cleaned first ledger" + first, (long) first, (long) cleaned.get(0));
}

/*
* in this scenario no ledger is created, so ledgeriterator's hasNext call would return false and next would be
* null. GarbageCollector.gc is expected to behave normally
*/
@Test
public void testGcLedgersWithNoLedgers() throws Exception {
final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
final List<Long> cleaned = new ArrayList<Long>();

// no ledger created

final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(getLedgerManager(),
new MockLedgerStorage(), baseConf);
AtomicBoolean cleanerCalled = new AtomicBoolean(false);

GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
@Override
public void clean(long ledgerId) {
LOG.info("Cleaned {}", ledgerId);
cleanerCalled.set(true);
}
};

validateLedgerRangeIterator(createdLedgers);

garbageCollector.gc(cleaner);
assertFalse("Should have cleaned nothing, since no ledger is created", cleanerCalled.get());
}

// in this scenario all the created ledgers are in one single ledger range.
@Test
public void testGcLedgersWithLedgersInSameLedgerRange() throws Exception {
baseConf.setVerifyMetadataOnGc(true);
final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
final SortedSet<Long> cleaned = Collections.synchronizedSortedSet(new TreeSet<Long>());

// Create few ledgers which span over just one ledger range in the hierarchical ledger manager implementation
final int numLedgers = 5;

createLedgers(numLedgers, createdLedgers);

final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(getLedgerManager(),
new MockLedgerStorage(), baseConf);
GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
@Override
public void clean(long ledgerId) {
LOG.info("Cleaned {}", ledgerId);
cleaned.add(ledgerId);
}
};

validateLedgerRangeIterator(createdLedgers);

garbageCollector.gc(cleaner);
assertTrue("Should have cleaned nothing", cleaned.isEmpty());

for (long ledgerId : createdLedgers) {
removeLedger(ledgerId);
}

garbageCollector.gc(cleaner);
assertEquals("Should have cleaned all the created ledgers", createdLedgers, cleaned);
}

/*
* in this test scenario no created ledger is deleted, but ledgeriterator is screwed up and returns hasNext to be
* false and next to be null. So even in this case it is expected not to clean any ledger's data.
*
* This testcase is needed for validating fix of bug - W-4292747.
*
* ScanAndCompareGarbageCollector/GC should clean data of ledger only if both the LedgerManager.getLedgerRanges says
* that ledger is not existing and also ledgerManager.readLedgerMetadata fails with error
* NoSuchLedgerExistsException.
*
*/
@Test
public void testGcLedgersIfLedgerManagerIteratorFails() throws Exception {
baseConf.setVerifyMetadataOnGc(true);
final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
final SortedSet<Long> cleaned = Collections.synchronizedSortedSet(new TreeSet<Long>());

// Create few ledgers
final int numLedgers = 5;

createLedgers(numLedgers, createdLedgers);

LedgerManager mockLedgerManager = new CleanupLedgerManager(getLedgerManager()) {
@Override
public LedgerRangeIterator getLedgerRanges() {
return new LedgerRangeIterator() {
@Override
public LedgerRange next() throws IOException {
return null;
}

@Override
public boolean hasNext() throws IOException {
return false;
}
};
}
};

final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(mockLedgerManager,
new MockLedgerStorage(), baseConf);
GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
@Override
public void clean(long ledgerId) {
LOG.info("Cleaned {}", ledgerId);
cleaned.add(ledgerId);
}
};

validateLedgerRangeIterator(createdLedgers);

garbageCollector.gc(cleaner);
assertTrue("Should have cleaned nothing", cleaned.isEmpty());
}

/*
* In this test scenario no ledger is deleted, but LedgerManager.readLedgerMetadata says there is NoSuchLedger. So
* even in that case, GarbageCollector.gc shouldn't delete ledgers data.
*
* Consider the possible scenario - when the LedgerIterator is created that ledger is not deleted, so as per
* LedgerIterator that is live ledger. But right after the LedgerIterator creation that ledger is deleted, so
* readLedgerMetadata call would return NoSuchLedger. In this testscenario we are validating that as per Iterator if
* that ledger is alive though currently that ledger is deleted, we should not clean data of that ledger.
*
* ScanAndCompareGarbageCollector/GC should clean data of ledger only if both the LedgerManager.getLedgerRanges says
* that ledger is not existing and also ledgerManager.readLedgerMetadata fails with error
* NoSuchLedgerExistsException.
*
*/
@Test
public void testGcLedgersIfReadLedgerMetadataSaysNoSuchLedger() throws Exception {
final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
final SortedSet<Long> cleaned = Collections.synchronizedSortedSet(new TreeSet<Long>());

// Create few ledgers
final int numLedgers = 5;

createLedgers(numLedgers, createdLedgers);

LedgerManager mockLedgerManager = new CleanupLedgerManager(getLedgerManager()) {
@Override
public void readLedgerMetadata(long ledgerId, GenericCallback<LedgerMetadata> readCb) {
readCb.operationComplete(BKException.Code.NoSuchLedgerExistsException, null);
}
};

final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(mockLedgerManager,
new MockLedgerStorage(), baseConf);
GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
@Override
public void clean(long ledgerId) {
LOG.info("Cleaned {}", ledgerId);
cleaned.add(ledgerId);
}
};

validateLedgerRangeIterator(createdLedgers);

garbageCollector.gc(cleaner);
assertTrue("Should have cleaned nothing", cleaned.isEmpty());
}

/*
* In this test scenario all the created ledgers are deleted, but LedgerManager.readLedgerMetadata fails with
* ZKException. So even in this case, GarbageCollector.gc shouldn't delete ledgers data.
*
* ScanAndCompareGarbageCollector/GC should clean data of ledger only if both the LedgerManager.getLedgerRanges says
* that ledger is not existing and also ledgerManager.readLedgerMetadata fails with error
* NoSuchLedgerExistsException, but is shouldn't delete if the readLedgerMetadata fails with any other error.
*/
@Test
public void testGcLedgersIfReadLedgerMetadataFailsForDeletedLedgers() throws Exception {
baseConf.setVerifyMetadataOnGc(true);
final SortedSet<Long> createdLedgers = Collections.synchronizedSortedSet(new TreeSet<Long>());
final SortedSet<Long> cleaned = Collections.synchronizedSortedSet(new TreeSet<Long>());

// Create few ledgers
final int numLedgers = 5;

createLedgers(numLedgers, createdLedgers);

LedgerManager mockLedgerManager = new CleanupLedgerManager(getLedgerManager()) {
@Override
public void readLedgerMetadata(long ledgerId, GenericCallback<LedgerMetadata> readCb) {
readCb.operationComplete(BKException.Code.ZKException, null);
}
};

final GarbageCollector garbageCollector = new ScanAndCompareGarbageCollector(mockLedgerManager,
new MockLedgerStorage(), baseConf);
GarbageCollector.GarbageCleaner cleaner = new GarbageCollector.GarbageCleaner() {
@Override
public void clean(long ledgerId) {
LOG.info("Cleaned {}", ledgerId);
cleaned.add(ledgerId);
}
};

validateLedgerRangeIterator(createdLedgers);

for (long ledgerId : createdLedgers) {
removeLedger(ledgerId);
}

garbageCollector.gc(cleaner);
assertTrue("Should have cleaned nothing", cleaned.isEmpty());
}

public void validateLedgerRangeIterator(SortedSet<Long> createdLedgers) throws IOException {
SortedSet<Long> scannedLedgers = new TreeSet<Long>();
LedgerRangeIterator iterator = getLedgerManager().getLedgerRanges();
while (iterator.hasNext()) {
LedgerRange ledgerRange = iterator.next();
scannedLedgers.addAll(ledgerRange.getLedgers());
}

assertEquals(createdLedgers, scannedLedgers);
}

class MockLedgerStorage implements CompactableLedgerStorage {

@Override
Expand Down