diff options
121 files changed, 1208 insertions, 1017 deletions
diff --git a/abi-check-plugin/src/main/java/com/yahoo/abicheck/mojo/AbiCheck.java b/abi-check-plugin/src/main/java/com/yahoo/abicheck/mojo/AbiCheck.java index 2ee7bd495b3..a09e23bbe9e 100644 --- a/abi-check-plugin/src/main/java/com/yahoo/abicheck/mojo/AbiCheck.java +++ b/abi-check-plugin/src/main/java/com/yahoo/abicheck/mojo/AbiCheck.java @@ -1,6 +1,5 @@ package com.yahoo.abicheck.mojo; -import com.google.common.collect.Ordering; import com.google.gson.Gson; import com.google.gson.GsonBuilder; import com.google.gson.reflect.TypeToken; @@ -29,6 +28,7 @@ import org.apache.maven.plugin.AbstractMojo; import org.apache.maven.plugin.MojoExecutionException; import org.apache.maven.plugin.MojoFailureException; import org.apache.maven.plugin.logging.Log; +import org.apache.maven.plugins.annotations.InstantiationStrategy; import org.apache.maven.plugins.annotations.LifecyclePhase; import org.apache.maven.plugins.annotations.Mojo; import org.apache.maven.plugins.annotations.Parameter; @@ -39,13 +39,16 @@ import org.objectweb.asm.ClassReader; @Mojo( name = "abicheck", defaultPhase = LifecyclePhase.PACKAGE, - requiresDependencyResolution = ResolutionScope.RUNTIME + requiresDependencyResolution = ResolutionScope.RUNTIME, + instantiationStrategy = InstantiationStrategy.PER_LOOKUP, + threadSafe = true ) public class AbiCheck extends AbstractMojo { public static final String PACKAGE_INFO_CLASS_FILE_NAME = "package-info.class"; private static final String DEFAULT_SPEC_FILE = "abi-spec.json"; private static final String WRITE_SPEC_PROPERTY = "abicheck.writeSpec"; + @Parameter(defaultValue = "${project}", readonly = true) private MavenProject project = null; diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json index c932958b58b..0b36ea3bee2 100644 --- a/config-model-api/abi-spec.json +++ b/config-model-api/abi-spec.json @@ -303,6 +303,8 @@ "public static java.lang.String toMessageString(java.lang.Throwable)", "public java.util.Optional athenzDomain()", "public java.util.Optional athenzService(com.yahoo.config.provision.Environment, com.yahoo.config.provision.RegionName)", + "public boolean equals(java.lang.Object)", + "public int hashCode()", "public static void main(java.lang.String[])" ], "fields": [ diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java index a6cecefe940..d15d76fd11b 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java @@ -267,6 +267,27 @@ public class DeploymentSpec { return Optional.ofNullable(athenzService); } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + DeploymentSpec that = (DeploymentSpec) o; + return globalServiceId.equals(that.globalServiceId) && + upgradePolicy == that.upgradePolicy && + majorVersion.equals(that.majorVersion) && + changeBlockers.equals(that.changeBlockers) && + steps.equals(that.steps) && + xmlForm.equals(that.xmlForm) && + athenzDomain.equals(that.athenzDomain) && + athenzService.equals(that.athenzService) && + notifications.equals(that.notifications); + } + + @Override + public int hashCode() { + return Objects.hash(globalServiceId, upgradePolicy, majorVersion, changeBlockers, steps, xmlForm, athenzDomain, athenzService, notifications); + } + /** This may be invoked by a continuous build */ public static void main(String[] args) { if (args.length != 2 && args.length != 3) { diff --git a/config-provisioning/abi-spec.json b/config-provisioning/abi-spec.json index 3c5dd0fa834..849d92696ff 100644 --- a/config-provisioning/abi-spec.json +++ b/config-provisioning/abi-spec.json @@ -145,6 +145,27 @@ ], "fields": [] }, + "com.yahoo.config.provision.CloudName": { + "superClass": "java.lang.Object", + "interfaces": [ + "java.lang.Comparable" + ], + "attributes": [ + "public" + ], + "methods": [ + "public java.lang.String value()", + "public boolean isDefault()", + "public static com.yahoo.config.provision.CloudName defaultName()", + "public static com.yahoo.config.provision.CloudName from(java.lang.String)", + "public boolean equals(java.lang.Object)", + "public int hashCode()", + "public java.lang.String toString()", + "public int compareTo(com.yahoo.config.provision.CloudName)", + "public bridge synthetic int compareTo(java.lang.Object)" + ], + "fields": [] + }, "com.yahoo.config.provision.ClusterMembership": { "superClass": "java.lang.Object", "interfaces": [], @@ -671,9 +692,11 @@ "public void <init>(com.yahoo.cloud.config.ConfigserverConfig, com.yahoo.config.provision.NodeFlavors)", "public void <init>(com.yahoo.config.provision.Environment, com.yahoo.config.provision.RegionName)", "public void <init>(com.yahoo.config.provision.SystemName, com.yahoo.config.provision.Environment, com.yahoo.config.provision.RegionName)", + "public void <init>(com.yahoo.config.provision.CloudName, com.yahoo.config.provision.SystemName, com.yahoo.config.provision.Environment, com.yahoo.config.provision.RegionName)", + "public com.yahoo.config.provision.CloudName cloud()", + "public com.yahoo.config.provision.SystemName system()", "public com.yahoo.config.provision.Environment environment()", "public com.yahoo.config.provision.RegionName region()", - "public com.yahoo.config.provision.SystemName system()", "public java.lang.String defaultFlavor(com.yahoo.config.provision.ClusterSpec$Type)", "public java.util.Optional nodeFlavors()", "public static com.yahoo.config.provision.Zone defaultZone()", diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/CloudName.java b/config-provisioning/src/main/java/com/yahoo/config/provision/CloudName.java index e7a6b32b36e..53e00373713 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/CloudName.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/CloudName.java @@ -1,7 +1,5 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.controller.api.integration.zone; - -import org.jetbrains.annotations.NotNull; +package com.yahoo.config.provision; import java.util.Objects; @@ -55,7 +53,7 @@ public class CloudName implements Comparable<CloudName> { } @Override - public int compareTo(@NotNull CloudName o) { + public int compareTo(CloudName o) { return cloud.compareTo(o.cloud); } diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/Zone.java b/config-provisioning/src/main/java/com/yahoo/config/provision/Zone.java index fbbdb1df635..6b1e1406c8f 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/Zone.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/Zone.java @@ -17,15 +17,17 @@ import java.util.Optional; */ public class Zone { + private final CloudName cloudName; private final SystemName systemName; - private final FlavorDefaults flavorDefaults; - private final Optional<NodeFlavors> nodeFlavors; private final Environment environment; private final RegionName region; + private final FlavorDefaults flavorDefaults; + private final Optional<NodeFlavors> nodeFlavors; @Inject public Zone(ConfigserverConfig configserverConfig, NodeFlavors nodeFlavors) { - this(SystemName.from(configserverConfig.system()), + this(CloudName.from(configserverConfig.cloud()), + SystemName.from(configserverConfig.system()), Environment.from(configserverConfig.environment()), RegionName.from(configserverConfig.region()), new FlavorDefaults(configserverConfig), @@ -39,21 +41,34 @@ public class Zone { /** Create from system, environment and region. Use for testing. */ public Zone(SystemName systemName, Environment environment, RegionName region) { - this(systemName, environment, region, new FlavorDefaults("default"), null); + this(CloudName.defaultName(), systemName, environment, region, new FlavorDefaults("default"), null); } - private Zone(SystemName systemName, + /** Create from cloud, system, environment and region. Use for testing. */ + public Zone(CloudName cloudName, SystemName systemName, Environment environment, RegionName region) { + this(cloudName, systemName, environment, region, new FlavorDefaults("default"), null); + } + + private Zone(CloudName cloudName, + SystemName systemName, Environment environment, RegionName region, FlavorDefaults flavorDefaults, NodeFlavors nodeFlavors) { + this.cloudName = cloudName; + this.systemName = systemName; this.environment = environment; this.region = region; this.flavorDefaults = flavorDefaults; - this.systemName = systemName; this.nodeFlavors = Optional.ofNullable(nodeFlavors); } + /** Returns the current cloud */ + public CloudName cloud() { return cloudName; } + + /** Returns the current system */ + public SystemName system() { return systemName; } + /** Returns the current environment */ public Environment environment() { return environment; @@ -64,9 +79,6 @@ public class Zone { return region; } - /** Returns the current system */ - public SystemName system() { return systemName; } - /** Returns the default hardware flavor to assign in this zone */ public String defaultFlavor(ClusterSpec.Type clusterType) { return flavorDefaults.flavor(clusterType); } @@ -75,7 +87,7 @@ public class Zone { /** Do not use */ public static Zone defaultZone() { - return new Zone(SystemName.defaultSystem(), Environment.defaultEnvironment(), RegionName.defaultName()); + return new Zone(CloudName.defaultName(), SystemName.defaultSystem(), Environment.defaultEnvironment(), RegionName.defaultName()); } @Override diff --git a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UrlDownloadRpcServer.java b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UrlDownloadRpcServer.java index f9469b1ae65..3c24cb58cff 100644 --- a/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UrlDownloadRpcServer.java +++ b/config-proxy/src/main/java/com/yahoo/vespa/config/proxy/UrlDownloadRpcServer.java @@ -44,7 +44,7 @@ public class UrlDownloadRpcServer { private static final String LAST_MODFIED_FILE_NAME = "lastmodified"; private final File downloadBaseDir; - private final ExecutorService rpcDownloadExecutor = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors(), + private final ExecutorService rpcDownloadExecutor = Executors.newFixedThreadPool(Math.max(8, Runtime.getRuntime().availableProcessors()), new DaemonThreadFactory("Rpc download executor")); UrlDownloadRpcServer(Supervisor supervisor) { diff --git a/configdefinitions/src/vespa/configserver.def b/configdefinitions/src/vespa/configserver.def index cc563b2cf7c..de8f1b7cac8 100644 --- a/configdefinitions/src/vespa/configserver.def +++ b/configdefinitions/src/vespa/configserver.def @@ -33,6 +33,7 @@ numParallelTenantLoaders int default=4 applicationDirectory string default="conf/configserver-app" # Zone information +cloud string default="default" environment string default="prod" region string default="default" system string default="main" diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ConfigServerBootstrap.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ConfigServerBootstrap.java index f240129eda1..991cd99968d 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/ConfigServerBootstrap.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ConfigServerBootstrap.java @@ -212,6 +212,8 @@ public class ConfigServerBootstrap extends AbstractComponent implements Runnable do { applicationsNotRedeployed = redeployApplications(applicationsNotRedeployed); if ( ! applicationsNotRedeployed.isEmpty()) { + log.log(LogLevel.INFO, "Redeployment of " + applicationsNotRedeployed + + " failed, will retry in " + sleepTimeWhenRedeployingFails); Thread.sleep(sleepTimeWhenRedeployingFails.toMillis()); } } while ( ! applicationsNotRedeployed.isEmpty() && Instant.now().isBefore(end)); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java index 16aaef048b5..3034c7dfd53 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java @@ -85,8 +85,8 @@ public class FileServer { private FileServer(ConnectionPool connectionPool, File rootDir) { this.downloader = new FileDownloader(connectionPool); this.root = new FileDirectory(rootDir); - this.pushExecutor = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()); - this.pullExecutor = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors()); + this.pushExecutor = Executors.newFixedThreadPool(Math.max(8, Runtime.getRuntime().availableProcessors())); + this.pullExecutor = Executors.newFixedThreadPool(Math.max(8, Runtime.getRuntime().availableProcessors())); } boolean hasFile(String fileReference) { diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java index 1ce90fad465..e8abecc3236 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java @@ -126,7 +126,9 @@ public class RpcServer implements Runnable, ReloadListener, TenantListener { this.metrics = metrics.getOrCreateMetricUpdater(Collections.emptyMap()); this.hostLivenessTracker = hostLivenessTracker; BlockingQueue<Runnable> workQueue = new LinkedBlockingQueue<>(config.maxgetconfigclients()); - int numberOfRpcThreads = (config.numRpcThreads() == 0) ? Runtime.getRuntime().availableProcessors() : config.numRpcThreads(); + int numberOfRpcThreads = (config.numRpcThreads() == 0) + ? Math.max(8, Runtime.getRuntime().availableProcessors()) + : config.numRpcThreads(); executorService = new ThreadPoolExecutor(numberOfRpcThreads, numberOfRpcThreads, 0, TimeUnit.SECONDS, workQueue, ThreadFactoryFactory.getThreadFactory(THREADPOOL_NAME)); delayedConfigResponses = new DelayedConfigResponses(this, config.numDelayedResponseThreads()); diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java index 73abd70a5ae..aa157366a60 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java @@ -11,7 +11,9 @@ import com.yahoo.vespa.orchestrator.status.HostStatus; import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; +import java.util.function.Function; /** * (Only the suspended applications part of this is in use) @@ -34,6 +36,11 @@ public class OrchestratorMock implements Orchestrator { } @Override + public Function<HostName, Optional<HostStatus>> getNodeStatuses() { + return hostName -> Optional.of(getNodeStatus(hostName)); + } + + @Override public void setNodeStatus(HostName hostName, HostStatus state) {} @Override diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java index 612941ece7a..5700e316493 100644 --- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java +++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java @@ -24,6 +24,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.OptionalInt; import java.util.Set; /** @@ -69,7 +70,7 @@ public class FS4InvokerFactory { * @return Optional containing the SearchInvoker or <i>empty</i> if some node in the * list is invalid and the remaining coverage is not sufficient */ - public Optional<SearchInvoker> getSearchInvoker(Query query, int groupId, List<Node> nodes, boolean acceptIncompleteCoverage) { + public Optional<SearchInvoker> getSearchInvoker(Query query, OptionalInt groupId, List<Node> nodes, boolean acceptIncompleteCoverage) { List<SearchInvoker> invokers = new ArrayList<>(nodes.size()); Set<Integer> failed = null; for (Node node : nodes) { diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java index 1d39cffa9d2..146b132be22 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java @@ -21,6 +21,7 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.OptionalInt; import java.util.Set; /** @@ -106,7 +107,7 @@ public class Dispatcher extends AbstractComponent { @FunctionalInterface private interface SearchInvokerSupplier { - Optional<SearchInvoker> supply(Query query, int groupId, List<Node> nodes, boolean acceptIncompleteCoverage); + Optional<SearchInvoker> supply(Query query, OptionalInt groupId, List<Node> nodes, boolean acceptIncompleteCoverage); } // build invoker based on searchpath @@ -121,7 +122,7 @@ public class Dispatcher extends AbstractComponent { return Optional.empty(); } else { query.trace(false, 2, "Dispatching internally with search path ", searchPath); - return invokerFactory.supply(query, -1, nodes, true); + return invokerFactory.supply(query, OptionalInt.empty(), nodes, true); } } catch (InvalidSearchPathException e) { return Optional.of(new SearchErrorInvoker(ErrorMessage.createIllegalQuery(e.getMessage()))); @@ -133,7 +134,7 @@ public class Dispatcher extends AbstractComponent { if (directNode.isPresent()) { Node node = directNode.get(); query.trace(false, 2, "Dispatching directly to ", node); - return invokerFactory.supply(query, -1, Arrays.asList(node), true); + return invokerFactory.supply(query, OptionalInt.empty(), Arrays.asList(node), true); } int covered = searchCluster.groupsWithSufficientCoverage(); @@ -148,7 +149,8 @@ public class Dispatcher extends AbstractComponent { } Group group = groupInCluster.get(); boolean acceptIncompleteCoverage = (i == max - 1); - Optional<SearchInvoker> invoker = invokerFactory.supply(query, group.id(), group.nodes(), acceptIncompleteCoverage); + Optional<SearchInvoker> invoker = invokerFactory.supply(query, OptionalInt.of(group.id()), group.nodes(), + acceptIncompleteCoverage); if (invoker.isPresent()) { query.trace(false, 2, "Dispatching internally to search group ", group.id()); query.getModel().setSearchPath("/" + group.id()); diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java index e497ef6751b..a1658684b87 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java @@ -18,6 +18,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.OptionalInt; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; import java.util.concurrent.FutureTask; @@ -62,11 +63,11 @@ public class SearchCluster implements NodeManager<Node> { public SearchCluster(String clusterId, DispatchConfig dispatchConfig, FS4ResourcePool fs4ResourcePool, int containerClusterSize, VipStatus vipStatus) { this.clusterId = clusterId; this.dispatchConfig = dispatchConfig; - this.size = dispatchConfig.node().size(); this.fs4ResourcePool = fs4ResourcePool; this.vipStatus = vipStatus; List<Node> nodes = toNodes(dispatchConfig); + this.size = nodes.size(); // Create groups ImmutableMap.Builder<Integer, Group> groupsBuilder = new ImmutableMap.Builder<>(); @@ -331,9 +332,10 @@ public class SearchCluster implements NodeManager<Node> { } } - private void logIfInsufficientCoverage(boolean sufficient, int groupId, int nodes) { + private void logIfInsufficientCoverage(boolean sufficient, OptionalInt groupId, int nodes) { if (!sufficient) { - log.warning(() -> String.format("Coverage of group %s is only %d/%d (requires %d)", groupId, nodes, groupSize(), + String group = groupId.isPresent()? Integer.toString(groupId.getAsInt()) : "(unspecified)"; + log.warning(() -> String.format("Coverage of group %s is only %d/%d (requires %d)", group, nodes, groupSize(), groupSize() - dispatchConfig.maxNodesDownPerGroup())); } } @@ -341,14 +343,22 @@ public class SearchCluster implements NodeManager<Node> { /** * Calculate whether a subset of nodes in a group has enough coverage */ - public boolean isPartialGroupCoverageSufficient(int groupId, List<Node> nodes) { + public boolean isPartialGroupCoverageSufficient(OptionalInt knownGroupId, List<Node> nodes) { if (orderedGroups.size() == 1) { boolean sufficient = nodes.size() >= groupSize() - dispatchConfig.maxNodesDownPerGroup(); - logIfInsufficientCoverage(sufficient, groupId, nodes.size()); + logIfInsufficientCoverage(sufficient, knownGroupId, nodes.size()); return sufficient; } - int nodesInGroup = groups.get(groupId).nodes().size(); + if (knownGroupId.isEmpty()) { + return false; + } + int groupId = knownGroupId.getAsInt(); + Group group = groups.get(groupId); + if (group == null) { + return false; + } + int nodesInGroup = group.nodes().size(); long sumOfActiveDocuments = 0; int otherGroups = 0; for (Group g : orderedGroups) { @@ -363,7 +373,7 @@ public class SearchCluster implements NodeManager<Node> { } long averageDocumentsInOtherGroups = sumOfActiveDocuments / otherGroups; boolean sufficient = isGroupCoverageSufficient(nodes.size(), nodesInGroup, activeDocuments, averageDocumentsInOtherGroups); - logIfInsufficientCoverage(sufficient, groupId, nodes.size()); + logIfInsufficientCoverage(sufficient, knownGroupId, nodes.size()); return sufficient; } } diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java index fae659b0d70..708caafa3f5 100644 --- a/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java +++ b/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java @@ -11,6 +11,7 @@ import org.junit.Test; import java.util.List; import java.util.Optional; +import java.util.OptionalInt; import static com.yahoo.search.dispatch.MockSearchCluster.createDispatchConfig; import static org.hamcrest.Matchers.is; @@ -112,7 +113,7 @@ public class DispatcherTest { } @Override - public Optional<SearchInvoker> getSearchInvoker(Query query, int groupId, List<Node> nodes, boolean acceptIncompleteCoverage) { + public Optional<SearchInvoker> getSearchInvoker(Query query, OptionalInt groupId, List<Node> nodes, boolean acceptIncompleteCoverage) { if (step >= events.length) { throw new RuntimeException("Was not expecting more calls to getSearchInvoker"); } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServer.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServer.java index ad52ba48d4e..9a75764befe 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServer.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/configserver/ConfigServer.java @@ -5,6 +5,7 @@ import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions; import com.yahoo.vespa.hosted.controller.api.application.v4.model.EndpointStatus; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.identifiers.Hostname; +import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.serviceview.bindings.ApplicationView; import java.io.IOException; @@ -71,6 +72,7 @@ public interface ConfigServer { /** Get service convergence status for given deployment */ Optional<ServiceConvergence> serviceConvergence(DeploymentId deployment); - List<LoadBalancer> getLoadBalancers(DeploymentId deployment); + /** Get all load balancers in given zone */ + List<LoadBalancer> getLoadBalancers(ZoneId zone); } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneFilter.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneFilter.java index c665afeb129..9282e612dac 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneFilter.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneFilter.java @@ -1,6 +1,8 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.integration.zone; +import com.yahoo.config.provision.CloudName; + /** * A ZoneId list which can be filtered in various ways; elements can be accessed after at least one filter. * diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneId.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneId.java index 1b13d9a5760..962aa1ad93e 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneId.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneId.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.integration.zone; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java index e63f665db58..d9d4408caac 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.controller.api.integration.zone; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.SystemName; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java index 3efb0db8cce..30c99288d51 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java @@ -265,71 +265,75 @@ public class ApplicationController { if (applicationId.instance().isTester()) throw new IllegalArgumentException("'" + applicationId + "' is a tester application!"); - try (Lock lock = lockForDeployment(applicationId)) { - LockedApplication application = get(applicationId) - .map(app -> new LockedApplication(app, lock)) - .orElseGet(() -> new LockedApplication(createApplication(applicationId, Optional.empty()), lock)); - - boolean canDeployDirectly = options.deployDirectly || zone.environment().isManuallyDeployed(); - boolean preferOldestVersion = options.deployCurrentVersion; - - // Determine versions to use. + try (Lock deploymentLock = lockForDeployment(applicationId, zone)) { Version platformVersion; ApplicationVersion applicationVersion; ApplicationPackage applicationPackage; - if (canDeployDirectly) { - platformVersion = options.vespaVersion.map(Version::new).orElse(application.get().deploymentSpec().majorVersion() - .flatMap(this::lastCompatibleVersion) - .orElse(controller.systemVersion())); - applicationVersion = applicationVersionFromDeployer.orElse(ApplicationVersion.unknown); - applicationPackage = applicationPackageFromDeployer.orElseThrow( - () -> new IllegalArgumentException("Application package must be given when deploying to " + zone)); - } - else { - JobType jobType = JobType.from(controller.system(), zone) - .orElseThrow(() -> new IllegalArgumentException("No job is known for " + zone + ".")); - Optional<JobStatus> job = Optional.ofNullable(application.get().deploymentJobs().jobStatus().get(jobType)); - if ( ! job.isPresent() - || ! job.get().lastTriggered().isPresent() - || job.get().lastCompleted().isPresent() && job.get().lastCompleted().get().at().isAfter(job.get().lastTriggered().get().at())) - return unexpectedDeployment(applicationId, zone); - JobRun triggered = job.get().lastTriggered().get(); - platformVersion = preferOldestVersion - ? triggered.sourcePlatform().orElse(triggered.platform()) - : triggered.platform(); - applicationVersion = preferOldestVersion - ? triggered.sourceApplication().orElse(triggered.application()) - : triggered.application(); - - applicationPackage = getApplicationPackage(application.get(), applicationVersion); - validateRun(application.get(), zone, platformVersion, applicationVersion); - } + Set<String> rotationNames = new HashSet<>(); + Set<String> cnames = new HashSet<>(); - // TODO: Remove this when all packages are validated upon submission, as in ApplicationApiHandler.submit(...). - verifyApplicationIdentityConfiguration(applicationId.tenant(), applicationPackage, deployingIdentity); + try (Lock lock = lock(applicationId)) { + LockedApplication application = get(applicationId) + .map(app -> new LockedApplication(app, lock)) + .orElseGet(() -> new LockedApplication(createApplication(applicationId, Optional.empty()), lock)); + + boolean manuallyDeployed = options.deployDirectly || zone.environment().isManuallyDeployed(); + boolean preferOldestVersion = options.deployCurrentVersion; + + // Determine versions to use. + if (manuallyDeployed) { + applicationVersion = applicationVersionFromDeployer.orElse(ApplicationVersion.unknown); + applicationPackage = applicationPackageFromDeployer.orElseThrow( + () -> new IllegalArgumentException("Application package must be given when deploying to " + zone)); + platformVersion = options.vespaVersion.map(Version::new).orElse(applicationPackage.deploymentSpec().majorVersion() + .flatMap(this::lastCompatibleVersion) + .orElse(controller.systemVersion())); + } + else { + JobType jobType = JobType.from(controller.system(), zone) + .orElseThrow(() -> new IllegalArgumentException("No job is known for " + zone + ".")); + Optional<JobStatus> job = Optional.ofNullable(application.get().deploymentJobs().jobStatus().get(jobType)); + if ( ! job.isPresent() + || ! job.get().lastTriggered().isPresent() + || job.get().lastCompleted().isPresent() && job.get().lastCompleted().get().at().isAfter(job.get().lastTriggered().get().at())) + return unexpectedDeployment(applicationId, zone); + JobRun triggered = job.get().lastTriggered().get(); + platformVersion = preferOldestVersion ? triggered.sourcePlatform().orElse(triggered.platform()) + : triggered.platform(); + applicationVersion = preferOldestVersion ? triggered.sourceApplication().orElse(triggered.application()) + : triggered.application(); + + applicationPackage = getApplicationPackage(application.get(), applicationVersion); + validateRun(application.get(), zone, platformVersion, applicationVersion); + } - // Update application with information from application package - if ( ! preferOldestVersion && ! application.get().deploymentJobs().deployedInternally()) - // TODO jvenstad: Store only on submissions (not on deployments to dev!!) - application = storeWithUpdatedConfig(application, applicationPackage); + // TODO: Remove this when all packages are validated upon submission, as in ApplicationApiHandler.submit(...). + verifyApplicationIdentityConfiguration(applicationId.tenant(), applicationPackage, deployingIdentity); + + // Assign global rotation + application = withRotation(application, zone); + Application app = application.get(); + app.globalDnsName(controller.system()).ifPresent(applicationRotation -> { + rotationNames.add(app.rotation().orElseThrow(() -> new RuntimeException("Global Dns assigned, but no rotation id present")).asString()); + cnames.add(applicationRotation.dnsName()); + cnames.add(applicationRotation.secureDnsName()); + cnames.add(applicationRotation.oathDnsName()); + }); - // Assign global rotation - application = withRotation(application, zone); - Set<String> rotationNames = new HashSet<>(); - Set<String> cnames = new HashSet<>(); - Application app = application.get(); - app.globalDnsName(controller.system()).ifPresent(applicationRotation -> { - rotationNames.add(app.rotation().orElseThrow(() -> new RuntimeException("Global Dns assigned, but no rotation id present")).asString()); - cnames.add(applicationRotation.dnsName()); - cnames.add(applicationRotation.secureDnsName()); - cnames.add(applicationRotation.oathDnsName()); - }); + // Update application with information from application package + if ( ! preferOldestVersion + && ! application.get().deploymentJobs().deployedInternally() + && ! zone.environment().isManuallyDeployed()) + // TODO jvenstad: Store only on submissions + storeWithUpdatedConfig(application, applicationPackage); + } // Release application lock while doing the deployment, which is a lengthy task. - // Carry out deployment + // Carry out deployment without holding the application lock. options = withVersion(platformVersion, options); ActivateResult result = deploy(applicationId, applicationPackage, zone, options, rotationNames, cnames); - application = application.withNewDeployment(zone, applicationVersion, platformVersion, clock.instant()); - store(application); + + lockOrThrow(applicationId, application -> + store(application.withNewDeployment(zone, applicationVersion, platformVersion, clock.instant()))); return result; } } @@ -667,13 +671,10 @@ public class ApplicationController { } /** - * Returns a lock which provides exclusive rights to changing this application, with a timeout that - * is suitable for deployments. - * Any operation which stores an application need to first acquire this lock, then read, modify - * and store the application, and finally release (close) the lock. + * Returns a lock which provides exclusive rights to deploying this application to the given zone. */ - private Lock lockForDeployment(ApplicationId application) { - return curator.lockForDeployment(application); + private Lock lockForDeployment(ApplicationId application, ZoneId zone) { + return curator.lockForDeployment(application, zone); } /** Verify that each of the production zones listed in the deployment spec exist in this system. */ diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java index 745436e8924..0b5cb199994 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java @@ -5,6 +5,7 @@ import com.google.inject.Inject; import com.yahoo.component.AbstractComponent; import com.yahoo.component.Version; import com.yahoo.component.Vtag; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.athenz.api.AthenzDomain; @@ -25,7 +26,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.entity.EntityService; import com.yahoo.vespa.hosted.controller.api.integration.github.GitHub; import com.yahoo.vespa.hosted.controller.api.integration.organization.Mailer; import com.yahoo.vespa.hosted.controller.api.integration.routing.RoutingGenerator; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry; import com.yahoo.vespa.hosted.controller.athenz.impl.ZmsClientFacade; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java index 1fb22d28ac7..e4b8a1d9e84 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java @@ -10,7 +10,6 @@ import com.yahoo.config.provision.HostName; import com.yahoo.vespa.curator.Lock; import com.yahoo.vespa.hosted.controller.api.integration.MetricsService; import com.yahoo.vespa.hosted.controller.api.integration.MetricsService.ApplicationMetrics; -import com.yahoo.vespa.hosted.controller.api.integration.configserver.LoadBalancer; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; @@ -28,13 +27,11 @@ import com.yahoo.vespa.hosted.controller.rotation.RotationId; import java.time.Instant; import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.OptionalInt; import java.util.OptionalLong; -import java.util.stream.Collectors; /** * An application that has been locked for modification. Provides methods for modifying an application's fields. @@ -150,8 +147,7 @@ public class LockedApplication { previousDeployment.clusterUtils(), previousDeployment.clusterInfo(), previousDeployment.metrics(), - previousDeployment.activity(), - previousDeployment.loadBalancers()); + previousDeployment.activity()); return with(newDeployment); } @@ -254,13 +250,6 @@ public class LockedApplication { outstandingChange, ownershipIssueId, owner, majorVersion, metrics, rotation, rotationStatus); } - public LockedApplication withLoadBalancersIn(ZoneId zoneId, List<LoadBalancer> loadBalancers) { - Map<ClusterSpec.Id, HostName> loadBalancersByCluster = loadBalancers.stream() - .collect(Collectors.toUnmodifiableMap(LoadBalancer::cluster, - LoadBalancer::hostname)); - return with(deployments.get(zoneId).withLoadBalancers(loadBalancersByCluster)); - } - /** Don't expose non-leaf sub-objects. */ private LockedApplication with(Deployment deployment) { Map<ZoneId, Deployment> deployments = new LinkedHashMap<>(this.deployments); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Deployment.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Deployment.java index 3fd2932c3e1..51ac25bc321 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Deployment.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Deployment.java @@ -4,7 +4,6 @@ package com.yahoo.vespa.hosted.controller.application; import com.google.common.collect.ImmutableMap; import com.yahoo.component.Version; import com.yahoo.config.provision.ClusterSpec.Id; -import com.yahoo.config.provision.HostName; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; @@ -30,17 +29,16 @@ public class Deployment { private final Map<Id, ClusterInfo> clusterInfo; private final DeploymentMetrics metrics; private final DeploymentActivity activity; - private final Map<Id, HostName> loadBalancers; public Deployment(ZoneId zone, ApplicationVersion applicationVersion, Version version, Instant deployTime) { this(zone, applicationVersion, version, deployTime, Collections.emptyMap(), Collections.emptyMap(), - DeploymentMetrics.none, DeploymentActivity.none, Collections.emptyMap()); + DeploymentMetrics.none, DeploymentActivity.none); } public Deployment(ZoneId zone, ApplicationVersion applicationVersion, Version version, Instant deployTime, Map<Id, ClusterUtilization> clusterUtilization, Map<Id, ClusterInfo> clusterInfo, DeploymentMetrics metrics, - DeploymentActivity activity, Map<Id, HostName> loadBalancers) { + DeploymentActivity activity) { this.zone = Objects.requireNonNull(zone, "zone cannot be null"); this.applicationVersion = Objects.requireNonNull(applicationVersion, "applicationVersion cannot be null"); this.version = Objects.requireNonNull(version, "version cannot be null"); @@ -49,7 +47,6 @@ public class Deployment { this.clusterInfo = ImmutableMap.copyOf(Objects.requireNonNull(clusterInfo, "clusterInfo cannot be null")); this.metrics = Objects.requireNonNull(metrics, "deploymentMetrics cannot be null"); this.activity = Objects.requireNonNull(activity, "activity cannot be null"); - this.loadBalancers = ImmutableMap.copyOf(Objects.requireNonNull(loadBalancers, "loadBalancers cannot be null")); } /** Returns the zone this was deployed to */ @@ -82,33 +79,24 @@ public class Deployment { return clusterUtilization; } - public Map<Id, HostName> loadBalancers() { - return loadBalancers; - } - public Deployment recordActivityAt(Instant instant) { return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, clusterInfo, metrics, - activity.recordAt(instant, metrics), loadBalancers); + activity.recordAt(instant, metrics)); } public Deployment withClusterUtils(Map<Id, ClusterUtilization> clusterUtilization) { return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, clusterInfo, metrics, - activity, loadBalancers); + activity); } public Deployment withClusterInfo(Map<Id, ClusterInfo> newClusterInfo) { return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, newClusterInfo, metrics, - activity, loadBalancers); + activity); } public Deployment withMetrics(DeploymentMetrics metrics) { return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, clusterInfo, metrics, - activity, loadBalancers); - } - - public Deployment withLoadBalancers(Map<Id, HostName> loadBalancers) { - return new Deployment(zone, applicationVersion, version, deployTime, clusterUtilization, clusterInfo, metrics, - activity, loadBalancers); + activity); } /** diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java index 16f08ad1e15..49788b643e0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java @@ -45,6 +45,7 @@ import static com.yahoo.vespa.hosted.controller.api.integration.BuildService.Job import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.component; import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.stagingTest; import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.systemTest; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static java.util.Comparator.comparing; import static java.util.Comparator.naturalOrder; @@ -326,11 +327,8 @@ public class DeploymentTrigger { if (completedAt.isPresent() && canTrigger(job, versions, application, stepJobs)) { jobs.add(deploymentJob(application, versions, change, job, reason, completedAt.get())); } - if ( ! alreadyTriggered(application, versions)) { - // Only remove test jobs that target this combination - if (testJobs == null) testJobs = new ArrayList<>(); - testJobs.removeIf(j -> versions.sourcesMatchIfPresent(j.triggering) && - versions.targetsMatch(j.triggering)); + if ( ! alreadyTriggered(application, versions) && testJobs == null) { + testJobs = emptyList(); } } else if (testJobs == null) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java index dcdd28544c1..5e7db9f58fd 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java @@ -3,9 +3,9 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.google.common.collect.ImmutableSet; import com.yahoo.component.Version; +import com.yahoo.config.provision.CloudName; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.SystemApplication; import com.yahoo.vespa.hosted.controller.versions.OsVersion; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java index 42e8a572815..bc684e753d1 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainer.java @@ -5,8 +5,6 @@ import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.HostName; import com.yahoo.log.LogLevel; import com.yahoo.vespa.curator.Lock; -import com.yahoo.vespa.hosted.controller.Application; -import com.yahoo.vespa.hosted.controller.ApplicationController; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.configserver.LoadBalancer; @@ -16,7 +14,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordData; import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordId; import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; -import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.application.RoutingPolicy; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; @@ -24,6 +21,7 @@ import java.time.Duration; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -33,9 +31,10 @@ import java.util.logging.Logger; import java.util.stream.Collectors; /** - * Maintains routing policies for all exclusive load balancers in this system. + * Maintains DNS records as defined by routing policies for all exclusive load balancers in this system. * * @author mortent + * @author mpolden */ public class RoutingPolicyMaintainer extends Maintainer { @@ -43,7 +42,6 @@ public class RoutingPolicyMaintainer extends Maintainer { private final NameService nameService; private final CuratorDb db; - private final ApplicationController applications; public RoutingPolicyMaintainer(Controller controller, Duration interval, @@ -53,39 +51,51 @@ public class RoutingPolicyMaintainer extends Maintainer { super(controller, interval, jobControl); this.nameService = nameService; this.db = db; - this.applications = controller.applications(); } @Override protected void maintain() { - updateDnsRecords(); - removeObsoleteDnsRecords(); + Map<DeploymentId, List<LoadBalancer>> loadBalancers = findLoadBalancers(); + updateDnsRecords(loadBalancers); + removeObsoleteDnsRecords(loadBalancers); } - /** Create DNS records for all exclusive load balancers */ - private void updateDnsRecords() { - for (Application application : applications.asList()) { - for (ZoneId zone : application.deployments().keySet()) { - List<LoadBalancer> loadBalancers = findLoadBalancersIn(zone, application.id()); - if (loadBalancers.isEmpty()) continue; - - applications.lockIfPresent(application.id(), (locked) -> { - applications.store(locked.withLoadBalancersIn(zone, loadBalancers)); + /** Find all exclusive load balancers owned by given applications, grouped by deployment */ + private Map<DeploymentId, List<LoadBalancer>> findLoadBalancers() { + Map<DeploymentId, List<LoadBalancer>> result = new LinkedHashMap<>(); + for (ZoneId zone : controller().zoneRegistry().zones().controllerUpgraded().ids()) { + List<LoadBalancer> loadBalancers = findLoadBalancersIn(zone); + for (LoadBalancer loadBalancer : loadBalancers) { + DeploymentId deployment = new DeploymentId(loadBalancer.application(), zone); + result.compute(deployment, (k, existing) -> { + if (existing == null) { + existing = new ArrayList<>(); + } + existing.add(loadBalancer); + return existing; }); + } + } + return Collections.unmodifiableMap(result); + } - try (Lock lock = db.lockRoutingPolicies()) { - Set<RoutingPolicy> policies = new LinkedHashSet<>(db.readRoutingPolicies(application.id())); - for (LoadBalancer loadBalancer : loadBalancers) { - try { - policies.add(registerDnsAlias(application.id(), zone, loadBalancer)); - } catch (Exception e) { - log.log(LogLevel.WARNING, "Failed to create or update DNS record for load balancer " + - loadBalancer.hostname() + ". Retrying in " + maintenanceInterval(), - e); - } + /** Create DNS records for all exclusive load balancers */ + private void updateDnsRecords(Map<DeploymentId, List<LoadBalancer>> loadBalancers) { + for (Map.Entry<DeploymentId, List<LoadBalancer>> entry : loadBalancers.entrySet()) { + ApplicationId application = entry.getKey().applicationId(); + ZoneId zone = entry.getKey().zoneId(); + try (Lock lock = db.lockRoutingPolicies()) { + Set<RoutingPolicy> policies = new LinkedHashSet<>(db.readRoutingPolicies(application)); + for (LoadBalancer loadBalancer : entry.getValue()) { + try { + policies.add(registerDnsAlias(application, zone, loadBalancer)); + } catch (Exception e) { + log.log(LogLevel.WARNING, "Failed to create or update DNS record for load balancer " + + loadBalancer.hostname() + ". Retrying in " + maintenanceInterval(), + e); } - db.writeRoutingPolicies(application.id(), policies); } + db.writeRoutingPolicies(application, policies); } } } @@ -111,30 +121,26 @@ public class RoutingPolicyMaintainer extends Maintainer { loadBalancer.rotations()); } - /** Find all load balancers assigned to application in given zone */ - private List<LoadBalancer> findLoadBalancersIn(ZoneId zone, ApplicationId application) { + /** Find all load balancers in given zone */ + private List<LoadBalancer> findLoadBalancersIn(ZoneId zone) { try { - return controller().applications().configServer().getLoadBalancers(new DeploymentId(application, zone)); + return controller().applications().configServer().getLoadBalancers(zone); } catch (Exception e) { log.log(LogLevel.WARNING, - String.format("Got exception fetching load balancers for application: %s, in zone: %s. Retrying in %s", - application.toShortString(), zone.value(), maintenanceInterval()), e); + String.format("Got exception fetching load balancers in zone: %s. Retrying in %s", + zone.value(), maintenanceInterval()), e); } return Collections.emptyList(); } /** Remove all DNS records that point to non-existing load balancers */ - private void removeObsoleteDnsRecords() { + private void removeObsoleteDnsRecords(Map<DeploymentId, List<LoadBalancer>> loadBalancers) { try (Lock lock = db.lockRoutingPolicies()) { List<RoutingPolicy> removalCandidates = new ArrayList<>(db.readRoutingPolicies()); - Set<HostName> activeLoadBalancers = controller().applications().asList().stream() - .map(Application::deployments) - .map(Map::values) - .flatMap(Collection::stream) - .map(Deployment::loadBalancers) - .map(Map::values) - .flatMap(Collection::stream) - .collect(Collectors.toUnmodifiableSet()); + Set<HostName> activeLoadBalancers = loadBalancers.values().stream() + .flatMap(Collection::stream) + .map(LoadBalancer::hostname) + .collect(Collectors.toSet()); // Remove any active load balancers removalCandidates.removeIf(lb -> activeLoadBalancers.contains(lb.canonicalName())); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java index 7f052fd7574..2242b3832de 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java @@ -89,7 +89,6 @@ public class ApplicationSerializer { private final String lastWrittenField = "lastWritten"; private final String lastQueriesPerSecondField = "lastQueriesPerSecond"; private final String lastWritesPerSecondField = "lastWritesPerSecond"; - private final String loadBalancers = "loadBalancers"; // DeploymentJobs fields private final String projectIdField = "projectId"; @@ -181,7 +180,6 @@ public class ApplicationSerializer { deployment.activity().lastWritten().ifPresent(instant -> object.setLong(lastWrittenField, instant.toEpochMilli())); deployment.activity().lastQueriesPerSecond().ifPresent(value -> object.setDouble(lastQueriesPerSecondField, value)); deployment.activity().lastWritesPerSecond().ifPresent(value -> object.setDouble(lastWritesPerSecondField, value)); - loadBalancersToSlime(deployment.loadBalancers(), object); } private void deploymentMetricsToSlime(DeploymentMetrics metrics, Cursor object) { @@ -304,13 +302,6 @@ public class ApplicationSerializer { }); } - private void loadBalancersToSlime(Map<ClusterSpec.Id, HostName> loadBalancerMap, Cursor parentObject) { - Cursor root = parentObject.setObject(loadBalancers); - for (Map.Entry<ClusterSpec.Id, HostName> entry : loadBalancerMap.entrySet()) { - root.setString(entry.getKey().value(), entry.getValue().value()); - } - } - // ------------------ Deserialization public Application fromSlime(Slime slime) { @@ -353,8 +344,7 @@ public class ApplicationSerializer { DeploymentActivity.create(optionalInstant(deploymentObject.field(lastQueriedField)), optionalInstant(deploymentObject.field(lastWrittenField)), optionalDouble(deploymentObject.field(lastQueriesPerSecondField)), - optionalDouble(deploymentObject.field(lastWritesPerSecondField))), - loadBalancerMapFromSlime(deploymentObject.field(loadBalancers))); + optionalDouble(deploymentObject.field(lastWritesPerSecondField)))); } private DeploymentMetrics deploymentMetricsFromSlime(Inspector object) { @@ -510,12 +500,6 @@ public class ApplicationSerializer { return field.valid() ? optionalString(field).map(RotationId::new) : Optional.empty(); } - private Map<ClusterSpec.Id, HostName> loadBalancerMapFromSlime(Inspector object) { - Map<ClusterSpec.Id, HostName> loadBalancers = new HashMap<>(); - object.traverse((String name, Inspector value) -> loadBalancers.put(new ClusterSpec.Id(name), HostName.from(value.asString()))); - return loadBalancers; - } - private OptionalLong optionalLong(Inspector field) { return field.valid() ? OptionalLong.of(field.asLong()) : OptionalLong.empty(); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java index e9920c9f1c6..0f43aaece7f 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java @@ -16,6 +16,7 @@ import com.yahoo.vespa.curator.Lock; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; +import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.RoutingPolicy; import com.yahoo.vespa.hosted.controller.deployment.Run; import com.yahoo.vespa.hosted.controller.deployment.Step; @@ -63,7 +64,7 @@ import static java.util.stream.Collectors.collectingAndThen; public class CuratorDb { private static final Logger log = Logger.getLogger(CuratorDb.class.getName()); - private static final Duration deployLockTimeout = Duration.ofMinutes(20); + private static final Duration deployLockTimeout = Duration.ofMinutes(30); private static final Duration defaultLockTimeout = Duration.ofMinutes(5); private static final Duration defaultTryLockTimeout = Duration.ofSeconds(1); @@ -119,6 +120,7 @@ public class CuratorDb { /** Creates a reentrant lock */ private Lock lock(Path path, Duration timeout) { + curator.create(path); Lock lock = locks.computeIfAbsent(path, (pathArg) -> new Lock(pathArg.getAbsolute(), curator)); lock.acquire(timeout); return lock; @@ -132,11 +134,8 @@ public class CuratorDb { return lock(lockPath(id), defaultLockTimeout.multipliedBy(2)); } - // Timeout should be higher than the time deployment takes, since there might be deployments wanting - // to run in parallel, too low timeout in that case has been seen to lead to deployments not - // getting the lock before it times out - public Lock lockForDeployment(ApplicationId id) { - return lock(lockPath(id), deployLockTimeout); + public Lock lockForDeployment(ApplicationId id, ZoneId zone) { + return lock(lockPath(id, zone), deployLockTimeout); } public Lock lock(ApplicationId id, JobType type) { @@ -504,48 +503,36 @@ public class CuratorDb { // -------------- Paths --------------------------------------------------- private Path lockPath(TenantName tenant) { - Path lockPath = lockRoot + return lockRoot .append(tenant.value()); - curator.create(lockPath); - return lockPath; } private Path lockPath(ApplicationId application) { - Path lockPath = lockRoot - .append(application.tenant().value()) + return lockPath(application.tenant()) .append(application.application().value()) .append(application.instance().value()); - curator.create(lockPath); - return lockPath; + } + + private Path lockPath(ApplicationId application, ZoneId zone) { + return lockPath(application) + .append(zone.environment().value()) + .append(zone.region().value()); } private Path lockPath(ApplicationId application, JobType type) { - Path lockPath = lockRoot - .append(application.tenant().value()) - .append(application.application().value()) - .append(application.instance().value()) + return lockPath(application) .append(type.jobName()); - curator.create(lockPath); - return lockPath; } private Path lockPath(ApplicationId application, JobType type, Step step) { - Path lockPath = lockRoot - .append(application.tenant().value()) - .append(application.application().value()) - .append(application.instance().value()) - .append(type.jobName()) + return lockPath(application, type) .append(step.name()); - curator.create(lockPath); - return lockPath; } private Path lockPath(String provisionId) { - Path lockPath = lockRoot + return lockRoot .append(provisionStatePath()) .append(provisionId); - curator.create(lockPath); - return lockPath; } private static Path inactiveJobsPath() { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializer.java index 0a4a13d3723..d996b79fe18 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializer.java @@ -2,11 +2,11 @@ package com.yahoo.vespa.hosted.controller.persistence; import com.yahoo.component.Version; +import com.yahoo.config.provision.CloudName; import com.yahoo.slime.ArrayTraverser; import com.yahoo.slime.Cursor; import com.yahoo.slime.Inspector; import com.yahoo.slime.Slime; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.versions.OsVersion; import java.util.Collections; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/cost/CostCalculator.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/cost/CostCalculator.java index d1774517099..88fe28a3613 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/cost/CostCalculator.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/cost/CostCalculator.java @@ -1,12 +1,12 @@ package com.yahoo.vespa.hosted.controller.restapi.cost; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.api.identifiers.Property; import com.yahoo.vespa.hosted.controller.api.integration.noderepository.NodeOwner; import com.yahoo.vespa.hosted.controller.api.integration.noderepository.NodeRepositoryClientInterface; import com.yahoo.vespa.hosted.controller.api.integration.noderepository.NodeRepositoryNode; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.restapi.cost.config.SelfHostedCostConfig; import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiHandler.java index 76e15c8de5a..aa3f5f9f909 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiHandler.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.controller.restapi.os; import com.yahoo.component.Version; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; import com.yahoo.container.jdisc.HttpRequest; @@ -14,7 +15,6 @@ import com.yahoo.slime.Inspector; import com.yahoo.slime.Slime; import com.yahoo.vespa.config.SlimeUtils; import com.yahoo.vespa.hosted.controller.Controller; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneList; import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse; @@ -27,11 +27,9 @@ import java.io.IOException; import java.io.InputStream; import java.io.UncheckedIOException; import java.util.List; -import java.util.Map; import java.util.Set; import java.util.StringJoiner; import java.util.logging.Level; -import java.util.stream.Collectors; /** * This implements the /os/v1 API which provides operators with information about, and scheduling of OS upgrades for diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersion.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersion.java index 89009270f10..df3899b9b23 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersion.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersion.java @@ -2,7 +2,7 @@ package com.yahoo.vespa.hosted.controller.versions; import com.yahoo.component.Version; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; +import com.yahoo.config.provision.CloudName; import org.jetbrains.annotations.NotNull; import java.util.Objects; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java index aa174b0df2b..78d6dababb5 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java @@ -3,11 +3,11 @@ package com.yahoo.vespa.hosted.controller.versions; import com.google.common.collect.ImmutableMap; import com.yahoo.component.Version; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.RegionName; import com.yahoo.vespa.hosted.controller.Controller; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.SystemApplication; import com.yahoo.vespa.hosted.controller.maintenance.OsUpgrader; diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java index 06417da8157..32068b006f0 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.controller; import com.yahoo.component.Version; +import com.yahoo.config.application.api.DeploymentSpec; import com.yahoo.config.application.api.ValidationId; import com.yahoo.config.application.api.ValidationOverrides; import com.yahoo.config.provision.ApplicationId; @@ -426,7 +427,7 @@ public class ControllerTest { } @Test - public void testDeployDirectly() { + public void testIntegrationTestDeployment() { DeploymentTester tester = new DeploymentTester(); Version six = Version.fromString("6.1"); tester.upgradeSystem(six); @@ -461,6 +462,28 @@ public class ControllerTest { } @Test + public void testDevDeployment() { + DeploymentTester tester = new DeploymentTester(); + ApplicationPackage applicationPackage = new ApplicationPackageBuilder() + .environment(Environment.dev) + .majorVersion(6) + .region("us-east-1") + .build(); + + // Create application + Application app = tester.createApplication("app1", "tenant1", 1, 2L); + ZoneId zone = ZoneId.from("dev", "us-east-1"); + + // Deploy + tester.controller().applications().deploy(app.id(), zone, Optional.of(applicationPackage), DeployOptions.none()); + assertTrue("Application deployed and activated", + tester.controllerTester().configServer().application(app.id()).get().activated()); + assertTrue("No job status added", + tester.applications().require(app.id()).deploymentJobs().jobStatus().isEmpty()); + assertEquals("DeploymentSpec is not persisted", DeploymentSpec.empty, tester.applications().require(app.id()).deploymentSpec()); + } + + @Test public void testSuspension() { DeploymentTester tester = new DeploymentTester(); Application app = tester.createApplication("app1", "tenant1", 1, 11L); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java index 4c60e8be677..69f348c9174 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java @@ -52,7 +52,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer private final Map<DeploymentId, ServiceConvergence> serviceStatus = new HashMap<>(); private final Version initialVersion = new Version(6, 1, 0); private final Set<DeploymentId> suspendedApplications = new HashSet<>(); - private final Map<DeploymentId, List<LoadBalancer>> loadBalancers = new HashMap<>(); + private final Map<ZoneId, List<LoadBalancer>> loadBalancers = new HashMap<>(); private Version lastPrepareVersion = null; private RuntimeException prepareException = null; @@ -171,16 +171,22 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer } @Override - public List<LoadBalancer> getLoadBalancers(DeploymentId deployment) { - return loadBalancers.getOrDefault(deployment, Collections.emptyList()); + public List<LoadBalancer> getLoadBalancers(ZoneId zone) { + return loadBalancers.getOrDefault(zone, Collections.emptyList()); } - public void addLoadBalancers(ZoneId zoneId, ApplicationId applicationId, List<LoadBalancer> loadBalancers) { - this.loadBalancers.put(new DeploymentId(applicationId, zoneId), loadBalancers); + public void addLoadBalancers(ZoneId zone, List<LoadBalancer> loadBalancers) { + this.loadBalancers.compute(zone, (k, existing) -> { + if (existing == null) { + existing = new ArrayList<>(); + } + existing.addAll(loadBalancers); + return existing; + }); } - public void removeLoadBalancers(DeploymentId deployment) { - this.loadBalancers.remove(deployment); + public void removeLoadBalancers(ApplicationId application, ZoneId zone) { + getLoadBalancers(zone).removeIf(lb -> lb.application().equals(application)); } @Override diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java index 9b13749a71a..02dadc300b3 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java @@ -5,13 +5,13 @@ import com.google.common.collect.ImmutableList; import com.google.inject.Inject; import com.yahoo.component.AbstractComponent; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.athenz.api.AthenzService; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.UpgradePolicy; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneFilter; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneFilterMock; diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java index c406cd346be..76fce662e27 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java @@ -2,10 +2,10 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.yahoo.component.Version; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.UpgradePolicy; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.SystemApplication; diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java index 0ea90a22910..abaaf3a5d86 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java @@ -2,8 +2,8 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.yahoo.component.Version; +import com.yahoo.config.provision.CloudName; import com.yahoo.vespa.hosted.controller.ControllerTester; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.UpgradePolicy; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb; diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainerTest.java index f1c571da451..addf43b7549 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicyMaintainerTest.java @@ -7,10 +7,7 @@ import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.RotationName; import com.yahoo.vespa.hosted.controller.Application; -import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.configserver.LoadBalancer; -import com.yahoo.vespa.hosted.controller.api.integration.dns.Record; -import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordId; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.application.RoutingPolicy; @@ -26,7 +23,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Supplier; +import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; @@ -35,99 +32,137 @@ import static org.junit.Assert.assertEquals; */ public class RoutingPolicyMaintainerTest { - @Test - public void maintains_routing_policies() { - DeploymentTester tester = new DeploymentTester(); - Application application = tester.createApplication("app1", "tenant1", 1, 1L); - RoutingPolicyMaintainer maintainer = new RoutingPolicyMaintainer(tester.controller(), Duration.ofHours(12), - new JobControl(new MockCuratorDb()), - tester.controllerTester().nameService(), - tester.controllerTester().curator()); - - ApplicationPackage applicationPackage = new ApplicationPackageBuilder() - .environment(Environment.prod) - .region("us-west-1") - .region("us-central-1") - .build(); - - int numberOfClustersPerZone = 2; + private final DeploymentTester tester = new DeploymentTester(); + private final Application app1 = tester.createApplication("app1", "tenant1", 1, 1L); + private final RoutingPolicyMaintainer maintainer = new RoutingPolicyMaintainer(tester.controller(), Duration.ofHours(12), + new JobControl(new MockCuratorDb()), + tester.controllerTester().nameService(), + tester.controllerTester().curator()); + private final ApplicationPackage applicationPackage = new ApplicationPackageBuilder() + .environment(Environment.prod) + .region("us-west-1") + .region("us-central-1") + .build(); + @Test + public void maintains_routing_policies_per_zone() { // Deploy application - tester.deployCompletely(application, applicationPackage); - setupClustersWithLoadBalancers(tester, application, numberOfClustersPerZone); + int clustersPerZone = 2; + tester.deployCompletely(app1, applicationPackage); + provisionLoadBalancers(app1, clustersPerZone); + // Creates records and policies for all clusters in all zones maintainer.maintain(); - Map<RecordId, Record> records = tester.controllerTester().nameService().records(); - Supplier<Long> recordCount = () -> records.entrySet().stream().filter(entry -> entry.getValue().data().asString().contains("loadbalancer")).count(); - assertEquals(4, (long) recordCount.get()); - - Set<RoutingPolicy> policies = tester.controller().curator().readRoutingPolicies(application.id()); - assertEquals(4, policies.size()); - - - // no update + Set<String> expectedRecords = Set.of( + "c0--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c0--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-central-1.vespa.oath.cloud" + ); + assertEquals(expectedRecords, records()); + assertEquals(4, policies(app1).size()); + + // Next run does nothing maintainer.maintain(); - Map<RecordId, Record> records2 = tester.controllerTester().nameService().records(); - assertEquals(4, (long) recordCount.get()); - assertEquals(records, records2); - - - // add 1 cluster per zone - setupClustersWithLoadBalancers(tester, application, numberOfClustersPerZone + 1); + assertEquals(expectedRecords, records()); + assertEquals(4, policies(app1).size()); + // Add 1 cluster in each zone + provisionLoadBalancers(app1, clustersPerZone + 1); maintainer.maintain(); - assertEquals(6, (long) recordCount.get()); - - Set<RoutingPolicy> policies2 = tester.controller().curator().readRoutingPolicies(application.id()); - assertEquals(6, policies2.size()); - - - // Add application - Application application2 = tester.createApplication("app2", "tenant1", 1, 1L); - tester.deployCompletely(application2, applicationPackage); - setupClustersWithLoadBalancers(tester, application2, numberOfClustersPerZone); - + expectedRecords = Set.of( + "c0--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c2--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c0--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c2--app1--tenant1.prod.us-central-1.vespa.oath.cloud" + ); + assertEquals(expectedRecords, records()); + assertEquals(6, policies(app1).size()); + + // Add another application + Application app2 = tester.createApplication("app2", "tenant1", 1, 1L); + tester.deployCompletely(app2, applicationPackage); + provisionLoadBalancers(app2, clustersPerZone); maintainer.maintain(); - assertEquals(10, (long) recordCount.get()); - - Set<RoutingPolicy> aliases4 = tester.controller().curator().readRoutingPolicies(application2.id()); - assertEquals(4, aliases4.size()); - - - // Remove cluster in app1 - setupClustersWithLoadBalancers(tester, application, numberOfClustersPerZone); - + expectedRecords = Set.of( + "c0--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c2--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c0--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c2--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c0--app2--tenant1.prod.us-central-1.vespa.oath.cloud", + "c1--app2--tenant1.prod.us-central-1.vespa.oath.cloud", + "c0--app2--tenant1.prod.us-west-1.vespa.oath.cloud", + "c1--app2--tenant1.prod.us-west-1.vespa.oath.cloud" + ); + assertEquals(expectedRecords, records()); + assertEquals(4, policies(app2).size()); + + + // Remove cluster from app1 + provisionLoadBalancers(app1, clustersPerZone); + maintainer.maintain(); + expectedRecords = Set.of( + "c0--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c0--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c0--app2--tenant1.prod.us-central-1.vespa.oath.cloud", + "c1--app2--tenant1.prod.us-central-1.vespa.oath.cloud", + "c0--app2--tenant1.prod.us-west-1.vespa.oath.cloud", + "c1--app2--tenant1.prod.us-west-1.vespa.oath.cloud" + ); + assertEquals(expectedRecords, records()); + + // Remove app2 completely + tester.controller().applications().require(app2.id()).deployments().keySet() + .forEach(zone -> { + tester.controller().applications().deactivate(app2.id(), zone); + tester.configServer().removeLoadBalancers(app2.id(), zone); + }); maintainer.maintain(); - assertEquals(8, (long) recordCount.get()); + expectedRecords = Set.of( + "c0--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-west-1.vespa.oath.cloud", + "c0--app1--tenant1.prod.us-central-1.vespa.oath.cloud", + "c1--app1--tenant1.prod.us-central-1.vespa.oath.cloud" + ); + assertEquals(expectedRecords, records()); + } - // Remove application app2 - tester.controller().applications().get(application2.id()) - .map(app -> app.deployments().keySet()) - .orElse(Collections.emptySet()) - .forEach(zone -> tester.controller().applications().deactivate(application2.id(), zone)); + private Set<RoutingPolicy> policies(Application application) { + return tester.controller().curator().readRoutingPolicies(application.id()); + } - maintainer.maintain(); - assertEquals(4, (long) recordCount.get()); + private Set<String> records() { + return tester.controllerTester().nameService().records().values().stream() + .map(r -> r.name().asString()) + .collect(Collectors.toSet()); } - private void setupClustersWithLoadBalancers(DeploymentTester tester, Application application, int numberOfClustersPerZone) { - tester.controller().applications().get(application.id()).orElseThrow(()->new RuntimeException("No deployments")).deployments().keySet() - .forEach(zone -> tester.configServer() - .removeLoadBalancers(new DeploymentId(application.id(), zone))); - tester.controller().applications().get(application.id()).orElseThrow(()->new RuntimeException("No deployments")).deployments().keySet() - .forEach(zone -> tester.configServer() - .addLoadBalancers(zone, application.id(), makeLoadBalancers(zone, application.id(), numberOfClustersPerZone))); + private void provisionLoadBalancers(Application application, int numberOfClustersPerZone) { + tester.controller().applications().require(application.id()) + .deployments().keySet() + .forEach(zone -> tester.configServer().removeLoadBalancers(application.id(), zone)); + tester.controller().applications().require(application.id()) + .deployments().keySet() + .forEach(zone -> tester.configServer() + .addLoadBalancers(zone, createLoadBalancers(zone, application.id(), numberOfClustersPerZone))); } - private List<LoadBalancer> makeLoadBalancers(ZoneId zone, ApplicationId application, int count) { + private static List<LoadBalancer> createLoadBalancers(ZoneId zone, ApplicationId application, int count, + Map<Integer, Set<RotationName>> clusterRotations) { List<LoadBalancer> loadBalancers = new ArrayList<>(); - Set<RotationName> rotations = Collections.singleton(RotationName.from("r1")); for (int i = 0; i < count; i++) { + Set<RotationName> rotations = clusterRotations.getOrDefault(i, Collections.emptySet()); loadBalancers.add( new LoadBalancer("LB-" + i + "-Z-" + zone.value(), application, - ClusterSpec.Id.from("cluster-" + i), + ClusterSpec.Id.from("c" + i), HostName.from("loadbalancer-" + i + "-" + application.serializedForm() + "-zone-" + zone.value()), Optional.of("dns-zone-1"), @@ -136,4 +171,8 @@ public class RoutingPolicyMaintainerTest { return loadBalancers; } + private static List<LoadBalancer> createLoadBalancers(ZoneId zone, ApplicationId application, int count) { + return createLoadBalancers(zone, application, count, Collections.emptyMap()); + } + } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java index 849b251a230..b07e983099a 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java @@ -1,7 +1,6 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.persistence; -import com.google.common.collect.ImmutableMap; import com.yahoo.component.Version; import com.yahoo.config.application.api.DeploymentSpec; import com.yahoo.config.application.api.ValidationOverrides; @@ -80,7 +79,7 @@ public class ApplicationSerializerTest { createClusterUtils(3, 0.2), createClusterInfo(3, 4), new DeploymentMetrics(2, 3, 4, 5, 6, Optional.of(Instant.now().truncatedTo(ChronoUnit.MILLIS))), DeploymentActivity.create(Optional.of(activityAt), Optional.of(activityAt), - OptionalDouble.of(200), OptionalDouble.of(10)), createLoadBalancers("default", "foo.bar"))); + OptionalDouble.of(200), OptionalDouble.of(10)))); OptionalLong projectId = OptionalLong.of(123L); List<JobStatus> statusList = new ArrayList<>(); @@ -200,9 +199,6 @@ public class ApplicationSerializerTest { Application original6 = writable(original).withOutstandingChange(Change.of(ApplicationVersion.from(new SourceRevision("a", "b", "c"), 42))).get(); Application serialized6 = applicationSerializer.fromSlime(applicationSerializer.toSlime(original6)); assertEquals(original6.outstandingChange(), serialized6.outstandingChange()); - - assertEquals(1, serialized.deployments().get(zone2).loadBalancers().size()); - assertEquals(original.deployments().get(zone2).loadBalancers(), serialized.deployments().get(zone2).loadBalancers()); } } @@ -236,10 +232,6 @@ public class ApplicationSerializerTest { return result; } - private Map<ClusterSpec.Id, HostName> createLoadBalancers(String clusterId, String hostName) { - return ImmutableMap.of(ClusterSpec.Id.from(clusterId), HostName.from(hostName)); - } - @Test public void testCompleteApplicationDeserialization() throws Exception { byte[] applicationJson = Files.readAllBytes(testData.resolve("complete-application.json")); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializerTest.java index bd510cc9aef..c625d9f7f64 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionSerializerTest.java @@ -3,7 +3,7 @@ package com.yahoo.vespa.hosted.controller.persistence; import com.google.common.collect.ImmutableSet; import com.yahoo.component.Version; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; +import com.yahoo.config.provision.CloudName; import com.yahoo.vespa.hosted.controller.versions.OsVersion; import org.junit.Test; diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionStatusSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionStatusSerializerTest.java index 6e2298fd64e..5073f651fd3 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionStatusSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionStatusSerializerTest.java @@ -2,10 +2,10 @@ package com.yahoo.vespa.hosted.controller.persistence; import com.yahoo.component.Version; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.RegionName; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.versions.OsVersion; import com.yahoo.vespa.hosted.controller.versions.OsVersionStatus; import org.junit.Test; diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application.json index 443a2c23b6a..3744e44152a 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application.json @@ -66,7 +66,7 @@ "gitCommit": "commit1" } }, - "reason": "Testing deployment for production-us-central-1 (platform 6.1, application 1.0.42-commit1)", + "reason": "Testing last changes outside prod", "at": "(ignore)" }, "lastCompleted": { @@ -80,7 +80,7 @@ "gitCommit": "commit1" } }, - "reason": "Testing deployment for production-us-central-1 (platform 6.1, application 1.0.42-commit1)", + "reason": "Testing last changes outside prod", "at": "(ignore)" }, "lastSuccess": { @@ -94,7 +94,7 @@ "gitCommit": "commit1" } }, - "reason": "Testing deployment for production-us-central-1 (platform 6.1, application 1.0.42-commit1)", + "reason": "Testing last changes outside prod", "at": "(ignore)" } }, diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application1-recursive.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application1-recursive.json index 64c92f9cde4..822bc447d8d 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application1-recursive.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application1-recursive.json @@ -66,7 +66,7 @@ "gitCommit": "commit1" } }, - "reason": "Testing deployment for production-us-central-1 (platform 6.1, application 1.0.42-commit1)", + "reason": "Testing last changes outside prod", "at": "(ignore)" }, "lastCompleted": { @@ -80,7 +80,7 @@ "gitCommit": "commit1" } }, - "reason": "Testing deployment for production-us-central-1 (platform 6.1, application 1.0.42-commit1)", + "reason": "Testing last changes outside prod", "at": "(ignore)" }, "lastSuccess": { @@ -94,7 +94,7 @@ "gitCommit": "commit1" } }, - "reason": "Testing deployment for production-us-central-1 (platform 6.1, application 1.0.42-commit1)", + "reason": "Testing last changes outside prod", "at": "(ignore)" } }, diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/cost/CostApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/cost/CostApiTest.java index 4b523446b13..333d3155262 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/cost/CostApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/cost/CostApiTest.java @@ -2,10 +2,10 @@ package com.yahoo.vespa.hosted.controller.restapi.cost; import com.yahoo.application.container.handler.Request; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.athenz.api.AthenzIdentity; import com.yahoo.vespa.athenz.api.AthenzUser; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.integration.ZoneRegistryMock; import com.yahoo.vespa.hosted.controller.restapi.ContainerControllerTester; diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java index 9ec254b59fd..62aa157f1ab 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java @@ -4,11 +4,11 @@ package com.yahoo.vespa.hosted.controller.restapi.os; import com.google.common.collect.ImmutableList; import com.yahoo.application.container.handler.Request; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.athenz.api.AthenzIdentity; import com.yahoo.vespa.athenz.api.AthenzUser; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node; -import com.yahoo.vespa.hosted.controller.api.integration.zone.CloudName; import com.yahoo.vespa.hosted.controller.api.integration.zone.UpgradePolicy; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.SystemApplication; diff --git a/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java b/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java index 1053c5ff44d..418837ca2a0 100644 --- a/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java +++ b/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java @@ -152,8 +152,11 @@ public class JsonSerializationHelper { } public static <T extends FieldValue> void serializeWeightedSet(JsonGenerator generator, FieldBase field, WeightedSet<T> value) { + // Hide empty fields + if (value.size() == 0) { + return; + } fieldNameIfNotNull(generator, field); - wrapIOException(() -> { generator.writeStartObject(); @@ -168,8 +171,11 @@ public class JsonSerializationHelper { } public static <T extends FieldValue> void serializeCollectionField(FieldWriter fieldWriter, JsonGenerator generator, FieldBase field, CollectionFieldValue<T> value) { + // Hide empty fields + if (value.size() == 0) { + return; + } fieldNameIfNotNull(generator, field); - wrapIOException(() -> { generator.writeStartArray(); Iterator<T> i = value.iterator(); @@ -184,6 +190,10 @@ public class JsonSerializationHelper { public static <K extends FieldValue, V extends FieldValue> void serializeMapField(FieldWriter fieldWriter, JsonGenerator generator, FieldBase field, MapFieldValue<K, V> map) { + // Hide empty fields + if (map.size() == 0) { + return; + } fieldNameIfNotNull(generator, field); wrapIOException(() -> { generator.writeStartObject(); @@ -203,6 +213,10 @@ public class JsonSerializationHelper { } public static <T extends FieldValue> void serializeArrayField(FieldWriter fieldWriter, JsonGenerator generator, FieldBase field, Array<T> value) { + // Hide empty fields + if (value.size() == 0) { + return; + } wrapIOException(() -> { fieldNameIfNotNull(generator, field); generator.writeStartArray(); diff --git a/document/src/tests/documentupdatetestcase.cpp b/document/src/tests/documentupdatetestcase.cpp index c7660e5d527..5100e3683e5 100644 --- a/document/src/tests/documentupdatetestcase.cpp +++ b/document/src/tests/documentupdatetestcase.cpp @@ -12,8 +12,8 @@ #include <vespa/document/update/fieldupdate.h> #include <vespa/document/update/mapvalueupdate.h> #include <vespa/document/update/removevalueupdate.h> -#include <vespa/document/update/tensoraddupdate.h> -#include <vespa/document/update/tensormodifyupdate.h> +#include <vespa/document/update/tensor_add_update.h> +#include <vespa/document/update/tensor_modify_update.h> #include <vespa/document/update/valueupdate.h> #include <vespa/document/serialization/vespadocumentserializer.h> #include <vespa/document/util/bytebuffer.h> @@ -178,6 +178,12 @@ std::unique_ptr<Tensor> createExpectedUpdatedTensorWith2Cells() { {{{"x", "9"}, {"y", "9"}}, 11} }, {"x", "y"}); } +std::unique_ptr<Tensor> createExpectedAddUpdatedTensorWith3Cells() { + return createTensor({ {{{"x", "8"}, {"y", "8"}}, 2}, + {{{"x", "8"}, {"y", "9"}}, 2}, + {{{"x", "9"}, {"y", "9"}}, 11} }, {"x", "y"}); +} + FieldValue::UP createTensorFieldValueWith2Cells() { auto fv(std::make_unique<TensorFieldValue>()); *fv = createTensorWith2Cells(); @@ -973,14 +979,21 @@ DocumentUpdateTest::testTensorAddUpdate() updated.setValue(updated.getField("tensor"), *oldTensor); CPPUNIT_ASSERT(*doc != updated); testValueUpdate(*createTensorAddUpdate(), *DataType::TENSOR); + std::string expTensorAddUpdateString("TensorAddUpdate(" + "{TensorFieldValue: " + "{\"dimensions\":[\"x\",\"y\"]," + "\"cells\":[" + "{\"address\":{\"x\":\"8\",\"y\":\"9\"},\"value\":2}," + "{\"address\":{\"x\":\"8\",\"y\":\"8\"},\"value\":2}" + "]}})"); + CPPUNIT_ASSERT_EQUAL(expTensorAddUpdateString, createTensorAddUpdate()->toString()); DocumentUpdate upd(docMan.getTypeRepo(), *doc->getDataType(), doc->getId()); upd.addUpdate(FieldUpdate(upd.getType().getField("tensor")).addUpdate(*createTensorAddUpdate())); upd.applyTo(updated); FieldValue::UP fval(updated.getValue("tensor")); CPPUNIT_ASSERT(fval); auto &tensor = asTensor(*fval); - // Note: Placeholder value for now - auto expectedUpdatedTensor = createTensorWith2Cells(); + auto expectedUpdatedTensor = createExpectedAddUpdatedTensorWith3Cells(); CPPUNIT_ASSERT(tensor.equals(*expectedUpdatedTensor)); } @@ -994,6 +1007,13 @@ DocumentUpdateTest::testTensorModifyUpdate() updated.setValue(updated.getField("tensor"), *oldTensor); CPPUNIT_ASSERT(*doc != updated); testValueUpdate(*createTensorModifyUpdate(), *DataType::TENSOR); + std::string expTensorModifyUpdateString("TensorModifyUpdate(replace," + "{TensorFieldValue: " + "{\"dimensions\":[\"x\",\"y\"]," + "\"cells\":[" + "{\"address\":{\"x\":\"8\",\"y\":\"9\"},\"value\":2}" + "]}})"); + CPPUNIT_ASSERT_EQUAL(expTensorModifyUpdateString, createTensorModifyUpdate()->toString()); DocumentUpdate upd(docMan.getTypeRepo(), *doc->getDataType(), doc->getId()); upd.addUpdate(FieldUpdate(upd.getType().getField("tensor")).addUpdate(*createTensorModifyUpdate())); upd.applyTo(updated); diff --git a/document/src/vespa/document/update/CMakeLists.txt b/document/src/vespa/document/update/CMakeLists.txt index 34f539ee4aa..2ece7877bdb 100644 --- a/document/src/vespa/document/update/CMakeLists.txt +++ b/document/src/vespa/document/update/CMakeLists.txt @@ -13,8 +13,8 @@ vespa_add_library(document_updates OBJECT mapvalueupdate.cpp removefieldpathupdate.cpp removevalueupdate.cpp - tensoraddupdate.cpp - tensormodifyupdate.cpp + tensor_add_update.cpp + tensor_modify_update.cpp valueupdate.cpp DEPENDS AFTER diff --git a/document/src/vespa/document/update/tensoraddupdate.cpp b/document/src/vespa/document/update/tensor_add_update.cpp index eb708d9f651..6d251b04937 100644 --- a/document/src/vespa/document/update/tensoraddupdate.cpp +++ b/document/src/vespa/document/update/tensor_add_update.cpp @@ -1,6 +1,6 @@ // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include "tensoraddupdate.h" +#include "tensor_add_update.h" #include <vespa/document/base/exceptions.h> #include <vespa/document/base/field.h> #include <vespa/document/fieldvalue/document.h> @@ -81,7 +81,11 @@ TensorAddUpdate::checkCompatibility(const Field& field) const std::unique_ptr<Tensor> TensorAddUpdate::applyTo(const Tensor &tensor) const { - return tensor.clone(); + auto &addTensor = _tensor->getAsTensorPtr(); + if (addTensor) { + return tensor.add(*addTensor); + } + return std::unique_ptr<Tensor>(); } bool @@ -112,9 +116,11 @@ TensorAddUpdate::printXml(XmlOutputStream& xos) const void TensorAddUpdate::print(std::ostream& out, bool verbose, const std::string& indent) const { - (void) verbose; - (void) indent; - out << "{TensorAddUpdate::print not yet implemented}"; + out << indent << "TensorAddUpdate("; + if (_tensor) { + _tensor->print(out, verbose, indent); + } + out << ")"; } void diff --git a/document/src/vespa/document/update/tensoraddupdate.h b/document/src/vespa/document/update/tensor_add_update.h index a92ff0101f0..a92ff0101f0 100644 --- a/document/src/vespa/document/update/tensoraddupdate.h +++ b/document/src/vespa/document/update/tensor_add_update.h diff --git a/document/src/vespa/document/update/tensormodifyupdate.cpp b/document/src/vespa/document/update/tensor_modify_update.cpp index a02379e4991..bb846581697 100644 --- a/document/src/vespa/document/update/tensormodifyupdate.cpp +++ b/document/src/vespa/document/update/tensor_modify_update.cpp @@ -1,6 +1,6 @@ // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include "tensormodifyupdate.h" +#include "tensor_modify_update.h" #include <vespa/document/base/exceptions.h> #include <vespa/document/base/field.h> #include <vespa/document/fieldvalue/document.h> @@ -50,6 +50,23 @@ getJoinFunction(TensorModifyUpdate::Operation operation) } } +vespalib::string +getJoinFunctionName(TensorModifyUpdate::Operation operation) +{ + using Operation = TensorModifyUpdate::Operation; + + switch (operation) { + case Operation::REPLACE: + return "replace"; + case Operation::ADD: + return "add"; + case Operation::MUL: + return "multiply"; + default: + throw IllegalArgumentException("Bad operation", VESPA_STRLOC); + } +} + } IMPLEMENT_IDENTIFIABLE(TensorModifyUpdate, ValueUpdate); @@ -156,9 +173,11 @@ TensorModifyUpdate::printXml(XmlOutputStream& xos) const void TensorModifyUpdate::print(std::ostream& out, bool verbose, const std::string& indent) const { - (void) verbose; - (void) indent; - out << "{TensorModifyUpdate::print not yet implemented}"; + out << indent << "TensorModifyUpdate(" << getJoinFunctionName(_operation) << ","; + if (_tensor) { + _tensor->print(out, verbose, indent); + } + out << ")"; } void diff --git a/document/src/vespa/document/update/tensormodifyupdate.h b/document/src/vespa/document/update/tensor_modify_update.h index dcb9bcf0470..dcb9bcf0470 100644 --- a/document/src/vespa/document/update/tensormodifyupdate.h +++ b/document/src/vespa/document/update/tensor_modify_update.h diff --git a/document/src/vespa/document/update/updates.h b/document/src/vespa/document/update/updates.h index 1609c5bc3a3..4e520e61690 100644 --- a/document/src/vespa/document/update/updates.h +++ b/document/src/vespa/document/update/updates.h @@ -10,6 +10,6 @@ #include "clearvalueupdate.h" #include "mapvalueupdate.h" #include "removevalueupdate.h" -#include "tensormodifyupdate.h" -#include "tensoraddupdate.h" +#include "tensor_add_update.h" +#include "tensor_modify_update.h" diff --git a/eval/src/vespa/eval/tensor/dense/mutable_dense_tensor_view.cpp b/eval/src/vespa/eval/tensor/dense/mutable_dense_tensor_view.cpp index e3b4c8dee42..79a8b994480 100644 --- a/eval/src/vespa/eval/tensor/dense/mutable_dense_tensor_view.cpp +++ b/eval/src/vespa/eval/tensor/dense/mutable_dense_tensor_view.cpp @@ -21,13 +21,13 @@ MutableDenseTensorView::MutableValueType::MutableValueType(ValueType type_in) MutableDenseTensorView::MutableValueType::~MutableValueType() = default; MutableDenseTensorView::MutableDenseTensorView(ValueType type_in) - : DenseTensorView(_concreteType.fast_type(), CellsRef()), + : DenseTensorView(_concreteType._type, CellsRef()), _concreteType(type_in) { } MutableDenseTensorView::MutableDenseTensorView(ValueType type_in, CellsRef cells_in) - : DenseTensorView(_concreteType.fast_type(), cells_in), + : DenseTensorView(_concreteType._type, cells_in), _concreteType(type_in) { } diff --git a/eval/src/vespa/eval/tensor/dense/mutable_dense_tensor_view.h b/eval/src/vespa/eval/tensor/dense/mutable_dense_tensor_view.h index b68a1594905..2132f861896 100644 --- a/eval/src/vespa/eval/tensor/dense/mutable_dense_tensor_view.h +++ b/eval/src/vespa/eval/tensor/dense/mutable_dense_tensor_view.h @@ -15,8 +15,8 @@ class MutableDenseTensorView : public DenseTensorView private: struct MutableValueType { - private: eval::ValueType _type; + private: std::vector<eval::ValueType::Dimension::size_type *> _unboundDimSizes; public: diff --git a/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/FieldUpdateAdapter.java b/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/FieldUpdateAdapter.java index 0e3970db7fa..6c894dcdf4c 100644 --- a/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/FieldUpdateAdapter.java +++ b/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/FieldUpdateAdapter.java @@ -139,6 +139,8 @@ public class FieldUpdateAdapter implements UpdateAdapter { } } else if (upd instanceof TensorModifyUpdate) { lst.add(upd); + } else if (upd instanceof TensorAddUpdate) { + lst.add(upd); } else { throw new UnsupportedOperationException( "Value update type " + upd.getClass().getName() + " not supported."); diff --git a/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/FieldUpdateHelper.java b/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/FieldUpdateHelper.java index 61e38bd8bc6..40a3f832592 100644 --- a/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/FieldUpdateHelper.java +++ b/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/FieldUpdateHelper.java @@ -91,7 +91,10 @@ public abstract class FieldUpdateHelper { throw new IllegalArgumentException("Expected multi-value data type, got " + val.getDataType().getName() + "."); } } else if (upd instanceof TensorModifyUpdate) { - // TODO: apply update to field value when supported in TensorModifyUpdate in Java. + // TODO: apply update to field value when supported in TensorModifyUpdate in Java? + return val; + } else if (upd instanceof TensorAddUpdate) { + // TODO: apply update to field value when supported in TensorAddUpdate in Java? return val; } throw new UnsupportedOperationException("Value update type " + upd.getClass().getName() + " not supported."); diff --git a/metrics/src/vespa/metrics/xmlwriter.cpp b/metrics/src/vespa/metrics/xmlwriter.cpp index 90b25621cf8..25bc4e23d96 100644 --- a/metrics/src/vespa/metrics/xmlwriter.cpp +++ b/metrics/src/vespa/metrics/xmlwriter.cpp @@ -11,8 +11,8 @@ namespace metrics { XmlWriter::XmlWriter(vespalib::xml::XmlOutputStream& xos, - uint32_t period, int verbosity) - : _period(period), _xos(xos), _verbosity(verbosity) {} + [[maybe_unused]] uint32_t period, int verbosity) + : _xos(xos), _verbosity(verbosity) {} bool XmlWriter::visitSnapshot(const MetricSnapshot& snapshot) diff --git a/metrics/src/vespa/metrics/xmlwriter.h b/metrics/src/vespa/metrics/xmlwriter.h index fbaf59d2ecd..31feb18bae8 100644 --- a/metrics/src/vespa/metrics/xmlwriter.h +++ b/metrics/src/vespa/metrics/xmlwriter.h @@ -8,7 +8,6 @@ namespace metrics { class XmlWriter : public MetricVisitor { - uint32_t _period; vespalib::xml::XmlOutputStream& _xos; int _verbosity; diff --git a/node-admin/pom.xml b/node-admin/pom.xml index fda4acebfa0..d550ceb7c9d 100644 --- a/node-admin/pom.xml +++ b/node-admin/pom.xml @@ -55,6 +55,16 @@ <version>${project.version}</version> <scope>provided</scope> </dependency> + <dependency> + <groupId>com.fasterxml.jackson.core</groupId> + <artifactId>jackson-databind</artifactId> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.fasterxml.jackson.core</groupId> + <artifactId>jackson-annotations</artifactId> + <scope>provided</scope> + </dependency> <!-- Compile --> <dependency> diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeAttributes.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeAttributes.java index 28e812e6ca1..a5635507ab7 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeAttributes.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeAttributes.java @@ -145,6 +145,10 @@ public class NodeAttributes { currentFirmwareCheck, hardwareDivergence, hardwareFailureDescription, wantToDeprovision, reports); } + public boolean isEmpty() { + return equals(new NodeAttributes()); + } + @Override public boolean equals(final Object o) { if (!(o instanceof NodeAttributes)) { diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/BaseReport.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/BaseReport.java index 26c1b90218f..42804b69b17 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/BaseReport.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/BaseReport.java @@ -10,6 +10,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import java.util.Objects; +import java.util.Optional; import static com.yahoo.yolean.Exceptions.uncheck; @@ -21,8 +22,8 @@ import static com.yahoo.yolean.Exceptions.uncheck; * <p><strong>Subclass requirements</strong> * * <ol> - * <li>A subclass must maintain the property that {@link ObjectMapper} can map an instance to {@link JsonNode}, - * see {@link #toJsonNode()}.</li> + * <li>A subclass mush be a Jackson class that can be mapped to {@link JsonNode} with {@link #toJsonNode()}, + * and from {@link JsonNode} with {@link #fromJsonNode(JsonNode, Class)}.</li> * <li>A subclass must override {@link #updates(BaseReport)} and make sure to return false if * {@code !super.updates(current)}.</li> * </ol> @@ -52,12 +53,12 @@ public class BaseReport { } @JsonGetter(CREATED_FIELD) - public Long getCreatedMillisOrNull() { + public final Long getCreatedMillisOrNull() { return createdMillis; } @JsonGetter(DESCRIPTION_FIELD) - public String getDescriptionOrNull() { + public final String getDescriptionOrNull() { return description; } @@ -73,8 +74,36 @@ public class BaseReport { return !Objects.equals(description, current.description); } + /** A variant of {@link #updates(BaseReport)} handling possibly absent reports, whether new or old. */ + public static <TNEW extends BaseReport, TOLD extends BaseReport> + boolean updates2(Optional<TNEW> newReport, Optional<TOLD> oldReport) { + if (newReport.isPresent() ^ oldReport.isPresent()) return true; + return newReport.map(r -> r.updates(oldReport.get())).orElse(false); + } + + public static BaseReport fromJsonNode(JsonNode jsonNode) { + return fromJsonNode(jsonNode, BaseReport.class); + } + + public static <R extends BaseReport> R fromJsonNode(JsonNode jsonNode, Class<R> jacksonClass) { + return uncheck(() -> mapper.treeToValue(jsonNode, jacksonClass)); + } + + public static BaseReport fromJson(String json) { + return fromJson(json, BaseReport.class); + } + + public static <R extends BaseReport> R fromJson(String json, Class<R> jacksonClass) { + return uncheck(() -> mapper.readValue(json, jacksonClass)); + } + /** Returns {@code this} as a {@link JsonNode}. */ public JsonNode toJsonNode() { return uncheck(() -> mapper.valueToTree(this)); } + + /** Returns {@code this} as a compact JSON string. */ + public String toJson() { + return uncheck(() -> mapper.writeValueAsString(this)); + } } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/BaseReportTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/BaseReportTest.java index eec17f019bd..561733d7d76 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/BaseReportTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/reports/BaseReportTest.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.node.admin.configserver.noderepository.reports; import com.yahoo.test.json.JsonTestHelper; import org.junit.Test; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -11,11 +12,11 @@ import static org.junit.Assert.assertTrue; * @author hakonhall */ public class BaseReportTest { + private static final String JSON_1 = "{\"createdMillis\": 1, \"description\": \"desc\"}"; @Test public void testSerialization() { - BaseReport report = new BaseReport(1L, "desc"); JsonTestHelper.assertJsonEquals(new BaseReport(1L, "desc").toJsonNode(), - "{\"createdMillis\": 1, \"description\": \"desc\"}"); + JSON_1); JsonTestHelper.assertJsonEquals(new BaseReport(null, "desc").toJsonNode(), "{\"description\": \"desc\"}"); JsonTestHelper.assertJsonEquals(new BaseReport(1L, null).toJsonNode(), @@ -35,4 +36,12 @@ public class BaseReportTest { assertTrue(new BaseReport(1L, "desc 2").updates(report)); assertTrue(new BaseReport(1L, null).updates(report)); } + + @Test + public void testJsonSerialization() { + BaseReport report = BaseReport.fromJson(JSON_1); + assertEquals(1L, (long) report.getCreatedMillisOrNull()); + assertEquals("desc", report.getDescriptionOrNull()); + JsonTestHelper.assertJsonEquals(report.toJson(), JSON_1); + } }
\ No newline at end of file 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 39a2787ca9b..42dbcdf7a86 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 @@ -25,6 +25,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -34,7 +35,7 @@ import java.util.stream.Collectors; public class MetricsReporter extends Maintainer { private final Metric metric; - private final Orchestrator orchestrator; + private final Function<HostName, Optional<HostStatus>> orchestrator; private final ServiceMonitor serviceMonitor; private final Map<Map<String, String>, Metric.Context> contextMap = new HashMap<>(); private final Supplier<Integer> pendingRedeploymentsSupplier; @@ -48,7 +49,7 @@ public class MetricsReporter extends Maintainer { JobControl jobControl) { super(nodeRepository, interval, jobControl); this.metric = metric; - this.orchestrator = orchestrator; + this.orchestrator = orchestrator.getNodeStatuses(); this.serviceMonitor = serviceMonitor; this.pendingRedeploymentsSupplier = pendingRedeploymentsSupplier; } @@ -129,13 +130,9 @@ public class MetricsReporter extends Maintainer { node.status().hardwareDivergence().isPresent() ? 1 : 0, context); - try { - HostStatus status = orchestrator.getNodeStatus(new HostName(node.hostname())); - boolean allowedToBeDown = status == HostStatus.ALLOWED_TO_BE_DOWN; - metric.set("allowedToBeDown", allowedToBeDown ? 1 : 0, context); - } catch (HostNameNotFoundException e) { - // Ignore - } + orchestrator.apply(new HostName(node.hostname())) + .map(status -> status == HostStatus.ALLOWED_TO_BE_DOWN ? 1 : 0) + .ifPresent(allowedToBeDown -> metric.set("allowedToBeDown", allowedToBeDown, context)); long numberOfServices; HostName hostName = new HostName(node.hostname()); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesResponse.java index 2a2ca6bfd87..5b942497be8 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesResponse.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesResponse.java @@ -22,7 +22,9 @@ import java.io.IOException; import java.io.OutputStream; import java.net.URI; import java.util.List; +import java.util.Optional; import java.util.Set; +import java.util.function.Function; /** * @author bratseth @@ -40,7 +42,7 @@ class NodesResponse extends HttpResponse { private final NodeFilter filter; private final boolean recursive; - private final Orchestrator orchestrator; + private final Function<HostName, Optional<HostStatus>> orchestrator; private final NodeRepository nodeRepository; private final Slime slime; private final NodeSerializer serializer = new NodeSerializer(); @@ -52,7 +54,7 @@ class NodesResponse extends HttpResponse { this.nodeParentUrl = toNodeParentUrl(request); filter = NodesApiHandler.toNodeFilter(request); this.recursive = request.getBooleanProperty("recursive"); - this.orchestrator = orchestrator; + this.orchestrator = orchestrator.getNodeStatuses(); this.nodeRepository = nodeRepository; slime = new Slime(); @@ -158,11 +160,9 @@ class NodesResponse extends HttpResponse { object.setLong("currentRestartGeneration", node.allocation().get().restartGeneration().current()); object.setString("wantedDockerImage", nodeRepository.dockerImage().withTag(node.allocation().get().membership().cluster().vespaVersion()).asString()); object.setString("wantedVespaVersion", node.allocation().get().membership().cluster().vespaVersion().toFullString()); - try { - object.setBool("allowedToBeDown", - orchestrator.getNodeStatus(new HostName(node.hostname())) == HostStatus.ALLOWED_TO_BE_DOWN); - } - catch (HostNameNotFoundException e) {/* ok */ } + orchestrator.apply(new HostName(node.hostname())) + .map(status -> status == HostStatus.ALLOWED_TO_BE_DOWN) + .ifPresent(allowedToBeDown -> object.setBool("allowedToBeDown", allowedToBeDown)); } object.setLong("rebootGeneration", node.status().reboot().wanted()); object.setLong("currentRebootGeneration", node.status().reboot().current()); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java index 70750dd6672..96ec0349fb2 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java @@ -11,7 +11,9 @@ import com.yahoo.vespa.orchestrator.status.HostStatus; import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; +import java.util.function.Function; /** * @author bratseth @@ -32,6 +34,11 @@ public class OrchestratorMock implements Orchestrator { } @Override + public Function<HostName, Optional<HostStatus>> getNodeStatuses() { + return hostName -> Optional.of(getNodeStatus(hostName)); + } + + @Override public void setNodeStatus(HostName hostName, HostStatus state) {} @Override diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/ServiceMonitorStub.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/ServiceMonitorStub.java index 8215dc8ecd0..b9c5df9d999 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/ServiceMonitorStub.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/ServiceMonitorStub.java @@ -62,7 +62,11 @@ public class ServiceMonitorStub implements ServiceMonitor { } @Override - public Map<ApplicationInstanceReference, ApplicationInstance> getAllApplicationInstances() { + public ServiceModel getServiceModelSnapshot() { + return new ServiceModel(getAllApplicationInstances()); + } + + private Map<ApplicationInstanceReference, ApplicationInstance> getAllApplicationInstances() { // Convert apps information to the response payload to return Map<ApplicationInstanceReference, ApplicationInstance> status = new HashMap<>(); for (Map.Entry<ApplicationId, MockDeployer.ApplicationContext> app : apps.entrySet()) { @@ -84,8 +88,4 @@ public class ServiceMonitorStub implements ServiceMonitor { return status; } - @Override - public ServiceModel getServiceModelSnapshot() { - return new ServiceModel(getAllApplicationInstances()); - } } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java index 1e502439aeb..57b942d85e4 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java @@ -38,7 +38,6 @@ import java.util.Optional; import java.util.Set; import static org.junit.Assert.assertEquals; -import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -90,7 +89,7 @@ public class MetricsReporterTest { Orchestrator orchestrator = mock(Orchestrator.class); ServiceMonitor serviceMonitor = mock(ServiceMonitor.class); - when(orchestrator.getNodeStatus(any())).thenReturn(HostStatus.NO_REMARKS); + when(orchestrator.getNodeStatuses()).thenReturn(hostName -> Optional.of(HostStatus.NO_REMARKS)); ServiceModel serviceModel = mock(ServiceModel.class); when(serviceMonitor.getServiceModelSnapshot()).thenReturn(serviceModel); when(serviceModel.getServiceInstancesByHostName()).thenReturn(Collections.emptyMap()); @@ -137,7 +136,7 @@ public class MetricsReporterTest { Orchestrator orchestrator = mock(Orchestrator.class); ServiceMonitor serviceMonitor = mock(ServiceMonitor.class); - when(orchestrator.getNodeStatus(any())).thenReturn(HostStatus.NO_REMARKS); + when(orchestrator.getNodeStatuses()).thenReturn(hostName -> Optional.of(HostStatus.NO_REMARKS)); ServiceModel serviceModel = mock(ServiceModel.class); when(serviceMonitor.getServiceModelSnapshot()).thenReturn(serviceModel); when(serviceModel.getServiceInstancesByHostName()).thenReturn(Collections.emptyMap()); diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java index a639d07e504..59b320cf501 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java @@ -10,7 +10,9 @@ import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus; import com.yahoo.vespa.orchestrator.status.HostStatus; import java.util.List; +import java.util.Optional; import java.util.Set; +import java.util.function.Function; /** * The orchestrator is used to coordinate the need of vespa services to restart or @@ -46,6 +48,14 @@ public interface Orchestrator { */ HostStatus getNodeStatus(HostName hostName) throws HostNameNotFoundException; + /** + * Returns a not necessarily consistent mapping from host names to their statuses, for hosts known by this. + * + * Prefer this to {@link #getNodeStatus(HostName)} when consistency is not required, and when doing bulk reads. + * @return a mapping from host names to their statuses. Unknown hosts map to {@code Optional.empty()}. + */ + Function<HostName, Optional<HostStatus>> getNodeStatuses(); + void setNodeStatus(HostName hostName, HostStatus state) throws OrchestrationException; /** diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java index 33cfa310a68..bd8e31cd283 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java @@ -35,8 +35,10 @@ import java.time.Clock; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -104,6 +106,14 @@ public class OrchestratorImpl implements Orchestrator { } @Override + public Function<HostName, Optional<HostStatus>> getNodeStatuses() { + Function<ApplicationInstanceReference, Set<HostName>> suspendedHosts = statusService.getSuspendedHostsByApplication(); + return hostName -> instanceLookupService.findInstanceByHost(hostName) + .map(application -> suspendedHosts.apply(application.reference()).contains(hostName) + ? HostStatus.ALLOWED_TO_BE_DOWN : HostStatus.NO_REMARKS); + } + + @Override public void setNodeStatus(HostName hostName, HostStatus status) throws OrchestrationException { ApplicationInstanceReference reference = getApplicationInstance(hostName).reference(); OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); @@ -133,13 +143,13 @@ public class OrchestratorImpl implements Orchestrator { OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); try (MutableStatusRegistry statusRegistry = statusService .lockApplicationInstance_forCurrentThreadOnly(context, appInstance.reference())) { - final HostStatus currentHostState = statusRegistry.getHostStatus(hostName); + HostStatus currentHostState = statusRegistry.getHostStatus(hostName); if (HostStatus.NO_REMARKS == currentHostState) { return; } - ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(appInstance.reference()).getApplicationInstanceStatus(); + ApplicationInstanceStatus appStatus = statusRegistry.getStatus(); if (appStatus == ApplicationInstanceStatus.NO_REMARKS) { policy.releaseSuspensionGrant(context.createSubcontextWithinLock(), appInstance, hostName, statusRegistry); } @@ -181,7 +191,7 @@ public class OrchestratorImpl implements Orchestrator { try (MutableStatusRegistry hostStatusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly(context, applicationReference)) { - ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(applicationReference).getApplicationInstanceStatus(); + ApplicationInstanceStatus appStatus = hostStatusRegistry.getStatus(); if (appStatus == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN) { return; } @@ -195,8 +205,8 @@ public class OrchestratorImpl implements Orchestrator { @Override public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) throws ApplicationIdNotFoundException { - ApplicationInstanceReference appRef = OrchestratorUtil.toApplicationInstanceReference(appId,instanceLookupService); - return statusService.forApplicationInstance(appRef).getApplicationInstanceStatus(); + ApplicationInstanceReference appRef = OrchestratorUtil.toApplicationInstanceReference(appId, instanceLookupService); + return statusService.getApplicationInstanceStatus(appRef); } @Override @@ -305,7 +315,7 @@ public class OrchestratorImpl implements Orchestrator { } private HostStatus getNodeStatus(ApplicationInstanceReference applicationRef, HostName hostName) { - return statusService.forApplicationInstance(applicationRef).getHostStatus(hostName); + return statusService.getHostStatus(applicationRef, hostName); } private void setApplicationStatus(ApplicationId appId, ApplicationInstanceStatus status) @@ -316,7 +326,7 @@ public class OrchestratorImpl implements Orchestrator { statusService.lockApplicationInstance_forCurrentThreadOnly(context, appRef)) { // Short-circuit if already in wanted state - if (status == statusRegistry.getApplicationInstanceStatus()) return; + if (status == statusRegistry.getStatus()) return; // Set content clusters for this application in maintenance on suspend if (status == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN) { diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java index 79506d042e2..91da046840d 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java @@ -12,20 +12,15 @@ import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.applicationmodel.ServiceCluster; import com.yahoo.vespa.applicationmodel.ServiceInstance; import com.yahoo.vespa.applicationmodel.TenantId; -import com.yahoo.vespa.orchestrator.status.HostStatus; -import com.yahoo.vespa.orchestrator.status.ReadOnlyStatusRegistry; import java.util.Collection; import java.util.List; -import java.util.Map; import java.util.Set; -import java.util.function.Function; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; -import static java.util.stream.Collectors.toMap; import static java.util.stream.Collectors.toSet; /** @@ -57,14 +52,6 @@ public class OrchestratorUtil { .collect(toSet()); } - public static Map<HostName, HostStatus> getHostStatusMap(Collection<HostName> hosts, - ReadOnlyStatusRegistry hostStatusService) { - return hosts.stream() - .collect(Collectors.toMap( - hostName -> hostName, - hostName -> hostStatusService.getHostStatus(hostName))); - } - private static boolean hasServiceInstanceOnHost(ServiceCluster serviceCluster, HostName hostName) { return serviceInstancesOnHost(serviceCluster, hostName).count() > 0; } @@ -75,11 +62,6 @@ public class OrchestratorUtil { .filter(instance -> instance.hostName().equals(hostName)); } - public static <K, V1, V2> Map<K, V2> mapValues(Map<K, V1> map, Function<V1, V2> valueConverter) { - return map.entrySet().stream() - .collect(toMap(Map.Entry::getKey, entry -> valueConverter.apply(entry.getValue()))); - } - private static final Pattern APPLICATION_INSTANCE_REFERENCE_REST_FORMAT_PATTERN = Pattern.compile("^([^:]+):(.+)$"); /** Returns an ApplicationInstanceReference constructed from the serialized format used in the REST API. */ diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/ServiceMonitorInstanceLookupService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/ServiceMonitorInstanceLookupService.java index d1d5f3e8c95..1a859cfacc5 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/ServiceMonitorInstanceLookupService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/ServiceMonitorInstanceLookupService.java @@ -29,42 +29,17 @@ public class ServiceMonitorInstanceLookupService implements InstanceLookupServic @Override public Optional<ApplicationInstance> findInstanceById(ApplicationInstanceReference applicationInstanceReference) { - Map<ApplicationInstanceReference, ApplicationInstance> instanceMap - = serviceMonitor.getAllApplicationInstances(); - return Optional.ofNullable(instanceMap.get(applicationInstanceReference)); + return serviceMonitor.getServiceModelSnapshot().getApplicationInstance(applicationInstanceReference); } @Override public Optional<ApplicationInstance> findInstanceByHost(HostName hostName) { - Map<ApplicationInstanceReference, ApplicationInstance> instanceMap - = serviceMonitor.getAllApplicationInstances(); - List<ApplicationInstance> applicationInstancesUsingHost = instanceMap.entrySet().stream() - .filter(entry -> applicationInstanceUsesHost(entry.getValue(), hostName)) - .map(Map.Entry::getValue) - .collect(Collectors.toList()); - if (applicationInstancesUsingHost.isEmpty()) { - return Optional.empty(); - } - if (applicationInstancesUsingHost.size() > 1) { - throw new IllegalStateException( - "Major assumption broken: Multiple application instances contain host " + hostName.s() - + ": " + applicationInstancesUsingHost); - } - return Optional.of(applicationInstancesUsingHost.get(0)); + return Optional.ofNullable(serviceMonitor.getServiceModelSnapshot().getApplicationsByHostName().get(hostName)); } @Override public Set<ApplicationInstanceReference> knownInstances() { - return serviceMonitor.getAllApplicationInstances().keySet(); - } - - private static boolean applicationInstanceUsesHost(ApplicationInstance applicationInstance, - HostName hostName) { - return applicationInstance.serviceClusters().stream() - .anyMatch(serviceCluster -> - serviceCluster.serviceInstances().stream() - .anyMatch(serviceInstance -> - serviceInstance.hostName().equals(hostName))); + return serviceMonitor.getServiceModelSnapshot().getAllApplicationInstances().keySet(); } } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java index 5800b48da75..db2fb9b4a18 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java @@ -12,6 +12,7 @@ import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactory; import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus; import com.yahoo.vespa.orchestrator.status.HostStatus; import com.yahoo.vespa.orchestrator.status.MutableStatusRegistry; +import com.yahoo.vespa.orchestrator.status.StatusService; import java.util.Collection; import java.util.Comparator; @@ -40,7 +41,9 @@ public class ApplicationApiImpl implements ApplicationApi { this.nodeGroup = nodeGroup; this.hostStatusService = hostStatusService; Collection<HostName> hosts = getHostsUsedByApplicationInstance(applicationInstance); - this.hostStatusMap = hosts.stream().collect(Collectors.toMap(Function.identity(), hostStatusService::getHostStatus)); + this.hostStatusMap = hosts.stream().collect(Collectors.toMap(Function.identity(), + hostName -> hostStatusService.getSuspendedHosts().contains(hostName) + ? HostStatus.ALLOWED_TO_BE_DOWN : HostStatus.NO_REMARKS)); this.clusterInOrder = makeClustersInOrder(nodeGroup, hostStatusMap, clusterControllerClientFactory); } @@ -91,7 +94,7 @@ public class ApplicationApiImpl implements ApplicationApi { @Override public ApplicationInstanceStatus getApplicationStatus() { - return hostStatusService.getApplicationInstanceStatus(); + return hostStatusService.getStatus(); } @Override diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicy.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicy.java index b6e7014cac0..96930c1cda2 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicy.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicy.java @@ -14,6 +14,7 @@ import com.yahoo.vespa.orchestrator.model.StorageNode; import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus; import com.yahoo.vespa.orchestrator.status.HostStatus; import com.yahoo.vespa.orchestrator.status.MutableStatusRegistry; +import com.yahoo.vespa.orchestrator.status.StatusService; import java.util.logging.Logger; diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/Policy.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/Policy.java index e2487301326..aa7636227c8 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/Policy.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/Policy.java @@ -6,6 +6,7 @@ import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.orchestrator.OrchestratorContext; import com.yahoo.vespa.orchestrator.model.ApplicationApi; import com.yahoo.vespa.orchestrator.status.MutableStatusRegistry; +import com.yahoo.vespa.orchestrator.status.StatusService; /** * @author oyving diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/InstanceResource.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/InstanceResource.java index cd20a01f6af..65ef1c9eaf0 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/InstanceResource.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/InstanceResource.java @@ -34,7 +34,6 @@ import java.util.Map; import java.util.Set; import java.util.stream.Collectors; -import static com.yahoo.vespa.orchestrator.OrchestratorUtil.getHostStatusMap; import static com.yahoo.vespa.orchestrator.OrchestratorUtil.getHostsUsedByApplicationInstance; import static com.yahoo.vespa.orchestrator.OrchestratorUtil.parseAppInstanceReference; @@ -82,11 +81,13 @@ public class InstanceResource { = instanceLookupService.findInstanceById(instanceId) .orElseThrow(() -> new WebApplicationException(Response.status(Response.Status.NOT_FOUND).build())); - Set<HostName> hostsUsedByApplicationInstance = getHostsUsedByApplicationInstance(applicationInstance); - Map<HostName, HostStatus> hostStatusMap = getHostStatusMap(hostsUsedByApplicationInstance, - statusService.forApplicationInstance(instanceId)); - Map<HostName, String> hostStatusStringMap = OrchestratorUtil.mapValues(hostStatusMap, HostStatus::name); - return InstanceStatusResponse.create(applicationInstance, hostStatusStringMap); + Set<HostName> suspendedHosts = statusService.getSuspendedHostsByApplication().apply(applicationInstance.reference()); + Map<HostName, String> hostStatusMap = getHostsUsedByApplicationInstance(applicationInstance) + .stream() + .collect(Collectors.toMap(hostName -> hostName, + hostName -> suspendedHosts.contains(hostName) ? HostStatus.ALLOWED_TO_BE_DOWN.name() + : HostStatus.NO_REMARKS.name())); + return InstanceStatusResponse.create(applicationInstance, hostStatusMap); } @GET diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java deleted file mode 100644 index 70128ae12eb..00000000000 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java +++ /dev/null @@ -1,121 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.orchestrator.status; - -import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; -import com.yahoo.vespa.applicationmodel.HostName; -import com.yahoo.vespa.orchestrator.OrchestratorContext; - -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; - -/** - * Implementation of the StatusService interface for testing. - * - * @author oyving - */ -public class InMemoryStatusService implements StatusService { - - private final Map<HostName, HostStatus> hostServiceStatus = new HashMap<>(); - private final Set<ApplicationInstanceReference> applicationStatus = new HashSet<>(); - private final LockService<ApplicationInstanceReference> instanceLockService = new LockService<>(); - - private void setHostStatus(HostName hostName, HostStatus status) { - hostServiceStatus.put(hostName, status); - } - - @Override - public ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference) { - return new ReadOnlyStatusRegistry() { - @Override - public HostStatus getHostStatus(HostName hostName) { - return hostServiceStatus.getOrDefault(hostName, HostStatus.NO_REMARKS); - } - - @Override - public ApplicationInstanceStatus getApplicationInstanceStatus() { - return applicationStatus.contains(applicationInstanceReference) ? - ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN : ApplicationInstanceStatus.NO_REMARKS; - } - }; - } - - - @Override - public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( - OrchestratorContext context, - ApplicationInstanceReference applicationInstanceReference) { - Lock lock = instanceLockService.get(applicationInstanceReference); - return new InMemoryMutableStatusRegistry(lock, applicationInstanceReference); - } - - @Override - public Set<ApplicationInstanceReference> getAllSuspendedApplications() { - return applicationStatus; - } - - private class InMemoryMutableStatusRegistry implements MutableStatusRegistry { - - private final Lock lockHandle; - private final ApplicationInstanceReference ref; - - public InMemoryMutableStatusRegistry(Lock lockHandle, ApplicationInstanceReference ref) { - this.lockHandle = lockHandle; - this.ref = ref; - } - - @Override - public void setHostState(HostName hostName, HostStatus status) { - setHostStatus(hostName, status); - } - - @Override - public void setApplicationInstanceStatus(ApplicationInstanceStatus applicationInstanceStatus) { - if (applicationInstanceStatus == ApplicationInstanceStatus.NO_REMARKS) { - applicationStatus.remove(ref); - } else { - applicationStatus.add(ref); - } - } - - @Override - public HostStatus getHostStatus(HostName hostName) { - return hostServiceStatus.getOrDefault(hostName, HostStatus.NO_REMARKS); - } - - @Override - public ApplicationInstanceStatus getApplicationInstanceStatus() { - return applicationStatus.contains(ref) ? ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN : - ApplicationInstanceStatus.NO_REMARKS; - } - - @Override - public void close() { - //lockHandle.unlock(); TODO this casues illeal state monitor exception - how to use it properly - } - } - - private static class LockService<T> { - - private final Map<T, Lock> locks; - - public LockService() { - this.locks = new HashMap<>(); - } - - public Lock get(T lockId) { - synchronized (this) { - Lock lock = locks.computeIfAbsent( - lockId, - id -> new ReentrantLock() - ); - - return lock; - } - } - } - -} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/MutableStatusRegistry.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/MutableStatusRegistry.java index b699a9f8962..e36f0f70bbd 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/MutableStatusRegistry.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/MutableStatusRegistry.java @@ -3,6 +3,8 @@ package com.yahoo.vespa.orchestrator.status; import com.yahoo.vespa.applicationmodel.HostName; +import java.util.Set; + /** * Registry of the suspension and host statuses for an application instance. * @@ -10,7 +12,22 @@ import com.yahoo.vespa.applicationmodel.HostName; * @author Tony Vaagenes * @author bakksjo */ -public interface MutableStatusRegistry extends ReadOnlyStatusRegistry, AutoCloseable { +public interface MutableStatusRegistry extends AutoCloseable { + + /** + * Returns the status of this application. + */ + ApplicationInstanceStatus getStatus(); + + /** + * Returns the status of the given host. + */ + HostStatus getHostStatus(HostName hostName); + + /** + * Returns the set of all suspended hosts for this application. + */ + Set<HostName> getSuspendedHosts(); /** * Sets the state for the given host. @@ -27,7 +44,6 @@ public interface MutableStatusRegistry extends ReadOnlyStatusRegistry, AutoClose * so we override it here to strip the exception from the signature. */ @Override - @NoThrow void close(); } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/NoThrow.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/NoThrow.java deleted file mode 100644 index fd6757f83cf..00000000000 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/NoThrow.java +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.orchestrator.status; - -import java.lang.annotation.ElementType; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; - -/** - * Used to annotate methods that do not throw Exceptions. - * They are still allowed to throw Errors, such as AssertionError - * - * TODO: move to vespajlib or find a suitable replacement - * @author Tony Vaagenes - */ -@Target(ElementType.METHOD) -@Retention(RetentionPolicy.SOURCE) -@interface NoThrow {} - diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ReadOnlyStatusRegistry.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ReadOnlyStatusRegistry.java deleted file mode 100644 index 09300ef18a8..00000000000 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ReadOnlyStatusRegistry.java +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.orchestrator.status; - -import com.yahoo.vespa.applicationmodel.HostName; - -/** - * Read-only view of statuses for the application instance and its hosts. - * - * @author oyving - * @author Tony Vaagenes - * @author bakksjo - */ -public interface ReadOnlyStatusRegistry { - - /** - * Gets the current state for the given host. - */ - HostStatus getHostStatus(HostName hostName); - - /** - * Gets the current status for the application instance. - */ - ApplicationInstanceStatus getApplicationInstanceStatus(); - -} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java index 99f6c113193..9f91e08d344 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java @@ -2,32 +2,22 @@ package com.yahoo.vespa.orchestrator.status; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; +import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.orchestrator.OrchestratorContext; import java.util.Set; +import java.util.function.Function; /** - * Service that can produce registries for the suspension of an application - * and its hosts. + * Service that can produce registries for the suspension of an application and its hosts. * - * The registry classes are pr application instance. - * TODO Remove readonly registry class (replace with actual methods) - only adds complexity. + * The registry class is per locked application instance. * - * @author oyving + * @author Øyvind Grønnesby * @author Tony Vaagenes * @author smorgrav */ public interface StatusService { - /** - * Returns a readable host status registry for the given application instance. No locking is involved, - * so this call will never block. However, since it is possible that mutations are going on simultaneously - * with accessing this registry, the view obtained through the returned registry must be considered to be - * possibly inconsistent snapshot values. It is not recommended that this method is used for anything other - * than monitoring, logging, debugging, etc. It should never be used for multi-step operations (e.g. - * read-then-write) where consistency is required. For those cases, use - * {@link #lockApplicationInstance_forCurrentThreadOnly(OrchestratorContext, ApplicationInstanceReference)}. - */ - ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference); /** * Returns a mutable host status registry for a locked application instance. All operations performed on @@ -64,4 +54,24 @@ public interface StatusService { * @return A Map between the application instance and its status. */ Set<ApplicationInstanceReference> getAllSuspendedApplications(); + + /** + * Returns a fresh, but not necessarily consistent mapping from applications to their set of suspended hosts. + * + * If the lock for an application is held when this mapping is acquired, new sets returned for that application + * are consistent and up to date for as long as the lock is held. (The sets themselves don't reflect changes.) + */ + Function<ApplicationInstanceReference, Set<HostName>> getSuspendedHostsByApplication(); + + /** + * Returns the status of the given application. This is consistent if its lock is held. + */ + ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationInstanceReference application); + + + /** + * Returns the status of the given host, for the given application. This is consistent if the application's lock is held. + */ + HostStatus getHostStatus(ApplicationInstanceReference application, HostName host); + } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java index c56ff661bba..f6e01a49ce8 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java @@ -7,20 +7,23 @@ import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.curator.Curator; import com.yahoo.vespa.curator.Lock; +import com.yahoo.vespa.curator.recipes.CuratorCounter; import com.yahoo.vespa.orchestrator.OrchestratorContext; import com.yahoo.vespa.orchestrator.OrchestratorUtil; -import org.apache.curator.framework.recipes.locks.InterProcessSemaphoreMutex; import org.apache.zookeeper.KeeperException.NoNodeException; import org.apache.zookeeper.KeeperException.NodeExistsException; import org.apache.zookeeper.data.Stat; import javax.inject.Inject; import java.time.Duration; +import java.util.Collections; import java.util.HashSet; +import java.util.Map; import java.util.Set; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; import java.util.logging.Logger; +import java.util.stream.Collectors; /** * Stores instance suspension status and which hosts are allowed to go down in zookeeper. @@ -34,27 +37,22 @@ public class ZookeeperStatusService implements StatusService { final static String HOST_STATUS_BASE_PATH = "/vespa/host-status-service"; final static String APPLICATION_STATUS_BASE_PATH = "/vespa/application-status-service"; + final static String HOST_STATUS_CACHE_COUNTER_PATH = "/vespa/host-status-service-cache-counter"; private final Curator curator; + private final CuratorCounter counter; + + /** A cache of hosts allowed to be down. Access only through {@link #getValidCache()}! */ + private final Map<ApplicationInstanceReference, Set<HostName>> hostsDown; + + private volatile long cacheRefreshedAt; @Inject public ZookeeperStatusService(@Component Curator curator) { this.curator = curator; - } - - @Override - public ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference) { - return new ReadOnlyStatusRegistry() { - @Override - public HostStatus getHostStatus(HostName hostName) { - return getInternalHostStatus(applicationInstanceReference, hostName); - } - - @Override - public ApplicationInstanceStatus getApplicationInstanceStatus() { - return getInternalApplicationInstanceStatus(applicationInstanceReference); - } - }; + this.counter = new CuratorCounter(curator, HOST_STATUS_CACHE_COUNTER_PATH); + this.cacheRefreshedAt = counter.get(); + this.hostsDown = new ConcurrentHashMap<>(); } @Override @@ -80,6 +78,18 @@ public class ZookeeperStatusService implements StatusService { } /** + * Cache is checked for freshness when this mapping is created, and may be invalidated again later + * by other users of the cache. Since this function is backed by the cache, any such invalidations + * will be reflected in the returned mapping; all users of the cache collaborate in repopulating it. + */ + @Override + public Function<ApplicationInstanceReference, Set<HostName>> getSuspendedHostsByApplication() { + Map<ApplicationInstanceReference, Set<HostName>> suspendedHostsByApplication = getValidCache(); + return application -> suspendedHostsByApplication.computeIfAbsent(application, this::hostsDownFor); + } + + + /** * 1) locks the status service for an application instance. * 2) fails all operations in this thread when the session is lost, * since session loss might cause the lock to be lost. @@ -107,72 +117,89 @@ public class ZookeeperStatusService implements StatusService { } } - private InterProcessSemaphoreMutex acquireMutexOrThrow(long timeout, TimeUnit timeoutTimeUnit, String lockPath) throws Exception { - InterProcessSemaphoreMutex mutex = new InterProcessSemaphoreMutex(curator.framework(), lockPath); - - log.log(LogLevel.DEBUG, "Waiting for lock on " + lockPath); - boolean acquired = mutex.acquire(timeout, timeoutTimeUnit); - if (!acquired) { - log.log(LogLevel.DEBUG, "Timed out waiting for lock on " + lockPath); - throw new TimeoutException("Timed out waiting for lock on " + lockPath); - } - log.log(LogLevel.DEBUG, "Successfully acquired lock on " + lockPath); - return mutex; - } - private void setHostStatus(ApplicationInstanceReference applicationInstanceReference, HostName hostName, HostStatus status) { String path = hostAllowedDownPath(applicationInstanceReference, hostName); + boolean invalidate = false; try { switch (status) { case NO_REMARKS: - deleteNode_ignoreNoNodeException(path,"Host already has state NO_REMARKS, path = " + path); + invalidate = deleteNode_ignoreNoNodeException(path, "Host already has state NO_REMARKS, path = " + path); break; case ALLOWED_TO_BE_DOWN: - createNode_ignoreNodeExistsException(path, - "Host already has state ALLOWED_TO_BE_DOWN, path = " + path); + invalidate = createNode_ignoreNodeExistsException(path, "Host already has state ALLOWED_TO_BE_DOWN, path = " + path); + break; + default: + throw new IllegalArgumentException("Unexpected status '" + status + "'."); } } catch (Exception e) { - //TODO: IOException with explanation + invalidate = true; throw new RuntimeException(e); } + finally { + if (invalidate) { + counter.next(); + hostsDown.remove(applicationInstanceReference); + } + } } - private void deleteNode_ignoreNoNodeException(String path, String debugLogMessageIfNotExists) throws Exception { + private boolean deleteNode_ignoreNoNodeException(String path, String debugLogMessageIfNotExists) throws Exception { try { curator.framework().delete().forPath(path); + return true; } catch (NoNodeException e) { log.log(LogLevel.DEBUG, debugLogMessageIfNotExists, e); + return false; } } - private void createNode_ignoreNodeExistsException(String path, String debugLogMessageIfExists) throws Exception { + private boolean createNode_ignoreNodeExistsException(String path, String debugLogMessageIfExists) throws Exception { try { curator.framework().create() .creatingParentsIfNeeded() .forPath(path); + return true; } catch (NodeExistsException e) { log.log(LogLevel.DEBUG, debugLogMessageIfExists, e); + return false; + } + } + + @Override + public HostStatus getHostStatus(ApplicationInstanceReference applicationInstanceReference, HostName hostName) { + return getValidCache().computeIfAbsent(applicationInstanceReference, this::hostsDownFor) + .contains(hostName) ? HostStatus.ALLOWED_TO_BE_DOWN : HostStatus.NO_REMARKS; + } + + /** Holding an application's lock ensures the cache is up to date for that application. */ + private Map<ApplicationInstanceReference, Set<HostName>> getValidCache() { + long cacheGeneration = counter.get(); + if (counter.get() != cacheRefreshedAt) { + cacheRefreshedAt = cacheGeneration; + hostsDown.clear(); } + return hostsDown; } - //TODO: Eliminate repeated calls to getHostStatus, replace with bulk operation. - private HostStatus getInternalHostStatus(ApplicationInstanceReference applicationInstanceReference, HostName hostName) { + private Set<HostName> hostsDownFor(ApplicationInstanceReference application) { try { - Stat statOrNull = curator.framework().checkExists().forPath( - hostAllowedDownPath(applicationInstanceReference, hostName)); + if (curator.framework().checkExists().forPath(hostsAllowedDownPath(application)) == null) + return Collections.emptySet(); - return (statOrNull == null) ? HostStatus.NO_REMARKS : HostStatus.ALLOWED_TO_BE_DOWN; - } catch (Exception e) { - //TODO: IOException with explanation - Should we only catch IOExceptions or are they a special case? + return curator.framework().getChildren().forPath(hostsAllowedDownPath(application)) + .stream().map(HostName::new) + .collect(Collectors.toUnmodifiableSet()); + } + catch (Exception e) { throw new RuntimeException(e); } } - /** Common implementation for the two internal classes that sets ApplicationInstanceStatus. */ - private ApplicationInstanceStatus getInternalApplicationInstanceStatus(ApplicationInstanceReference applicationInstanceReference) { + @Override + public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationInstanceReference applicationInstanceReference) { try { Stat statOrNull = curator.framework().checkExists().forPath( applicationInstanceSuspendedPath(applicationInstanceReference)); @@ -183,12 +210,6 @@ public class ZookeeperStatusService implements StatusService { } } - private HostStatus getHostStatusWithLock( - final ApplicationInstanceReference applicationInstanceReference, - final HostName hostName) { - return getInternalHostStatus(applicationInstanceReference, hostName); - } - private static String applicationInstancePath(ApplicationInstanceReference applicationInstanceReference) { return HOST_STATUS_BASE_PATH + '/' + applicationInstanceReference.tenantId() + ":" + applicationInstanceReference.applicationInstanceId(); @@ -198,10 +219,6 @@ public class ZookeeperStatusService implements StatusService { return applicationInstancePath(applicationInstanceReference) + "/hosts-allowed-down"; } - private static String applicationInstanceLockPath(ApplicationInstanceReference applicationInstanceReference) { - return applicationInstancePath(applicationInstanceReference) + "/lock"; - } - private static String applicationInstanceLock2Path(ApplicationInstanceReference applicationInstanceReference) { return applicationInstancePath(applicationInstanceReference) + "/lock2"; } @@ -229,6 +246,21 @@ public class ZookeeperStatusService implements StatusService { } @Override + public ApplicationInstanceStatus getStatus() { + return getApplicationInstanceStatus(applicationInstanceReference); + } + + @Override + public HostStatus getHostStatus(HostName hostName) { + return ZookeeperStatusService.this.getHostStatus(applicationInstanceReference, hostName); + } + + @Override + public Set<HostName> getSuspendedHosts() { + return getValidCache().computeIfAbsent(applicationInstanceReference, ZookeeperStatusService.this::hostsDownFor); + } + + @Override public void setHostState(final HostName hostName, final HostStatus status) { if (probe) return; log.log(LogLevel.INFO, "Setting host " + hostName + " to status " + status); @@ -259,17 +291,6 @@ public class ZookeeperStatusService implements StatusService { } @Override - public HostStatus getHostStatus(final HostName hostName) { - return getHostStatusWithLock(applicationInstanceReference, hostName); - } - - @Override - public ApplicationInstanceStatus getApplicationInstanceStatus() { - return getInternalApplicationInstanceStatus(applicationInstanceReference); - } - - @Override - @NoThrow public void close() { try { lock.close(); diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java index cc6b9a7dbf7..89172a831f4 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java @@ -13,6 +13,7 @@ import com.yahoo.vespa.applicationmodel.ServiceInstance; import com.yahoo.vespa.applicationmodel.ServiceStatus; import com.yahoo.vespa.applicationmodel.ServiceType; import com.yahoo.vespa.applicationmodel.TenantId; +import com.yahoo.vespa.curator.mock.MockCurator; import com.yahoo.vespa.orchestrator.config.OrchestratorConfig; import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactory; import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactoryMock; @@ -20,9 +21,9 @@ import com.yahoo.vespa.orchestrator.model.NodeGroup; import com.yahoo.vespa.orchestrator.policy.BatchHostStateChangeDeniedException; import com.yahoo.vespa.orchestrator.policy.HostStateChangeDeniedException; import com.yahoo.vespa.orchestrator.status.HostStatus; -import com.yahoo.vespa.orchestrator.status.InMemoryStatusService; -import com.yahoo.vespa.orchestrator.status.ReadOnlyStatusRegistry; import com.yahoo.vespa.orchestrator.status.StatusService; +import com.yahoo.vespa.orchestrator.status.ZookeeperStatusService; +import com.yahoo.vespa.service.monitor.ServiceModel; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -31,9 +32,8 @@ import org.mockito.InOrder; import java.util.Arrays; import java.util.Iterator; -import java.util.Optional; -import java.util.stream.Collectors; -import java.util.stream.Stream; +import java.util.Map; +import java.util.Set; import static com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN; import static com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus.NO_REMARKS; @@ -49,12 +49,10 @@ import static org.mockito.Matchers.any; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.when; /** - * Test Orchestrator with a mock backend (the InMemoryStatusService) + * Test Orchestrator with a mock backend (the MockCurator) * * @author smorgrav */ @@ -79,7 +77,7 @@ public class OrchestratorImplTest { clustercontroller = new ClusterControllerClientFactoryMock(); orchestrator = new OrchestratorImpl( clustercontroller, - new InMemoryStatusService(), + new ZookeeperStatusService(new MockCurator()), new OrchestratorConfig(new OrchestratorConfig.Builder()), new DummyInstanceLookupService()); @@ -312,16 +310,8 @@ public class OrchestratorImplTest { @Test public void testGetHost() throws Exception { - ClusterControllerClientFactory clusterControllerClientFactory = - mock(ClusterControllerClientFactory.class); - StatusService statusService = mock(StatusService.class); - InstanceLookupService lookupService = mock(InstanceLookupService.class); - - orchestrator = new OrchestratorImpl( - clusterControllerClientFactory, - statusService, - new OrchestratorConfig(new OrchestratorConfig.Builder()), - lookupService); + ClusterControllerClientFactory clusterControllerClientFactory = new ClusterControllerClientFactoryMock(); + StatusService statusService = new ZookeeperStatusService(new MockCurator()); HostName hostName = new HostName("host.yahoo.com"); TenantId tenantId = new TenantId("tenant"); @@ -335,32 +325,30 @@ public class OrchestratorImplTest { new ApplicationInstance( tenantId, applicationInstanceId, - Stream.of(new ServiceCluster( + Set.of(new ServiceCluster( new ClusterId("clusterId"), new ServiceType("serviceType"), - Stream.of( - new ServiceInstance( - new ConfigId("configId1"), - hostName, - ServiceStatus.UP), - new ServiceInstance( - new ConfigId("configId2"), - hostName, - ServiceStatus.NOT_CHECKED)) - .collect(Collectors.toSet()))) - .collect(Collectors.toSet())); - - when(lookupService.findInstanceByHost(hostName)) - .thenReturn(Optional.of(applicationInstance)); - - ReadOnlyStatusRegistry readOnlyStatusRegistry = mock(ReadOnlyStatusRegistry.class); - when(statusService.forApplicationInstance(reference)) - .thenReturn(readOnlyStatusRegistry); - when(readOnlyStatusRegistry.getHostStatus(hostName)) - .thenReturn(HostStatus.ALLOWED_TO_BE_DOWN); + Set.of(new ServiceInstance( + new ConfigId("configId1"), + hostName, + ServiceStatus.UP), + new ServiceInstance( + new ConfigId("configId2"), + hostName, + ServiceStatus.NOT_CHECKED))))); + + InstanceLookupService lookupService = new ServiceMonitorInstanceLookupService( + () -> new ServiceModel(Map.of(reference, applicationInstance))); - Host host = orchestrator.getHost(hostName); + orchestrator = new OrchestratorImpl( + clusterControllerClientFactory, + statusService, + new OrchestratorConfig(new OrchestratorConfig.Builder()), + lookupService); + + orchestrator.setNodeStatus(hostName, HostStatus.ALLOWED_TO_BE_DOWN); + Host host = orchestrator.getHost(hostName); assertEquals(reference, host.getApplicationInstanceReference()); assertEquals(hostName, host.getHostName()); assertEquals(HostStatus.ALLOWED_TO_BE_DOWN, host.getHostStatus()); diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java index a712c1db3e8..5a11d3aa640 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java @@ -167,8 +167,7 @@ public class ApplicationApiImplTest { } private void verifyUpConditionWith(HostStatus hostStatus, ServiceStatus serviceStatus, boolean expectUp) { - HostName hostName1 = modelUtils.createNode("host1", hostStatus); - + HostName hostName1 = new HostName("host1"); ApplicationInstance applicationInstance = modelUtils.createApplicationInstance(Arrays.asList( modelUtils.createServiceCluster( @@ -178,6 +177,8 @@ public class ApplicationApiImplTest { ) )); + modelUtils.createNode("host1", hostStatus); + ApplicationApiImpl applicationApi = modelUtils.createApplicationApiImpl(applicationInstance, hostName1); List<HostName> upStorageNodes = expectUp ? Arrays.asList(hostName1) : new ArrayList<>(); @@ -189,9 +190,9 @@ public class ApplicationApiImplTest { @Test public void testGetNodesInGroupWithStatus() { - HostName hostName1 = modelUtils.createNode("host1", HostStatus.NO_REMARKS); - HostName hostName2 = modelUtils.createNode("host2", HostStatus.NO_REMARKS); - HostName hostName3 = modelUtils.createNode("host3", HostStatus.ALLOWED_TO_BE_DOWN); + HostName hostName1 = new HostName("host1"); + HostName hostName2 = new HostName("host2"); + HostName hostName3 = new HostName("host3"); ApplicationInstance applicationInstance = modelUtils.createApplicationInstance(Arrays.asList( @@ -213,6 +214,10 @@ public class ApplicationApiImplTest { ) )); + modelUtils.createNode(hostName1, HostStatus.NO_REMARKS); + modelUtils.createNode(hostName2, HostStatus.NO_REMARKS); + modelUtils.createNode(hostName3, HostStatus.ALLOWED_TO_BE_DOWN); + verifyNodesInGroupWithoutRemarks( modelUtils.createApplicationApiImpl(applicationInstance, hostName1), Arrays.asList(hostName1), @@ -242,13 +247,13 @@ public class ApplicationApiImplTest { @Test public void testGetStorageNodesAllowedToBeDownInGroupInReverseClusterOrder() { - HostName allowedToBeDownHost1 = modelUtils.createNode("host1", HostStatus.ALLOWED_TO_BE_DOWN); - HostName noRemarksHost2 = modelUtils.createNode("host2", HostStatus.NO_REMARKS); - HostName allowedToBeDownHost3 = modelUtils.createNode("host3", HostStatus.ALLOWED_TO_BE_DOWN); - HostName allowedToBeDownHost4 = modelUtils.createNode("host4", HostStatus.ALLOWED_TO_BE_DOWN); - HostName noRemarksHost5 = modelUtils.createNode("host5", HostStatus.ALLOWED_TO_BE_DOWN); - HostName noRemarksHost6 = modelUtils.createNode("host6", HostStatus.NO_REMARKS); - HostName allowedToBeDownHost7 = modelUtils.createNode("host7", HostStatus.ALLOWED_TO_BE_DOWN); + HostName allowedToBeDownHost1 = new HostName("host1"); + HostName noRemarksHost2 = new HostName("host2"); + HostName allowedToBeDownHost3 = new HostName("host3"); + HostName allowedToBeDownHost4 = new HostName("host4"); + HostName noRemarksHost5 = new HostName("host5"); + HostName noRemarksHost6 = new HostName("host6"); + HostName allowedToBeDownHost7 = new HostName("host7"); ApplicationInstance applicationInstance = modelUtils.createApplicationInstance(Arrays.asList( @@ -286,6 +291,14 @@ public class ApplicationApiImplTest { ) )); + modelUtils.createNode(allowedToBeDownHost1, HostStatus.ALLOWED_TO_BE_DOWN); + modelUtils.createNode(noRemarksHost2, HostStatus.NO_REMARKS); + modelUtils.createNode(allowedToBeDownHost3, HostStatus.ALLOWED_TO_BE_DOWN); + modelUtils.createNode(allowedToBeDownHost4, HostStatus.ALLOWED_TO_BE_DOWN); + modelUtils.createNode(noRemarksHost5, HostStatus.ALLOWED_TO_BE_DOWN); // Really? + modelUtils.createNode(noRemarksHost6, HostStatus.NO_REMARKS); + modelUtils.createNode(allowedToBeDownHost7, HostStatus.ALLOWED_TO_BE_DOWN); + verifyStorageNodesAllowedToBeDown( modelUtils.createApplicationApiImpl(applicationInstance, allowedToBeDownHost1), allowedToBeDownHost1); verifyStorageNodesAllowedToBeDown( diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ClusterApiImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ClusterApiImplTest.java index 8e11b85241f..0a53972b30c 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ClusterApiImplTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ClusterApiImplTest.java @@ -10,6 +10,7 @@ import org.junit.Test; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.Optional; @@ -24,12 +25,11 @@ public class ClusterApiImplTest { @Test public void testServicesDownAndNotInGroup() { - HostName hostName1 = modelUtils.createNode("host1", HostStatus.NO_REMARKS); - HostName hostName2 = modelUtils.createNode("host2", HostStatus.NO_REMARKS); - HostName hostName3 = modelUtils.createNode("host3", HostStatus.ALLOWED_TO_BE_DOWN); - HostName hostName4 = modelUtils.createNode("host4", HostStatus.ALLOWED_TO_BE_DOWN); - HostName hostName5 = modelUtils.createNode("host5", HostStatus.NO_REMARKS); - + HostName hostName1 = new HostName("host1"); + HostName hostName2 = new HostName("host2"); + HostName hostName3 = new HostName("host3"); + HostName hostName4 = new HostName("host4"); + HostName hostName5 = new HostName("host5"); ServiceCluster serviceCluster = modelUtils.createServiceCluster( "cluster", @@ -42,6 +42,13 @@ public class ClusterApiImplTest { modelUtils.createServiceInstance("service-5", hostName5, ServiceStatus.UP) ) ); + modelUtils.createApplicationInstance(Collections.singletonList(serviceCluster)); + + modelUtils.createNode(hostName1, HostStatus.NO_REMARKS); + modelUtils.createNode(hostName2, HostStatus.NO_REMARKS); + modelUtils.createNode(hostName3, HostStatus.ALLOWED_TO_BE_DOWN); + modelUtils.createNode(hostName4, HostStatus.ALLOWED_TO_BE_DOWN); + modelUtils.createNode(hostName5, HostStatus.NO_REMARKS); ClusterApiImpl clusterApi = new ClusterApiImpl( applicationApi, @@ -67,12 +74,11 @@ public class ClusterApiImplTest { @Test public void testNoServices() { - HostName hostName1 = modelUtils.createNode("host1", HostStatus.NO_REMARKS); - HostName hostName2 = modelUtils.createNode("host2", HostStatus.NO_REMARKS); - HostName hostName3 = modelUtils.createNode("host3", HostStatus.ALLOWED_TO_BE_DOWN); - HostName hostName4 = modelUtils.createNode("host4", HostStatus.ALLOWED_TO_BE_DOWN); - HostName hostName5 = modelUtils.createNode("host5", HostStatus.NO_REMARKS); - + HostName hostName1 = new HostName("host1"); + HostName hostName2 = new HostName("host2"); + HostName hostName3 = new HostName("host3"); + HostName hostName4 = new HostName("host4"); + HostName hostName5 = new HostName("host5"); ServiceCluster serviceCluster = modelUtils.createServiceCluster( "cluster", @@ -85,6 +91,13 @@ public class ClusterApiImplTest { modelUtils.createServiceInstance("service-5", hostName5, ServiceStatus.UP) ) ); + modelUtils.createApplicationInstance(Collections.singletonList(serviceCluster)); + + modelUtils.createNode(hostName1, HostStatus.NO_REMARKS); + modelUtils.createNode(hostName2, HostStatus.NO_REMARKS); + modelUtils.createNode(hostName3, HostStatus.ALLOWED_TO_BE_DOWN); + modelUtils.createNode(hostName4, HostStatus.ALLOWED_TO_BE_DOWN); + modelUtils.createNode(hostName5, HostStatus.NO_REMARKS); verifyNoServices(serviceCluster, false, false, hostName1); verifyNoServices(serviceCluster, true, false, hostName2); diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java index 9586f92af30..78f50dbfc3f 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java @@ -3,6 +3,7 @@ package com.yahoo.vespa.orchestrator.model; import com.yahoo.vespa.applicationmodel.ApplicationInstance; import com.yahoo.vespa.applicationmodel.ApplicationInstanceId; +import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; import com.yahoo.vespa.applicationmodel.ClusterId; import com.yahoo.vespa.applicationmodel.ConfigId; import com.yahoo.vespa.applicationmodel.HostName; @@ -11,37 +12,58 @@ import com.yahoo.vespa.applicationmodel.ServiceInstance; import com.yahoo.vespa.applicationmodel.ServiceStatus; import com.yahoo.vespa.applicationmodel.ServiceType; import com.yahoo.vespa.applicationmodel.TenantId; +import com.yahoo.vespa.curator.mock.MockCurator; +import com.yahoo.vespa.orchestrator.OrchestrationException; +import com.yahoo.vespa.orchestrator.Orchestrator; +import com.yahoo.vespa.orchestrator.OrchestratorContext; +import com.yahoo.vespa.orchestrator.OrchestratorImpl; +import com.yahoo.vespa.orchestrator.ServiceMonitorInstanceLookupService; +import com.yahoo.vespa.orchestrator.config.OrchestratorConfig; +import com.yahoo.vespa.orchestrator.config.OrchestratorConfig.Builder; import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactory; +import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactoryMock; import com.yahoo.vespa.orchestrator.status.HostStatus; import com.yahoo.vespa.orchestrator.status.MutableStatusRegistry; +import com.yahoo.vespa.orchestrator.status.StatusService; +import com.yahoo.vespa.orchestrator.status.ZookeeperStatusService; +import com.yahoo.vespa.service.monitor.ServiceModel; +import com.yahoo.yolean.Exceptions; +import java.time.Clock; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.stream.Collectors; -import static org.mockito.Matchers.any; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +class ModelTestUtils { -public class ModelTestUtils { - private final MutableStatusRegistry statusRegistry = mock(MutableStatusRegistry.class); - private final ClusterControllerClientFactory clusterControllerClientFactory = mock(ClusterControllerClientFactory.class); + private final Map<ApplicationInstanceReference, ApplicationInstance> applications = new HashMap<>(); + private final ClusterControllerClientFactory clusterControllerClientFactory = new ClusterControllerClientFactoryMock(); private final Map<HostName, HostStatus> hostStatusMap = new HashMap<>(); - - ModelTestUtils() { - when(statusRegistry.getHostStatus(any())).thenReturn(HostStatus.NO_REMARKS); - } + private final StatusService statusService = new ZookeeperStatusService(new MockCurator()); + private final Orchestrator orchestrator = new OrchestratorImpl(clusterControllerClientFactory, + statusService, + new OrchestratorConfig(new Builder()), + new ServiceMonitorInstanceLookupService(() -> new ServiceModel(applications))); Map<HostName, HostStatus> getHostStatusMap() { return hostStatusMap; } HostName createNode(String name, HostStatus hostStatus) { - HostName hostName = new HostName(name); + return createNode(new HostName(name), hostStatus); + } + + HostName createNode(HostName hostName, HostStatus hostStatus) { hostStatusMap.put(hostName, hostStatus); - when(statusRegistry.getHostStatus(hostName)).thenReturn(hostStatus); + try { + orchestrator.setNodeStatus(hostName, hostStatus); + } + catch (OrchestrationException e) { + throw new AssertionError("Host '" + hostName + "' not owned by any application — please assign it first: " + + Exceptions.toMessageString(e)); + } return hostName; } @@ -49,26 +71,29 @@ public class ModelTestUtils { ApplicationInstance applicationInstance, HostName... hostnames) { NodeGroup nodeGroup = new NodeGroup(applicationInstance, hostnames); - return new ApplicationApiImpl(nodeGroup, statusRegistry, clusterControllerClientFactory); + MutableStatusRegistry registry = statusService.lockApplicationInstance_forCurrentThreadOnly( + OrchestratorContext.createContextForSingleAppOp(Clock.systemUTC()), + applicationInstance.reference()); + return new ApplicationApiImpl(nodeGroup, registry, clusterControllerClientFactory); } ApplicationInstance createApplicationInstance( List<ServiceCluster> serviceClusters) { - Set<ServiceCluster> serviceClusterSet = serviceClusters.stream() - .collect(Collectors.toSet()); + Set<ServiceCluster> serviceClusterSet = new HashSet<>(serviceClusters); - return new ApplicationInstance( + ApplicationInstance application = new ApplicationInstance( new TenantId("tenant"), new ApplicationInstanceId("application-name:foo:bar:default"), serviceClusterSet); + applications.put(application.reference(), application); + return application; } ServiceCluster createServiceCluster( String clusterId, ServiceType serviceType, List<ServiceInstance> serviceInstances) { - Set<ServiceInstance> serviceInstanceSet = serviceInstances.stream() - .collect(Collectors.toSet()); + Set<ServiceInstance> serviceInstanceSet = new HashSet<>(serviceInstances); return new ServiceCluster( new ClusterId(clusterId), @@ -86,7 +111,8 @@ public class ModelTestUtils { status); } - public ClusterControllerClientFactory getClusterControllerClientFactory() { + ClusterControllerClientFactory getClusterControllerClientFactory() { return clusterControllerClientFactory; } + } diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/ApplicationSuspensionResourceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/ApplicationSuspensionResourceTest.java index c575811fd6d..94df28a6921 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/ApplicationSuspensionResourceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/ApplicationSuspensionResourceTest.java @@ -158,7 +158,8 @@ public class ApplicationSuspensionResourceTest { " <config name=\"container.handler.threadpool\">\n" + " <maxthreads>10</maxthreads>\n" + " </config>\n" + - " <component id=\"com.yahoo.vespa.orchestrator.status.InMemoryStatusService\" bundle=\"orchestrator\" />\n" + + " <component id=\"com.yahoo.vespa.curator.mock.MockCurator\" bundle=\"zkfacade\" />\n" + + " <component id=\"com.yahoo.vespa.orchestrator.status.ZookeeperStatusService\" bundle=\"orchestrator\" />\n" + " <component id=\"com.yahoo.vespa.orchestrator.DummyInstanceLookupService\" bundle=\"orchestrator\" />\n" + " <component id=\"com.yahoo.vespa.orchestrator.OrchestratorImpl\" bundle=\"orchestrator\" />\n" + " <component id=\"com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactoryMock\" bundle=\"orchestrator\" />\n" + diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java index e943a4f105b..55ac65c036c 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java @@ -13,6 +13,7 @@ import com.yahoo.vespa.applicationmodel.ServiceInstance; import com.yahoo.vespa.applicationmodel.ServiceStatus; import com.yahoo.vespa.applicationmodel.ServiceType; import com.yahoo.vespa.applicationmodel.TenantId; +import com.yahoo.vespa.curator.mock.MockCurator; import com.yahoo.vespa.orchestrator.BatchHostNameNotFoundException; import com.yahoo.vespa.orchestrator.BatchInternalErrorException; import com.yahoo.vespa.orchestrator.Host; @@ -29,14 +30,13 @@ import com.yahoo.vespa.orchestrator.policy.HostStateChangeDeniedException; import com.yahoo.vespa.orchestrator.policy.Policy; import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult; import com.yahoo.vespa.orchestrator.restapi.wire.GetHostResponse; -import com.yahoo.vespa.orchestrator.restapi.wire.HostStateChangeDenialReason; import com.yahoo.vespa.orchestrator.restapi.wire.PatchHostRequest; import com.yahoo.vespa.orchestrator.restapi.wire.PatchHostResponse; import com.yahoo.vespa.orchestrator.restapi.wire.UpdateHostResponse; -import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus; import com.yahoo.vespa.orchestrator.status.HostStatus; import com.yahoo.vespa.orchestrator.status.MutableStatusRegistry; import com.yahoo.vespa.orchestrator.status.StatusService; +import com.yahoo.vespa.orchestrator.status.ZookeeperStatusService; import org.junit.Before; import org.junit.Test; @@ -58,7 +58,6 @@ import static org.fest.assertions.Assertions.assertThat; import static org.fest.assertions.Fail.fail; import static org.junit.Assert.assertEquals; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -73,21 +72,7 @@ public class HostResourceTest { private static final int SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS = 0; private static final TenantId TENANT_ID = new TenantId("tenantId"); private static final ApplicationInstanceId APPLICATION_INSTANCE_ID = new ApplicationInstanceId("applicationId"); - private static final ApplicationInstanceReference APPLICATION_INSTANCE_REFERENCE = - new ApplicationInstanceReference(TENANT_ID, APPLICATION_INSTANCE_ID); - - private static final StatusService EVERY_HOST_IS_UP_HOST_STATUS_SERVICE = mock(StatusService.class); - private static final MutableStatusRegistry EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY = mock(MutableStatusRegistry.class); - static { - when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.forApplicationInstance(eq(APPLICATION_INSTANCE_REFERENCE))) - .thenReturn(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY); - when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.lockApplicationInstance_forCurrentThreadOnly(any(), eq(APPLICATION_INSTANCE_REFERENCE))) - .thenReturn(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY); - when(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY.getHostStatus(any())) - .thenReturn(HostStatus.NO_REMARKS); - when(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY.getApplicationInstanceStatus()) - .thenReturn(ApplicationInstanceStatus.NO_REMARKS); - } + private static final StatusService EVERY_HOST_IS_UP_HOST_STATUS_SERVICE = new ZookeeperStatusService(new MockCurator()); private static final InstanceLookupService mockInstanceLookupService = mock(InstanceLookupService.class); static { @@ -99,7 +84,6 @@ public class HostResourceTest { makeServiceClusterSet()))); } - private static final InstanceLookupService alwaysEmptyInstanceLookUpService = new InstanceLookupService() { @Override public Optional<ApplicationInstance> findInstanceById( diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java index d57b0106b5b..9b1be12121d 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java @@ -7,7 +7,6 @@ import com.yahoo.vespa.curator.Curator; import com.yahoo.vespa.orchestrator.OrchestratorContext; import com.yahoo.vespa.orchestrator.TestIds; import org.apache.commons.lang.exception.ExceptionUtils; -import org.apache.curator.SessionFailRetryLoop.SessionFailedException; import org.apache.curator.framework.CuratorFramework; import org.apache.curator.test.KillSession; import org.apache.curator.test.TestingServer; @@ -56,10 +55,6 @@ public class ZookeeperStatusServiceTest { when(context.isProbe()).thenReturn(false); } - private static Curator createConnectedCuratorFramework(TestingServer server) throws InterruptedException { - return createConnectedCurator(server); - } - private static Curator createConnectedCurator(TestingServer server) throws InterruptedException { Curator curator = Curator.create(server.getConnectString()); curator.framework().blockUntilConnected(1, TimeUnit.MINUTES); @@ -80,8 +75,7 @@ public class ZookeeperStatusServiceTest { @Test public void host_state_for_unknown_hosts_is_no_remarks() { assertThat( - zookeeperStatusService.forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getHostStatus(TestIds.HOST_NAME1), + zookeeperStatusService.getHostStatus(TestIds.APPLICATION_INSTANCE_REFERENCE, TestIds.HOST_NAME1), is(HostStatus.NO_REMARKS)); } @@ -92,71 +86,65 @@ public class ZookeeperStatusServiceTest { //shuffling to catch "clean database" failures for all cases. for (HostStatus hostStatus: shuffledList(HostStatus.values())) { - doTimes(2, () -> { + for (int i = 0; i < 2; i++) { statusRegistry.setHostState( TestIds.HOST_NAME1, hostStatus); - assertThat(statusRegistry.getHostStatus( - TestIds.HOST_NAME1), - is(hostStatus)); - }); + assertThat(statusRegistry.getHostStatus(TestIds.HOST_NAME1), + is(hostStatus)); + } } } } @Test public void locks_are_exclusive() throws Exception { - try (Curator curator = createConnectedCuratorFramework(testingServer)) { - ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); - - final CompletableFuture<Void> lockedSuccessfullyFuture; - try (MutableStatusRegistry statusRegistry = zookeeperStatusService - .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) { + ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); - lockedSuccessfullyFuture = CompletableFuture.runAsync(() -> { - try (MutableStatusRegistry statusRegistry2 = zookeeperStatusService2 - .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) - { - } - }); + final CompletableFuture<Void> lockedSuccessfullyFuture; + try (MutableStatusRegistry statusRegistry = zookeeperStatusService + .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) { - try { - lockedSuccessfullyFuture.get(3, TimeUnit.SECONDS); - fail("Both zookeeper host status services locked simultaneously for the same application instance"); - } catch (TimeoutException ignored) { + lockedSuccessfullyFuture = CompletableFuture.runAsync(() -> { + try (MutableStatusRegistry statusRegistry2 = zookeeperStatusService2 + .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) + { } - } + }); - lockedSuccessfullyFuture.get(1, TimeUnit.MINUTES); + try { + lockedSuccessfullyFuture.get(3, TimeUnit.SECONDS); + fail("Both zookeeper host status services locked simultaneously for the same application instance"); + } catch (TimeoutException ignored) { + } } + + lockedSuccessfullyFuture.get(1, TimeUnit.MINUTES); } @Test public void failing_to_get_lock_closes_SessionFailRetryLoop() throws Exception { - try (Curator curator = createConnectedCuratorFramework(testingServer)) { - ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); + ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); - try (MutableStatusRegistry statusRegistry = zookeeperStatusService - .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) { + try (MutableStatusRegistry statusRegistry = zookeeperStatusService + .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) { - //must run in separate thread, since having 2 locks in the same thread fails - CompletableFuture<Void> resultOfZkOperationAfterLockFailure = CompletableFuture.runAsync(() -> { - try { - zookeeperStatusService2.lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE); - fail("Both zookeeper host status services locked simultaneously for the same application instance"); - } catch (RuntimeException e) { - } + //must run in separate thread, since having 2 locks in the same thread fails + CompletableFuture<Void> resultOfZkOperationAfterLockFailure = CompletableFuture.runAsync(() -> { + try { + zookeeperStatusService2.lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE); + fail("Both zookeeper host status services locked simultaneously for the same application instance"); + } catch (RuntimeException e) { + } - killSession(curator.framework(), testingServer); + killSession(curator.framework(), testingServer); - //Throws SessionFailedException if the SessionFailRetryLoop has not been closed. - zookeeperStatusService2.forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getHostStatus(TestIds.HOST_NAME1); - }); + //Throws SessionFailedException if the SessionFailRetryLoop has not been closed. + statusRegistry.getHostStatus(TestIds.HOST_NAME1); + }); - assertThat(resultOfZkOperationAfterLockFailure, notHoldsException()); - } + assertThat(resultOfZkOperationAfterLockFailure, notHoldsException()); } } @@ -211,8 +199,7 @@ public class ZookeeperStatusServiceTest { // Initial state is NO_REMARK assertThat( zookeeperStatusService - .forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getApplicationInstanceStatus(), + .getApplicationInstanceStatus(TestIds.APPLICATION_INSTANCE_REFERENCE), is(ApplicationInstanceStatus.NO_REMARKS)); // Suspend @@ -223,8 +210,7 @@ public class ZookeeperStatusServiceTest { assertThat( zookeeperStatusService - .forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getApplicationInstanceStatus(), + .getApplicationInstanceStatus(TestIds.APPLICATION_INSTANCE_REFERENCE), is(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN)); // Resume @@ -235,8 +221,7 @@ public class ZookeeperStatusServiceTest { assertThat( zookeeperStatusService - .forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getApplicationInstanceStatus(), + .getApplicationInstanceStatus(TestIds.APPLICATION_INSTANCE_REFERENCE), is(ApplicationInstanceStatus.NO_REMARKS)); } @@ -262,17 +247,6 @@ public class ZookeeperStatusServiceTest { assertThat(suspendedApps, hasItem(TestIds.APPLICATION_INSTANCE_REFERENCE2)); } - private static void assertSessionFailed(Runnable statusServiceOperations) { - try { - statusServiceOperations.run(); - fail("Expected session expired exception"); - } catch (RuntimeException e) { - if (!(e.getCause() instanceof SessionFailedException)) { - throw e; - } - } - } - //TODO: move to vespajlib private static <T> List<T> shuffledList(T[] values) { //new ArrayList necessary to avoid "write through" behaviour @@ -281,10 +255,4 @@ public class ZookeeperStatusServiceTest { return list; } - //TODO: move to vespajlib - private static void doTimes(int numberOfIterations, Runnable runnable) { - for (int i = 0; i < numberOfIterations; i++) { - runnable.run(); - } - } } diff --git a/searchcore/src/tests/proton/common/attribute_updater/attribute_updater_test.cpp b/searchcore/src/tests/proton/common/attribute_updater/attribute_updater_test.cpp index 1620daa67c2..c1e63a15653 100644 --- a/searchcore/src/tests/proton/common/attribute_updater/attribute_updater_test.cpp +++ b/searchcore/src/tests/proton/common/attribute_updater/attribute_updater_test.cpp @@ -17,7 +17,8 @@ #include <vespa/document/update/documentupdate.h> #include <vespa/document/update/mapvalueupdate.h> #include <vespa/document/update/removevalueupdate.h> -#include <vespa/document/update/tensormodifyupdate.h> +#include <vespa/document/update/tensor_add_update.h> +#include <vespa/document/update/tensor_modify_update.h> #include <vespa/eval/tensor/default_tensor_engine.h> #include <vespa/eval/tensor/tensor.h> #include <vespa/searchcore/proton/common/attribute_updater.h> @@ -25,6 +26,7 @@ #include <vespa/searchlib/attribute/attributevector.hpp> #include <vespa/searchlib/attribute/reference_attribute.h> #include <vespa/searchlib/tensor/dense_tensor_attribute.h> +#include <vespa/searchlib/tensor/generic_tensor_attribute.h> #include <vespa/vespalib/testkit/testapp.h> #include <vespa/log/log.h> @@ -42,6 +44,8 @@ using search::attribute::Reference; using search::attribute::ReferenceAttribute; using search::tensor::ITensorAttribute; using search::tensor::DenseTensorAttribute; +using search::tensor::GenericTensorAttribute; +using search::tensor::TensorAttribute; using vespalib::eval::ValueType; using vespalib::eval::TensorSpec; using vespalib::tensor::DefaultTensorEngine; @@ -373,12 +377,13 @@ TEST_F("require that weighted set attributes are updated", Fixture) } } -std::unique_ptr<DenseTensorAttribute> -makeDenseTensorAttribute(const vespalib::string &name, const vespalib::string &tensorType) +template <typename TensorAttributeType> +std::unique_ptr<TensorAttributeType> +makeTensorAttribute(const vespalib::string &name, const vespalib::string &tensorType) { Config cfg(BasicType::TENSOR, CollectionType::SINGLE); cfg.setTensorType(ValueType::from_spec(tensorType)); - auto result = std::make_unique<DenseTensorAttribute>(name, cfg); + auto result = std::make_unique<TensorAttributeType>(name, cfg); result->addReservedDoc(); result->addDocs(1); return result; @@ -400,7 +405,7 @@ makeTensorFieldValue(const TensorSpec &spec) } void -setTensor(DenseTensorAttribute &attribute, uint32_t lid, const TensorSpec &spec) +setTensor(TensorAttribute &attribute, uint32_t lid, const TensorSpec &spec) { auto tensor = makeTensor(spec); attribute.setTensor(lid, *tensor); @@ -410,13 +415,24 @@ setTensor(DenseTensorAttribute &attribute, uint32_t lid, const TensorSpec &spec) TEST_F("require that tensor modify update is applied", Fixture) { vespalib::string type = "tensor(x[2])"; - auto attribute = makeDenseTensorAttribute("dense_tensor", type); + auto attribute = makeTensorAttribute<DenseTensorAttribute>("dense_tensor", type); setTensor(*attribute, 1, TensorSpec(type).add({{"x", 0}}, 3).add({{"x", 1}}, 5)); - TensorModifyUpdate update(TensorModifyUpdate::Operation::REPLACE, - makeTensorFieldValue(TensorSpec("tensor(x{})").add({{"x",0}}, 7))); - f.applyValueUpdate(*attribute, 1, update); - EXPECT_EQUAL(TensorSpec(type).add({{"x",0}}, 7).add({{"x", 1}}, 5), attribute->getTensor(1)->toSpec()); + f.applyValueUpdate(*attribute, 1, + TensorModifyUpdate(TensorModifyUpdate::Operation::REPLACE, + makeTensorFieldValue(TensorSpec("tensor(x{})").add({{"x", 0}}, 7)))); + EXPECT_EQUAL(TensorSpec(type).add({{"x", 0}}, 7).add({{"x", 1}}, 5), attribute->getTensor(1)->toSpec()); +} + +TEST_F("require that tensor add update is applied", Fixture) +{ + vespalib::string type = "tensor(x{})"; + auto attribute = makeTensorAttribute<GenericTensorAttribute>("dense_tensor", type); + setTensor(*attribute, 1, TensorSpec(type).add({{"x", "a"}}, 2)); + + f.applyValueUpdate(*attribute, 1, + TensorAddUpdate(makeTensorFieldValue(TensorSpec(type).add({{"x", "a"}}, 3)))); + EXPECT_EQUAL(TensorSpec(type).add({{"x", "a"}}, 3), attribute->getTensor(1)->toSpec()); } } diff --git a/searchcore/src/tests/proton/docsummary/docsummary.cpp b/searchcore/src/tests/proton/docsummary/docsummary.cpp index 40c2733d230..fb8674b5255 100644 --- a/searchcore/src/tests/proton/docsummary/docsummary.cpp +++ b/searchcore/src/tests/proton/docsummary/docsummary.cpp @@ -778,12 +778,7 @@ Test::requireThatAttributesAreUsed() "bj", *rep, 0, rclass)); // empty doc - EXPECT_TRUE(assertSlime("{bd:[]," - "be:[]," - "bf:[]," - "bg:[]," - "bh:[]," - "bi:[]}", *rep, 1, false)); + EXPECT_TRUE(assertSlime("{}", *rep, 1, false)); TEST_DO(assertTensor(Tensor::UP(), "bj", *rep, 1, rclass)); proton::IAttributeManager::SP attributeManager = dc._ddb->getReadySubDB()->getAttributeManager(); @@ -807,9 +802,7 @@ Test::requireThatAttributesAreUsed() req3.hits.push_back(DocsumRequest::Hit(gid3)); DocsumReply::UP rep3 = dc._ddb->getDocsums(req3); - EXPECT_TRUE(assertSlime("{bd:[],be:[],bf:[],bg:[]," - "bh:[],bi:[]," - "bj:'0x01020178017901016101624010000000000000'}", + EXPECT_TRUE(assertSlime("{bj:'0x01020178017901016101624010000000000000'}", *rep3, 0, true)); } diff --git a/searchcore/src/vespa/searchcore/proton/common/attribute_updater.cpp b/searchcore/src/vespa/searchcore/proton/common/attribute_updater.cpp index 3ad838c595b..933857cffed 100644 --- a/searchcore/src/vespa/searchcore/proton/common/attribute_updater.cpp +++ b/searchcore/src/vespa/searchcore/proton/common/attribute_updater.cpp @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "attribute_updater.h" -#include <vespa/eval/tensor/tensor.h> #include <vespa/document/base/forcelink.h> #include <vespa/document/fieldvalue/arrayfieldvalue.h> #include <vespa/document/fieldvalue/literalfieldvalue.h> @@ -15,7 +14,9 @@ #include <vespa/document/update/clearvalueupdate.h> #include <vespa/document/update/mapvalueupdate.h> #include <vespa/document/update/removevalueupdate.h> -#include <vespa/document/update/tensormodifyupdate.h> +#include <vespa/document/update/tensor_add_update.h> +#include <vespa/document/update/tensor_modify_update.h> +#include <vespa/eval/tensor/tensor.h> #include <vespa/searchlib/attribute/attributevector.hpp> #include <vespa/searchlib/attribute/changevector.hpp> #include <vespa/searchlib/attribute/predicate_attribute.h> @@ -205,8 +206,9 @@ AttributeUpdater::handleUpdate(PredicateAttribute &vec, uint32_t lid, const Valu namespace { +template <typename TensorUpdateType> void -applyTensorModifyUpdate(TensorAttribute &vec, uint32_t lid, const TensorModifyUpdate &update) +applyTensorUpdate(TensorAttribute &vec, uint32_t lid, const TensorUpdateType &update) { auto oldTensor = vec.getTensor(lid); if (oldTensor) { @@ -233,7 +235,9 @@ AttributeUpdater::handleUpdate(TensorAttribute &vec, uint32_t lid, const ValueUp updateValue(vec, lid, assign.getValue()); } } else if (op == ValueUpdate::TensorModifyUpdate) { - applyTensorModifyUpdate(vec, lid, static_cast<const TensorModifyUpdate &>(upd)); + applyTensorUpdate(vec, lid, static_cast<const TensorModifyUpdate &>(upd)); + } else if (op == ValueUpdate::TensorAddUpdate) { + applyTensorUpdate(vec, lid, static_cast<const TensorAddUpdate &>(upd)); } else if (op == ValueUpdate::Clear) { vec.clearDoc(lid); } else { diff --git a/searchlib/src/vespa/searchlib/attribute/enumstorebase.cpp b/searchlib/src/vespa/searchlib/attribute/enumstorebase.cpp index 20c66993486..2d991c7d565 100644 --- a/searchlib/src/vespa/searchlib/attribute/enumstorebase.cpp +++ b/searchlib/src/vespa/searchlib/attribute/enumstorebase.cpp @@ -636,6 +636,6 @@ namespace attribute { } -template class vespalib::hash_map<search::EnumStoreIndex, search::EnumStoreIndex, - vespalib::hash<search::EnumStoreIndex>, std::equal_to<search::EnumStoreIndex>, - vespalib::hashtable_base::and_modulator>; +VESPALIB_HASH_MAP_INSTANTIATE_H_E_M(search::EnumStoreIndex, search::EnumStoreIndex, + vespalib::hash<search::EnumStoreIndex>, std::equal_to<search::EnumStoreIndex>, + vespalib::hashtable_base::and_modulator); diff --git a/searchlib/src/vespa/searchlib/attribute/multistringattribute.cpp b/searchlib/src/vespa/searchlib/attribute/multistringattribute.cpp index 38d9160de2a..6c0d9342f39 100644 --- a/searchlib/src/vespa/searchlib/attribute/multistringattribute.cpp +++ b/searchlib/src/vespa/searchlib/attribute/multistringattribute.cpp @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "multistringattribute.h" +#include "multistringattribute.hpp" #include <vespa/searchlib/query/queryterm.h> namespace search { diff --git a/searchlib/src/vespa/searchlib/docstore/storebybucket.cpp b/searchlib/src/vespa/searchlib/docstore/storebybucket.cpp index 3bb1481969a..fe6da8a4cf0 100644 --- a/searchlib/src/vespa/searchlib/docstore/storebybucket.cpp +++ b/searchlib/src/vespa/searchlib/docstore/storebybucket.cpp @@ -85,3 +85,5 @@ StoreByBucket::drain(IWrite & drainer) } } + +VESPALIB_HASH_MAP_INSTANTIATE(uint64_t, vespalib::ConstBufferRef); diff --git a/searchsummary/src/vespa/searchsummary/docsummary/attributedfw.cpp b/searchsummary/src/vespa/searchsummary/docsummary/attributedfw.cpp index 19dd096c46c..1ca1a336d2d 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/attributedfw.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/attributedfw.cpp @@ -149,9 +149,13 @@ MultiAttrDFW::insertField(uint32_t docid, GetDocsumsState *state, ResType, Inser using vespalib::slime::Cursor; using vespalib::Memory; const IAttributeVector & v = vec(*state); - uint32_t entries = v.getValueCount(docid); bool isWeightedSet = v.hasWeightedSetType(); + uint32_t entries = v.getValueCount(docid); + if (entries == 0) { + return; // Don't insert empty fields + } + Cursor &arr = target.insertArray(); BasicType::Type t = v.getBasicType(); switch (t) { diff --git a/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.cpp b/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.cpp index 91a7fd45061..72cedb05f7c 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/summaryfieldconverter.cpp @@ -231,10 +231,6 @@ class SummaryFieldValueConverter : protected ConstFieldValueVisitor FieldValue::UP _field_value; FieldValueConverter &_structuredFieldConverter; - void visit(const ArrayFieldValue &value) override { - _field_value = _structuredFieldConverter.convert(value); - } - template <typename T> void visitPrimitive(const T &t) { _field_value.reset(t.clone()); @@ -274,8 +270,16 @@ class SummaryFieldValueConverter : protected ConstFieldValueVisitor visitPrimitive(value); } - void visit(const MapFieldValue & v) override { - _field_value = _structuredFieldConverter.convert(v); + void visit(const ArrayFieldValue &value) override { + if (value.size() > 0) { + _field_value = _structuredFieldConverter.convert(value); + } // else: implicit empty string + } + + void visit(const MapFieldValue & value) override { + if (value.size() > 0) { + _field_value = _structuredFieldConverter.convert(value); + } // else: implicit empty string } void visit(const StructFieldValue &value) override { @@ -292,7 +296,9 @@ class SummaryFieldValueConverter : protected ConstFieldValueVisitor } void visit(const WeightedSetFieldValue &value) override { - _field_value = _structuredFieldConverter.convert(value); + if (value.size() > 0) { + _field_value = _structuredFieldConverter.convert(value); + } // else: implicit empty string } void visit(const TensorFieldValue &value) override { diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceMonitorImpl.java b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceMonitorImpl.java index 50ea31eb9c4..0a40555036c 100644 --- a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceMonitorImpl.java +++ b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceMonitorImpl.java @@ -17,6 +17,7 @@ import com.yahoo.vespa.service.slobrok.SlobrokMonitorManagerImpl; import java.util.Map; public class ServiceMonitorImpl implements ServiceMonitor { + private final ServiceModelCache serviceModelProvider; @Inject @@ -37,12 +38,8 @@ public class ServiceMonitorImpl implements ServiceMonitor { } @Override - public Map<ApplicationInstanceReference, ApplicationInstance> getAllApplicationInstances() { - return serviceModelProvider.get().getAllApplicationInstances(); - } - - @Override public ServiceModel getServiceModelSnapshot() { return serviceModelProvider.get(); } + } diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceModel.java b/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceModel.java index b62552188e1..ed72893400a 100644 --- a/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceModel.java +++ b/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceModel.java @@ -4,9 +4,13 @@ package com.yahoo.vespa.service.monitor; import com.yahoo.vespa.applicationmodel.ApplicationInstance; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; import com.yahoo.vespa.applicationmodel.HostName; +import com.yahoo.vespa.applicationmodel.ServiceCluster; import com.yahoo.vespa.applicationmodel.ServiceInstance; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -20,9 +24,11 @@ import java.util.stream.Collectors; public class ServiceModel { private final Map<ApplicationInstanceReference, ApplicationInstance> applications; + private final Map<HostName, ApplicationInstance> applicationsByHostName; public ServiceModel(Map<ApplicationInstanceReference, ApplicationInstance> applications) { this.applications = Collections.unmodifiableMap(applications); + this.applicationsByHostName = Collections.unmodifiableMap(applicationsByHostNames(applications.values())); } public Map<ApplicationInstanceReference, ApplicationInstance> getAllApplicationInstances() { @@ -33,6 +39,10 @@ public class ServiceModel { return Optional.ofNullable(applications.get(reference)); } + public Map<HostName, ApplicationInstance> getApplicationsByHostName() { + return applicationsByHostName; + } + public Map<HostName, List<ServiceInstance>> getServiceInstancesByHostName() { return applications.values().stream() .flatMap(application -> application.serviceClusters().stream()) @@ -40,4 +50,17 @@ public class ServiceModel { .collect(Collectors.groupingBy(ServiceInstance::hostName, Collectors.toList())); } + private static Map<HostName, ApplicationInstance> applicationsByHostNames(Collection<ApplicationInstance> applications) { + Map<HostName, ApplicationInstance> hosts = new HashMap<>(); + for (ApplicationInstance application : applications) + for (ServiceCluster cluster : application.serviceClusters()) + for (ServiceInstance instance : cluster.serviceInstances()) { + ApplicationInstance previous = hosts.put(instance.hostName(), application); + if (previous != null && ! previous.equals(application)) + throw new IllegalStateException("Major assumption broken: Multiple application instances contain host " + + instance.hostName().s() + ": " + Arrays.asList(previous, application)); + } + return hosts; + } + } diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceMonitor.java b/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceMonitor.java index 5ed34673da5..49539c61e5d 100644 --- a/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceMonitor.java +++ b/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceMonitor.java @@ -1,11 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.service.monitor; -import com.yahoo.vespa.applicationmodel.ApplicationInstance; -import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; - -import java.util.Map; - /** * The service monitor interface. A service monitor provides up to date information about the liveness status * (up, down or not known) of each service instance in a Vespa zone @@ -15,11 +10,9 @@ import java.util.Map; public interface ServiceMonitor { /** - * Returns the current liveness status (up, down or unknown) of all instances + * Returns a ServiceModel which contains the current liveness status (up, down or unknown) of all instances * of all services of all clusters of all applications in a zone. */ - Map<ApplicationInstanceReference, ApplicationInstance> getAllApplicationInstances(); - ServiceModel getServiceModelSnapshot(); } diff --git a/storage/src/tests/bucketdb/bucketmanagertest.cpp b/storage/src/tests/bucketdb/bucketmanagertest.cpp index 829a080ad01..54b3bf4b8d0 100644 --- a/storage/src/tests/bucketdb/bucketmanagertest.cpp +++ b/storage/src/tests/bucketdb/bucketmanagertest.cpp @@ -67,6 +67,7 @@ public: CPPUNIT_TEST(testRemoveLastModifiedFailed); CPPUNIT_TEST(testSwallowNotifyBucketChangeReply); CPPUNIT_TEST(testMetricsGeneration); + CPPUNIT_TEST(metrics_are_tracked_per_bucket_space); CPPUNIT_TEST(testSplitReplyOrderedAfterBucketReply); CPPUNIT_TEST(testJoinReplyOrderedAfterBucketReply); CPPUNIT_TEST(testDeleteReplyOrderedAfterBucketReply); @@ -136,6 +137,7 @@ public: void testSwallowNotifyBucketChangeReply(); void testMetricsGeneration(); + void metrics_are_tracked_per_bucket_space(); void testSplitReplyOrderedAfterBucketReply(); void testJoinReplyOrderedAfterBucketReply(); void testDeleteReplyOrderedAfterBucketReply(); @@ -152,7 +154,6 @@ public: void testConflictSetOnlyClearedAfterAllBucketRequestsDone(); void testRejectRequestWithMismatchingDistributionHash(); void testDbNotIteratedWhenAllRequestsRejected(); - void testReceivedDistributionHashIsNormalized(); public: static constexpr uint32_t DIR_SPREAD = 3; @@ -641,6 +642,51 @@ BucketManagerTest::testMetricsGeneration() CPPUNIT_ASSERT_EQUAL(int64_t(2), m.ready.getLast()); } +void BucketManagerTest::metrics_are_tracked_per_bucket_space() { + setupTestEnvironment(); + _top->open(); + auto& repo = _node->getComponentRegister().getBucketSpaceRepo(); + { + bucketdb::StorageBucketInfo entry; + entry.disk = 0; + api::BucketInfo info(50, 100, 200); + info.setReady(true); + entry.setBucketInfo(info); + repo.get(document::FixedBucketSpaces::default_space()).bucketDatabase() + .insert(document::BucketId(16, 1234), entry, "foo"); + } + { + bucketdb::StorageBucketInfo entry; + entry.disk = 0; + api::BucketInfo info(60, 150, 300); + info.setActive(true); + entry.setBucketInfo(info); + repo.get(document::FixedBucketSpaces::global_space()).bucketDatabase() + .insert(document::BucketId(16, 1234), entry, "foo"); + } + _node->getDoneInitializeHandler().notifyDoneInitializing(); + _top->doneInit(); + vespalib::Monitor l; + _manager->updateMetrics(BucketManager::MetricLockGuard(l)); + + auto& spaces = _manager->_metrics->bucket_spaces; + auto default_m = spaces.find(document::FixedBucketSpaces::default_space()); + CPPUNIT_ASSERT(default_m != spaces.end()); + CPPUNIT_ASSERT_EQUAL(int64_t(1), default_m->second->buckets_total.getLast()); + CPPUNIT_ASSERT_EQUAL(int64_t(100), default_m->second->docs.getLast()); + CPPUNIT_ASSERT_EQUAL(int64_t(200), default_m->second->bytes.getLast()); + CPPUNIT_ASSERT_EQUAL(int64_t(0), default_m->second->active_buckets.getLast()); + CPPUNIT_ASSERT_EQUAL(int64_t(1), default_m->second->ready_buckets.getLast()); + + auto global_m = spaces.find(document::FixedBucketSpaces::global_space()); + CPPUNIT_ASSERT(global_m != spaces.end()); + CPPUNIT_ASSERT_EQUAL(int64_t(1), global_m->second->buckets_total.getLast()); + CPPUNIT_ASSERT_EQUAL(int64_t(150), global_m->second->docs.getLast()); + CPPUNIT_ASSERT_EQUAL(int64_t(300), global_m->second->bytes.getLast()); + CPPUNIT_ASSERT_EQUAL(int64_t(1), global_m->second->active_buckets.getLast()); + CPPUNIT_ASSERT_EQUAL(int64_t(0), global_m->second->ready_buckets.getLast()); +} + void BucketManagerTest::insertSingleBucket(const document::BucketId& bucket, const api::BucketInfo& info) diff --git a/storage/src/tests/bucketdb/lockablemaptest.cpp b/storage/src/tests/bucketdb/lockablemaptest.cpp index c94121062ec..10f806f2e97 100644 --- a/storage/src/tests/bucketdb/lockablemaptest.cpp +++ b/storage/src/tests/bucketdb/lockablemaptest.cpp @@ -115,7 +115,6 @@ namespace { void LockableMapTest::testSimpleUsage() { // Tests insert, erase, size, empty, operator[] - typedef LockableMap<JudyMultiMap<A> > Map; Map map; // Do some insertions CPPUNIT_ASSERT(map.empty()); @@ -154,7 +153,6 @@ LockableMapTest::testSimpleUsage() { void LockableMapTest::testComparison() { - typedef LockableMap<JudyMultiMap<A> > Map; Map map1; Map map2; bool preExisted; @@ -579,7 +577,6 @@ printBuckets(const std::map<document::BucketId, Map::WrappedEntry>& results) { void LockableMapTest::testFindBucketsSimple() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(17, 0x0ffff); @@ -608,7 +605,6 @@ LockableMapTest::testFindBucketsSimple() { void LockableMapTest::testFindBuckets() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); @@ -637,7 +633,6 @@ LockableMapTest::testFindBuckets() { void LockableMapTest::testFindBuckets2() { // ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); @@ -666,7 +661,6 @@ LockableMapTest::testFindBuckets2() { // ticket 3121525 void LockableMapTest::testFindBuckets3() { // ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); @@ -689,7 +683,6 @@ LockableMapTest::testFindBuckets3() { // ticket 3121525 void LockableMapTest::testFindBuckets4() { // ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); @@ -714,7 +707,6 @@ LockableMapTest::testFindBuckets4() { // ticket 3121525 void LockableMapTest::testFindBuckets5() { // ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); @@ -739,7 +731,6 @@ LockableMapTest::testFindBuckets5() { // ticket 3121525 void LockableMapTest::testFindNoBuckets() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id(16, 0x0ffff); @@ -753,7 +744,6 @@ LockableMapTest::testFindNoBuckets() { void LockableMapTest::testFindAll() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0aaaa); // contains id2-id7 @@ -815,7 +805,6 @@ LockableMapTest::testFindAll() { void LockableMapTest::testFindAll2() { // Ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(17, 0x00001); @@ -839,7 +828,6 @@ LockableMapTest::testFindAll2() { // Ticket 3121525 void LockableMapTest::testFindAllUnusedBitIsSet() { // ticket 2938896 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(24, 0x000dc7089); @@ -868,7 +856,6 @@ LockableMapTest::testFindAllUnusedBitIsSet() { // ticket 2938896 void LockableMapTest::testFindAllInconsistentlySplit() { // Ticket 2938896 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x00001); // contains id2-id3 @@ -895,7 +882,6 @@ LockableMapTest::testFindAllInconsistentlySplit() { // Ticket 2938896 void LockableMapTest::testFindAllInconsistentlySplit2() { // ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(17, 0x10000); @@ -923,7 +909,6 @@ LockableMapTest::testFindAllInconsistentlySplit2() { // ticket 3121525 void LockableMapTest::testFindAllInconsistentlySplit3() { // ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); // contains id2 @@ -946,7 +931,6 @@ LockableMapTest::testFindAllInconsistentlySplit3() { // ticket 3121525 void LockableMapTest::testFindAllInconsistentlySplit4() { // ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); // contains id2-id3 @@ -972,7 +956,6 @@ LockableMapTest::testFindAllInconsistentlySplit4() { // ticket 3121525 void LockableMapTest::testFindAllInconsistentlySplit5() { // ticket 3121525 #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); // contains id2-id3 @@ -997,7 +980,6 @@ LockableMapTest::testFindAllInconsistentlySplit5() { // ticket 3121525 void LockableMapTest::testFindAllInconsistentlySplit6() { - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x0ffff); // contains id2-id3 @@ -1022,7 +1004,6 @@ LockableMapTest::testFindAllInconsistentlySplit6() { void LockableMapTest::testFindAllInconsistentBelow16Bits() { - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(1, 0x1); // contains id2-id3 @@ -1048,7 +1029,6 @@ LockableMapTest::testFindAllInconsistentBelow16Bits() void LockableMapTest::testCreate() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; { document::BucketId id1(58, 0x43d6c878000004d2ull); @@ -1081,7 +1061,6 @@ LockableMapTest::testCreate() { void LockableMapTest::testCreate2() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; { document::BucketId id1(58, 0xeaf77782000004d2); @@ -1108,7 +1087,6 @@ LockableMapTest::testCreate2() { void LockableMapTest::testCreate3() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; { document::BucketId id1(58, 0xeaf77780000004d2); @@ -1137,7 +1115,6 @@ LockableMapTest::testCreate3() { void LockableMapTest::testCreate4() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; { document::BucketId id1(16, 0x00000000000004d1); @@ -1162,7 +1139,6 @@ LockableMapTest::testCreate4() { void LockableMapTest::testCreate6() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; { document::BucketId id1(0x8c000000000004d2); @@ -1199,7 +1175,6 @@ LockableMapTest::testCreate6() { void LockableMapTest::testCreate5() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; { document::BucketId id1(58, 0xeaf77780000004d2); @@ -1224,7 +1199,6 @@ LockableMapTest::testCreate5() { void LockableMapTest::testCreateEmpty() { #if __WORDSIZE == 64 - typedef LockableMap<JudyMultiMap<A> > Map; Map map; { document::BucketId id1(58, 0x00000000010004d2); @@ -1238,7 +1212,6 @@ LockableMapTest::testCreateEmpty() { void LockableMapTest::testIsConsistent() { - typedef LockableMap<JudyMultiMap<A> > Map; Map map; document::BucketId id1(16, 0x00001); // contains id2-id3 document::BucketId id2(17, 0x00001); diff --git a/storage/src/tests/common/metricstest.cpp b/storage/src/tests/common/metricstest.cpp index 867d132031e..b0c82aa3166 100644 --- a/storage/src/tests/common/metricstest.cpp +++ b/storage/src/tests/common/metricstest.cpp @@ -82,44 +82,43 @@ MetricsTest::MetricsTest() } void MetricsTest::setUp() { - _config.reset(new vdstestlib::DirConfig(getStandardConfig(true, "metricstest"))); + _config = std::make_unique<vdstestlib::DirConfig>(getStandardConfig(true, "metricstest")); assert(system(("rm -rf " + getRootFolder(*_config)).c_str()) == 0); try { - _node.reset(new TestServiceLayerApp(DiskCount(4), NodeIndex(0), - _config->getConfigId())); + _node = std::make_unique<TestServiceLayerApp>( + DiskCount(4), NodeIndex(0), _config->getConfigId()); _node->setupDummyPersistence(); _clock = &_node->getClock(); _clock->setAbsoluteTimeInSeconds(1000000); - _top.reset(new DummyStorageLink); + _top = std::make_unique<DummyStorageLink>(); } catch (config::InvalidConfigException& e) { fprintf(stderr, "%s\n", e.what()); } - _metricManager.reset(new metrics::MetricManager( - std::unique_ptr<metrics::MetricManager::Timer>( - new MetricClock(*_clock)))); + _metricManager = std::make_unique<metrics::MetricManager>( + std::make_unique<MetricClock>(*_clock)); _topSet.reset(new metrics::MetricSet("vds", {}, "")); { metrics::MetricLockGuard guard(_metricManager->getMetricLock()); _metricManager->registerMetric(guard, *_topSet); } - _metricsConsumer.reset(new StatusMetricConsumer( + _metricsConsumer = std::make_unique<StatusMetricConsumer>( _node->getComponentRegister(), *_metricManager, - "status")); + "status"); uint16_t diskCount = _node->getPartitions().size(); documentapi::LoadTypeSet::SP loadTypes(_node->getLoadTypes()); - _filestorMetrics.reset(new FileStorMetrics(_node->getLoadTypes()->getMetricLoadTypes())); + _filestorMetrics = std::make_shared<FileStorMetrics>(_node->getLoadTypes()->getMetricLoadTypes()); _filestorMetrics->initDiskMetrics(diskCount, loadTypes->getMetricLoadTypes(), 1, 1); _topSet->registerMetric(*_filestorMetrics); - _bucketManagerMetrics.reset(new BucketManagerMetrics); + _bucketManagerMetrics = std::make_shared<BucketManagerMetrics>(_node->getComponentRegister().getBucketSpaceRepo()); _bucketManagerMetrics->setDisks(diskCount); _topSet->registerMetric(*_bucketManagerMetrics); - _visitorMetrics.reset(new VisitorMetrics); + _visitorMetrics = std::make_shared<VisitorMetrics>(); _visitorMetrics->initThreads(4, loadTypes->getMetricLoadTypes()); _topSet->registerMetric(*_visitorMetrics); _metricManager->init(_config->getConfigId(), _node->getThreadPool()); @@ -127,12 +126,12 @@ void MetricsTest::setUp() { void MetricsTest::tearDown() { _metricManager->stop(); - _metricsConsumer.reset(0); - _topSet.reset(0); - _metricManager.reset(0); - _top.reset(0); - _node.reset(0); - _config.reset(0); + _metricsConsumer.reset(); + _topSet.reset(); + _metricManager.reset(); + _top.reset(); + _node.reset(); + _config.reset(); _filestorMetrics.reset(); _bucketManagerMetrics.reset(); _visitorMetrics.reset(); diff --git a/storage/src/tests/distributor/putoperationtest.cpp b/storage/src/tests/distributor/putoperationtest.cpp index 5551d0a5010..aa5fd80df91 100644 --- a/storage/src/tests/distributor/putoperationtest.cpp +++ b/storage/src/tests/distributor/putoperationtest.cpp @@ -209,7 +209,6 @@ PutOperationTest::testBucketDatabaseGetsSpecialEntryWhenCreateBucketSent() setupDistributor(2, 1, "storage:1 distributor:1"); Document::SP doc(createDummyDocument("test", "test")); - document::BucketId bucketId(getExternalOperationHandler().getBucketId(doc->getId())); sendPut(createPut(doc)); // Database updated before CreateBucket is sent diff --git a/storage/src/tests/persistence/filestorage/deactivatebucketstest.cpp b/storage/src/tests/persistence/filestorage/deactivatebucketstest.cpp index 4656924c1b6..f9375790ebb 100644 --- a/storage/src/tests/persistence/filestorage/deactivatebucketstest.cpp +++ b/storage/src/tests/persistence/filestorage/deactivatebucketstest.cpp @@ -44,7 +44,6 @@ DeactivateBucketsTest::bucketsInDatabaseDeactivatedWhenNodeDownInClusterState() lib::ClusterState::CSP(new lib::ClusterState(upState))); document::BucketId bucket(8, 123); - spi::Bucket spiBucket(makeSpiBucket(bucket)); createBucket(bucket); api::BucketInfo serviceLayerInfo(1, 2, 3, 4, 5, true, true); diff --git a/storage/src/vespa/storage/bucketdb/bucketmanager.cpp b/storage/src/vespa/storage/bucketdb/bucketmanager.cpp index 551f6fc726d..41de215d877 100644 --- a/storage/src/vespa/storage/bucketdb/bucketmanager.cpp +++ b/storage/src/vespa/storage/bucketdb/bucketmanager.cpp @@ -42,10 +42,10 @@ BucketManager::BucketManager(const config::ConfigUri & configUri, _firstEqualClusterStateVersion(0), _lastClusterStateSeen(0), _lastUnifiedClusterState(""), - _metrics(new BucketManagerMetrics), _doneInitialized(false), _requestsCurrentlyProcessing(0), - _component(compReg, "bucketmanager") + _component(compReg, "bucketmanager"), + _metrics(std::make_shared<BucketManagerMetrics>(_component.getBucketSpaceRepo())) { _metrics->setDisks(_component.getDiskCount()); _component.registerStatusPage(*this); @@ -197,6 +197,19 @@ namespace { return StorBucketDatabase::CONTINUE; }; + + void add(const MetricsUpdater& rhs) { + assert(diskCount == rhs.diskCount); + for (uint16_t i = 0; i < diskCount; i++) { + auto& d = disk[i]; + auto& s = rhs.disk[i]; + d.buckets += s.buckets; + d.docs += s.docs; + d.bytes += s.bytes; + d.ready += s.ready; + d.active += s.active; + } + } }; } // End of anonymous namespace @@ -216,18 +229,33 @@ BucketManager::updateMetrics(bool updateDocCount) updateDocCount ? "" : ", minusedbits only", _doneInitialized ? "" : ", server is not done initializing"); - uint32_t diskCount = _component.getDiskCount(); + uint16_t diskCount = _component.getDiskCount(); + assert(diskCount >= 1); if (!updateDocCount || _doneInitialized) { - MetricsUpdater m(diskCount); - _component.getBucketSpaceRepo().forEachBucketChunked( - m, "BucketManager::updateMetrics"); + MetricsUpdater total(diskCount); + for (auto& space : _component.getBucketSpaceRepo()) { + MetricsUpdater m(diskCount); + space.second->bucketDatabase().chunkedAll(m, "BucketManager::updateMetrics"); + total.add(m); + if (updateDocCount) { + auto bm = _metrics->bucket_spaces.find(space.first); + assert(bm != _metrics->bucket_spaces.end()); + // No system with multiple bucket spaces has more than 1 "disk" + // TODO remove disk concept entirely as it's a VDS relic + bm->second->buckets_total.set(m.disk[0].buckets); + bm->second->docs.set(m.disk[0].docs); + bm->second->bytes.set(m.disk[0].bytes); + bm->second->active_buckets.set(m.disk[0].active); + bm->second->ready_buckets.set(m.disk[0].ready); + } + } if (updateDocCount) { for (uint16_t i = 0; i< diskCount; i++) { - _metrics->disks[i]->buckets.addValue(m.disk[i].buckets); - _metrics->disks[i]->docs.addValue(m.disk[i].docs); - _metrics->disks[i]->bytes.addValue(m.disk[i].bytes); - _metrics->disks[i]->active.addValue(m.disk[i].active); - _metrics->disks[i]->ready.addValue(m.disk[i].ready); + _metrics->disks[i]->buckets.addValue(total.disk[i].buckets); + _metrics->disks[i]->docs.addValue(total.disk[i].docs); + _metrics->disks[i]->bytes.addValue(total.disk[i].bytes); + _metrics->disks[i]->active.addValue(total.disk[i].active); + _metrics->disks[i]->ready.addValue(total.disk[i].ready); } } } diff --git a/storage/src/vespa/storage/bucketdb/bucketmanager.h b/storage/src/vespa/storage/bucketdb/bucketmanager.h index 440e0d3b125..0503cb33d49 100644 --- a/storage/src/vespa/storage/bucketdb/bucketmanager.h +++ b/storage/src/vespa/storage/bucketdb/bucketmanager.h @@ -79,10 +79,10 @@ private: * The unified version of the last cluster state. */ std::string _lastUnifiedClusterState; - std::shared_ptr<BucketManagerMetrics> _metrics; bool _doneInitialized; size_t _requestsCurrentlyProcessing; ServiceLayerComponent _component; + std::shared_ptr<BucketManagerMetrics> _metrics; framework::Thread::UP _thread; BucketManager(const BucketManager&); diff --git a/storage/src/vespa/storage/bucketdb/bucketmanagermetrics.cpp b/storage/src/vespa/storage/bucketdb/bucketmanagermetrics.cpp index a0f5f2f7ec9..b5efb5570bb 100644 --- a/storage/src/vespa/storage/bucketdb/bucketmanagermetrics.cpp +++ b/storage/src/vespa/storage/bucketdb/bucketmanagermetrics.cpp @@ -1,6 +1,8 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "bucketmanagermetrics.h" +#include <vespa/document/bucket/fixed_bucket_spaces.h> +#include <vespa/storage/common/content_bucket_space_repo.h> #include <vespa/vespalib/util/exceptions.h> #include <cassert> @@ -23,11 +25,28 @@ DataStoredMetrics::DataStoredMetrics(const std::string& name, metrics::MetricSet ready.logOnlyIfSet(); } -DataStoredMetrics::~DataStoredMetrics() { } +DataStoredMetrics::~DataStoredMetrics() = default; -BucketManagerMetrics::BucketManagerMetrics() +BucketSpaceMetrics::BucketSpaceMetrics(const vespalib::string& space_name, metrics::MetricSet* owner) + : metrics::MetricSet("bucket_space", {{"bucketSpace", space_name}}, "", owner), + buckets_total("buckets_total", {}, "Total number buckets present in the bucket space (ready + not ready)", this), + docs("docs", {}, "Documents stored in the bucket space", this), + bytes("bytes", {}, "Bytes stored across all documents in the bucket space", this), + active_buckets("active_buckets", {}, "Number of active buckets in the bucket space", this), + ready_buckets("ready_buckets", {}, "Number of ready buckets in the bucket space", this) +{ + docs.logOnlyIfSet(); + bytes.logOnlyIfSet(); + active_buckets.logOnlyIfSet(); + ready_buckets.logOnlyIfSet(); +} + +BucketSpaceMetrics::~BucketSpaceMetrics() = default; + +BucketManagerMetrics::BucketManagerMetrics(const ContentBucketSpaceRepo& repo) : metrics::MetricSet("datastored", {}, ""), disks(), + bucket_spaces(), total("alldisks", {{"sum"}}, "Sum of data stored metrics for all disks", this), simpleBucketInfoRequestSize("simplebucketinforeqsize", {}, "Amount of buckets returned in simple bucket info requests", @@ -36,9 +55,14 @@ BucketManagerMetrics::BucketManagerMetrics() "Amount of distributors answered at once in full bucket info requests.", this), fullBucketInfoLatency("fullbucketinfolatency", {}, "Amount of time spent to process a full bucket info request", this) -{ } +{ + for (const auto& space : repo) { + bucket_spaces.emplace(space.first, std::make_unique<BucketSpaceMetrics>( + document::FixedBucketSpaces::to_string(space.first), this)); + } +} -BucketManagerMetrics::~BucketManagerMetrics() { } +BucketManagerMetrics::~BucketManagerMetrics() = default; void BucketManagerMetrics::setDisks(uint16_t numDisks) { @@ -47,8 +71,7 @@ BucketManagerMetrics::setDisks(uint16_t numDisks) { throw IllegalStateException("Cannot initialize disks twice", VESPA_STRLOC); } for (uint16_t i = 0; i<numDisks; i++) { - disks.push_back(DataStoredMetrics::SP( - new DataStoredMetrics(make_string("disk%d", i), this))); + disks.push_back(std::make_shared<DataStoredMetrics>(make_string("disk%d", i), this)); total.addMetricToSum(*disks.back()); } } diff --git a/storage/src/vespa/storage/bucketdb/bucketmanagermetrics.h b/storage/src/vespa/storage/bucketdb/bucketmanagermetrics.h index 259075bf518..1e7b8960f14 100644 --- a/storage/src/vespa/storage/bucketdb/bucketmanagermetrics.h +++ b/storage/src/vespa/storage/bucketdb/bucketmanagermetrics.h @@ -2,12 +2,15 @@ #pragma once +#include <vespa/document/bucket/bucketspace.h> #include <vespa/metrics/metrics.h> +#include <unordered_map> +#include <memory> + namespace storage { -struct DataStoredMetrics : public metrics::MetricSet -{ +struct DataStoredMetrics : metrics::MetricSet { typedef std::shared_ptr<DataStoredMetrics> SP; metrics::LongValueMetric buckets; @@ -17,20 +20,35 @@ struct DataStoredMetrics : public metrics::MetricSet metrics::LongValueMetric ready; DataStoredMetrics(const std::string& name, metrics::MetricSet* owner); - ~DataStoredMetrics(); + ~DataStoredMetrics() override; }; -class BucketManagerMetrics : public metrics::MetricSet -{ +struct BucketSpaceMetrics : metrics::MetricSet { + // Superficially very similar to DataStoredMetrics, but metric naming and dimensions differ + metrics::LongValueMetric buckets_total; + metrics::LongValueMetric docs; + metrics::LongValueMetric bytes; + metrics::LongValueMetric active_buckets; + metrics::LongValueMetric ready_buckets; + + BucketSpaceMetrics(const vespalib::string& space_name, metrics::MetricSet* owner); + ~BucketSpaceMetrics() override; +}; + +class ContentBucketSpaceRepo; + +class BucketManagerMetrics : public metrics::MetricSet { public: - std::vector<std::shared_ptr<DataStoredMetrics> > disks; + std::vector<std::shared_ptr<DataStoredMetrics>> disks; + using BucketSpaceMap = std::unordered_map<document::BucketSpace, std::unique_ptr<BucketSpaceMetrics>, document::BucketSpace::hash>; + BucketSpaceMap bucket_spaces; metrics::SumMetric<metrics::MetricSet> total; metrics::LongValueMetric simpleBucketInfoRequestSize; metrics::LongAverageMetric fullBucketInfoRequestSize; metrics::LongAverageMetric fullBucketInfoLatency; - BucketManagerMetrics(); - ~BucketManagerMetrics(); + explicit BucketManagerMetrics(const ContentBucketSpaceRepo& repo); + ~BucketManagerMetrics() override; void setDisks(uint16_t numDisks); }; diff --git a/storage/src/vespa/storage/bucketmover/htmltable.h b/storage/src/vespa/storage/bucketmover/htmltable.h index c032c15ee02..85e9438edcb 100644 --- a/storage/src/vespa/storage/bucketmover/htmltable.h +++ b/storage/src/vespa/storage/bucketmover/htmltable.h @@ -47,8 +47,7 @@ struct Column { } out << ">"; } - virtual void printElementStop(std::ostream& out, uint16_t row) { - std::map<uint16_t, Color>::iterator color(_colors.find(row)); + virtual void printElementStop(std::ostream& out, [[maybe_unused]] uint16_t row) { out << "</td>"; } diff --git a/storage/src/vespa/storage/bucketmover/run.h b/storage/src/vespa/storage/bucketmover/run.h index eb7a6df2d17..914781eec30 100644 --- a/storage/src/vespa/storage/bucketmover/run.h +++ b/storage/src/vespa/storage/bucketmover/run.h @@ -38,7 +38,6 @@ class Run : public document::Printable { std::shared_ptr<const lib::Distribution> _distribution; lib::NodeState _nodeState; uint16_t _nodeIndex; - uint32_t _maxEntriesToKeep; std::list<Move> _entries; std::list<Move> _pending; bool _iterationDone; diff --git a/storage/src/vespa/storage/distributor/bucketdbupdater.h b/storage/src/vespa/storage/distributor/bucketdbupdater.h index 89c5000b542..ea67e7ea72a 100644 --- a/storage/src/vespa/storage/distributor/bucketdbupdater.h +++ b/storage/src/vespa/storage/distributor/bucketdbupdater.h @@ -191,13 +191,12 @@ private: public: NodeRemover(const lib::ClusterState& oldState, const lib::ClusterState& s, - const document::BucketIdFactory& factory, + [[maybe_unused]] const document::BucketIdFactory& factory, uint16_t localIndex, const lib::Distribution& distribution, const char* upStates) : _oldState(oldState), _state(s), - _factory(factory), _localIndex(localIndex), _distribution(distribution), _upStates(upStates) {} @@ -218,7 +217,6 @@ private: const lib::ClusterState _state; std::vector<document::BucketId> _removedBuckets; - const document::BucketIdFactory& _factory; uint16_t _localIndex; const lib::Distribution& _distribution; const char* _upStates; diff --git a/storage/src/vespa/storage/distributor/operations/external/statbucketoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/statbucketoperation.cpp index 4e2f8a3169a..8e6494a588d 100644 --- a/storage/src/vespa/storage/distributor/operations/external/statbucketoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/statbucketoperation.cpp @@ -12,11 +12,10 @@ namespace storage { namespace distributor { StatBucketOperation::StatBucketOperation( - DistributorComponent& manager, + [[maybe_unused]] DistributorComponent& manager, DistributorBucketSpace &bucketSpace, const std::shared_ptr<api::StatBucketCommand> & cmd) : Operation(), - _manager(manager), _bucketSpace(bucketSpace), _command(cmd) { diff --git a/storage/src/vespa/storage/distributor/operations/external/statbucketoperation.h b/storage/src/vespa/storage/distributor/operations/external/statbucketoperation.h index d40924e23f0..914e104943a 100644 --- a/storage/src/vespa/storage/distributor/operations/external/statbucketoperation.h +++ b/storage/src/vespa/storage/distributor/operations/external/statbucketoperation.h @@ -30,7 +30,6 @@ public: void onStart(DistributorMessageSender& sender) override; void onReceive(DistributorMessageSender& sender, const std::shared_ptr<api::StorageReply> & msg) override; private: - DistributorComponent& _manager; DistributorBucketSpace &_bucketSpace; std::shared_ptr<api::StatBucketCommand> _command; diff --git a/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp b/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp index 3ccb35ef5a7..d7207f43c7b 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp @@ -23,10 +23,9 @@ using document::BucketSpace; namespace storage { FileStorHandlerImpl::FileStorHandlerImpl(uint32_t numStripes, MessageSender& sender, FileStorMetrics& metrics, - const spi::PartitionStateList& partitions, + [[maybe_unused]] const spi::PartitionStateList& partitions, ServiceLayerComponentRegister& compReg) - : _partitions(partitions), - _component(compReg, "filestorhandlerimpl"), + : _component(compReg, "filestorhandlerimpl"), _diskInfo(), _messageSender(sender), _bucketIdFactory(_component.getBucketIdFactory()), diff --git a/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.h b/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.h index fe9982018b9..390b7284b85 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.h +++ b/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.h @@ -282,7 +282,6 @@ public: void abortQueuedOperations(const AbortBucketOperationsCommand& cmd); private: - const spi::PartitionStateList& _partitions; ServiceLayerComponent _component; std::vector<Disk> _diskInfo; MessageSender& _messageSender; diff --git a/storage/src/vespa/storage/storageserver/documentapiconverter.cpp b/storage/src/vespa/storage/storageserver/documentapiconverter.cpp index b6ac2a4b219..f9deb76c3e2 100644 --- a/storage/src/vespa/storage/storageserver/documentapiconverter.cpp +++ b/storage/src/vespa/storage/storageserver/documentapiconverter.cpp @@ -152,7 +152,7 @@ DocumentApiConverter::toStorageAPI(documentapi::DocumentMessage& fromMsg) toMsg->toString().c_str(), toMsg->getLoadType().getId(), fromMsg.getPriority(), toMsg->getPriority()); } - return std::move(toMsg); + return toMsg; } std::unique_ptr<api::StorageReply> @@ -195,7 +195,7 @@ DocumentApiConverter::toStorageAPI(documentapi::DocumentReply& fromReply, toMsg->setPriority(_priConverter->toStoragePriority(fromReply.getPriority())); } } - return std::move(toMsg); + return toMsg; } std::unique_ptr<mbus::Message> @@ -318,7 +318,7 @@ DocumentApiConverter::toDocumentAPI(api::StorageCommand& fromMsg) toMsg->getTrace().setLevel(9); } } - return std::move(toMsg); + return toMsg; } void diff --git a/storage/src/vespa/storage/visiting/visitor.cpp b/storage/src/vespa/storage/visiting/visitor.cpp index 60b94b6b33e..9e927a24d9c 100644 --- a/storage/src/vespa/storage/visiting/visitor.cpp +++ b/storage/src/vespa/storage/visiting/visitor.cpp @@ -1105,8 +1105,6 @@ Visitor::getStatus(std::ostream& out, bool verbose) const out << "</table>\n"; out << "<h4>Buckets to visit</h4>"; - typedef std::pair<api::Timestamp, api::Timestamp> TimePair; - TimePair lastTime; for (uint32_t i=0; i<_buckets.size(); ++i) { out << _buckets[i] << "\n<br>"; } diff --git a/storage/src/vespa/storage/visiting/visitor.h b/storage/src/vespa/storage/visiting/visitor.h index f8da49202ad..c8d34139364 100644 --- a/storage/src/vespa/storage/visiting/visitor.h +++ b/storage/src/vespa/storage/visiting/visitor.h @@ -105,7 +105,6 @@ public: uint32_t _secondPassHits; uint64_t _secondPassBytes; const document::OrderingSpecification* _ordering; - bool _allowFirstPass; }; enum VisitorState |