diff options
Diffstat (limited to 'node-repository/src/main/java/com')
6 files changed, 123 insertions, 72 deletions
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 852d47a4882..8b66b7f3fea 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 @@ -81,7 +81,6 @@ public class NodeRepository extends AbstractComponent { private final NameResolver nameResolver; private final DockerImage dockerImage; private final OsVersions osVersions; - private final Flags flags; /** * Creates a node repository from a zookeeper provider. @@ -98,28 +97,19 @@ public class NodeRepository extends AbstractComponent { */ public NodeRepository(NodeFlavors flavors, Curator curator, Clock clock, Zone zone, NameResolver nameResolver, DockerImage dockerImage, boolean useCuratorClientCache) { - this.db = new CuratorDatabaseClient(flavors, curator, clock, zone, useCacheIn(zone, useCuratorClientCache)); + this.db = new CuratorDatabaseClient(flavors, curator, clock, zone, useCuratorClientCache); this.zone = zone; this.clock = clock; this.flavors = flavors; this.nameResolver = nameResolver; this.dockerImage = dockerImage; this.osVersions = new OsVersions(this.db); - this.flags = new Flags(this.db); // read and write all nodes to make sure they are stored in the latest version of the serialized format for (Node.State state : Node.State.values()) db.writeTo(state, db.getNodes(state), Agent.system, Optional.empty()); } - private static boolean useCacheIn(Zone zone, boolean useCache) { - if (zone.region().value().equals("cd-us-central-1")) { - // TODO: Temporarily disabled in CD to see if allocation conflict is related to caching - return false; - } - return useCache; - } - /** Returns the curator database client used by this */ public CuratorDatabaseClient database() { return db; } @@ -134,7 +124,7 @@ public class NodeRepository extends AbstractComponent { /** Returns feature flags of this node repository */ public Flags flags() { - return flags; + return db.flags(); } // ---------------- Query API ---------------------------------------------------------------- diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/flag/FlagId.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/flag/FlagId.java index 1b798c14588..f0714111cea 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/flag/FlagId.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/flag/FlagId.java @@ -11,7 +11,10 @@ import java.util.Arrays; public enum FlagId { /** Indicates whether a exclusive load balancer should be provisioned */ - exclusiveLoadBalancer("exclusive-load-balancer"); + exclusiveLoadBalancer("exclusive-load-balancer"), + + /** Temporary. Indicates whether to use the new cache generation counting, or the old one (with a known bug) */ + newCacheCounting("new-cache-counting"); private final String serializedValue; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java index 89f77d02b6e..f5576ae00fc 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java @@ -35,7 +35,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { this.jobControl = jobControl; HostName hostname = HostName.from(com.yahoo.net.HostName.getLocalhost()); - long delay = staggeredDelay(nodeRepository.database().curator().cluster(), hostname, nodeRepository.clock().instant(), interval); + long delay = staggeredDelay(nodeRepository.database().cluster(), hostname, nodeRepository.clock().instant(), interval); service = new ScheduledThreadPoolExecutor(1); service.scheduleAtFixedRate(this, delay, interval.toMillis(), TimeUnit.MILLISECONDS); jobControl.started(name()); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java new file mode 100644 index 00000000000..de803281a87 --- /dev/null +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java @@ -0,0 +1,61 @@ +package com.yahoo.vespa.hosted.provision.persistence; + +import com.yahoo.path.Path; +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.CuratorOperation; +import com.yahoo.vespa.curator.transaction.CuratorTransaction; +import com.yahoo.vespa.curator.transaction.TransactionChanges; + +/** + * CuratorTransaction wrapper which increments a counter, to signal invalidation of node repository caches. + * + * This class ensures a CuratorTransaction against the cached data (the node repository data) is + * accompanied by an increment of the data generation counter. An increment must occur <em>after</em> + * the write has completed, successfully or not. It is therefore placed in a {@code finally} block, + * wrapping the super class' {@link #commit()}. + * Likewise, {@link #prepare()} is also wrapped with an increment, in case it fails due to an inconsistent cache. + * The cache is invalid whenever the generation counter is higher than what the cache contents were read with. + * The usual locking for modifications of shared data is then enough to ensure the cache provides a + * consistent view of the shared data, with one exception: when incrementing the counter fails. This is + * assumed to be extremely rare, and the consequence is temporary neglect of cache invalidation. + * + * @author jonmv + */ +class CountingCuratorTransaction extends CuratorTransaction { + + private final CuratorCounter counter; + + public CountingCuratorTransaction(Curator curator, CuratorCounter counter) { + super(curator); + this.counter = counter; + } + + @Override + public void prepare() { + try { + counter.get(); + super.prepare(); + } + finally { + counter.next(); + } + } + + @Override + public void commit() { + try { + super.commit(); + } + finally { + counter.next(); + } + } + + @Override + public String toString() { + return "(" + super.toString() + "), INCREMENT " + counter; + } + +} 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 d8c1323151d..dd1b58dee8e 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 @@ -23,9 +23,10 @@ import java.util.stream.Collectors; * This encapsulated the curator database of the node repo. * It serves reads from an in-memory cache of the content which is invalidated when changed on another node * using a global, shared counter. The counter is updated on all write operations, ensured by wrapping write - * operations in a 2pc transaction containing the counter update. + * operations in a try block, with the counter increment in a finally block. Locks must be used to ensure consistency. * * @author bratseth + * @author jonmv */ public class CuratorDatabase { @@ -86,6 +87,15 @@ public class CuratorDatabase { * Important: It is the nested transaction which must be committed - never the curator transaction directly. */ public CuratorTransaction newCuratorTransactionIn(NestedTransaction transaction) { + // Wrap the curator transaction with an increment of the generation counter. + CountingCuratorTransaction curatorTransaction = new CountingCuratorTransaction(curator, changeGenerationCounter); + transaction.add(curatorTransaction); + return curatorTransaction; + } + + // TODO jvenstad: remove. + /** Kept for now to be able to revert to old caching behaviour. */ + CuratorTransaction newEagerCuratorTransactionIn(NestedTransaction transaction) { // Add a counting transaction first, to make sure we always invalidate the current state on any transaction commit transaction.add(new EagerCountingCuratorTransaction(changeGenerationCounter), CuratorTransaction.class); CuratorTransaction curatorTransaction = new CuratorTransaction(curator); @@ -94,60 +104,35 @@ public class CuratorDatabase { } /** Creates a path in curator and all its parents as necessary. If the path already exists this does nothing. */ - // As this operation does not depend on the prior state we do not need to increment the write counter - public void create(Path path) { + void create(Path path) { curator.create(path); + changeGenerationCounter.next(); // Increment counter to ensure getChildren sees any change. } /** Returns whether given path exists */ - public boolean exists(Path path) { + boolean exists(Path path) { return curator.exists(path); } // --------- Read operations ------------------------------------------------------------------------------- // These can read from the memory file system, which accurately mirrors the ZooKeeper content IF + // the current generation counter is the same as it was when data was put into the cache, AND + // the data to read is protected by a lock which is held now, and during any writes of the data. /** Returns the immediate, local names of the children under this node in any order */ - public List<String> getChildren(Path path) { return getCache().getChildren(path); } + List<String> getChildren(Path path) { return getCache().getChildren(path); } - public Optional<byte[]> getData(Path path) { return getCache().getData(path); } + Optional<byte[]> getData(Path path) { return getCache().getData(path); } - private static class CacheAndGeneration { - public CacheAndGeneration(CuratorDatabaseCache cache, long generation) - { - this.cache = cache; - this.generation = generation; - } - public boolean expired() { - return generation != cache.generation(); - } - public CuratorDatabaseCache validCache() { - if (expired()) { - throw new IllegalStateException("The cache has generation " + cache.generation() + - " while the root genration counter in zookeeper says " + generation + - ". That is totally unacceptable and must be a sever programming error in my close vicinity."); - } - return cache; - } - - private CuratorDatabaseCache cache; - private long generation; - } - private CacheAndGeneration getCacheSnapshot() { - return new CacheAndGeneration(cache.get(), changeGenerationCounter.get()); - } + /** Invalidates the current cache if outdated. */ private CuratorDatabaseCache getCache() { - CacheAndGeneration cacheAndGeneration = getCacheSnapshot(); - while (cacheAndGeneration.expired()) { - synchronized (cacheCreationLock) { // Prevent a race for creating new caches - cacheAndGeneration = getCacheSnapshot(); - if (cacheAndGeneration.expired()) { + if (changeGenerationCounter.get() != cache.get().generation) + synchronized (cacheCreationLock) { + while (changeGenerationCounter.get() != cache.get().generation) cache.set(newCache(changeGenerationCounter.get())); - cacheAndGeneration = getCacheSnapshot(); - } } - } - return cacheAndGeneration.validCache(); + + return cache.get(); } /** Caches must only be instantiated using this method */ @@ -174,7 +159,7 @@ public class CuratorDatabase { private final Map<Path, Optional<byte[]>> data = new ConcurrentHashMap<>(); /** Create an empty snapshot at a given generation (as an empty snapshot is a valid partial snapshot) */ - public CuratorDatabaseCache(long generation, Curator curator) { + private CuratorDatabaseCache(long generation, Curator curator) { this.generation = generation; this.curator = curator; } @@ -183,18 +168,16 @@ public class CuratorDatabase { /** * Returns the children of this path, which may be empty. - * Returns null only if it is not present in this state mirror */ public List<String> getChildren(Path path) { return children.computeIfAbsent(path, key -> ImmutableList.copyOf(curator.getChildren(path))); } /** - * Returns the content of this child - which may be empty. - * Returns null only if it is not present in this state mirror + * Returns the a copy of the content of this child - which may be empty. */ public Optional<byte[]> getData(Path path) { - return data.computeIfAbsent(path, key -> curator.getData(path).map(data -> Arrays.copyOf(data, data.length))); + return data.computeIfAbsent(path, key -> curator.getData(path)).map(data -> Arrays.copyOf(data, data.length)); } } @@ -202,7 +185,7 @@ public class CuratorDatabase { /** An implementation of the curator database cache which does no caching */ private static class DeactivatedCache extends CuratorDatabaseCache { - public DeactivatedCache(long generation, Curator curator) { super(generation, curator); } + private DeactivatedCache(long generation, Curator curator) { super(generation, curator); } @Override public List<String> getChildren(Path path) { return curator.getChildren(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 fb3f801cf65..9bcfdd8b494 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 @@ -5,6 +5,7 @@ import com.google.common.util.concurrent.UncheckedTimeoutException; import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationLockException; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.NodeFlavors; import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.Zone; @@ -19,6 +20,7 @@ import com.yahoo.vespa.curator.transaction.CuratorTransaction; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.flag.Flag; import com.yahoo.vespa.hosted.provision.flag.FlagId; +import com.yahoo.vespa.hosted.provision.flag.Flags; import com.yahoo.vespa.hosted.provision.lb.LoadBalancer; import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId; import com.yahoo.vespa.hosted.provision.node.Agent; @@ -67,19 +69,17 @@ public class CuratorDatabaseClient { private final CuratorDatabase curatorDatabase; private final Clock clock; private final Zone zone; + private final Flags flags; public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, Zone zone, boolean useCache) { this.nodeSerializer = new NodeSerializer(flavors); this.zone = zone; + this.flags = new Flags(this); this.curatorDatabase = new CuratorDatabase(curator, root, useCache); this.clock = clock; initZK(); } - public CuratorDatabase curator() { - return curatorDatabase; - } - private void initZK() { curatorDatabase.create(root); for (Node.State state : Node.State.values()) @@ -91,12 +91,20 @@ public class CuratorDatabaseClient { curatorDatabase.create(flagsRoot); } + public List<HostName> cluster() { + return curatorDatabase.cluster(); + } + + public Flags flags() { + return flags; + } + /** * Adds a set of nodes. Rollbacks/fails transaction if any node is not in the expected state. */ public List<Node> addNodesInState(List<Node> nodes, Node.State expectedState) { NestedTransaction transaction = new NestedTransaction(); - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); for (Node node : nodes) { if (node.state() != expectedState) throw new IllegalArgumentException(node + " is not in the " + node.state() + " state"); @@ -131,7 +139,7 @@ public class CuratorDatabaseClient { for (Node node : nodes) { Path path = toPath(node.state(), node.hostname()); - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); curatorTransaction.add(CuratorOperations.delete(path.getAbsolute())); } @@ -201,7 +209,7 @@ public class CuratorDatabaseClient { List<Node> writtenNodes = new ArrayList<>(nodes.size()); - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); for (Node node : nodes) { Node newNode = new Node(node.openStackId(), node.ipAddresses(), node.ipAddressPool().asSet(), node.hostname(), node.parentHostname(), node.flavor(), @@ -360,7 +368,7 @@ public class CuratorDatabaseClient { public void writeInactiveJobs(Set<String> inactiveJobs) { NestedTransaction transaction = new NestedTransaction(); - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); curatorTransaction.add(CuratorOperations.setData(inactiveJobsPath().getAbsolute(), stringSetSerializer.toJson(inactiveJobs))); transaction.commit(); @@ -380,7 +388,7 @@ public class CuratorDatabaseClient { public void writeInfrastructureVersions(Map<NodeType, Version> infrastructureVersions) { NestedTransaction transaction = new NestedTransaction(); - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); curatorTransaction.add(CuratorOperations.setData(infrastructureVersionsPath().getAbsolute(), NodeTypeVersionsSerializer.toJson(infrastructureVersions))); transaction.commit(); @@ -400,7 +408,7 @@ public class CuratorDatabaseClient { public void writeOsVersions(Map<NodeType, Version> versions) { NestedTransaction transaction = new NestedTransaction(); - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); curatorTransaction.add(CuratorOperations.setData(osVersionsPath().getAbsolute(), NodeTypeVersionsSerializer.toJson(versions))); transaction.commit(); @@ -441,7 +449,7 @@ public class CuratorDatabaseClient { } public void writeLoadBalancers(Collection<LoadBalancer> loadBalancers, NestedTransaction transaction) { - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); loadBalancers.forEach(loadBalancer -> { curatorTransaction.add(createOrSet(loadBalancerPath(loadBalancer.id()), LoadBalancerSerializer.toJson(loadBalancer))); @@ -450,7 +458,7 @@ public class CuratorDatabaseClient { public void removeLoadBalancer(LoadBalancer loadBalancer) { NestedTransaction transaction = new NestedTransaction(); - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); curatorTransaction.add(CuratorOperations.delete(loadBalancerPath(loadBalancer.id()).getAbsolute())); transaction.commit(); } @@ -466,7 +474,7 @@ public class CuratorDatabaseClient { public void writeFlag(Flag flag) { Path path = flagPath(flag.id()); NestedTransaction transaction = new NestedTransaction(); - CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); + CuratorTransaction curatorTransaction = newCuratorTransactionIn(transaction); curatorTransaction.add(createOrSet(path, FlagSerializer.toJson(flag))); transaction.commit(); } @@ -490,4 +498,10 @@ public class CuratorDatabaseClient { return CuratorOperations.create(path.getAbsolute(), data); } + private CuratorTransaction newCuratorTransactionIn(NestedTransaction transaction) { + return flags.get(FlagId.newCacheCounting).isEnabled() + ? curatorDatabase.newCuratorTransactionIn(transaction) + : curatorDatabase.newEagerCuratorTransactionIn(transaction); + } + } |