Skip to content

Commit 2693220

Browse files
committed
MB-58135: Warmup: Ignore corrupted accessLog entries
When loading the access logs during warmup, any read errors (MutationLog::ReadException) are caught and cause that file to be skipped; however other errors due to logical corruption of the file are not. If an invalid magic is found in the access log entry (or similar logical error), then the thrown std::invalid_argument is not caught and will terminate the process as it is not caught at any higher levels. As such, change to catch any std::exception thrown. Change-Id: I7cbfdd000a18d1269c886b0636534f0b08561945 Reviewed-on: https://review.couchbase.org/c/kv_engine/+/199027 Tested-by: Build Bot <[email protected]> Reviewed-by: Paolo Cocchi <[email protected]> Well-Formed: Restriction Checker
1 parent 194b4ea commit 2693220

File tree

4 files changed

+118
-2
lines changed

4 files changed

+118
-2
lines changed

engines/ep/src/warmup.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1786,7 +1786,7 @@ void Warmup::loadingAccessLog(uint16_t shardId)
17861786
(size_t)-1) {
17871787
success = true;
17881788
}
1789-
} catch (MutationLog::ReadException &e) {
1789+
} catch (const std::exception& e) {
17901790
corruptAccessLog = true;
17911791
EP_LOG_WARN("Error reading warmup access log: {}", e.what());
17921792
}
@@ -1804,7 +1804,7 @@ void Warmup::loadingAccessLog(uint16_t shardId)
18041804
(size_t)-1) {
18051805
success = true;
18061806
}
1807-
} catch (MutationLog::ReadException &e) {
1807+
} catch (const std::exception& e) {
18081808
corruptAccessLog = true;
18091809
EP_LOG_WARN("Error reading old access log: {}", e.what());
18101810
}
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
* Copyright 2023-Present Couchbase, Inc.
3+
*
4+
* Use of this software is governed by the Business Source License included
5+
* in the file licenses/BSL-Couchbase.txt. As of the Change Date specified
6+
* in that file, in accordance with the Business Source License, use of this
7+
* software will be governed by the Apache License, Version 2.0, included in
8+
* the file licenses/APL2.txt.
9+
*/
10+
11+
#pragma once
12+
13+
#include "access_scanner.h"
14+
15+
/***
16+
* Test class to expose the behaviour needed to create an ItemAccessVisitor
17+
*/
18+
class MockAccessScanner : public AccessScanner {
19+
public:
20+
MockAccessScanner(KVBucket& _store,
21+
Configuration& conf,
22+
EPStats& st,
23+
double sleeptime = 0,
24+
bool useStartTime = false,
25+
bool completeBeforeShutdown = false)
26+
: AccessScanner(_store,
27+
conf,
28+
st,
29+
sleeptime,
30+
useStartTime,
31+
completeBeforeShutdown) {
32+
}
33+
34+
void public_createAndScheduleTask(const size_t shard) {
35+
return createAndScheduleTask(shard);
36+
}
37+
};
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/*
2+
* Copyright 2023-Present Couchbase, Inc.
3+
*
4+
* Use of this software is governed by the Business Source License included
5+
* in the file licenses/BSL-Couchbase.txt. As of the Change Date specified
6+
* in that file, in accordance with the Business Source License, use of this
7+
* software will be governed by the Apache License, Version 2.0, included in
8+
* the file licenses/APL2.txt.
9+
*/
10+
11+
#pragma once
12+
13+
#include "mutation_log.h"
14+
15+
class MockMutationLog : public MutationLog {
16+
public:
17+
using MutationLog::MutationLog;
18+
19+
/* NOLINTNEXTLINE(modernize-avoid-c-arrays) */
20+
std::unique_ptr<uint8_t[]>& public_getBlockBuffer() {
21+
return blockBuffer;
22+
}
23+
24+
size_t public_getBlockPos() {
25+
return blockPos;
26+
}
27+
};

engines/ep/tests/module_tests/evp_store_warmup_test.cc

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "../mock/mock_dcp_producer.h"
1414
#include "../mock/mock_ep_bucket.h"
1515
#include "../mock/mock_item_freq_decayer.h"
16+
#include "../mock/mock_mutation_log.h"
1617
#include "../mock/mock_kvstore.h"
1718
#include "../mock/mock_synchronous_ep_engine.h"
1819
#include "checkpoint_manager.h"
@@ -666,6 +667,57 @@ TEST_F(WarmupTest, MB_31450) {
666667
MB_31450(false);
667668
}
668669

670+
TEST_F(WarmupTest, MB_58135_CorruptAccessLog) {
671+
setVBucketStateAndRunPersistTask(vbid, vbucket_state_active);
672+
673+
auto key = makeStoredDocKey("key");
674+
for (int i = 0; i < 1; i++) {
675+
store_item(vbid, makeStoredDocKey("key" + std::to_string(i)), "value");
676+
}
677+
flush_vbucket_to_disk(vbid, 1);
678+
679+
// Create an access log for each shard (warmup only uses access logs if it
680+
// has a complete set). First one will be valid, second one will be
681+
// corrupted.
682+
ASSERT_EQ(2, engine->getWorkLoadPolicy().getNumShards());
683+
{
684+
MutationLog mlog("access_log.0");
685+
mlog.open();
686+
mlog.newItem(vbid, key);
687+
}
688+
{
689+
MockMutationLog mlog("access_log.1");
690+
mlog.open();
691+
mlog.newItem(vbid, key);
692+
bool modified = false;
693+
for (size_t i = 0; i < mlog.public_getBlockPos(); i++) {
694+
// Corrupt the magic
695+
auto& c = mlog.public_getBlockBuffer()[i];
696+
if (c == MutationLogEntryV3::MagicMarker) {
697+
c = 0xff;
698+
modified = true;
699+
}
700+
}
701+
ASSERT_TRUE(modified);
702+
}
703+
// Copy corrupted access log to .old so we check both
704+
// code paths (warmup will fallback to .old if the main one cannot be read).
705+
namespace fs = boost::filesystem;
706+
auto alog = fs::path("access_log.1");
707+
EXPECT_TRUE(fs::exists(alog));
708+
auto alog_old = fs::path("access_log.1.old");
709+
fs::remove(alog_old);
710+
fs::copy_file(alog, alog_old);
711+
EXPECT_TRUE(fs::exists(alog_old));
712+
713+
// Test: restart and warmup. Should not crash, warmup should succeed
714+
// without access log.
715+
resetEngineAndWarmup("alog_path=access_log");
716+
717+
auto* warmup = engine->getKVBucket()->getWarmup();
718+
EXPECT_EQ(WarmupState::State::Done, warmup->getWarmupState());
719+
}
720+
669721
// Test fixture for Durability-related Warmup tests.
670722
class DurabilityWarmupTest : public DurabilityKVBucketTest {
671723
protected:

0 commit comments

Comments
 (0)