diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2021-11-23 21:46:41 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-11-23 21:46:41 +0100 |
commit | 3fc1bdd5329c6b3796bbf2619f78225a675e705b (patch) | |
tree | 267354537445d8a5169b483d3570b04819c488cc /storage | |
parent | 6e7385e7858ee5491f028c7012d9928ea340d678 (diff) |
Revert "Continue serving search queries when in Maintenance node state [run-systemtest]"
Diffstat (limited to 'storage')
6 files changed, 39 insertions, 156 deletions
diff --git a/storage/src/tests/persistence/filestorage/deactivatebucketstest.cpp b/storage/src/tests/persistence/filestorage/deactivatebucketstest.cpp index 5978f71db2f..597bb4b07ff 100644 --- a/storage/src/tests/persistence/filestorage/deactivatebucketstest.cpp +++ b/storage/src/tests/persistence/filestorage/deactivatebucketstest.cpp @@ -14,46 +14,11 @@ using namespace ::testing; namespace storage { struct DeactivateBucketsTest : FileStorTestFixture { - std::unique_ptr<TestFileStorComponents> _c; - - [[nodiscard]] bool is_active(const document::BucketId&) const; - - void SetUp() override { - FileStorTestFixture::SetUp(); - _c = std::make_unique<TestFileStorComponents>(*this); - - std::string up_state("storage:2 distributor:2"); - _node->getStateUpdater().setClusterState( - std::make_shared<const lib::ClusterState>(up_state)); - - createBucket(test_bucket()); - - api::BucketInfo serviceLayerInfo(1, 2, 3, 4, 5, true, true); - { - StorBucketDatabase::WrappedEntry entry( - _node->getStorageBucketDatabase().get(test_bucket(), "foo", - StorBucketDatabase::CREATE_IF_NONEXISTING)); - entry->info = serviceLayerInfo; - entry.write(); - } - } - - void TearDown() override { - _c.reset(); - FileStorTestFixture::TearDown(); - } - - static document::BucketId test_bucket() noexcept { - return {8, 123}; - } - - static std::shared_ptr<const lib::ClusterState> state_of(const char* str) { - return std::make_shared<const lib::ClusterState>(str); - } + bool isActive(const document::BucketId&) const; }; bool -DeactivateBucketsTest::is_active(const document::BucketId& bucket) const +DeactivateBucketsTest::isActive(const document::BucketId& bucket) const { StorBucketDatabase::WrappedEntry entry( _node->getStorageBucketDatabase().get(bucket, "foo")); @@ -61,53 +26,31 @@ DeactivateBucketsTest::is_active(const document::BucketId& bucket) const return entry->info.isActive(); } -TEST_F(DeactivateBucketsTest, buckets_deactivated_when_node_marked_down) -{ - EXPECT_TRUE(is_active(test_bucket())); - _node->getStateUpdater().setClusterState(state_of("storage:2 .1.s:d distributor:2")); - // Buckets should have been deactivated in content layer - EXPECT_FALSE(is_active(test_bucket())); -} - -TEST_F(DeactivateBucketsTest, buckets_not_deactivated_when_node_marked_maintenance) -{ - EXPECT_TRUE(is_active(test_bucket())); - _node->getStateUpdater().setClusterState(state_of("storage:2 .1.s:m distributor:2")); - EXPECT_TRUE(is_active(test_bucket())); -} - -TEST_F(DeactivateBucketsTest, buckets_deactivated_when_node_goes_from_maintenance_to_up) -{ - EXPECT_TRUE(is_active(test_bucket())); - _node->getStateUpdater().setClusterState(state_of("storage:2 .1.s:m distributor:2")); - _node->getStateUpdater().setClusterState(state_of("storage:2 distributor:2")); - EXPECT_FALSE(is_active(test_bucket())); -} - -TEST_F(DeactivateBucketsTest, buckets_deactivated_when_node_goes_from_maintenance_to_down) -{ - EXPECT_TRUE(is_active(test_bucket())); - _node->getStateUpdater().setClusterState(state_of("storage:2 .1.s:m distributor:2")); - _node->getStateUpdater().setClusterState(state_of("storage:2 distributor:2")); - EXPECT_FALSE(is_active(test_bucket())); -} +TEST_F(DeactivateBucketsTest, buckets_in_database_deactivated_when_node_down_in_cluster_state) { + TestFileStorComponents c(*this); + // Must set state to up first, or down-edge case won't trigger. + std::string upState("storage:2 distributor:2"); + _node->getStateUpdater().setClusterState( + lib::ClusterState::CSP(new lib::ClusterState(upState))); + + document::BucketId bucket(8, 123); + + createBucket(bucket); + api::BucketInfo serviceLayerInfo(1, 2, 3, 4, 5, true, true); + { + StorBucketDatabase::WrappedEntry entry( + _node->getStorageBucketDatabase().get(bucket, "foo", + StorBucketDatabase::CREATE_IF_NONEXISTING)); + entry->info = serviceLayerInfo; + entry.write(); + } + EXPECT_TRUE(isActive(bucket)); + std::string downState("storage:2 .1.s:d distributor:2"); + _node->getStateUpdater().setClusterState( + lib::ClusterState::CSP(new lib::ClusterState(downState))); -// If we only have a subset of the bucket spaces in maintenance mode (i.e. global -// bucket merge enforcement), we treat this as the node being down from the perspective -// of default space bucket deactivation. -TEST_F(DeactivateBucketsTest, bucket_space_subset_in_maintenance_deactivates_buckets) -{ - EXPECT_TRUE(is_active(test_bucket())); - auto derived = lib::ClusterStateBundle::BucketSpaceStateMapping({ - {document::FixedBucketSpaces::default_space(), state_of("storage:2 .1.s:m distributor:2")}, - {document::FixedBucketSpaces::global_space(), state_of("storage:2 distributor:2")} - }); - _node->getStateUpdater().setClusterStateBundle( - std::make_shared<const lib::ClusterStateBundle>(*state_of("storage:2 .1.s:m distributor:2"), - std::move(derived))); - EXPECT_FALSE(is_active(test_bucket())); + // Buckets should have been deactivated in content layer + EXPECT_FALSE(isActive(bucket)); } -// TODO should also test SPI interaction - } // namespace storage diff --git a/storage/src/vespa/storage/common/content_bucket_space.cpp b/storage/src/vespa/storage/common/content_bucket_space.cpp index 1d78f77525a..56bab9c6edc 100644 --- a/storage/src/vespa/storage/common/content_bucket_space.cpp +++ b/storage/src/vespa/storage/common/content_bucket_space.cpp @@ -57,18 +57,4 @@ ContentBucketSpace::setNodeUpInLastNodeStateSeenByProvider(bool nodeUpInLastNode _nodeUpInLastNodeStateSeenByProvider = nodeUpInLastNodeStateSeenByProvider; } -bool -ContentBucketSpace::getNodeMaintenanceInLastNodeStateSeenByProvider() const -{ - std::lock_guard guard(_lock); - return _nodeMaintenanceInLastNodeStateSeenByProvider; -} - -void -ContentBucketSpace::setNodeMaintenanceInLastNodeStateSeenByProvider(bool nodeMaintenanceInLastNodeStateSeenByProvider) -{ - std::lock_guard guard(_lock); - _nodeMaintenanceInLastNodeStateSeenByProvider = nodeMaintenanceInLastNodeStateSeenByProvider; -} - } diff --git a/storage/src/vespa/storage/common/content_bucket_space.h b/storage/src/vespa/storage/common/content_bucket_space.h index 836cd6e15f8..63379d6b8ee 100644 --- a/storage/src/vespa/storage/common/content_bucket_space.h +++ b/storage/src/vespa/storage/common/content_bucket_space.h @@ -23,7 +23,6 @@ private: std::shared_ptr<const lib::ClusterState> _clusterState; std::shared_ptr<const lib::Distribution> _distribution; bool _nodeUpInLastNodeStateSeenByProvider; - bool _nodeMaintenanceInLastNodeStateSeenByProvider; public: using UP = std::unique_ptr<ContentBucketSpace>; @@ -37,8 +36,6 @@ public: std::shared_ptr<const lib::Distribution> getDistribution() const; bool getNodeUpInLastNodeStateSeenByProvider() const; void setNodeUpInLastNodeStateSeenByProvider(bool nodeUpInLastNodeStateSeenByProvider); - bool getNodeMaintenanceInLastNodeStateSeenByProvider() const; - void setNodeMaintenanceInLastNodeStateSeenByProvider(bool nodeMaintenanceInLastNodeStateSeenByProvider); }; } diff --git a/storage/src/vespa/storage/common/content_bucket_space_repo.h b/storage/src/vespa/storage/common/content_bucket_space_repo.h index 048c2c266f0..7038e9cb1aa 100644 --- a/storage/src/vespa/storage/common/content_bucket_space_repo.h +++ b/storage/src/vespa/storage/common/content_bucket_space_repo.h @@ -21,8 +21,8 @@ private: public: explicit ContentBucketSpaceRepo(const ContentBucketDbOptions&); ContentBucketSpace &get(document::BucketSpace bucketSpace) const; - BucketSpaceMap::const_iterator begin() const noexcept { return _map.begin(); } - BucketSpaceMap::const_iterator end() const noexcept { return _map.end(); } + BucketSpaceMap::const_iterator begin() const { return _map.begin(); } + BucketSpaceMap::const_iterator end() const { return _map.end(); } BucketSpaces getBucketSpaces() const; size_t getBucketMemoryUsage() const; diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp index fc87845315f..c32efb6aa66 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp @@ -23,7 +23,6 @@ #include <vespa/vespalib/util/stringfmt.h> #include <vespa/vespalib/util/idestructorcallback.h> #include <vespa/vespalib/util/sequencedtaskexecutor.h> -#include <algorithm> #include <thread> #include <vespa/log/bufferedlogger.h> @@ -883,64 +882,28 @@ namespace { }; } -bool -FileStorManager::maintenance_in_all_spaces(const lib::Node& node) const noexcept -{ - return std::ranges::all_of(_component.getBucketSpaceRepo(), [&](const auto& elem) { - ContentBucketSpace& bucket_space = *elem.second; - auto derived_cluster_state = bucket_space.getClusterState(); - return derived_cluster_state->getNodeState(node).getState().oneOf("m"); - }); -} - -bool -FileStorManager::should_deactivate_buckets(const ContentBucketSpace& space, - bool node_up_in_space, - bool maintenance_in_all_spaces) noexcept -{ - // Important: this MUST match the semantics in proton::BucketHandler::notifyClusterStateChanged()! - // Otherwise, the content layer and proton will be out of sync in terms of bucket activation state. - if (maintenance_in_all_spaces) { - return false; - } - return ((space.getNodeUpInLastNodeStateSeenByProvider() && !node_up_in_space) - || space.getNodeMaintenanceInLastNodeStateSeenByProvider()); -} - -void -FileStorManager::maybe_log_received_cluster_state() -{ - if (LOG_WOULD_LOG(debug)) { - auto cluster_state_bundle = _component.getStateUpdater().getClusterStateBundle(); - auto baseline_state = cluster_state_bundle->getBaselineClusterState(); - LOG(debug, "FileStorManager received baseline cluster state '%s'", baseline_state->toString().c_str()); - } -} - void FileStorManager::updateState() { - maybe_log_received_cluster_state(); - const lib::Node node(_component.getNodeType(), _component.getIndex()); - const bool in_maintenance = maintenance_in_all_spaces(node); + auto clusterStateBundle = _component.getStateUpdater().getClusterStateBundle(); + lib::ClusterState::CSP state(clusterStateBundle->getBaselineClusterState()); + lib::Node node(_component.getNodeType(), _component.getIndex()); + LOG(debug, "FileStorManager received cluster state '%s'", state->toString().c_str()); for (const auto &elem : _component.getBucketSpaceRepo()) { BucketSpace bucketSpace(elem.first); - ContentBucketSpace& contentBucketSpace = *elem.second; + ContentBucketSpace &contentBucketSpace = *elem.second; auto derivedClusterState = contentBucketSpace.getClusterState(); - const bool node_up_in_space = derivedClusterState->getNodeState(node).getState().oneOf("uir"); - if (should_deactivate_buckets(contentBucketSpace, node_up_in_space, in_maintenance)) { - LOG(debug, "Received cluster state where this node is down; de-activating all buckets " - "in database for bucket space %s", bucketSpace.toString().c_str()); + bool nodeUp = derivedClusterState->getNodeState(node).getState().oneOf("uir"); + // If edge where we go down + if (contentBucketSpace.getNodeUpInLastNodeStateSeenByProvider() && !nodeUp) { + LOG(debug, "Received cluster state where this node is down; de-activating all buckets in database for bucket space %s", bucketSpace.toString().c_str()); Deactivator deactivator; contentBucketSpace.bucketDatabase().for_each_mutable_unordered( std::ref(deactivator), "FileStorManager::updateState"); } - contentBucketSpace.setNodeUpInLastNodeStateSeenByProvider(node_up_in_space); - contentBucketSpace.setNodeMaintenanceInLastNodeStateSeenByProvider(in_maintenance); - spi::ClusterState spiState(*derivedClusterState, _component.getIndex(), - *contentBucketSpace.getDistribution(), - in_maintenance); + contentBucketSpace.setNodeUpInLastNodeStateSeenByProvider(nodeUp); + spi::ClusterState spiState(*derivedClusterState, _component.getIndex(), *contentBucketSpace.getDistribution()); _provider->setClusterState(bucketSpace, spiState); } } diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.h b/storage/src/vespa/storage/persistence/filestorage/filestormanager.h index b7450de13d8..76cb31f32d4 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.h +++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.h @@ -39,7 +39,6 @@ namespace api { } namespace spi { struct PersistenceProvider; } -class ContentBucketSpace; struct FileStorManagerTest; class ReadBucketList; class BucketOwnershipNotifier; @@ -171,11 +170,6 @@ private: void onFlush(bool downwards) override; void reportHtmlStatus(std::ostream&, const framework::HttpUrlPath&) const override; void storageDistributionChanged() override; - [[nodiscard]] static bool should_deactivate_buckets(const ContentBucketSpace& space, - bool node_up_in_space, - bool maintenance_in_all_spaces) noexcept; - [[nodiscard]] bool maintenance_in_all_spaces(const lib::Node& node) const noexcept; - void maybe_log_received_cluster_state(); void updateState(); void propagateClusterStates(); void update_reported_state_after_db_init(); |