From eeb5d23759e51450abcfbc76e34beecd320ce166 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Tue, 19 Nov 2019 16:52:40 +0000 Subject: [PATCH 1/6] Add params to batched app --- src/apps/batched/batched.lua | 8 +++++--- tests/e2e_batched.py | 36 +++++++++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/apps/batched/batched.lua b/src/apps/batched/batched.lua index 2e462db734e1..8f5e13b9d142 100644 --- a/src/apps/batched/batched.lua +++ b/src/apps/batched/batched.lua @@ -15,10 +15,12 @@ return { BATCH_submit = [[ tables, gov_tables, args = ... count = 0 - for n, e in ipairs(args.params) do + for n, e in ipairs(args.params.entries) do id = e.id - msg = e.msg - tables.priv0:put(id, msg) + if id % args.params.write_key_divisor == 0 then + msg = string.rep(e.msg, args.params.write_size_multiplier) + tables.priv0:put(id, msg) + end count = count + 1 end return env.jsucc(count) diff --git a/tests/e2e_batched.py b/tests/e2e_batched.py index 19821378ff90..f6d8d91157b0 100644 --- a/tests/e2e_batched.py +++ b/tests/e2e_batched.py @@ -18,11 +18,13 @@ @reqs.supports_methods("BATCH_submit", "BATCH_fetch") -def test(network, args, batch_size=100): +def test(network, args, batch_size=100, write_key_divisor=1, write_size_multiplier=1): LOG.info(f"Running batch submission of {batch_size} new entries") primary, _ = network.find_primary() with primary.user_client() as c: + check = infra.checker.Checker() + message_ids = [next(id_gen) for _ in range(batch_size)] messages = [ {"id": i, "msg": f"A unique message: {md5(bytes(i)).hexdigest()}"} @@ -30,20 +32,26 @@ def test(network, args, batch_size=100): ] pre_submit = time.time() - submit_response = c.rpc("BATCH_submit", messages) + check( + c.rpc( + "BATCH_submit", + { + "entries": messages, + "write_key_divisor": write_key_divisor, + "write_size_multiplier": write_size_multiplier, + }, + ), + result=len(messages), + ) post_submit = time.time() LOG.warning( f"Submitting {batch_size} new keys took {post_submit - pre_submit}s" ) - assert submit_response.result == len(messages) fetch_response = c.rpc("BATCH_fetch", message_ids) - assert fetch_response.result is not None - assert len(fetch_response.result) == len(message_ids) - for n, m in enumerate(messages): - fetched = fetch_response.result[n] - assert m["id"] == fetched["id"] - assert m["msg"] == fetched["msg"] + + if write_key_divisor == 1 and write_size_multiplier == 1: + check(fetch_response, result=messages) return network @@ -61,6 +69,16 @@ def run(args): network = test(network, args, batch_size=100) network = test(network, args, batch_size=1000) + network = test(network, args, batch_size=1000, write_key_divisor=100) + network = test(network, args, batch_size=1000, write_size_multiplier=100) + network = test( + network, + args, + batch_size=1000, + write_key_divisor=100, + write_size_multiplier=100, + ) + # TODO: CI already takes ~25s for batch of 10k, so avoid large batches for now # bs = 10000 # step_size = 10000 From 84e879f4538544a4cdc85a08d48f2c8ebfe2e3d6 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Wed, 20 Nov 2019 15:30:08 +0000 Subject: [PATCH 2/6] Add test of exceptional serialisation failure --- src/kv/test/kv_serialisation.cpp | 83 ++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/src/kv/test/kv_serialisation.cpp b/src/kv/test/kv_serialisation.cpp index da7d2c8c5f47..b2c6bf7ad605 100644 --- a/src/kv/test/kv_serialisation.cpp +++ b/src/kv/test/kv_serialisation.cpp @@ -479,4 +479,87 @@ TEST_CASE("replicated and derived table serialisation") REQUIRE(serialised.replicated.size() > 0); REQUIRE(serialised.derived.size() > 0); } +} + +struct NonSerialisable +{ + std::string message = "NonSerialisable"; + enum + { + LogicError, + BadAlloc, + } kind = LogicError; +}; + +namespace msgpack +{ + MSGPACK_API_VERSION_NAMESPACE(MSGPACK_DEFAULT_API_NS) + { + namespace adaptor + { + // msgpack conversion for uint256_t + template <> + struct convert + { + msgpack::object const& operator()( + msgpack::object const& o, NonSerialisable& ns) const + { + const auto msg = fmt::format("Deserialise failure: {}", ns.message); + switch (ns.kind) + { + case NonSerialisable::LogicError: + { + throw std::logic_error(msg); + } + case NonSerialisable::BadAlloc: + { + throw std::bad_alloc(); + } + } + } + }; + + template <> + struct pack + { + template + packer& operator()( + msgpack::packer& o, NonSerialisable const& ns) const + { + const auto msg = fmt::format("Serialise failure: {}", ns.message); + switch (ns.kind) + { + case NonSerialisable::LogicError: + { + throw std::logic_error(msg); + } + case NonSerialisable::BadAlloc: + { + throw std::bad_alloc(); + } + } + } + }; + } + } +} + +TEST_CASE("Exceptional serdes" * doctest::test_suite("serialisation")) +{ + auto encryptor = std::make_shared(); + auto consensus = std::make_shared(); + + Store store(consensus); + store.set_encryptor(encryptor); + + auto& map = store.create("map"); + + { + Store::Tx tx; + auto view = tx.get_view(map); + + view->put(0, {}); + + REQUIRE(tx.commit() == kv::CommitSuccess::OK); + } } \ No newline at end of file From cc8af36e0cb6c976dc31ebce7f7b3c86fcae2c0f Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Wed, 20 Nov 2019 17:03:46 +0000 Subject: [PATCH 3/6] Catch exceptions and commit --- src/kv/kv.h | 57 +++++++++++++++++++++++++++++++++--------------- src/kv/kvtypes.h | 3 ++- 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/src/kv/kv.h b/src/kv/kv.h index 7f5a7a3ad75f..66355580a489 100644 --- a/src/kv/kv.h +++ b/src/kv/kv.h @@ -1010,29 +1010,50 @@ namespace kv LOG_FAIL_FMT("Could not commit transaction {}", version); return CommitSuccess::CONFLICT; } + else + { + version = c.value(); - version = c.value(); + // From here, we have received a unique commit version - we must commit + // this to make progress. So if any of these steps fail, we must still + // commit an empty transaction + try + { + auto data = serialise(); - auto data = serialise(); - if (data.empty()) - { - auto h = store->get_history(); - if (h != nullptr) + if (data.empty()) + { + auto h = store->get_history(); + if (h != nullptr) + { + // This tx does not have a write set, so this is a read only tx + // because of this we are returning NoVersion + h->add_result(req_id, NoVersion); + } + return CommitSuccess::OK; + } + + return store->commit( + version, + [data = std::move(data), + req_id = std::move(req_id)]() -> PendingTx::result_type { + return {CommitSuccess::OK, std::move(req_id), std::move(data)}; + }, + false); + } + catch (const std::exception& e) { - // This tx does not have a write set, so this is a read only tx - // because of this we are returning NoVersion - h->add_result(req_id, NoVersion); + LOG_FAIL_FMT("Error during serialisation: {}", e.what()); + store->commit( + version, + []() -> PendingTx::result_type { + return {CommitSuccess::NO_SERIALISE, {}, {}}; + }, + false); + + return CommitSuccess::NO_SERIALISE; } - return CommitSuccess::OK; } - - return store->commit( - version, - [data = std::move(data), req_id = std::move(req_id)]() - -> std::tuple { - return {CommitSuccess::OK, std::move(req_id), std::move(data)}; - }, - false); } /** Commit version if committed diff --git a/src/kv/kvtypes.h b/src/kv/kvtypes.h index 2a9a1014b051..0b1a075e81b6 100644 --- a/src/kv/kvtypes.h +++ b/src/kv/kvtypes.h @@ -30,7 +30,8 @@ namespace kv { OK, CONFLICT, - NO_REPLICATE + NO_REPLICATE, + NO_SERIALISE }; enum SecurityDomain From 311b7947636656d50af7b2153a1057efcdd110c4 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Thu, 21 Nov 2019 11:38:26 +0000 Subject: [PATCH 4/6] Abort hope, all ye who fail to serialise --- src/kv/genericserialisewrapper.h | 14 ---------- src/kv/kv.h | 16 ++++------- src/kv/kvtypes.h | 17 ++++++++++-- src/kv/test/kv_serialisation.cpp | 47 ++++++++------------------------ 4 files changed, 32 insertions(+), 62 deletions(-) diff --git a/src/kv/genericserialisewrapper.h b/src/kv/genericserialisewrapper.h index e37653ad3d6b..a4bad97b9281 100644 --- a/src/kv/genericserialisewrapper.h +++ b/src/kv/genericserialisewrapper.h @@ -38,20 +38,6 @@ namespace kv static_cast(a) | static_cast(b)); } - class KvSerialiserException : public std::exception - { - private: - std::string msg; - - public: - KvSerialiserException(const std::string& msg_) : msg(msg_) {} - - virtual const char* what() const throw() - { - return msg.c_str(); - } - }; - template struct KeyValVersion { diff --git a/src/kv/kv.h b/src/kv/kv.h index 66355580a489..290995f5b084 100644 --- a/src/kv/kv.h +++ b/src/kv/kv.h @@ -1014,9 +1014,9 @@ namespace kv { version = c.value(); - // From here, we have received a unique commit version - we must commit - // this to make progress. So if any of these steps fail, we must still - // commit an empty transaction + // From here, we have received a unique commit version and made + // modifications to our local kv. If we fail in any way, we cannot + // recover. try { auto data = serialise(); @@ -1044,14 +1044,10 @@ namespace kv catch (const std::exception& e) { LOG_FAIL_FMT("Error during serialisation: {}", e.what()); - store->commit( - version, - []() -> PendingTx::result_type { - return {CommitSuccess::NO_SERIALISE, {}, {}}; - }, - false); - return CommitSuccess::NO_SERIALISE; + // Discard original exception type, throw as now fatal + // KvSerialiserException + throw KvSerialiserException(e.what()); } } } diff --git a/src/kv/kvtypes.h b/src/kv/kvtypes.h index 0b1a075e81b6..d8662231b45f 100644 --- a/src/kv/kvtypes.h +++ b/src/kv/kvtypes.h @@ -30,8 +30,7 @@ namespace kv { OK, CONFLICT, - NO_REPLICATE, - NO_SERIALISE + NO_REPLICATE }; enum SecurityDomain @@ -78,6 +77,20 @@ namespace kv } }; + class KvSerialiserException : public std::exception + { + private: + std::string msg; + + public: + KvSerialiserException(const std::string& msg_) : msg(msg_) {} + + virtual const char* what() const throw() + { + return msg.c_str(); + } + }; + class TxHistory { public: diff --git a/src/kv/test/kv_serialisation.cpp b/src/kv/test/kv_serialisation.cpp index b2c6bf7ad605..4287b574f365 100644 --- a/src/kv/test/kv_serialisation.cpp +++ b/src/kv/test/kv_serialisation.cpp @@ -482,14 +482,7 @@ TEST_CASE("replicated and derived table serialisation") } struct NonSerialisable -{ - std::string message = "NonSerialisable"; - enum - { - LogicError, - BadAlloc, - } kind = LogicError; -}; +{}; namespace msgpack { @@ -504,18 +497,7 @@ namespace msgpack msgpack::object const& operator()( msgpack::object const& o, NonSerialisable& ns) const { - const auto msg = fmt::format("Deserialise failure: {}", ns.message); - switch (ns.kind) - { - case NonSerialisable::LogicError: - { - throw std::logic_error(msg); - } - case NonSerialisable::BadAlloc: - { - throw std::bad_alloc(); - } - } + throw std::runtime_error("Deserialise failure"); } }; @@ -526,18 +508,7 @@ namespace msgpack packer& operator()( msgpack::packer& o, NonSerialisable const& ns) const { - const auto msg = fmt::format("Serialise failure: {}", ns.message); - switch (ns.kind) - { - case NonSerialisable::LogicError: - { - throw std::logic_error(msg); - } - case NonSerialisable::BadAlloc: - { - throw std::bad_alloc(); - } - } + throw std::runtime_error("Serialise failure"); } }; } @@ -552,14 +523,18 @@ TEST_CASE("Exceptional serdes" * doctest::test_suite("serialisation")) Store store(consensus); store.set_encryptor(encryptor); - auto& map = store.create("map"); + auto& good_map = store.create("good_map"); + auto& bad_map = store.create("bad_map"); { Store::Tx tx; - auto view = tx.get_view(map); - view->put(0, {}); + auto good_view = tx.get_view(good_map); + good_view->put(1, 2); - REQUIRE(tx.commit() == kv::CommitSuccess::OK); + auto bad_view = tx.get_view(bad_map); + bad_view->put(0, {}); + + REQUIRE_THROWS_AS(tx.commit(), kv::KvSerialiserException); } } \ No newline at end of file From 1a03a37963b8122eb1d910562273cbecdbfe55a3 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Thu, 21 Nov 2019 13:19:37 +0000 Subject: [PATCH 5/6] Add disabled run_to_destruction test --- tests/e2e_batched.py | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/tests/e2e_batched.py b/tests/e2e_batched.py index f6d8d91157b0..4656d441a18a 100644 --- a/tests/e2e_batched.py +++ b/tests/e2e_batched.py @@ -69,14 +69,14 @@ def run(args): network = test(network, args, batch_size=100) network = test(network, args, batch_size=1000) - network = test(network, args, batch_size=1000, write_key_divisor=100) - network = test(network, args, batch_size=1000, write_size_multiplier=100) + network = test(network, args, batch_size=1000, write_key_divisor=10) + network = test(network, args, batch_size=1000, write_size_multiplier=10) network = test( network, args, batch_size=1000, - write_key_divisor=100, - write_size_multiplier=100, + write_key_divisor=10, + write_size_multiplier=10, ) # TODO: CI already takes ~25s for batch of 10k, so avoid large batches for now @@ -90,9 +90,33 @@ def run(args): # bs += step_size +def run_to_destruction(args): + hosts = ["localhost", "localhost", "localhost"] + + with infra.ccf.network( + hosts, args.build_dir, args.debug_nodes, args.perf_nodes, pdb=args.pdb + ) as network: + network.start_and_join(args) + + try: + wsm = 5000 + while True: + LOG.info(f"Trying with writes scaled by {wsm}") + network = test(network, args, batch_size=10, write_size_multiplier=wsm) + wsm += 5000 + except Exception as e: + LOG.info(f"Large write set caused an exception, as expected") + time.sleep(3) + assert ( + network.nodes[0].remote.remote.proc.poll() is not None + ), "Primary should have been terminated" + + if __name__ == "__main__": args = e2e_args.cli_args() args.package = "libluagenericenc" args.enforce_reqs = True run(args) + + # run_to_destruction(args) From 3f705b36707ee3f23c08b237ee1d178318e68572 Mon Sep 17 00:00:00 2001 From: Eddy Ashton Date: Thu, 21 Nov 2019 17:02:00 +0000 Subject: [PATCH 6/6] Decomment test --- tests/e2e_batched.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e_batched.py b/tests/e2e_batched.py index 4656d441a18a..73b3d0be215b 100644 --- a/tests/e2e_batched.py +++ b/tests/e2e_batched.py @@ -119,4 +119,4 @@ def run_to_destruction(args): run(args) - # run_to_destruction(args) + run_to_destruction(args)