Revert "Invalidate node object cache based on znode version"

15 files changed, 59 insertions, 63 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 45815284dce..b054f434322 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,3 +9,6 @@ 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 17832ff9550..7f54fff5c70 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,7 +84,8 @@ public class NodeRepository extends AbstractComponent {
              flagSource,
              metricsDb,
              config.useCuratorClientCache(),
-             zone.environment().isProduction() && !zone.getCloud().dynamicProvisioning() && !zone.system().isCd() ? 1 : 0);
+             zone.environment().isProduction() && !zone.getCloud().dynamicProvisioning() && !zone.system().isCd() ? 1 : 0,
+             config.nodeCacheSize());
     }
 
     /**
@@ -102,13 +103,14 @@ public class NodeRepository extends AbstractComponent {
                           FlagSource flagSource,
                           MetricsDb metricsDb,
                           boolean useCuratorClientCache,
-                          int spareCount) {
+                          int spareCount,
+                          long nodeCacheSize) {
         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);
+        this.db = new CuratorDatabaseClient(flavors, curator, clock, useCuratorClientCache, nodeCacheSize);
         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 acf7aa5c28e..ca14a1be4c4 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().objectCacheStats();
+        CacheStats nodeCacheStats = nodeRepository().database().nodeSerializerCacheStats();
         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 83cfe72d9e9..659636de581 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,6 +2,7 @@
 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;
@@ -9,14 +10,12 @@ 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;
@@ -156,7 +155,7 @@ public class CuratorDatabase {
 
         @Override
         public List<String> getChildren(Path path) {
-            return get(children, path, () -> List.copyOf(curator.getChildren(path)));
+            return get(children, path, () -> ImmutableList.copyOf(curator.getChildren(path)));
         }
 
         @Override
@@ -164,11 +163,6 @@ 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) {
@@ -208,13 +202,10 @@ public class CuratorDatabase {
         List<String> getChildren(Path path);
 
         /**
-         * Returns a copy of the content of this child - which may be empty.
+         * Returns the 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 37b2f9ba665..379bb2566df 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,8 +1,6 @@
 // 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;
@@ -37,9 +35,7 @@ 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;
@@ -80,13 +76,8 @@ public class CuratorDatabaseClient {
     private final Clock clock;
     private final CuratorCounter provisionIndexCounter;
 
-    // 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);
+    public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, boolean useCache, long nodeCacheSize) {
+        this.nodeSerializer = new NodeSerializer(flavors, nodeCacheSize);
         this.db = new CuratorDatabase(curator, root, useCache);
         this.clock = clock;
         this.provisionIndexCounter = new CuratorCounter(curator, root.append("provisionIndexCounter"));
@@ -276,27 +267,9 @@ public class CuratorDatabaseClient {
         if (states.length == 0)
             states = Node.State.values();
         for (Node.State state : states) {
-            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());
-            }
+            Optional<byte[]> nodeData = session.getData(toPath(state, hostname));
+            if (nodeData.isPresent())
+                return nodeData.map((data) -> nodeSerializer.fromJson(state, data));
         }
         return Optional.empty();
     }
@@ -552,9 +525,8 @@ public class CuratorDatabaseClient {
         return db.cacheStats();
     }
 
-    public CacheStats objectCacheStats() {
-        var snapshot = this.cacheStats.snapshot();
-        return new CacheStats(snapshot.hitRate(), snapshot.evictionCount(), cachedNodes.size());
+    public CacheStats nodeSerializerCacheStats() {
+        return nodeSerializer.cacheStats();
     }
 
     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 2424fb92b57..cd1b786afd1 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,7 +1,11 @@
 // 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;
@@ -35,11 +39,13 @@ 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;
 
 /**
@@ -122,10 +128,17 @@ 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) {
+    public NodeSerializer(NodeFlavors flavors, long cacheSize) {
         this.flavors = flavors;
+        this.cache = CacheBuilder.newBuilder().maximumSize(cacheSize).recordStats().build();
     }
 
     public byte[] toJson(Node node) {
@@ -139,6 +152,12 @@ 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));
@@ -235,7 +254,15 @@ public class NodeSerializer {
     // ---------------- Deserialization --------------------------------------------------
 
     public Node fromJson(Node.State state, byte[] data) {
-        return nodeFromSlime(state, SlimeUtils.jsonToSlime(data).get());
+        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);
+        }
     }
 
     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 c1e26099ee4..1a2d5294aa5 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);
+              0, 1000);
         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 1287c073981..b391292884f 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);
+                                            0, 1000);
     }
     
     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 0944138e06c..0e732984227 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());
+        NodeSerializer nodeSerializer = new NodeSerializer(nodeRepository.flavors(), 1000);
         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 1c9532e4665..e06bdba90fb 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);
+                                            0, 1000);
     }
 
     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 eda5d201949..8c649243d61 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.5D);
+        expectedMetrics.put("cache.curator.hitRate", 0.52D);
         expectedMetrics.put("cache.curator.evictionCount", 0L);
-        expectedMetrics.put("cache.curator.size", 11L);
+        expectedMetrics.put("cache.curator.size", 12L);
 
         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 ca117ab1395..373bfe20162 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);
+                                                1, 1000);
             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 5a04986338d..f11cfa9a15d 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);
+            FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), true, 1000);
 
     @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 31bf8ee43e2..3de3f4139d1 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);
+    private final NodeSerializer nodeSerializer = new NodeSerializer(nodeFlavors, 1000);
     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 9a709748c1f..c478840780f 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,7 +115,8 @@ public class ProvisioningTester {
                                                  flagSource,
                                                  new MemoryMetricsDb(clock),
                                                  true,
-                                                 spareCount);
+                                                 spareCount,
+                                                 1000);
         this.orchestrator = orchestrator;
         this.provisioner = new NodeRepositoryProvisioner(nodeRepository,
                                                          zone,