From 5dbe52248cb1358b7ae8fbc1c3f299a2e372cd46 Mon Sep 17 00:00:00 2001 From: heyuchen Date: Tue, 2 Feb 2021 16:55:19 +0800 Subject: [PATCH 1/4] feat(split): replica add validate partition_hash --- include/dsn/utility/error_code.h | 2 + src/replica/replica.cpp | 13 ++++++ src/replica/replica.h | 6 +++ src/replica/replica_2pc.cpp | 13 ++++++ src/replica/replica_config.cpp | 34 ++++++++------- src/replica/split/replica_split_manager.h | 16 +++++++ src/replica/split/test/replica_split_test.cpp | 27 ++++++++++++ src/replica/test/replica_test.cpp | 42 +++++++++++++++++++ 8 files changed, 139 insertions(+), 14 deletions(-) diff --git a/include/dsn/utility/error_code.h b/include/dsn/utility/error_code.h index b6738db5b5..940e0636c2 100644 --- a/include/dsn/utility/error_code.h +++ b/include/dsn/utility/error_code.h @@ -126,4 +126,6 @@ DEFINE_ERR_CODE(ERR_KRB5_INTERNAL) DEFINE_ERR_CODE(ERR_SASL_INTERNAL) DEFINE_ERR_CODE(ERR_SASL_INCOMPLETE) DEFINE_ERR_CODE(ERR_ACL_DENY) +DEFINE_ERR_CODE(ERR_SPLITTING) +DEFINE_ERR_CODE(ERR_PARENT_PARTITION_MISUSED) } // namespace dsn diff --git a/src/replica/replica.cpp b/src/replica/replica.cpp index 9f8b27966d..c61e511507 100644 --- a/src/replica/replica.cpp +++ b/src/replica/replica.cpp @@ -176,6 +176,19 @@ void replica::on_client_read(dsn::message_ex *request, bool ignore_throttling) response_client_read(request, ERR_ACL_DENY); } + // validate partition_hash + if (_validate_partition_hash) { + if (_split_mgr->should_reject_request()) { + response_client_read(request, ERR_SPLITTING); + return; + } + if (!_split_mgr->check_partition_hash( + ((dsn::message_ex *)request)->header->client.partition_hash, "read")) { + response_client_read(request, ERR_PARENT_PARTITION_MISUSED); + return; + } + } + if (status() == partition_status::PS_INACTIVE || status() == partition_status::PS_POTENTIAL_SECONDARY) { response_client_read(request, ERR_INVALID_STATE); diff --git a/src/replica/replica.h b/src/replica/replica.h index 61526a7df2..f4c091e646 100644 --- a/src/replica/replica.h +++ b/src/replica/replica.h @@ -414,6 +414,11 @@ class replica : public serverlet, public ref_counter, public replica_ba // update allowed users for access controller void update_ac_allowed_users(const std::map &envs); + // update bool app envs + void update_bool_envs(const std::map &envs, + const std::string &name, + /*out*/ bool &value); + private: friend class ::dsn::replication::test::test_checker; friend class ::dsn::replication::mutation_queue; @@ -519,6 +524,7 @@ class replica : public serverlet, public ref_counter, public replica_ba // partition split std::unique_ptr _split_mgr; + bool _validate_partition_hash{false}; // disk migrator std::unique_ptr _disk_migrator; diff --git a/src/replica/replica_2pc.cpp b/src/replica/replica_2pc.cpp index 5670300c44..3c1dfaedf0 100644 --- a/src/replica/replica_2pc.cpp +++ b/src/replica/replica_2pc.cpp @@ -77,6 +77,19 @@ void replica::on_client_write(dsn::message_ex *request, bool ignore_throttling) return; } + // validate partition_hash + if (_validate_partition_hash) { + if (_split_mgr->should_reject_request()) { + response_client_read(request, ERR_SPLITTING); + return; + } + if (!_split_mgr->check_partition_hash( + ((dsn::message_ex *)request)->header->client.partition_hash, "write")) { + response_client_read(request, ERR_PARENT_PARTITION_MISUSED); + return; + } + } + if (partition_status::PS_PRIMARY != status()) { response_client_write(request, ERR_INVALID_STATE); return; diff --git a/src/replica/replica_config.cpp b/src/replica/replica_config.cpp index ab3c028e80..6fd156786e 100644 --- a/src/replica/replica_config.cpp +++ b/src/replica/replica_config.cpp @@ -554,26 +554,32 @@ void replica::update_app_envs(const std::map &envs) void replica::update_app_envs_internal(const std::map &envs) { - // DENY_CLIENT_WRITE - bool deny_client_write = false; - auto find = envs.find(replica_envs::DENY_CLIENT_WRITE); - if (find != envs.end()) { - if (!buf2bool(find->second, deny_client_write)) { - dwarn_replica( - "invalid value of env {}: \"{}\"", replica_envs::DENY_CLIENT_WRITE, find->second); - } - } - if (deny_client_write != _deny_client_write) { - ddebug_replica( - "switch _deny_client_write from {} to {}", _deny_client_write, deny_client_write); - _deny_client_write = deny_client_write; - } + update_bool_envs(envs, replica_envs::DENY_CLIENT_WRITE, _deny_client_write); + + update_bool_envs(envs, replica_envs::SPLIT_VALIDATE_PARTITION_HASH, _validate_partition_hash); update_throttle_envs(envs); update_ac_allowed_users(envs); } +void replica::update_bool_envs(const std::map &envs, + const std::string &name, + bool &value) +{ + bool new_value = value; + auto iter = envs.find(name); + if (iter != envs.end()) { + if (!buf2bool(iter->second, new_value)) { + dwarn_replica("invalid value of env {}: \"{}\"", name, iter->second); + } + } + if (new_value != value) { + ddebug_replica("switch env[{}] from {} to {}", name, value, new_value); + value = new_value; + } +} + void replica::update_ac_allowed_users(const std::map &envs) { std::string allowed_users; diff --git a/src/replica/split/replica_split_manager.h b/src/replica/split/replica_split_manager.h index 93a5e2f4e6..185d096765 100644 --- a/src/replica/split/replica_split_manager.h +++ b/src/replica/split/replica_split_manager.h @@ -170,6 +170,22 @@ class replica_split_manager : replica_base ballot get_ballot() const { return _replica->get_ballot(); } decree last_committed_decree() const { return _replica->last_committed_decree(); } task_tracker *tracker() { return _replica->tracker(); } + bool should_reject_request() const { return _partition_version == -1; } + bool check_partition_hash(const uint64_t &partition_hash, const std::string &op) const + { + auto target_pidx = get_partition_version() & partition_hash; + if (target_pidx != get_gpid().get_partition_index()) { + derror_replica( + "receive {} request with wrong partition_hash({}), partition_version = {}, " + "target_pidx = {}", + op, + partition_hash, + get_partition_version(), + target_pidx); + return false; + } + return true; + } private: replica *_replica; diff --git a/src/replica/split/test/replica_split_test.cpp b/src/replica/split/test/replica_split_test.cpp index 4cdb978b6a..c6bf32f5ee 100644 --- a/src/replica/split/test/replica_split_test.cpp +++ b/src/replica/split/test/replica_split_test.cpp @@ -410,6 +410,12 @@ class replica_split_test : public replica_test_base _parent_replica->tracker()->wait_outstanding_tasks(); } + bool test_check_partition_hash(int32_t partition_version, uint64_t partition_hash) + { + _parent_split_mgr->_partition_version.store(partition_version); + return _parent_split_mgr->check_partition_hash(partition_hash, "write"); + } + /// helper functions void cleanup_prepare_list(mock_replica_ptr rep) { rep->_prepare_list->reset(0); } void cleanup_child_split_context() @@ -918,5 +924,26 @@ TEST_F(replica_split_test, primary_parent_handle_stop_test) } } +TEST_F(replica_split_test, check_partition_hash_test) +{ + uint64_t send_to_parent_after_split = 1; + uint64_t send_to_child_after_split = 9; + + struct check_partition_hash_test + { + int32_t partition_version; + uint64_t partition_hash; + bool expected_result; + } tests[]{{OLD_PARTITION_COUNT - 1, send_to_parent_after_split, true}, + {OLD_PARTITION_COUNT - 1, send_to_child_after_split, true}, + {NEW_PARTITION_COUNT - 1, send_to_parent_after_split, true}, + {NEW_PARTITION_COUNT - 1, send_to_child_after_split, false}}; + + for (auto test : tests) { + ASSERT_EQ(test_check_partition_hash(test.partition_version, test.partition_hash), + test.expected_result); + } +} + } // namespace replication } // namespace dsn diff --git a/src/replica/test/replica_test.cpp b/src/replica/test/replica_test.cpp index f3df84669a..297c4a4d16 100644 --- a/src/replica/test/replica_test.cpp +++ b/src/replica/test/replica_test.cpp @@ -7,6 +7,7 @@ #include #include "replica_test_base.h" #include +#include #include "replica/replica_http_service.h" namespace dsn { @@ -38,6 +39,22 @@ class replica_test : public replica_test_base return _mock_replica->_counter_backup_request_qps->get_integer_value(); } + bool get_validate_partition_hash() { return _mock_replica->_validate_partition_hash; } + + void reset_validate_partition_hash() { _mock_replica->_validate_partition_hash = false; } + + void update_validate_partition_hash(bool old_value, bool set_in_map, std::string new_value) + { + _mock_replica->_validate_partition_hash = old_value; + std::map envs; + if (set_in_map) { + envs[replica_envs::SPLIT_VALIDATE_PARTITION_HASH] = new_value; + } + _mock_replica->update_bool_envs(envs, + replica_envs::SPLIT_VALIDATE_PARTITION_HASH, + _mock_replica->_validate_partition_hash); + } + void mock_app_info() { _app_info.app_id = 2; @@ -146,5 +163,30 @@ TEST_F(replica_test, query_compaction_test) } } +TEST_F(replica_test, update_validate_partition_hash_test) +{ + struct update_validate_partition_hash_test + { + bool old_value; + bool set_in_map; + std::string new_value; + bool expected_value; + } tests[]{ + {false, false, "false", false}, + {false, true, "false", false}, + {false, false, "true", false}, + {false, true, "true", true}, + {false, true, "ture", false}, + {true, true, "false", false}, + {true, true, "true", true}, + {true, true, "flase", true}, + }; + for (const auto &test : tests) { + update_validate_partition_hash(test.old_value, test.set_in_map, test.new_value); + ASSERT_EQ(get_validate_partition_hash(), test.expected_value); + reset_validate_partition_hash(); + } +} + } // namespace replication } // namespace dsn From a764100f6cf1ba213b2180e3c8c816c76d7799e3 Mon Sep 17 00:00:00 2001 From: heyuchen Date: Tue, 2 Feb 2021 17:02:09 +0800 Subject: [PATCH 2/4] some small fix --- src/replica/replica_2pc.cpp | 4 ++-- src/replica/split/replica_split_manager.h | 2 +- src/replica/split/test/replica_split_test.cpp | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/replica/replica_2pc.cpp b/src/replica/replica_2pc.cpp index 3c1dfaedf0..1eb13cde0a 100644 --- a/src/replica/replica_2pc.cpp +++ b/src/replica/replica_2pc.cpp @@ -80,12 +80,12 @@ void replica::on_client_write(dsn::message_ex *request, bool ignore_throttling) // validate partition_hash if (_validate_partition_hash) { if (_split_mgr->should_reject_request()) { - response_client_read(request, ERR_SPLITTING); + response_client_write(request, ERR_SPLITTING); return; } if (!_split_mgr->check_partition_hash( ((dsn::message_ex *)request)->header->client.partition_hash, "write")) { - response_client_read(request, ERR_PARENT_PARTITION_MISUSED); + response_client_write(request, ERR_PARENT_PARTITION_MISUSED); return; } } diff --git a/src/replica/split/replica_split_manager.h b/src/replica/split/replica_split_manager.h index 185d096765..3565e03dce 100644 --- a/src/replica/split/replica_split_manager.h +++ b/src/replica/split/replica_split_manager.h @@ -170,7 +170,7 @@ class replica_split_manager : replica_base ballot get_ballot() const { return _replica->get_ballot(); } decree last_committed_decree() const { return _replica->last_committed_decree(); } task_tracker *tracker() { return _replica->tracker(); } - bool should_reject_request() const { return _partition_version == -1; } + bool should_reject_request() const { return get_partition_version() == -1; } bool check_partition_hash(const uint64_t &partition_hash, const std::string &op) const { auto target_pidx = get_partition_version() & partition_hash; diff --git a/src/replica/split/test/replica_split_test.cpp b/src/replica/split/test/replica_split_test.cpp index c6bf32f5ee..e59a2eb591 100644 --- a/src/replica/split/test/replica_split_test.cpp +++ b/src/replica/split/test/replica_split_test.cpp @@ -410,7 +410,7 @@ class replica_split_test : public replica_test_base _parent_replica->tracker()->wait_outstanding_tasks(); } - bool test_check_partition_hash(int32_t partition_version, uint64_t partition_hash) + bool test_check_partition_hash(const int32_t &partition_version, const uint64_t &partition_hash) { _parent_split_mgr->_partition_version.store(partition_version); return _parent_split_mgr->check_partition_hash(partition_hash, "write"); @@ -939,7 +939,7 @@ TEST_F(replica_split_test, check_partition_hash_test) {NEW_PARTITION_COUNT - 1, send_to_parent_after_split, true}, {NEW_PARTITION_COUNT - 1, send_to_child_after_split, false}}; - for (auto test : tests) { + for (const auto &test : tests) { ASSERT_EQ(test_check_partition_hash(test.partition_version, test.partition_hash), test.expected_result); } From ef0746d0cc8063f02cc9fadbec56c802ae150ff0 Mon Sep 17 00:00:00 2001 From: heyuchen Date: Fri, 5 Feb 2021 10:09:57 +0800 Subject: [PATCH 3/4] update by code review --- src/replica/replica.cpp | 13 +------------ src/replica/replica.h | 13 +++++++++++++ src/replica/replica_2pc.cpp | 13 +------------ src/replica/test/replica_test.cpp | 2 +- 4 files changed, 16 insertions(+), 25 deletions(-) diff --git a/src/replica/replica.cpp b/src/replica/replica.cpp index c61e511507..1c322e47a3 100644 --- a/src/replica/replica.cpp +++ b/src/replica/replica.cpp @@ -176,18 +176,7 @@ void replica::on_client_read(dsn::message_ex *request, bool ignore_throttling) response_client_read(request, ERR_ACL_DENY); } - // validate partition_hash - if (_validate_partition_hash) { - if (_split_mgr->should_reject_request()) { - response_client_read(request, ERR_SPLITTING); - return; - } - if (!_split_mgr->check_partition_hash( - ((dsn::message_ex *)request)->header->client.partition_hash, "read")) { - response_client_read(request, ERR_PARENT_PARTITION_MISUSED); - return; - } - } + CHECK_REQUEST_IF_SPLITTING(read) if (status() == partition_status::PS_INACTIVE || status() == partition_status::PS_POTENTIAL_SECONDARY) { diff --git a/src/replica/replica.h b/src/replica/replica.h index f4c091e646..5886f7ee6b 100644 --- a/src/replica/replica.h +++ b/src/replica/replica.h @@ -86,6 +86,19 @@ enum manual_compaction_status }; const char *manual_compaction_status_to_string(manual_compaction_status status); +#define CHECK_REQUEST_IF_SPLITTING(op_type) \ + if (_validate_partition_hash) { \ + if (_split_mgr->should_reject_request()) { \ + response_client_##op_type(request, ERR_SPLITTING); \ + return; \ + } \ + if (!_split_mgr->check_partition_hash( \ + ((dsn::message_ex *)request)->header->client.partition_hash, #op_type)) { \ + response_client_##op_type(request, ERR_PARENT_PARTITION_MISUSED); \ + return; \ + } \ + } + class replica : public serverlet, public ref_counter, public replica_base { public: diff --git a/src/replica/replica_2pc.cpp b/src/replica/replica_2pc.cpp index 1eb13cde0a..ad3522c296 100644 --- a/src/replica/replica_2pc.cpp +++ b/src/replica/replica_2pc.cpp @@ -77,18 +77,7 @@ void replica::on_client_write(dsn::message_ex *request, bool ignore_throttling) return; } - // validate partition_hash - if (_validate_partition_hash) { - if (_split_mgr->should_reject_request()) { - response_client_write(request, ERR_SPLITTING); - return; - } - if (!_split_mgr->check_partition_hash( - ((dsn::message_ex *)request)->header->client.partition_hash, "write")) { - response_client_write(request, ERR_PARENT_PARTITION_MISUSED); - return; - } - } + CHECK_REQUEST_IF_SPLITTING(write) if (partition_status::PS_PRIMARY != status()) { response_client_write(request, ERR_INVALID_STATE); diff --git a/src/replica/test/replica_test.cpp b/src/replica/test/replica_test.cpp index 297c4a4d16..7921ab31c3 100644 --- a/src/replica/test/replica_test.cpp +++ b/src/replica/test/replica_test.cpp @@ -39,7 +39,7 @@ class replica_test : public replica_test_base return _mock_replica->_counter_backup_request_qps->get_integer_value(); } - bool get_validate_partition_hash() { return _mock_replica->_validate_partition_hash; } + bool get_validate_partition_hash() const { return _mock_replica->_validate_partition_hash; } void reset_validate_partition_hash() { _mock_replica->_validate_partition_hash = false; } From 0c9f993537afec067d68f031928144a811ac4da2 Mon Sep 17 00:00:00 2001 From: heyuchen Date: Fri, 5 Feb 2021 10:44:08 +0800 Subject: [PATCH 4/4] small update --- src/replica/split/replica_split_manager.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/replica/split/replica_split_manager.h b/src/replica/split/replica_split_manager.h index 3565e03dce..bafa38b3ac 100644 --- a/src/replica/split/replica_split_manager.h +++ b/src/replica/split/replica_split_manager.h @@ -174,7 +174,7 @@ class replica_split_manager : replica_base bool check_partition_hash(const uint64_t &partition_hash, const std::string &op) const { auto target_pidx = get_partition_version() & partition_hash; - if (target_pidx != get_gpid().get_partition_index()) { + if (dsn_unlikely(target_pidx != get_gpid().get_partition_index())) { derror_replica( "receive {} request with wrong partition_hash({}), partition_version = {}, " "target_pidx = {}",