diff options
Diffstat (limited to 'slobrok')
12 files changed, 216 insertions, 83 deletions
diff --git a/slobrok/CMakeLists.txt b/slobrok/CMakeLists.txt index 153a029e74d..6acbf5d5134 100644 --- a/slobrok/CMakeLists.txt +++ b/slobrok/CMakeLists.txt @@ -22,6 +22,7 @@ vespa_define_module( src/tests/mirrorapi src/tests/registerapi src/tests/service_map_history + src/tests/service_map_mirror src/tests/standalone src/tests/startsome src/tests/startup diff --git a/slobrok/src/tests/service_map_mirror/CMakeLists.txt b/slobrok/src/tests/service_map_mirror/CMakeLists.txt new file mode 100644 index 00000000000..2af34702068 --- /dev/null +++ b/slobrok/src/tests/service_map_mirror/CMakeLists.txt @@ -0,0 +1,9 @@ +# Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +vespa_add_executable(slobrok_service_map_mirror_test_app TEST + SOURCES + service_map_mirror_test.cpp + DEPENDS + slobrok_slobrokserver + GTest::GTest +) +vespa_add_test(NAME slobrok_service_map_mirror_test_app COMMAND slobrok_service_map_mirror_test_app) diff --git a/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp b/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp new file mode 100644 index 00000000000..b1d6c3fb1c1 --- /dev/null +++ b/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp @@ -0,0 +1,101 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include <vespa/slobrok/server/mock_map_listener.h> +#include <vespa/slobrok/server/service_map_mirror.h> +#include <vespa/vespalib/gtest/gtest.h> +#include <vespa/vespalib/util/stringfmt.h> +#include <map> + +using namespace vespalib; +using namespace slobrok; +using vespalib::make_string_short::fmt; + +using Map = std::map<vespalib::string, vespalib::string>; + +Map dump(const ServiceMapMirror &history) { + Map result; + for (const auto & entry : history.allMappings()) { + result[entry.name] = entry.spec; + } + return result; +} + + +void addTo(ServiceMapMirror &target, const ServiceMapping &mapping) { + auto cur = target.currentGeneration(); + std::vector<vespalib::string> removes = {}; + ServiceMappingList updates = { mapping }; + auto nxt = cur; + nxt.add(); + MapDiff diff{cur, removes, updates, nxt}; + target.apply(diff); +} + +void removeFrom(ServiceMapMirror &target, const ServiceMapping &mapping) { + auto cur = target.currentGeneration(); + std::vector<vespalib::string> removes = { mapping.name }; + ServiceMappingList updates = { }; + auto nxt = cur; + nxt.add(); + MapDiff diff{cur, removes, updates, nxt}; + target.apply(diff); +} + +TEST(ServiceMapMirrorTest, empty_inspection) { + ServiceMapMirror mirror; + auto bar = dump(mirror); + EXPECT_TRUE(bar.empty()); + + MockMapListener observer; + mirror.registerListener(observer); + mirror.unregisterListener(observer); + EXPECT_EQ(observer.last_event, MockEvent::NONE); +} + +TEST(ServiceMapMirrorTest, full_inspection) { + ServiceMapMirror mirror; + MockMapListener observer; + mirror.registerListener(observer); + for (int i = 0; i < 1984; ++i) { + EXPECT_EQ(mirror.currentGeneration(), GenCnt(i)); + auto name = fmt("key/%d/name", i); + auto spec = fmt("tcp/host%d.domain.tld:19099", 10000+i); + ServiceMapping toAdd{name, spec}; + addTo(mirror, toAdd); + EXPECT_EQ(observer.last_event, MockEvent::ADD); + EXPECT_EQ(observer.last_add, toAdd); + } + EXPECT_EQ(mirror.currentGeneration(), GenCnt(1984)); + ServiceMapping toRemove{"key/666/name", "tcp/host10666.domain.tld:19099"}; + removeFrom(mirror, toRemove); + EXPECT_EQ(observer.last_event, MockEvent::REMOVE); + EXPECT_EQ(observer.last_remove, toRemove); + EXPECT_EQ(mirror.currentGeneration(), GenCnt(1985)); + + ServiceMapping oldMapping{"key/1969/name", "tcp/host11969.domain.tld:19099"}; + ServiceMapping toUpdate{"key/1969/name", "tcp/woodstock:19069"}; + addTo(mirror, toUpdate); + EXPECT_EQ(observer.last_event, MockEvent::UPDATE); + EXPECT_EQ(observer.last_remove, oldMapping); + EXPECT_EQ(observer.last_add, toUpdate); + EXPECT_EQ(mirror.currentGeneration(), GenCnt(1986)); + + auto map = dump(mirror); + EXPECT_FALSE(map.contains("foo")); + EXPECT_TRUE(map.contains("key/0/name")); + EXPECT_FALSE(map.contains("key/666/name")); + EXPECT_TRUE(map.contains("key/1983/name")); + EXPECT_FALSE(map.contains("key/1984/name")); + EXPECT_TRUE(map.contains("key/1969/name")); + EXPECT_EQ(map["key/0/name"], "tcp/host10000.domain.tld:19099"); + EXPECT_EQ(map["key/123/name"], "tcp/host10123.domain.tld:19099"); + EXPECT_EQ(map["key/1983/name"], "tcp/host11983.domain.tld:19099"); + EXPECT_EQ(map["key/1969/name"], "tcp/woodstock:19069"); + EXPECT_EQ(map.size(), 1983ul); + + mirror.unregisterListener(observer); +} + + +GTEST_MAIN_RUN_ALL_TESTS() + diff --git a/slobrok/src/tests/union_service_map/union_service_map_test.cpp b/slobrok/src/tests/union_service_map/union_service_map_test.cpp index 5f1f70fb9fb..30db95324bb 100644 --- a/slobrok/src/tests/union_service_map/union_service_map_test.cpp +++ b/slobrok/src/tests/union_service_map/union_service_map_test.cpp @@ -1,5 +1,6 @@ // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#include <vespa/slobrok/server/mock_map_listener.h> #include <vespa/slobrok/server/union_service_map.h> #include <vespa/vespalib/gtest/gtest.h> #include <vespa/vespalib/util/stringfmt.h> @@ -8,70 +9,32 @@ using namespace vespalib; using namespace slobrok; using vespalib::make_string_short::fmt; -enum class Event { NONE, ADD, REMOVE, UPDATE }; - -struct MapObserver : public MapListener { - MapObserver(); - virtual ~MapObserver(); - void add(const ServiceMapping &mapping) override; - void remove(const ServiceMapping &mapping) override; - void update(const ServiceMapping &old_mapping, - const ServiceMapping &new_mapping) override; - - Event last_event = Event::NONE; - ServiceMapping last_add = {{}, {}}; - ServiceMapping last_remove = {{}, {}}; - - void clear() { last_event = Event::NONE; } -}; - -MapObserver::MapObserver() = default; -MapObserver::~MapObserver() = default; - -void MapObserver::add(const ServiceMapping &mapping) { - last_event = Event::ADD; - last_add = mapping; -} - -void MapObserver::remove(const ServiceMapping &mapping) { - last_event = Event::REMOVE; - last_remove = mapping; -} - -void MapObserver::update(const ServiceMapping &old_mapping, - const ServiceMapping &new_mapping) -{ - last_event = Event::UPDATE; - last_remove = old_mapping; - last_add = new_mapping; -} - TEST(UnionServiceMapTest, forwards_simple_requests) { ProxyMapSource source; UnionServiceMap unionizer; - MapObserver observer; + MockMapListener observer; unionizer.registerListener(observer); source.registerListener(unionizer); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); ServiceMapping one{"foo/1", "bar/1"}; source.add(one); - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_add, one); ServiceMapping two{"foo/2", "bar/2"}; source.add(two); - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_add, two); source.remove(one); - EXPECT_EQ(observer.last_event, Event::REMOVE); + EXPECT_EQ(observer.last_event, MockEvent::REMOVE); EXPECT_EQ(observer.last_remove, one); ServiceMapping two_q{"foo/2", "qux/2"}; source.update(two, two_q); // update implemented ass remove+add: - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_remove, two); EXPECT_EQ(observer.last_add, two_q); } @@ -81,47 +44,47 @@ TEST(UnionServiceMapTest, handles_refcount) { ProxyMapSource source2; ProxyMapSource source3; UnionServiceMap unionizer; - MapObserver observer; + MockMapListener observer; unionizer.registerListener(observer); source1.registerListener(unionizer); source2.registerListener(unionizer); source3.registerListener(unionizer); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); ServiceMapping one{"foo/1", "bar/1"}; source1.add(one); - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_add, one); observer.clear(); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source2.add(one); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source3.add(one); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); ServiceMapping two{"foo/2", "bar/2"}; source1.add(two); - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_add, two); observer.clear(); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source2.add(two); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source1.remove(one); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source2.remove(one); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source1.remove(two); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source2.remove(two); - EXPECT_EQ(observer.last_event, Event::REMOVE); + EXPECT_EQ(observer.last_event, MockEvent::REMOVE); EXPECT_EQ(observer.last_remove, two); observer.clear(); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source3.remove(one); - EXPECT_EQ(observer.last_event, Event::REMOVE); + EXPECT_EQ(observer.last_event, MockEvent::REMOVE); EXPECT_EQ(observer.last_remove, one); } @@ -130,48 +93,48 @@ TEST(UnionServiceMapTest, handles_conflicts) { ProxyMapSource source2; ProxyMapSource source3; UnionServiceMap unionizer; - MapObserver observer; + MockMapListener observer; unionizer.registerListener(observer); source1.registerListener(unionizer); source2.registerListener(unionizer); source3.registerListener(unionizer); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); ServiceMapping one{"foo/1", "bar/1"}; source1.add(one); - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_add, one); observer.clear(); source2.add(one); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); ServiceMapping two{"foo/2", "bar/2"}; source1.add(two); - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_add, two); observer.clear(); source2.add(two); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); ServiceMapping one_q{"foo/1", "qux/1"}; source3.add(one_q); - EXPECT_EQ(observer.last_event, Event::REMOVE); + EXPECT_EQ(observer.last_event, MockEvent::REMOVE); EXPECT_EQ(observer.last_remove, one); ServiceMapping two_q{"foo/2", "qux/2"}; source3.add(two_q); - EXPECT_EQ(observer.last_event, Event::REMOVE); + EXPECT_EQ(observer.last_event, MockEvent::REMOVE); EXPECT_EQ(observer.last_remove, two); source3.remove(one_q); - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_add, one); observer.clear(); source1.remove(two); - EXPECT_EQ(observer.last_event, Event::NONE); + EXPECT_EQ(observer.last_event, MockEvent::NONE); source2.remove(two); - EXPECT_EQ(observer.last_event, Event::ADD); + EXPECT_EQ(observer.last_event, MockEvent::ADD); EXPECT_EQ(observer.last_add, two_q); } diff --git a/slobrok/src/vespa/slobrok/server/CMakeLists.txt b/slobrok/src/vespa/slobrok/server/CMakeLists.txt index 1492676cd3a..20f5bda4278 100644 --- a/slobrok/src/vespa/slobrok/server/CMakeLists.txt +++ b/slobrok/src/vespa/slobrok/server/CMakeLists.txt @@ -11,8 +11,8 @@ vespa_add_library(slobrok_slobrokserver map_diff.cpp map_listener.cpp map_source.cpp - service_map_mirror.cpp metrics_producer.cpp + mock_map_listener.cpp monitor.cpp named_service.cpp proxy_map_source.cpp @@ -26,6 +26,7 @@ vespa_add_library(slobrok_slobrokserver rpcmirror.cpp sbenv.cpp service_map_history.cpp + service_map_mirror.cpp service_mapping.cpp slobrokserver.cpp union_service_map.cpp diff --git a/slobrok/src/vespa/slobrok/server/exchange_manager.cpp b/slobrok/src/vespa/slobrok/server/exchange_manager.cpp index a608c4b3cba..632c823a2c1 100644 --- a/slobrok/src/vespa/slobrok/server/exchange_manager.cpp +++ b/slobrok/src/vespa/slobrok/server/exchange_manager.cpp @@ -25,8 +25,7 @@ ExchangeManager::~ExchangeManager() = default; OkState ExchangeManager::addPartner(const std::string & name, const std::string & spec) { - RemoteSlobrok *oldremote = lookupPartner(name); - if (oldremote != nullptr) { + if (RemoteSlobrok *oldremote = lookupPartner(name)) { // already a partner, should be OK if (spec != oldremote->getSpec()) { return OkState(FRTE_RPC_METHOD_FAILED, "name already partner with different spec"); @@ -37,11 +36,9 @@ ExchangeManager::addPartner(const std::string & name, const std::string & spec) } return OkState(); } - - LOG_ASSERT(_partners.find(name) == _partners.end()); - auto newPartner = std::make_unique<RemoteSlobrok>(name, spec, *this); - RemoteSlobrok & partner = *newPartner; - _partners.emplace(name, std::move(newPartner)); + auto [ it, wasNew ] = _partners.emplace(name, std::make_unique<RemoteSlobrok>(name, spec, *this)); + LOG_ASSERT(wasNew); + RemoteSlobrok & partner = *it->second; partner.tryConnect(); return OkState(); } @@ -53,6 +50,7 @@ ExchangeManager::removePartner(const std::string & name) auto oldremote = std::move(_partners[name]); LOG_ASSERT(oldremote); _partners.erase(name); + oldremote->shutdown(); } std::vector<std::string> diff --git a/slobrok/src/vespa/slobrok/server/mock_map_listener.cpp b/slobrok/src/vespa/slobrok/server/mock_map_listener.cpp new file mode 100644 index 00000000000..9f4b670ddf4 --- /dev/null +++ b/slobrok/src/vespa/slobrok/server/mock_map_listener.cpp @@ -0,0 +1,28 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include "mock_map_listener.h" + +namespace slobrok { + +MockMapListener::MockMapListener() = default; +MockMapListener::~MockMapListener() = default; + +void MockMapListener::add(const ServiceMapping &mapping) { + last_event = MockEvent::ADD; + last_add = mapping; +} + +void MockMapListener::remove(const ServiceMapping &mapping) { + last_event = MockEvent::REMOVE; + last_remove = mapping; +} + +void MockMapListener::update(const ServiceMapping &old_mapping, + const ServiceMapping &new_mapping) +{ + last_event = MockEvent::UPDATE; + last_remove = old_mapping; + last_add = new_mapping; +} + +} diff --git a/slobrok/src/vespa/slobrok/server/mock_map_listener.h b/slobrok/src/vespa/slobrok/server/mock_map_listener.h new file mode 100644 index 00000000000..0c2b51cca2c --- /dev/null +++ b/slobrok/src/vespa/slobrok/server/mock_map_listener.h @@ -0,0 +1,26 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#pragma once + +#include "map_listener.h" + +namespace slobrok { + +enum class MockEvent { NONE, ADD, REMOVE, UPDATE }; + +struct MockMapListener : public MapListener { + MockMapListener(); + virtual ~MockMapListener(); + void add(const ServiceMapping &mapping) override; + void remove(const ServiceMapping &mapping) override; + void update(const ServiceMapping &old_mapping, + const ServiceMapping &new_mapping) override; + + MockEvent last_event = MockEvent::NONE; + ServiceMapping last_add = {{}, {}}; + ServiceMapping last_remove = {{}, {}}; + + void clear() { last_event = MockEvent::NONE; } +}; + +} diff --git a/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp b/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp index 0b9872c03e2..78ca25d4723 100644 --- a/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp +++ b/slobrok/src/vespa/slobrok/server/remote_slobrok.cpp @@ -33,8 +33,7 @@ RemoteSlobrok::RemoteSlobrok(const std::string &name, const std::string &spec, _rpcserver.healthCheck(); } -RemoteSlobrok::~RemoteSlobrok() -{ +void RemoteSlobrok::shutdown() { _reconnecter.disable(); _pending.clear(); @@ -59,9 +58,13 @@ RemoteSlobrok::~RemoteSlobrok() if (_remRemReq != nullptr) { _remRemReq->Abort(); } - // _rpcserver destructor called automatically + _serviceMapMirror.clear(); } +RemoteSlobrok::~RemoteSlobrok() { + shutdown(); + // _rpcserver destructor called automatically +} void RemoteSlobrok::doPending() diff --git a/slobrok/src/vespa/slobrok/server/remote_slobrok.h b/slobrok/src/vespa/slobrok/server/remote_slobrok.h index 9e3fb1df55f..e463ac6be21 100644 --- a/slobrok/src/vespa/slobrok/server/remote_slobrok.h +++ b/slobrok/src/vespa/slobrok/server/remote_slobrok.h @@ -76,6 +76,7 @@ public: const std::string & getName() const { return _rpcserver.getName(); } const std::string & getSpec() const { return _rpcserver.getSpec(); } ServiceMapMirror &remoteMap() { return _serviceMapMirror; } + void shutdown(); // interfaces implemented: void notifyFailedRpcSrv(ManagedRpcServer *rpcsrv, std::string errmsg) override; diff --git a/slobrok/src/vespa/slobrok/server/service_map_mirror.cpp b/slobrok/src/vespa/slobrok/server/service_map_mirror.cpp index 8b9a40efed7..51db84e24a4 100644 --- a/slobrok/src/vespa/slobrok/server/service_map_mirror.cpp +++ b/slobrok/src/vespa/slobrok/server/service_map_mirror.cpp @@ -13,7 +13,7 @@ ServiceMapMirror::ServiceMapMirror() {} ServiceMapMirror::~ServiceMapMirror() { - clear(); + LOG_ASSERT(_listeners.size() == 0); } void ServiceMapMirror::apply(const MapDiff &diff) { @@ -87,4 +87,3 @@ void ServiceMapMirror::unregisterListener(MapListener &listener) { } // namespace slobrok - diff --git a/slobrok/src/vespa/slobrok/server/union_service_map.cpp b/slobrok/src/vespa/slobrok/server/union_service_map.cpp index f3d63f2a087..4a8ca51d409 100644 --- a/slobrok/src/vespa/slobrok/server/union_service_map.cpp +++ b/slobrok/src/vespa/slobrok/server/union_service_map.cpp @@ -17,10 +17,12 @@ void UnionServiceMap::add(const ServiceMapping &mapping) if (iter == _mappings.end()) { _mappings[key].emplace_back(mapping.spec, 1u); _proxy.add(mapping); + LOG(debug, "add new %s->%s", mapping.name.c_str(), mapping.spec.c_str()); } else { Mappings &values = iter->second; for (CountedSpec &old : values) { if (old.spec == mapping.spec) { + LOG(debug, "add ref to existing %s->%s", mapping.name.c_str(), mapping.spec.c_str()); ++old.count; return; } @@ -41,6 +43,7 @@ void UnionServiceMap::remove(const ServiceMapping &mapping) LOG(error, "Broken invariant: did not find %s in mappings", key.c_str()); return; } + LOG(debug, "remove ref from %s->%s", mapping.name.c_str(), mapping.spec.c_str()); Mappings &values = iter->second; bool found = false; for (CountedSpec &old : values) { |