diff options
15 files changed, 63 insertions, 59 deletions
diff --git a/config-provisioning/src/main/resources/configdefinitions/config.provisioning.node-repository.def b/config-provisioning/src/main/resources/configdefinitions/config.provisioning.node-repository.def index b054f434322..45815284dce 100644 --- a/config-provisioning/src/main/resources/configdefinitions/config.provisioning.node-repository.def +++ b/config-provisioning/src/main/resources/configdefinitions/config.provisioning.node-repository.def @@ -9,6 +9,3 @@ tenantContainerImage string default="" # Whether to cache data read from ZooKeeper in-memory. useCuratorClientCache bool default=false - -# The number of Node objects to cache in-memory. -nodeCacheSize long default=3000 diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index 7f54fff5c70..17832ff9550 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -84,8 +84,7 @@ public class NodeRepository extends AbstractComponent { flagSource, metricsDb, config.useCuratorClientCache(), - zone.environment().isProduction() && !zone.getCloud().dynamicProvisioning() && !zone.system().isCd() ? 1 : 0, - config.nodeCacheSize()); + zone.environment().isProduction() && !zone.getCloud().dynamicProvisioning() && !zone.system().isCd() ? 1 : 0); } /** @@ -103,14 +102,13 @@ public class NodeRepository extends AbstractComponent { FlagSource flagSource, MetricsDb metricsDb, boolean useCuratorClientCache, - int spareCount, - long nodeCacheSize) { + int spareCount) { if (provisionServiceProvider.getHostProvisioner().isPresent() != zone.getCloud().dynamicProvisioning()) throw new IllegalArgumentException(String.format( "dynamicProvisioning property must be 1-to-1 with availability of HostProvisioner, was: dynamicProvisioning=%s, hostProvisioner=%s", zone.getCloud().dynamicProvisioning(), provisionServiceProvider.getHostProvisioner().map(__ -> "present").orElse("empty"))); - this.db = new CuratorDatabaseClient(flavors, curator, clock, useCuratorClientCache, nodeCacheSize); + this.db = new CuratorDatabaseClient(flavors, curator, clock, useCuratorClientCache); this.zone = zone; this.clock = clock; this.nodes = new Nodes(db, zone, clock); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java index ca14a1be4c4..acf7aa5c28e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporter.java @@ -133,7 +133,7 @@ public class MetricsReporter extends NodeRepositoryMaintainer { } private void updateCacheMetrics() { - CacheStats nodeCacheStats = nodeRepository().database().nodeSerializerCacheStats(); + CacheStats nodeCacheStats = nodeRepository().database().objectCacheStats(); metric.set("cache.nodeObject.hitRate", nodeCacheStats.hitRate(), null); metric.set("cache.nodeObject.evictionCount", nodeCacheStats.evictionCount(), null); metric.set("cache.nodeObject.size", nodeCacheStats.size(), null); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java index 659636de581..83cfe72d9e9 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java @@ -2,7 +2,6 @@ package com.yahoo.vespa.hosted.provision.persistence; import com.google.common.cache.AbstractCache; -import com.google.common.collect.ImmutableList; import com.yahoo.config.provision.HostName; import com.yahoo.path.Path; import com.yahoo.transaction.NestedTransaction; @@ -10,12 +9,14 @@ import com.yahoo.vespa.curator.Curator; import com.yahoo.vespa.curator.Lock; import com.yahoo.vespa.curator.recipes.CuratorCounter; import com.yahoo.vespa.curator.transaction.CuratorTransaction; +import org.apache.zookeeper.data.Stat; import java.time.Duration; import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.OptionalInt; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; @@ -155,7 +156,7 @@ public class CuratorDatabase { @Override public List<String> getChildren(Path path) { - return get(children, path, () -> ImmutableList.copyOf(curator.getChildren(path))); + return get(children, path, () -> List.copyOf(curator.getChildren(path))); } @Override @@ -163,6 +164,11 @@ public class CuratorDatabase { return get(data, path, () -> curator.getData(path)).map(data -> Arrays.copyOf(data, data.length)); } + @Override + public OptionalInt getVersion(Path path) { + return curator.getStat(path).stream().mapToInt(Stat::getVersion).findFirst(); + } + private <T> T get(Map<Path, T> values, Path path, Supplier<T> loader) { return values.compute(path, (key, value) -> { if (value == null) { @@ -202,10 +208,13 @@ public class CuratorDatabase { List<String> getChildren(Path path); /** - * Returns the a copy of the content of this child - which may be empty. + * Returns a copy of the content of this child - which may be empty. */ Optional<byte[]> getData(Path path); + /** Returns the current version of data stored in given path, if path exists. This should never be cached */ + OptionalInt getVersion(Path path); + } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java index 379bb2566df..37b2f9ba665 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java @@ -1,6 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.provision.persistence; +import com.google.common.cache.AbstractCache; +import com.yahoo.collections.Pair; import com.yahoo.component.Version; import com.yahoo.concurrent.UncheckedTimeoutException; import com.yahoo.config.provision.ApplicationId; @@ -35,7 +37,9 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.OptionalInt; import java.util.TreeMap; +import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; import java.util.function.Predicate; import java.util.logging.Level; @@ -76,8 +80,13 @@ public class CuratorDatabaseClient { private final Clock clock; private final CuratorCounter provisionIndexCounter; - public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, boolean useCache, long nodeCacheSize) { - this.nodeSerializer = new NodeSerializer(flavors, nodeCacheSize); + // Cache Node objects and their znode version. The cache is invalidated if node data is changed, i.e. the znode + // version is changed. This will grow to keep all Node objects in memory, which should be fine. + private final Map<Path, Pair<Integer, Node>> cachedNodes = new ConcurrentHashMap<>(); + private final AbstractCache.SimpleStatsCounter cacheStats = new AbstractCache.SimpleStatsCounter(); + + public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, boolean useCache) { + this.nodeSerializer = new NodeSerializer(flavors); this.db = new CuratorDatabase(curator, root, useCache); this.clock = clock; this.provisionIndexCounter = new CuratorCounter(curator, root.append("provisionIndexCounter")); @@ -267,9 +276,27 @@ public class CuratorDatabaseClient { if (states.length == 0) states = Node.State.values(); for (Node.State state : states) { - Optional<byte[]> nodeData = session.getData(toPath(state, hostname)); - if (nodeData.isPresent()) - return nodeData.map((data) -> nodeSerializer.fromJson(state, data)); + Path path = toPath(state, hostname); + OptionalInt version = session.getVersion(path); + if (version.isEmpty()) { + continue; + } + Pair<Integer, Node> versionAndNode = cachedNodes.compute(path, (ignored, old) -> { + if (old != null && old.getFirst() == version.getAsInt()) { + cacheStats.recordHits(1); + return old; // Cached entry has correct version + } else { + cacheStats.recordMisses(1); + } + Optional<byte[]> data = session.getData(path); + if (data.isEmpty()) { + return null; // Node disappeared after we read the version + } + return new Pair<>(version.getAsInt(), nodeSerializer.fromJson(state, data.get())); + }); + if (versionAndNode != null) { + return Optional.of(versionAndNode.getSecond()); + } } return Optional.empty(); } @@ -525,8 +552,9 @@ public class CuratorDatabaseClient { return db.cacheStats(); } - public CacheStats nodeSerializerCacheStats() { - return nodeSerializer.cacheStats(); + public CacheStats objectCacheStats() { + var snapshot = this.cacheStats.snapshot(); + return new CacheStats(snapshot.hitRate(), snapshot.evictionCount(), cachedNodes.size()); } private <T> Optional<T> read(Path path, Function<byte[], T> mapper) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java index 543972a9cb3..902505e82e3 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java @@ -1,11 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.provision.persistence; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; import com.google.common.collect.ImmutableSet; -import com.google.common.hash.Hashing; -import com.google.common.util.concurrent.UncheckedExecutionException; import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationName; @@ -39,13 +35,11 @@ import com.yahoo.vespa.hosted.provision.node.Status; import com.yahoo.vespa.hosted.provision.node.TrustStoreItem; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.time.Instant; import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.concurrent.ExecutionException; import java.util.stream.Collectors; /** @@ -128,17 +122,10 @@ public class NodeSerializer { private static final String fingerprintKey = "fingerprint"; private static final String expiresKey = "expires"; - // A cache of deserialized Node objects. The cache is keyed on the hash of serialized node data. - // - // Deserializing a Node from slime is expensive, and happens frequently. Node instances that have already been - // deserialized are returned from this cache instead of being deserialized again. - private final Cache<Long, Node> cache; - // ---------------- Serialization ---------------------------------------------------- - public NodeSerializer(NodeFlavors flavors, long cacheSize) { + public NodeSerializer(NodeFlavors flavors) { this.flavors = flavors; - this.cache = CacheBuilder.newBuilder().maximumSize(cacheSize).recordStats().build(); } public byte[] toJson(Node node) { @@ -152,12 +139,6 @@ public class NodeSerializer { } } - /** Returns cache statistics for this serializer */ - public CacheStats cacheStats() { - var stats = cache.stats(); - return new CacheStats(stats.hitRate(), stats.evictionCount(), cache.size()); - } - private void toSlime(Node node, Cursor object) { object.setString(hostnameKey, node.hostname()); toSlime(node.ipConfig().primary(), object.setArray(ipAddressesKey)); @@ -254,15 +235,7 @@ public class NodeSerializer { // ---------------- Deserialization -------------------------------------------------- public Node fromJson(Node.State state, byte[] data) { - long key = Hashing.sipHash24().newHasher() - .putString(state.name(), StandardCharsets.UTF_8) - .putBytes(data).hash() - .asLong(); - try { - return cache.get(key, () -> nodeFromSlime(state, SlimeUtils.jsonToSlime(data).get())); - } catch (ExecutionException e) { - throw new UncheckedExecutionException(e); - } + return nodeFromSlime(state, SlimeUtils.jsonToSlime(data).get()); } private Node nodeFromSlime(Node.State state, Inspector object) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java index 1a2d5294aa5..c1e26099ee4 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java @@ -71,7 +71,7 @@ public class MockNodeRepository extends NodeRepository { new InMemoryFlagSource(), new MemoryMetricsDb(Clock.fixed(Instant.ofEpochMilli(123), ZoneId.of("Z"))), true, - 0, 1000); + 0); this.flavors = flavors; curator.setZooKeeperEnsembleConnectionSpec("cfg1:1234,cfg2:1234,cfg3:1234"); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java index b391292884f..1287c073981 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java @@ -48,7 +48,7 @@ public class NodeRepositoryTester { new InMemoryFlagSource(), new MemoryMetricsDb(clock), true, - 0, 1000); + 0); } public NodeRepository nodeRepository() { return nodeRepository; } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java index 0e732984227..0944138e06c 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/RealDataScenarioTest.java @@ -136,7 +136,7 @@ public class RealDataScenarioTest { } private static void initFromZk(NodeRepository nodeRepository, Path pathToZkSnapshot) { - NodeSerializer nodeSerializer = new NodeSerializer(nodeRepository.flavors(), 1000); + NodeSerializer nodeSerializer = new NodeSerializer(nodeRepository.flavors()); AtomicReference<Node.State> state = new AtomicReference<>(); Pattern zkNodePathPattern = Pattern.compile(".?/provision/v1/([a-z]+)/[a-z0-9.-]+\\.(com|cloud).?"); Consumer<String> consumer = input -> { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTester.java index e06bdba90fb..1c9532e4665 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityCheckerTester.java @@ -74,7 +74,7 @@ public class CapacityCheckerTester { new InMemoryFlagSource(), new MemoryMetricsDb(clock), true, - 0, 1000); + 0); } private void updateCapacityChecker() { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java index 8c649243d61..eda5d201949 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java @@ -124,9 +124,9 @@ public class MetricsReporterTest { expectedMetrics.put("cache.nodeObject.size", 2L); nodeRepository.nodes().list(); - expectedMetrics.put("cache.curator.hitRate", 0.52D); + expectedMetrics.put("cache.curator.hitRate", 0.5D); expectedMetrics.put("cache.curator.evictionCount", 0L); - expectedMetrics.put("cache.curator.size", 12L); + expectedMetrics.put("cache.curator.size", 11L); tester.clock().setInstant(Instant.ofEpochSecond(124)); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java index 373bfe20162..ca117ab1395 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java @@ -268,7 +268,7 @@ public class SpareCapacityMaintainerTest { new InMemoryFlagSource(), new MemoryMetricsDb(clock), true, - 1, 1000); + 1); deployer = new MockDeployer(nodeRepository); maintainer = new SpareCapacityMaintainer(deployer, nodeRepository, metric, Duration.ofDays(1), maxIterations); } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java index f11cfa9a15d..5a04986338d 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java @@ -24,7 +24,7 @@ public class CuratorDatabaseClientTest { private final Curator curator = new MockCurator(); private final CuratorDatabaseClient zkClient = new CuratorDatabaseClient( - FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), true, 1000); + FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), true); @Test public void can_read_stored_host_information() throws Exception { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java index 3de3f4139d1..31bf8ee43e2 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java @@ -56,7 +56,7 @@ import static org.junit.Assert.assertTrue; public class NodeSerializerTest { private final NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default", "large", "ugccloud-container"); - private final NodeSerializer nodeSerializer = new NodeSerializer(nodeFlavors, 1000); + private final NodeSerializer nodeSerializer = new NodeSerializer(nodeFlavors); private final ManualClock clock = new ManualClock(); @Test diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java index c478840780f..9a709748c1f 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java @@ -115,8 +115,7 @@ public class ProvisioningTester { flagSource, new MemoryMetricsDb(clock), true, - spareCount, - 1000); + spareCount); this.orchestrator = orchestrator; this.provisioner = new NodeRepositoryProvisioner(nodeRepository, zone, |