From 9998f76df83bddf9fbc7ffb792189f8559198c59 Mon Sep 17 00:00:00 2001 From: Sandeep L <99765181+lingamsandeep@users.noreply.github.com> Date: Sat, 16 Mar 2024 06:09:48 -0700 Subject: [PATCH] [#21491] DocDB: Increment ClusterConfig version during universe_uuid update and yb-ts-cli to clear universe uuid. Summary: This fix addresses the following race condition which is possible when upgrading a universe from a release which does not have master_enable_universe_uuid_heartbeat_check to a release which has universe_uuid generation and checks. # Cluster gets upgraded to a release with commit fb98e56 and the feature master_enable_universe_uuid_heartbeat_check is enabled. # Reader reads the ClusterConfig at version 'X'. # Catalog Manager background thread runs and generates a new universe_uuid, persists it in ClusterConfig and propagates it to all the t-servers. # Reader from Step 2 updates the ClusterConfig using ChangeMasterClusterConfigRequestPB with version 'X'. # Update from Step 4 succeeds because ClusterConfig version 'X' on disk matches the one in the request 'X' - effectively overwriting the universe_uuid generated in Step 3. # Catalog Manager background thread runs again and since universe_uuid is empty, it generates a new universe_uuid again. Fix is to increment the ClusterConfig version during an update by the background task. Also added checks to ensure that universe_uuid does not modified once set by ChangeMasterClusterConfig. Added a yb-ts-cli to clear the universe uuid in case we ever run into issues because of this. **Upgrade/Rollback safety:** - The proto changes are strictly for the yb-ts-cli only and as such should not have any upgrade implications. Jira: DB-10377 Test Plan: - Manually verified with an upgrade test that the clusterconfig version gets bumped up correctly and ChangeMasterClusterConfig fails if it attempts to modify the universe_uuid. - Manually tested the yb-ts-cli command to validate that it clears the universe_uuid correctly. ybt integration-tests_master_heartbeat-itest MasterHeartbeatITestWithUpgrade.ClearUniverseUuidToRecoverUniverse Reviewers: hsunder Reviewed By: hsunder Subscribers: ybase, bogdan Differential Revision: https://phorge.dev.yugabyte.com/D33180 --- src/yb/fs/fs_manager.cc | 11 +++ src/yb/fs/fs_manager.h | 2 + .../master_heartbeat-itest.cc | 84 +++++++++++++++++++ src/yb/master/catalog_manager.cc | 6 ++ src/yb/master/sys_catalog-test.cc | 9 +- src/yb/tools/ts-cli.cc | 32 ++++++- src/yb/tserver/tablet_server.cc | 10 +++ src/yb/tserver/tablet_server.h | 2 + src/yb/tserver/tablet_service.cc | 11 +++ src/yb/tserver/tablet_service.h | 4 + src/yb/tserver/tserver.proto | 7 ++ src/yb/tserver/tserver_service.proto | 2 + 12 files changed, 176 insertions(+), 4 deletions(-) diff --git a/src/yb/fs/fs_manager.cc b/src/yb/fs/fs_manager.cc index 0109bc8fa699..a65b36556d91 100644 --- a/src/yb/fs/fs_manager.cc +++ b/src/yb/fs/fs_manager.cc @@ -339,12 +339,23 @@ Status FsManager::SetUniverseUuidOnTserverInstanceMetadata( const UniverseUuid& universe_uuid) { std::lock_guard lock(metadata_mutex_); SCHECK_NOTNULL(metadata_); + LOG(INFO) << "Setting the universe_uuid to " << universe_uuid; metadata_->mutable_tserver_instance_metadata()->set_universe_uuid(universe_uuid.ToString()); auto instance_metadata_path = VERIFY_RESULT(GetExistingInstanceMetadataPath()); return pb_util::WritePBContainerToPath( env_, instance_metadata_path, *metadata_.get(), pb_util::OVERWRITE, pb_util::SYNC); } +Status FsManager::ClearUniverseUuidOnTserverInstanceMetadata() { + std::lock_guard lock(metadata_mutex_); + SCHECK_NOTNULL(metadata_); + LOG(INFO) << "Clearing the universe_uuid from Instance Metadata"; + metadata_->mutable_tserver_instance_metadata()->clear_universe_uuid(); + auto instance_metadata_path = VERIFY_RESULT(GetExistingInstanceMetadataPath()); + return pb_util::WritePBContainerToPath( + env_, instance_metadata_path, *metadata_.get(), pb_util::OVERWRITE, pb_util::SYNC); +} + Status FsManager::CheckAndOpenFileSystemRoots() { RETURN_NOT_OK(Init()); diff --git a/src/yb/fs/fs_manager.h b/src/yb/fs/fs_manager.h index 28263860f978..e2866e881f4c 100644 --- a/src/yb/fs/fs_manager.h +++ b/src/yb/fs/fs_manager.h @@ -203,6 +203,8 @@ class FsManager { Status SetUniverseUuidOnTserverInstanceMetadata(const UniverseUuid& universe_uuid); + Status ClearUniverseUuidOnTserverInstanceMetadata(); + // Return the path where InstanceMetadataPB is stored. std::string GetInstanceMetadataPath(const std::string& root) const; diff --git a/src/yb/integration-tests/master_heartbeat-itest.cc b/src/yb/integration-tests/master_heartbeat-itest.cc index bb24df81dcba..93cb823bc5c4 100644 --- a/src/yb/integration-tests/master_heartbeat-itest.cc +++ b/src/yb/integration-tests/master_heartbeat-itest.cc @@ -52,6 +52,7 @@ DECLARE_bool(master_enable_universe_uuid_heartbeat_check); namespace yb { +using master::GetMasterClusterConfigResponsePB; namespace integration_tests { class MasterHeartbeatITest : public YBTableTestBase { @@ -147,6 +148,89 @@ TEST_F(MasterHeartbeatITest, IgnorePeerNotInConfig) { }, FLAGS_heartbeat_interval_ms * 5ms, "Wait for proper replica locations.")); } +class MasterHeartbeatITestWithUpgrade : public YBTableTestBase { + public: + void SetUp() override { + // Start the cluster without the universe_uuid generation FLAG to test upgrade. + ANNOTATE_UNPROTECTED_WRITE(FLAGS_master_enable_universe_uuid_heartbeat_check) = false; + YBTableTestBase::SetUp(); + proxy_cache_ = std::make_unique(client_->messenger()); + } + + Status GetClusterConfig(GetMasterClusterConfigResponsePB *config_resp) { + const auto* master = VERIFY_RESULT(mini_cluster_->GetLeaderMiniMaster()); + return master->catalog_manager().GetClusterConfig(config_resp); + } + + Status ClearUniverseUuid() { + for (auto& ts : mini_cluster_->mini_tablet_servers()) { + RETURN_NOT_OK(ts->server()->ClearUniverseUuid()); + } + return Status::OK(); + } + + protected: + std::unique_ptr proxy_cache_; +}; + +TEST_F(MasterHeartbeatITestWithUpgrade, ClearUniverseUuidToRecoverUniverse) { + GetMasterClusterConfigResponsePB resp; + ASSERT_OK(GetClusterConfig(&resp)); + auto cluster_config_version = resp.cluster_config().version(); + LOG(INFO) << "Cluster Config version : " << cluster_config_version; + + // Attempt to clear universe uuid. Should fail when it is not set. + ASSERT_NOK(ClearUniverseUuid()); + + // Enable the flag and wait for heartbeat to propagate the universe_uuid. + ANNOTATE_UNPROTECTED_WRITE(FLAGS_master_enable_universe_uuid_heartbeat_check) = true; + + // Wait for ClusterConfig version to increase. + ASSERT_OK(LoggedWaitFor([&]() { + if (!GetClusterConfig(&resp).ok()) { + return false; + } + + if (!resp.cluster_config().has_universe_uuid()) { + return false; + } + + return true; + }, 60s, "Waiting for new universe uuid to be generated")); + + ASSERT_GE(resp.cluster_config().version(), cluster_config_version); + LOG(INFO) << "Updated cluster config version:" << resp.cluster_config().version(); + LOG(INFO) << "Universe UUID:" << resp.cluster_config().universe_uuid(); + + // Wait for propagation of universe_uuid. + ASSERT_OK(LoggedWaitFor([&]() { + for (auto& ts : mini_cluster_->mini_tablet_servers()) { + auto uuid_str = ts->server()->fs_manager()->GetUniverseUuidFromTserverInstanceMetadata(); + if (!uuid_str.ok() || uuid_str.get() != resp.cluster_config().universe_uuid()) { + return false; + } + } + + return true; + }, 60s, "Waiting for tservers to pick up new universe uuid")); + + // Verify servers are heartbeating correctly. + master::TSDescriptorVector ts_descs; + ASSERT_OK(mini_cluster_->WaitForTabletServerCount(3, &ts_descs, true /* live_only */)); + + // Artificially generate a fake universe uuid and propagate that by clearing the universe_uuid. + ANNOTATE_UNPROTECTED_WRITE(FLAGS_TEST_master_universe_uuid) = Uuid::Generate().ToString(); + ANNOTATE_UNPROTECTED_WRITE(FLAGS_tserver_unresponsive_timeout_ms) = 10 * 1000; + + // Heartbeats should first fail due to universe_uuid mismatch. + ASSERT_OK(mini_cluster_->WaitForTabletServerCount(0, &ts_descs, true /* live_only */)); + + // Once t-server instance metadata is cleared, heartbeats should succeed again. + ASSERT_OK(ClearUniverseUuid()); + ASSERT_OK(mini_cluster_->WaitForTabletServerCount(3, &ts_descs, true /* live_only */)); +} + + class MasterHeartbeatITestWithExternal : public MasterHeartbeatITest { public: bool use_external_mini_cluster() { return true; } diff --git a/src/yb/master/catalog_manager.cc b/src/yb/master/catalog_manager.cc index 3f12d065b278..a953e6904fbf 100644 --- a/src/yb/master/catalog_manager.cc +++ b/src/yb/master/catalog_manager.cc @@ -1659,6 +1659,7 @@ Status CatalogManager::SetUniverseUuidIfNeeded(const LeaderEpoch& epoch) { universe_uuid); l.mutable_data()->pb.set_universe_uuid(universe_uuid); + l.mutable_data()->pb.set_version(l.mutable_data()->pb.version() + 1); RETURN_NOT_OK(sys_catalog_->Upsert(epoch, cluster_config_.get())); l.Commit(); return Status::OK(); @@ -12524,6 +12525,11 @@ Status CatalogManager::SetClusterConfig( return SetupError(resp->mutable_error(), MasterErrorPB::INVALID_CLUSTER_CONFIG, s); } + if (config.universe_uuid() != l->pb.universe_uuid()) { + Status s = STATUS(InvalidArgument, "Config Universe UUID cannot be updated"); + return SetupError(resp->mutable_error(), MasterErrorPB::INVALID_CLUSTER_CONFIG, s); + } + // TODO(bogdan): should this live here? const ReplicationInfoPB& replication_info = config.replication_info(); for (auto& read_replica : replication_info.read_replicas()) { diff --git a/src/yb/master/sys_catalog-test.cc b/src/yb/master/sys_catalog-test.cc index 328b09844bed..9ce7d6187acf 100644 --- a/src/yb/master/sys_catalog-test.cc +++ b/src/yb/master/sys_catalog-test.cc @@ -333,10 +333,15 @@ TEST_F(SysCatalogTest, TestSysCatalogPlacementOperations) { req.mutable_cluster_config()->set_cluster_uuid("some-cluster-uuid"); auto status = master_->catalog_manager()->SetClusterConfig(&req, &resp); ASSERT_TRUE(status.IsInvalidArgument()); - - // Setting the cluster uuid should make the request succeed. req.mutable_cluster_config()->set_cluster_uuid(config.cluster_uuid()); + // Verify that we receive an error when trying to change the universe uuid. + req.mutable_cluster_config()->set_universe_uuid("some-universe-uuid"); + status = master_->catalog_manager()->SetClusterConfig(&req, &resp); + ASSERT_TRUE(status.IsInvalidArgument()); + req.mutable_cluster_config()->set_universe_uuid(config.universe_uuid()); + + // Setting the cluster and universe uuid correctly should make the request succeed. ASSERT_OK(master_->catalog_manager()->SetClusterConfig(&req, &resp)); l.Commit(); } diff --git a/src/yb/tools/ts-cli.cc b/src/yb/tools/ts-cli.cc index da61dffe6021..98a8f4e29c27 100644 --- a/src/yb/tools/ts-cli.cc +++ b/src/yb/tools/ts-cli.cc @@ -72,12 +72,14 @@ using yb::consensus::RaftConfigPB; using yb::rpc::Messenger; using yb::rpc::MessengerBuilder; using yb::rpc::RpcController; -using yb::server::ServerStatusPB; using yb::server::ReloadCertificatesRequestPB; using yb::server::ReloadCertificatesResponsePB; +using yb::server::ServerStatusPB; using yb::tablet::TabletStatusPB; using yb::tserver::ClearAllMetaCachesOnServerRequestPB; using yb::tserver::ClearAllMetaCachesOnServerResponsePB; +using yb::tserver::ClearUniverseUuidRequestPB; +using yb::tserver::ClearUniverseUuidResponsePB; using yb::tserver::CountIntentsRequestPB; using yb::tserver::CountIntentsResponsePB; using yb::tserver::DeleteTabletRequestPB; @@ -116,6 +118,7 @@ const char* const kReloadCertificatesOp = "reload_certificates"; const char* const kRemoteBootstrapOp = "remote_bootstrap"; const char* const kListMasterServersOp = "list_master_servers"; const char* const kClearAllMetaCachesOnServerOp = "clear_server_metacache"; +const char* const kClearUniverseUuidOp = "clear_universe_uuid"; DEFINE_NON_RUNTIME_string(server_address, "localhost", "Address of server to run against"); @@ -251,6 +254,9 @@ class TsAdminClient { Status ClearAllMetaCachesOnServer(); + // Clear Universe Uuid. + Status ClearUniverseUuid(); + private: std::string addr_; MonoDelta timeout_; @@ -694,6 +700,22 @@ Status TsAdminClient::ClearAllMetaCachesOnServer() { return Status::OK(); } +Status TsAdminClient::ClearUniverseUuid() { + CHECK(initted_); + ClearUniverseUuidRequestPB req; + ClearUniverseUuidResponsePB resp; + RpcController rpc; + + rpc.set_timeout(timeout_); + RETURN_NOT_OK(ts_proxy_->ClearUniverseUuid(req, &resp, &rpc)); + if (resp.has_error()) { + return StatusFromPB(resp.error().status()); + } + + std::cout << "Universe UUID cleared from Instance Metadata" << std::endl; + return Status::OK(); +} + namespace { void SetUsage(const char* argv0) { @@ -721,7 +743,8 @@ void SetUsage(const char* argv0) { << " \n" << " " << kReloadCertificatesOp << "\n" << " " << kRemoteBootstrapOp << " \n" - << " " << kListMasterServersOp << "\n"; + << " " << kListMasterServersOp << "\n" + << " " << kClearUniverseUuidOp << "\n"; google::SetUsageMessage(str.str()); } @@ -942,6 +965,11 @@ static int TsCliMain(int argc, char** argv) { RETURN_NOT_OK_PREPEND_FROM_MAIN( client.ClearAllMetaCachesOnServer(), "Unable to clear the meta-cache on tablet server with address " + addr); + } else if (op == kClearUniverseUuidOp) { + CHECK_ARGC_OR_RETURN_WITH_USAGE(op, 2); + + RETURN_NOT_OK_PREPEND_FROM_MAIN(client.ClearUniverseUuid(), + "Unable to clear universe uuid on " + addr); } else { std::cerr << "Invalid operation: " << op << std::endl; google::ShowUsageWithFlagsRestrict(argv[0], __FILE__); diff --git a/src/yb/tserver/tablet_server.cc b/src/yb/tserver/tablet_server.cc index 059d56edd2d5..f698b90eee82 100644 --- a/src/yb/tserver/tablet_server.cc +++ b/src/yb/tserver/tablet_server.cc @@ -1283,6 +1283,15 @@ Status TabletServer::XClusterHandleMasterHeartbeatResponse( return Status::OK(); } +Status TabletServer::ClearUniverseUuid() { + auto instance_universe_uuid_str = VERIFY_RESULT( + fs_manager_->GetUniverseUuidFromTserverInstanceMetadata()); + auto instance_universe_uuid = VERIFY_RESULT(UniverseUuid::FromString(instance_universe_uuid_str)); + SCHECK_EQ(false, instance_universe_uuid.IsNil(), IllegalState, + "universe_uuid is not set in instance metadata"); + return fs_manager_->ClearUniverseUuidOnTserverInstanceMetadata(); +} + Status TabletServer::ValidateAndMaybeSetUniverseUuid(const UniverseUuid& universe_uuid) { auto instance_universe_uuid_str = VERIFY_RESULT( fs_manager_->GetUniverseUuidFromTserverInstanceMetadata()); @@ -1294,6 +1303,7 @@ Status TabletServer::ValidateAndMaybeSetUniverseUuid(const UniverseUuid& univers "uuid is $1", universe_uuid.ToString(), instance_universe_uuid.ToString())); return Status::OK(); } + return fs_manager_->SetUniverseUuidOnTserverInstanceMetadata(universe_uuid); } diff --git a/src/yb/tserver/tablet_server.h b/src/yb/tserver/tablet_server.h index 005ab10a7783..6722a8ab2e8f 100644 --- a/src/yb/tserver/tablet_server.h +++ b/src/yb/tserver/tablet_server.h @@ -310,6 +310,8 @@ class TabletServer : public DbServerBase, public TabletServerIf { Status ValidateAndMaybeSetUniverseUuid(const UniverseUuid& universe_uuid); + Status ClearUniverseUuid(); + XClusterConsumerIf* GetXClusterConsumer() const; // Mark the CDC service as enabled via heartbeat. diff --git a/src/yb/tserver/tablet_service.cc b/src/yb/tserver/tablet_service.cc index a75a3c3554f2..9d6e6d60a173 100644 --- a/src/yb/tserver/tablet_service.cc +++ b/src/yb/tserver/tablet_service.cc @@ -2929,6 +2929,17 @@ void TabletServiceImpl::ListMasterServers(const ListMasterServersRequestPB* req, context.RespondSuccess(); } +void TabletServiceImpl::ClearUniverseUuid(const ClearUniverseUuidRequestPB* req, + ClearUniverseUuidResponsePB* resp, + rpc::RpcContext context) { + const Status s = server_->tablet_manager()->server()->ClearUniverseUuid(); + if (!s.ok()) { + SetupErrorAndRespond(resp->mutable_error(), s, &context); + return; + } + context.RespondSuccess(); +} + void TabletServiceImpl::CheckTserverTabletHealth(const CheckTserverTabletHealthRequestPB* req, CheckTserverTabletHealthResponsePB* resp, rpc::RpcContext context) { diff --git a/src/yb/tserver/tablet_service.h b/src/yb/tserver/tablet_service.h index 10d9a4f42795..355e7666cd50 100644 --- a/src/yb/tserver/tablet_service.h +++ b/src/yb/tserver/tablet_service.h @@ -184,6 +184,10 @@ class TabletServiceImpl : public TabletServerServiceIf, public ReadTabletProvide ListMasterServersResponsePB* resp, rpc::RpcContext context) override; +void ClearUniverseUuid(const ClearUniverseUuidRequestPB* req, + ClearUniverseUuidResponsePB* resp, + rpc::RpcContext context) override; + void GetLockStatus(const GetLockStatusRequestPB* req, GetLockStatusResponsePB* resp, rpc::RpcContext context) override; diff --git a/src/yb/tserver/tserver.proto b/src/yb/tserver/tserver.proto index fab990e90629..291564f8be80 100644 --- a/src/yb/tserver/tserver.proto +++ b/src/yb/tserver/tserver.proto @@ -373,3 +373,10 @@ message CheckTserverTabletHealthResponsePB { message ClearAllMetaCachesOnServerRequestPB {} message ClearAllMetaCachesOnServerResponsePB {} + +message ClearUniverseUuidRequestPB { +} + +message ClearUniverseUuidResponsePB { + optional TabletServerErrorPB error = 1; +} diff --git a/src/yb/tserver/tserver_service.proto b/src/yb/tserver/tserver_service.proto index 6698c2e24f27..c2023426de22 100644 --- a/src/yb/tserver/tserver_service.proto +++ b/src/yb/tserver/tserver_service.proto @@ -124,6 +124,8 @@ service TabletServerService { rpc ClearAllMetaCachesOnServer(ClearAllMetaCachesOnServerRequestPB) returns (ClearAllMetaCachesOnServerResponsePB); + + rpc ClearUniverseUuid(ClearUniverseUuidRequestPB) returns (ClearUniverseUuidResponsePB); } // Note: Either among transactions_by_tablet or transaction_ids should be set. Both the fields