diff options
96 files changed, 1013 insertions, 974 deletions
diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterStateHistoryEntry.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterStateHistoryEntry.java index e0aefe8a90a..65c0f210fd5 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterStateHistoryEntry.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterStateHistoryEntry.java @@ -3,20 +3,27 @@ package com.yahoo.vespa.clustercontroller.core; import com.yahoo.vdslib.state.ClusterState; +import java.util.Map; import java.util.Objects; +import java.util.stream.Collectors; public class ClusterStateHistoryEntry { - private final ClusterState state; + private final ClusterStateBundle state; private final long time; - ClusterStateHistoryEntry(final ClusterState state, final long time) { + ClusterStateHistoryEntry(final ClusterStateBundle state, final long time) { this.state = state; this.time = time; } - public ClusterState state() { - return state; + public ClusterState getBaselineState() { + return state.getBaselineClusterState(); + } + + public Map<String, ClusterState> getDerivedBucketSpaceStates() { + return state.getDerivedBucketSpaceStates().entrySet().stream() + .collect(Collectors.toMap(entry -> entry.getKey(), entry -> entry.getValue().getClusterState())); } public long time() { diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateVersionTracker.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateVersionTracker.java index 0673a10c282..177efe31766 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateVersionTracker.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateVersionTracker.java @@ -141,7 +141,7 @@ public class StateVersionTracker { private void recordCurrentStateInHistoryAtTime(final long currentTimeMs) { clusterStateHistory.addFirst(new ClusterStateHistoryEntry( - currentClusterState.getBaselineClusterState(), currentTimeMs)); + currentClusterState, currentTimeMs)); while (clusterStateHistory.size() > maxHistoryEntryCount) { clusterStateHistory.removeLast(); } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/LegacyIndexPageRequestHandler.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/LegacyIndexPageRequestHandler.java index e85623f068c..07195d05aa8 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/LegacyIndexPageRequestHandler.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/LegacyIndexPageRequestHandler.java @@ -3,12 +3,12 @@ package com.yahoo.vespa.clustercontroller.core.status; import com.yahoo.vdslib.state.ClusterState; import com.yahoo.vespa.clustercontroller.core.*; +import com.yahoo.vespa.clustercontroller.core.Timer; import com.yahoo.vespa.clustercontroller.core.status.statuspage.StatusPageResponse; import com.yahoo.vespa.clustercontroller.core.status.statuspage.StatusPageServer; import com.yahoo.vespa.clustercontroller.core.status.statuspage.VdsClusterHtmlRendrer; -import java.util.Iterator; -import java.util.TimeZone; +import java.util.*; /** * @author Haakon Humberset @@ -106,6 +106,7 @@ public class LegacyIndexPageRequestHandler implements StatusPageServer.RequestHa } sb.append("<table border=\"1\" cellspacing=\"0\"><tr>\n") .append(" <th>Creation date (").append(tz.getDisplayName(false, TimeZone.SHORT)).append(")</th>\n") + .append(" <th>Bucket space</th>\n") .append(" <th>Cluster state</th>\n") .append("</tr>\n"); // Write cluster state history in reverse order (newest on top) @@ -113,7 +114,7 @@ public class LegacyIndexPageRequestHandler implements StatusPageServer.RequestHa ClusterStateHistoryEntry current = null; while (stateIterator.hasNext()) { ClusterStateHistoryEntry nextEntry = stateIterator.next(); - if (nextEntry.state().isOfficial() || showLocal) { + if (nextEntry.getBaselineState().isOfficial() || showLocal) { if (current != null) writeClusterStateEntry(current, nextEntry, sb, tz); current = nextEntry; } @@ -125,21 +126,64 @@ public class LegacyIndexPageRequestHandler implements StatusPageServer.RequestHa private static void writeClusterStates(StringBuilder sb, ClusterStateBundle clusterStates) { sb.append("<p>Baseline cluster state:<br><code>").append(clusterStates.getBaselineClusterState().toString()).append("</code></p>\n"); - clusterStates.getDerivedBucketSpaceStates().entrySet().forEach(entry -> { - String bucketSpace = entry.getKey(); - ClusterState clusterState = entry.getValue().getClusterState(); - sb.append("<p>" + bucketSpace + " cluster state:<br><code>").append(clusterState.toString()).append("</code></p>\n"); + clusterStates.getDerivedBucketSpaceStates().forEach((bucketSpace, state) -> { + sb.append("<p>" + bucketSpace + " cluster state:<br><code>").append(state.getClusterState().toString()).append("</code></p>\n"); }); } private void writeClusterStateEntry(ClusterStateHistoryEntry entry, ClusterStateHistoryEntry last, StringBuilder sb, TimeZone tz) { - sb.append("<tr><td>").append(RealTimer.printDate(entry.time(), tz)) - .append("</td><td>").append(entry.state().isOfficial() ? "" : "<font color=\"grey\">"); - sb.append(entry.state()); - if (last != null) { - sb.append("<br><b>Diff</b>: ").append(last.state().getHtmlDifference(entry.state())); + List<ClusterStateTransition> derivedTransitions = calculateDerivedClusterStateTransitions(entry, last); + sb.append("<tr><td rowspan=\"" + (derivedTransitions.size() + 1) + "\">").append(RealTimer.printDate(entry.time(), tz)).append("</td>"); + writeClusterStateTransition(ClusterStateTransition.forBaseline(entry.getBaselineState(), + (last != null ? last.getBaselineState() : null)), entry.getBaselineState().isOfficial(), sb); + derivedTransitions.forEach(transition -> { + sb.append("<tr>"); + writeClusterStateTransition(transition, entry.getBaselineState().isOfficial(), sb); + }); + } + + private void writeClusterStateTransition(ClusterStateTransition transition, boolean isOfficial, StringBuilder sb) { + sb.append("<td align=\"center\">").append(transition.bucketSpace).append("</td>"); + sb.append("<td>").append(isOfficial ? "" : "<font color=\"grey\">"); + sb.append(transition.current); + if (transition.last != null) { + sb.append("<br><b>Diff</b>: ").append(transition.last.getHtmlDifference(transition.current)); + } + sb.append(isOfficial ? "" : "</font>").append("</td></tr>\n"); + } + + private List<ClusterStateTransition> calculateDerivedClusterStateTransitions(ClusterStateHistoryEntry currentEntry, + ClusterStateHistoryEntry lastEntry) { + List<ClusterStateTransition> result = new ArrayList<>(); + currentEntry.getDerivedBucketSpaceStates().forEach((bucketSpace, currentState) -> { + ClusterState lastState = (lastEntry != null ? lastEntry.getDerivedBucketSpaceStates().get(bucketSpace) : null); + if ((!currentState.equals(currentEntry.getBaselineState())) || + ((lastState != null) && (!lastState.equals(lastEntry.getBaselineState())))) { + result.add(ClusterStateTransition.forBucketSpace(currentState, lastState, bucketSpace)); + } + }); + return result; + } + + private static class ClusterStateTransition { + public final ClusterState current; + public final ClusterState last; + public final String bucketSpace; + + private ClusterStateTransition(ClusterState current, ClusterState last, String bucketSpace) { + this.current = current; + this.last = last; + this.bucketSpace = bucketSpace; + } + + public static ClusterStateTransition forBaseline(ClusterState current, ClusterState last) { + return new ClusterStateTransition(current, last, "-"); + } + + public static ClusterStateTransition forBucketSpace(ClusterState current, ClusterState last, String bucketSpace) { + return new ClusterStateTransition(current, last, bucketSpace); } - sb.append(entry.state().isOfficial() ? "" : "</font>").append("</td></tr>\n"); + } } diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateVersionTrackerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateVersionTrackerTest.java index 1cb09fcf2d1..2a3be1e7194 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateVersionTrackerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/StateVersionTrackerTest.java @@ -24,8 +24,7 @@ import static org.mockito.Mockito.when; public class StateVersionTrackerTest { private static AnnotatedClusterState stateWithoutAnnotations(String stateStr) { - final ClusterState state = ClusterState.stateFromString(stateStr); - return new AnnotatedClusterState(state, Optional.empty(), AnnotatedClusterState.emptyNodeStateReasons()); + return AnnotatedClusterState.withoutAnnotations(ClusterState.stateFromString(stateStr)); } private static ClusterStateBundle stateBundleWithoutAnnotations(String stateStr) { @@ -184,7 +183,7 @@ public class StateVersionTrackerTest { } private static ClusterStateHistoryEntry historyEntry(final String state, final long time) { - return new ClusterStateHistoryEntry(ClusterState.stateFromString(state), time); + return new ClusterStateHistoryEntry(stateBundleWithoutAnnotations(state), time); } @Test diff --git a/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java b/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java index 6c665affd1e..4096eeb0168 100644 --- a/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java +++ b/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java @@ -5,6 +5,7 @@ import com.google.common.annotations.Beta; import com.yahoo.config.ConfigInstance; import com.yahoo.config.subscription.ConfigInstanceUtil; import com.yahoo.config.model.ApplicationConfigProducerRoot; +import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.application.api.DeployLogger; import com.yahoo.log.LogLevel; import com.yahoo.text.Utf8; @@ -34,7 +35,6 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce private static final long serialVersionUID = 1L; public static final Logger log = Logger.getLogger(AbstractConfigProducer.class.getPackage().toString()); private final String subId; - private final boolean hostedVespa; private String configId = null; private List<Service> descendantServices = new ArrayList<>(); @@ -45,11 +45,14 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce private final FreezableMap<String, CHILD> childrenBySubId = new FreezableMap<>(LinkedHashMap.class); - private static boolean isHostedVespa(AbstractConfigProducer parent) { - return (parent != null) - && (parent.getRoot() != null) - && (parent.getRoot().getDeployState() != null) - && parent.getRoot().getDeployState().isHosted(); + protected static boolean stateIsHosted(DeployState deployState) { + return (deployState != null) && deployState.isHosted(); + } + + protected static DeployState deployStateFrom(AbstractConfigProducer parent) { + if (parent == null) return null; + if (parent.getRoot() == null) return null; + return parent.getRoot().getDeployState(); } /** @@ -60,8 +63,7 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce * @param subId The fragment of the config id for the producer */ public AbstractConfigProducer(AbstractConfigProducer parent, String subId) { - this(subId, isHostedVespa(parent)); - + this(subId); if (parent != null) { parent.addChild(this); } @@ -69,9 +71,6 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce protected final void setParent(AbstractConfigProducer parent) { this.parent = parent; } public final String getSubId() { return subId; } - - /** Whether this is hosted Vespa: NOTE: This cannot be trusted to be correct :-/ */ - public final boolean isHostedVespa() { return hostedVespa; } /** * Create an config producer with a configId only. Used e.g. to create root nodes, and producers @@ -80,15 +79,10 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce * @param subId The sub configId. Note that this can be prefixed when calling addChild with this producer as arg. */ public AbstractConfigProducer(String subId) { - this(subId, false); - } - - private AbstractConfigProducer(String subId, boolean hostedVespa) { if (subId.indexOf('/') != -1) { throw new IllegalArgumentException("A subId might not contain '/' : '" + subId + "'"); } this.subId = subId; - this.hostedVespa = hostedVespa; } /** diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java index 711ea885a36..b1cc6257a7a 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java @@ -19,7 +19,6 @@ import com.yahoo.vespa.model.admin.monitoring.MetricsConsumer; import com.yahoo.vespa.model.admin.monitoring.Monitoring; import com.yahoo.vespa.model.admin.monitoring.builder.Metrics; import com.yahoo.vespa.model.container.ContainerCluster; -import com.yahoo.vespa.model.filedistribution.DummyFileDistributionConfigProducer; import com.yahoo.vespa.model.filedistribution.FileDistributionConfigProducer; import com.yahoo.vespa.model.filedistribution.FileDistributionConfigProvider; import com.yahoo.vespa.model.filedistribution.FileDistributor; @@ -40,6 +39,7 @@ public class Admin extends AbstractConfigProducer implements Serializable { private static final long serialVersionUID = 1L; + private final DeployState deployState; private final Monitoring monitoring; private final Metrics metrics; private final Map<String, MetricsConsumer> legacyMetricsConsumers; @@ -73,6 +73,7 @@ public class Admin extends AbstractConfigProducer implements Serializable { boolean multitenant, FileDistributionConfigProducer fileDistributionConfigProducer) { super(parent, "admin"); + this.deployState = deployStateFrom(parent); this.monitoring = monitoring; this.metrics = metrics; this.legacyMetricsConsumers = legacyMetricsConsumers; @@ -139,6 +140,10 @@ public class Admin extends AbstractConfigProducer implements Serializable { return zooKeepersConfigProvider; } + public boolean isHostedVespa() { + return stateIsHosted(deployState); + } + public void getConfig(LogdConfig.Builder builder) { builder. logserver(new LogdConfig.Logserver.Builder(). @@ -230,14 +235,11 @@ public class Admin extends AbstractConfigProducer implements Serializable { } FileDistributionConfigProvider configProvider = - new FileDistributionConfigProvider(fileDistributor, + new FileDistributionConfigProvider(fileDistribution, + fileDistributor, host == deployHost, host.getHost()); - DummyFileDistributionConfigProducer dummyFileDistributionConfigProducer = - new DummyFileDistributionConfigProducer(fileDistribution, - host.getHost().getHostname(), - configProvider); - fileDistribution.addFileDistributionConfigProducer(host.getHost(), dummyFileDistributionConfigProducer); + fileDistribution.addFileDistributionConfigProducer(host.getHost(), configProvider); } private boolean deployHostIsMissing(HostResource deployHost) { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/AttributeChangeValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/AttributeChangeValidator.java index 26bebad93b4..93cd64b76c9 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/AttributeChangeValidator.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/AttributeChangeValidator.java @@ -89,11 +89,34 @@ public class AttributeChangeValidator { validateAttributeSetting(currAttr, nextAttr, Attribute::isFastAccess, "fast-access", result); validateAttributeSetting(currAttr, nextAttr, Attribute::isHuge, "huge", result); validateAttributeSetting(currAttr, nextAttr, Attribute::densePostingListThreshold, "dense-posting-list-threshold", result); + validateAttributeSetting(currAttr, nextAttr, Attribute::isEnabledOnlyBitVector, "rank: filter", result); } } return result; } + private static void validateAttributeSetting(Attribute currentAttr, Attribute nextAttr, + Predicate<Attribute> predicate, String setting, + List<VespaConfigChangeAction> result) { + final boolean nextValue = predicate.test(nextAttr); + if (predicate.test(currentAttr) != nextValue) { + String change = nextValue ? "add" : "remove"; + result.add(new VespaRestartAction(new ChangeMessageBuilder(nextAttr.getName()). + addChange(change + " attribute '" + setting + "'").build())); + } + } + + private static <T> void validateAttributeSetting(Attribute currentAttr, Attribute nextAttr, + Function<Attribute, T> settingValueProvider, String setting, + List<VespaConfigChangeAction> result) { + T currentValue = settingValueProvider.apply(currentAttr); + T nextValue = settingValueProvider.apply(nextAttr); + if ( ! Objects.equals(currentValue, nextValue)) { + String message = String.format("change property '%s' from '%s' to '%s'", setting, currentValue, nextValue); + result.add(new VespaRestartAction(new ChangeMessageBuilder(nextAttr.getName()).addChange(message).build())); + } + } + private List<VespaConfigChangeAction> validateTensorTypes(final ValidationOverrides overrides, Instant now) { final List<VespaConfigChangeAction> result = new ArrayList<>(); @@ -128,26 +151,4 @@ public class AttributeChangeValidator { nextAttr.tensorType().get().toString()).build(), now); } - private static void validateAttributeSetting(Attribute currentAttr, Attribute nextAttr, - Predicate<Attribute> predicate, String setting, - List<VespaConfigChangeAction> result) { - final boolean nextValue = predicate.test(nextAttr); - if (predicate.test(currentAttr) != nextValue) { - String change = nextValue ? "add" : "remove"; - result.add(new VespaRestartAction(new ChangeMessageBuilder(nextAttr.getName()). - addChange(change + " attribute '" + setting + "'").build())); - } - } - - private static <T> void validateAttributeSetting(Attribute currentAttr, Attribute nextAttr, - Function<Attribute, T> settingValueProvider, String setting, - List<VespaConfigChangeAction> result) { - T currentValue = settingValueProvider.apply(currentAttr); - T nextValue = settingValueProvider.apply(nextAttr); - if ( ! Objects.equals(currentValue, nextValue)) { - String message = String.format("change property '%s' from '%s' to '%s'", setting, currentValue, nextValue); - result.add(new VespaRestartAction(new ChangeMessageBuilder(nextAttr.getName()).addChange(message).build())); - } - } - } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminBuilderBase.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminBuilderBase.java index a78e9ad30fc..96e120cae92 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminBuilderBase.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminBuilderBase.java @@ -25,9 +25,8 @@ import static com.yahoo.vespa.model.admin.monitoring.builder.PredefinedMetricSet /** * A base class for admin model builders, to support common functionality across versions. * - * @author lulf - * @author vegardh - * @since 5.12 + * @author Ulf Lilleengen + * @author Vegard Havdal */ public abstract class DomAdminBuilderBase extends VespaDomBuilder.DomConfigProducerBuilder<Admin> { @@ -80,7 +79,7 @@ public abstract class DomAdminBuilderBase extends VespaDomBuilder.DomConfigProdu } private FileDistributionConfigProducer getFileDistributionConfigProducer(AbstractConfigProducer parent) { - return new FileDistributionConfigProducer.Builder().build(parent, fileRegistry, configServerSpecs); + return new FileDistributionConfigProducer(parent, fileRegistry, configServerSpecs); } private Element getChildWithFallback(Element parent, String childName, String alternativeChildName) { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java index 5925ec978bb..5b07a966a82 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java @@ -3,6 +3,7 @@ package com.yahoo.vespa.model.container; import com.yahoo.component.ComponentId; import com.yahoo.component.ComponentSpecification; +import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.producer.AbstractConfigProducer; import com.yahoo.container.ComponentsConfig; import com.yahoo.container.QrConfig; @@ -55,6 +56,8 @@ public class Container extends AbstractService implements private final AbstractConfigProducer parent; private final String name; + private final DeployState deployState; + private String clusterName = null; private boolean rpcServerEnabled = true; @@ -93,6 +96,7 @@ public class Container extends AbstractService implements super(parent, name); this.name = name; this.parent = parent; + this.deployState = deployStateFrom(parent); this.portOverrides = Collections.unmodifiableList(new ArrayList<>(portOverrides)); this.retired = retired; this.index = index; @@ -309,6 +313,10 @@ public class Container extends AbstractService implements } } + private boolean isHostedVespa() { + return stateIsHosted(deployState); + } + /** Returns the jvm arguments this should start with */ @Override public String getJvmArgs() { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java index 4684cf5c2f0..d7ce3a9a975 100755 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java @@ -12,7 +12,9 @@ import com.yahoo.config.application.api.ComponentInfo; import com.yahoo.config.docproc.DocprocConfig; import com.yahoo.config.docproc.SchemamappingConfig; import com.yahoo.config.model.ApplicationConfigProducerRoot; +import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.producer.AbstractConfigProducer; +import com.yahoo.config.model.producer.AbstractConfigProducerRoot; import com.yahoo.config.provision.Zone; import com.yahoo.container.BundlesConfig; import com.yahoo.container.ComponentsConfig; @@ -82,6 +84,7 @@ import com.yahoo.vespaclient.config.FeederConfig; import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; + import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; @@ -171,6 +174,7 @@ public final class ContainerCluster private final ConfigProducerGroup<RestApi> restApiGroup; private final ConfigProducerGroup<Servlet> servletGroup; private final ContainerClusterVerifier clusterVerifier; + private final DeployState deployState; private Map<String, String> concreteDocumentTypes = new LinkedHashMap<>(); private MetricDefaultsConfig.Factory.Enum defaultMetricConsumerFactory; @@ -203,8 +207,9 @@ public final class ContainerCluster public ContainerCluster(AbstractConfigProducer<?> parent, String subId, String name, ContainerClusterVerifier verifier) { super(parent, subId); this.clusterVerifier = verifier; + this.deployState = deployStateFrom(parent); this.name = name; - this.zone = getRoot() != null ? getRoot().getDeployState().zone() : Zone.defaultZone(); + this.zone = (deployState != null) ? deployState.zone() : Zone.defaultZone(); componentGroup = new ComponentGroup<>(this, "component"); restApiGroup = new ConfigProducerGroup<>(this, "rest-api"); servletGroup = new ConfigProducerGroup<>(this, "servlet"); @@ -232,6 +237,8 @@ public final class ContainerCluster addJaxProviders(); } + public DeployState getDeployState() { return deployState; } + public void setZone(Zone zone) { this.zone = zone; } @@ -763,6 +770,10 @@ public final class ContainerCluster this.defaultMetricConsumerFactory = defaultMetricConsumerFactory; } + public boolean isHostedVespa() { + return stateIsHosted(deployState); + } + @Override public void getConfig(RoutingProviderConfig.Builder builder) { builder.enabled(isHostedVespa()); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/AccessLogComponent.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/AccessLogComponent.java index 7da7abe0546..ccb4e271102 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/AccessLogComponent.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/AccessLogComponent.java @@ -7,6 +7,7 @@ import com.yahoo.container.logging.YApacheAccessLog; import com.yahoo.container.logging.JSONAccessLog; import com.yahoo.osgi.provider.model.ComponentModel; import edu.umd.cs.findbugs.annotations.Nullable; +import com.yahoo.config.model.deploy.DeployState; import static com.yahoo.container.core.AccessLogConfig.FileHandler.RotateScheme; @@ -23,12 +24,13 @@ public final class AccessLogComponent extends SimpleComponent implements AccessL private final String rotationInterval; private final RotateScheme.Enum rotationScheme; private final Boolean compression; + private final DeployState deployState; private final String symlinkName; public AccessLogComponent(AccessLogType logType, String clusterName) { this(logType, String.format("logs/vespa/qrs/%s.%s.%s", capitalize(logType.name()), clusterName, "%Y%m%d%H%M%S"), - null, null, null, + null, null, null, null, capitalize(logType.name()) + "." + clusterName); } @@ -41,6 +43,7 @@ public final class AccessLogComponent extends SimpleComponent implements AccessL String rotationInterval, RotateScheme.Enum rotationScheme, Boolean compressOnRotation, + DeployState deployState, String symlinkName) { super(new ComponentModel(accessLogClass(logType), null, "container-core", null)); @@ -48,6 +51,7 @@ public final class AccessLogComponent extends SimpleComponent implements AccessL this.rotationInterval = rotationInterval; this.rotationScheme = rotationScheme; this.compression = compressOnRotation; + this.deployState = deployState; this.symlinkName = symlinkName; if (fileNamePattern == null) @@ -72,6 +76,10 @@ public final class AccessLogComponent extends SimpleComponent implements AccessL builder.fileHandler(fileHandlerConfig()); } + private boolean isHostedVespa() { + return stateIsHosted(deployState); + } + private AccessLogConfig.FileHandler.Builder fileHandlerConfig() { AccessLogConfig.FileHandler.Builder builder = new AccessLogConfig.FileHandler.Builder(); if (fileNamePattern != null) diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/AccessLogBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/AccessLogBuilder.java index 1b15a49462e..4f51933d8bb 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/AccessLogBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/AccessLogBuilder.java @@ -7,6 +7,7 @@ import com.yahoo.vespa.model.builder.xml.dom.VespaDomBuilder; import com.yahoo.vespa.model.container.ContainerCluster; import com.yahoo.vespa.model.container.component.AccessLogComponent; import com.yahoo.vespa.model.container.component.AccessLogComponent.AccessLogType; +import com.yahoo.config.model.deploy.DeployState; import org.w3c.dom.Element; import java.util.Optional; @@ -45,9 +46,11 @@ public class AccessLogBuilder { private static class DomBuilder extends VespaDomBuilder.DomConfigProducerBuilder<AccessLogComponent> { private final AccessLogType accessLogType; + private final DeployState deployState; - public DomBuilder(AccessLogType accessLogType) { + public DomBuilder(AccessLogType accessLogType, DeployState deployState) { this.accessLogType = accessLogType; + this.deployState = deployState; } @Override @@ -58,6 +61,7 @@ public class AccessLogBuilder { rotationInterval(spec), rotationScheme(spec), compressOnRotation(spec), + deployState, symlinkName(spec)); } @@ -83,23 +87,31 @@ public class AccessLogBuilder { } } - public static Optional<AccessLogComponent> buildIfNotDisabled(ContainerCluster cluster, Element accessLogSpec) { - AccessLogTypeLiteral typeLiteral = - getOptionalAttribute(accessLogSpec, "type"). - map(AccessLogTypeLiteral::fromAttributeValue). - orElse(AccessLogTypeLiteral.VESPA); - + private static AccessLogType logTypeFor(AccessLogTypeLiteral typeLiteral) { switch (typeLiteral) { case DISABLED: - return Optional.empty(); + return null; case VESPA: - return Optional.of(new DomBuilder(AccessLogType.queryAccessLog).build(cluster, accessLogSpec)); + return AccessLogType.queryAccessLog; case YAPACHE: - return Optional.of(new DomBuilder(AccessLogType.yApacheAccessLog).build(cluster, accessLogSpec)); + return AccessLogType.yApacheAccessLog; case JSON: - return Optional.of(new DomBuilder(AccessLogType.jsonAccessLog).build(cluster, accessLogSpec)); + return AccessLogType.jsonAccessLog; default: throw new InconsistentSchemaAndCodeError(); } } + + public static Optional<AccessLogComponent> buildIfNotDisabled(ContainerCluster cluster, Element accessLogSpec) { + AccessLogTypeLiteral typeLiteral = + getOptionalAttribute(accessLogSpec, "type"). + map(AccessLogTypeLiteral::fromAttributeValue). + orElse(AccessLogTypeLiteral.VESPA); + AccessLogType logType = logTypeFor(typeLiteral); + if (logType == null) { + return Optional.empty(); + } + DeployState deployState = cluster.getDeployState(); + return Optional.of(new DomBuilder(logType, deployState).build(cluster, accessLogSpec)); + } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java index a8e89af0a42..fabd7a0cc1a 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java @@ -16,6 +16,7 @@ import com.yahoo.vespa.config.content.core.StorDistributormanagerConfig; import com.yahoo.documentmodel.NewDocumentType; import com.yahoo.documentapi.messagebus.protocol.DocumentProtocol; import com.yahoo.metrics.MetricsmanagerConfig; +import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.producer.AbstractConfigProducer; import com.yahoo.vespa.model.HostResource; import com.yahoo.vespa.model.Service; @@ -64,6 +65,7 @@ public class ContentCluster extends AbstractConfigProducer implements StorDistri // TODO: Make private private String documentSelection; ContentSearchCluster search; + private final DeployState deployState; private final Map<String, NewDocumentType> documentDefinitions; private final Set<NewDocumentType> globallyDistributedDocuments; // Experimental flag (TODO: remove when feature is enabled by default) @@ -488,6 +490,7 @@ public class ContentCluster extends AbstractConfigProducer implements StorDistri Redundancy redundancy, Zone zone) { super(parent, clusterName); + this.deployState = deployStateFrom(parent); this.clusterName = clusterName; this.documentDefinitions = documentDefinitions; this.globallyDistributedDocuments = globallyDistributedDocuments; @@ -631,6 +634,10 @@ public class ContentCluster extends AbstractConfigProducer implements StorDistri } } + public boolean isHostedVespa() { + return stateIsHosted(deployState); + } + @Override public void validate() throws Exception { super.validate(); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/DummyFileDistributionConfigProducer.java b/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/DummyFileDistributionConfigProducer.java deleted file mode 100644 index d4993d01d1c..00000000000 --- a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/DummyFileDistributionConfigProducer.java +++ /dev/null @@ -1,35 +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.model.filedistribution; - -import com.yahoo.cloud.config.filedistribution.FiledistributorrpcConfig; -import com.yahoo.cloud.config.filedistribution.FilereferencesConfig; -import com.yahoo.config.model.producer.AbstractConfigProducer; - -/** - * @author hmusum - * <p> - * Dummy file distribution config producer, needed for serving file distribution config when there is no FiledistributorService. - */ -public class DummyFileDistributionConfigProducer extends AbstractConfigProducer implements - FiledistributorrpcConfig.Producer, - FilereferencesConfig.Producer { - - private final FileDistributionConfigProvider configProvider; - - public DummyFileDistributionConfigProducer(AbstractConfigProducer parent, - String hostname, - FileDistributionConfigProvider configProvider) { - super(parent, hostname); - this.configProvider = configProvider; - } - - @Override - public void getConfig(FiledistributorrpcConfig.Builder builder) { - configProvider.getConfig(builder); - } - - @Override - public void getConfig(FilereferencesConfig.Builder builder) { - configProvider.getConfig(builder); - } -} diff --git a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileDistributionConfigProducer.java b/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileDistributionConfigProducer.java index 9441b787b09..9662540e8df 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileDistributionConfigProducer.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileDistributionConfigProducer.java @@ -13,13 +13,17 @@ import java.util.Map; /** * @author hmusum * <p> - * File distribution config producer, delegates getting config to {@link DummyFileDistributionConfigProducer} (one per host) + * File distribution config producer, delegates getting config to {@link FileDistributionConfigProvider} (one per host) */ public class FileDistributionConfigProducer extends AbstractConfigProducer { - private final Map<Host, AbstractConfigProducer> fileDistributionConfigProducers = new IdentityHashMap<>(); + private final Map<Host, FileDistributionConfigProvider> fileDistributionConfigProviders = new IdentityHashMap<>(); private final FileDistributor fileDistributor; + public FileDistributionConfigProducer(AbstractConfigProducer ancestor, FileRegistry fileRegistry, List<ConfigServerSpec> configServerSpec) { + this(ancestor, new FileDistributor(fileRegistry, configServerSpec)); + } + private FileDistributionConfigProducer(AbstractConfigProducer parent, FileDistributor fileDistributor) { super(parent, "filedistribution"); this.fileDistributor = fileDistributor; @@ -29,20 +33,12 @@ public class FileDistributionConfigProducer extends AbstractConfigProducer { return fileDistributor; } - public void addFileDistributionConfigProducer(Host host, AbstractConfigProducer fileDistributionConfigProducer) { - fileDistributionConfigProducers.put(host, fileDistributionConfigProducer); - } - - public static class Builder { - - public FileDistributionConfigProducer build(AbstractConfigProducer ancestor, FileRegistry fileRegistry, List<ConfigServerSpec> configServerSpec) { - FileDistributor fileDistributor = new FileDistributor(fileRegistry, configServerSpec); - return new FileDistributionConfigProducer(ancestor, fileDistributor); - } + public void addFileDistributionConfigProducer(Host host, FileDistributionConfigProvider fileDistributionConfigProvider) { + fileDistributionConfigProviders.put(host, fileDistributionConfigProvider); } - public AbstractConfigProducer getConfigProducer(Host host) { - return fileDistributionConfigProducers.get(host); + public FileDistributionConfigProvider getConfigProducer(Host host) { + return fileDistributionConfigProviders.get(host); } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileDistributionConfigProvider.java b/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileDistributionConfigProvider.java index 34e242400d3..47ee546181a 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileDistributionConfigProvider.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/filedistribution/FileDistributionConfigProvider.java @@ -4,29 +4,35 @@ package com.yahoo.vespa.model.filedistribution; import com.yahoo.cloud.config.filedistribution.FiledistributorrpcConfig; import com.yahoo.cloud.config.filedistribution.FilereferencesConfig; import com.yahoo.config.FileReference; +import com.yahoo.config.model.producer.AbstractConfigProducer; import com.yahoo.vespa.model.ConfigProxy; import com.yahoo.vespa.model.Host; import java.util.Collection; -public class FileDistributionConfigProvider { +public class FileDistributionConfigProvider extends AbstractConfigProducer + implements FiledistributorrpcConfig.Producer, FilereferencesConfig.Producer { private final FileDistributor fileDistributor; private final boolean sendAllFiles; private final Host host; - public FileDistributionConfigProvider(FileDistributor fileDistributor, + public FileDistributionConfigProvider(AbstractConfigProducer parent, + FileDistributor fileDistributor, boolean sendAllFiles, Host host) { + super(parent, host.getHostname()); this.fileDistributor = fileDistributor; this.sendAllFiles = sendAllFiles; this.host = host; } + @Override public void getConfig(FiledistributorrpcConfig.Builder builder) { builder.connectionspec("tcp/" + host.getHostname() + ":" + ConfigProxy.BASEPORT); } + @Override public void getConfig(FilereferencesConfig.Builder builder) { for (FileReference reference : getFileReferences()) { builder.filereferences(reference.value()); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/search/SearchNode.java b/config-model/src/main/java/com/yahoo/vespa/model/search/SearchNode.java index df5c2c9c173..924c282d2e5 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/search/SearchNode.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/search/SearchNode.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.model.search; import com.yahoo.cloud.config.filedistribution.FiledistributorrpcConfig; +import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.producer.AbstractConfigProducer; import com.yahoo.metrics.MetricsmanagerConfig; import com.yahoo.searchlib.TranslogserverConfig; @@ -19,8 +20,8 @@ import com.yahoo.vespa.model.admin.monitoring.Monitoring; import com.yahoo.vespa.model.application.validation.RestartConfigs; import com.yahoo.vespa.model.builder.xml.dom.VespaDomBuilder; import com.yahoo.vespa.model.content.ContentNode; -import com.yahoo.vespa.model.filedistribution.DummyFileDistributionConfigProducer; import com.yahoo.vespa.model.filedistribution.FileDistributionConfigProducer; +import com.yahoo.vespa.model.filedistribution.FileDistributionConfigProvider; import org.w3c.dom.Element; import java.util.HashMap; @@ -49,6 +50,7 @@ public class SearchNode extends AbstractService implements TranslogserverConfig.Producer { private static final long serialVersionUID = 1L; + private final DeployState deployState; private final boolean flushOnShutdown; private NodeSpec nodeSpec; private int distributionKey; @@ -103,6 +105,7 @@ public class SearchNode extends AbstractService implements private SearchNode(AbstractConfigProducer parent, String name, NodeSpec nodeSpec, String clusterName, boolean flushOnShutdown, Optional<Tuning> tuning) { super(parent, name); + this.deployState = deployStateFrom(parent); this.nodeSpec = nodeSpec; this.clusterName = clusterName; this.flushOnShutdown = flushOnShutdown; @@ -228,11 +231,15 @@ public class SearchNode extends AbstractService implements public void getConfig(FiledistributorrpcConfig.Builder builder) { FileDistributionConfigProducer fileDistribution = getRoot().getFileDistributionConfigProducer(); if (fileDistribution != null) { - AbstractConfigProducer configProducer = fileDistribution.getConfigProducer(getHost()); - ((DummyFileDistributionConfigProducer) configProducer).getConfig(builder); + FileDistributionConfigProvider configProducer = fileDistribution.getConfigProducer(getHost()); + configProducer.getConfig(builder); } } + private boolean isHostedVespa() { + return stateIsHosted(deployState); + } + @Override public void getConfig(ProtonConfig.Builder builder) { builder. diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/AttributeChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/AttributeChangeValidatorTest.java index eab12db20ef..79bbde01898 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/AttributeChangeValidatorTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/AttributeChangeValidatorTest.java @@ -35,7 +35,7 @@ public class AttributeChangeValidatorTest { } @Test - public void requireThatAddingAttributeAspectRequireRestart() throws Exception { + public void adding_attribute_aspect_require_restart() throws Exception { Fixture f = new Fixture("field f1 type string { indexing: summary }", "field f1 type string { indexing: attribute | summary }"); f.assertValidation(newRestartAction( @@ -43,7 +43,7 @@ public class AttributeChangeValidatorTest { } @Test - public void requireThatRemovingAttributeAspectRequireRestart() throws Exception { + public void removing_attribute_aspect_require_restart() throws Exception { Fixture f = new Fixture("field f1 type string { indexing: attribute | summary }", "field f1 type string { indexing: summary }"); f.assertValidation(newRestartAction( @@ -51,19 +51,19 @@ public class AttributeChangeValidatorTest { } @Test - public void requireThatAddingAttributeFieldIsOk() throws Exception { + public void adding_attribute_field_is_ok() throws Exception { Fixture f = new Fixture("", "field f1 type string { indexing: attribute | summary \n attribute: fast-search }"); f.assertValidation(); } @Test - public void requireThatRemovingAttributeFieldIsOk() throws Exception { + public void removing_attribute_field_is_ok() throws Exception { Fixture f = new Fixture("field f1 type string { indexing: attribute | summary }", ""); f.assertValidation(); } @Test - public void requireThatChangingFastSearchRequireRestart() throws Exception { + public void changing_fast_search_require_restart() throws Exception { new Fixture("field f1 type string { indexing: attribute }", "field f1 type string { indexing: attribute \n attribute: fast-search }"). assertValidation(newRestartAction( @@ -71,7 +71,7 @@ public class AttributeChangeValidatorTest { } @Test - public void requireThatChangingFastAccessRequireRestart() throws Exception { + public void changing_fast_access_require_restart() throws Exception { new Fixture("field f1 type string { indexing: attribute \n attribute: fast-access }", "field f1 type string { indexing: attribute }"). assertValidation(newRestartAction( @@ -79,7 +79,7 @@ public class AttributeChangeValidatorTest { } @Test - public void requireThatChangingHugeRequireRestart() throws Exception { + public void changing_huge_require_restart() throws Exception { new Fixture("field f1 type string { indexing: attribute }", "field f1 type string { indexing: attribute \n attribute: huge }"). assertValidation(newRestartAction( @@ -87,7 +87,7 @@ public class AttributeChangeValidatorTest { } @Test - public void requireThatChangingDensePostingListThresholdRequireRestart() throws Exception { + public void changing_dense_posting_list_threshold_require_restart() throws Exception { new Fixture( "field f1 type predicate { indexing: attribute \n index { arity: 8 \n dense-posting-list-threshold: 0.2 } }", "field f1 type predicate { indexing: attribute \n index { arity: 8 \n dense-posting-list-threshold: 0.4 } }"). @@ -96,21 +96,21 @@ public class AttributeChangeValidatorTest { } @Test - public void requireThatRemovingAttributeAspectFromIndexFieldIsOk() throws Exception { + public void removing_attribute_aspect_from_index_field_is_ok() throws Exception { Fixture f = new Fixture("field f1 type string { indexing: index | attribute }", "field f1 type string { indexing: index }"); f.assertValidation(); } @Test - public void requireThatRemovingAttributeAspectFromIndexAndSummaryFieldIsOk() throws Exception { + public void removing_attribute_aspect_from_index_and_summary_field_is_ok() throws Exception { Fixture f = new Fixture("field f1 type string { indexing: index | attribute | summary }", "field f1 type string { indexing: index | summary }"); f.assertValidation(); } @Test - public void requireThatChangingTensorTypeOfTensorFieldRequiresRefeed() throws Exception { + public void changing_tensor_type_of_tensor_field_requires_refeed() throws Exception { new Fixture( "field f1 type tensor(x[]) { indexing: attribute \n attribute: tensor(x[100]) }", "field f1 type tensor(y[]) { indexing: attribute \n attribute: tensor(y[]) }") @@ -121,7 +121,7 @@ public class AttributeChangeValidatorTest { } @Test - public void requireThatCompatibleTensorTypeChangeIsOk() throws Exception { + public void compatible_tensor_type_change_is_ok() throws Exception { new Fixture( "field f1 type tensor(x[],y[]) { indexing: attribute \n attribute: tensor(x[104], y[52]) }", "field f1 type tensor(x[200],y[]) { indexing: attribute \n attribute: tensor(x[104], y[52]) }") @@ -129,7 +129,7 @@ public class AttributeChangeValidatorTest { } @Test - public void requireIncompatibleTensorTypeChangeIsNotOk() throws Exception { + public void incompatible_tensor_type_change_is_not_ok() throws Exception { try { new Fixture( "field f1 type tensor(x[],y[]) { indexing: attribute \n attribute: tensor(x[104], y[52]) }", @@ -141,4 +141,20 @@ public class AttributeChangeValidatorTest { } } + @Test + public void adding_rank_filter_requires_restart() throws Exception { + new Fixture("field f1 type string { indexing: attribute }", + "field f1 type string { indexing: attribute \n rank: filter }"). + assertValidation(newRestartAction( + "Field 'f1' changed: add attribute 'rank: filter'")); + } + + @Test + public void removing_rank_filter_requires_restart() throws Exception { + new Fixture("field f1 type string { indexing: attribute \n rank: filter }", + "field f1 type string { indexing: attribute }"). + assertValidation(newRestartAction( + "Field 'f1' changed: remove attribute 'rank: filter'")); + } + } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/utils/ContentClusterUtils.java b/config-model/src/test/java/com/yahoo/vespa/model/content/utils/ContentClusterUtils.java index 15acdf0737b..8e0c0d0b253 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/content/utils/ContentClusterUtils.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/content/utils/ContentClusterUtils.java @@ -60,8 +60,7 @@ public class ContentClusterUtils { public static ContentCluster createCluster(String clusterXml, MockRoot root) { Document doc = XML.getDocument(clusterXml); Admin admin = new Admin(root, new DefaultMonitoring("vespa", 60), new Metrics(), Collections.emptyMap(), false, - new FileDistributionConfigProducer.Builder() - .build(root, new MockFileRegistry(), null)); + new FileDistributionConfigProducer(root, new MockFileRegistry(), null)); ConfigModelContext context = ConfigModelContext.create(null, root.getDeployState(), null, root, null); return new ContentCluster.Builder(admin).build(Collections.emptyList(), context, doc.getDocumentElement()); diff --git a/config-provisioning/src/main/resources/configdefinitions/node-repository.def b/config-provisioning/src/main/resources/configdefinitions/node-repository.def index 7a08ae63e44..8ea9265aa23 100644 --- a/config-provisioning/src/main/resources/configdefinitions/node-repository.def +++ b/config-provisioning/src/main/resources/configdefinitions/node-repository.def @@ -4,3 +4,5 @@ namespace=config.provisioning # Docker image to use in REST API responses. This must be a fully qualified name, including registry, but excluding # version. Example: my-docker-registry.domain.tld:8080/dist/vespa dockerImage string default="dummyImage" + +useCuratorClientCache bool default=false diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java index c7a79556397..faf25a16321 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/ConfigCurator.java @@ -88,7 +88,7 @@ public class ConfigCurator { @Inject public ConfigCurator(Curator curator, ZooKeeperServer server) { - this(curator, server.getConfig().juteMaxBuffer()); + this(curator, server.getZookeeperServerConfig().juteMaxBuffer()); } private ConfigCurator(Curator curator, int maxNodeSize) { 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 9e4e5b19f64..1d3fff57a78 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 @@ -307,9 +307,12 @@ public class ApplicationController { // Clean up deployment jobs that are no longer referenced by deployment spec application = deleteUnreferencedDeploymentJobs(application); + // TODO jvenstad: Store triggering information here, including versions, when job status is read from the build service. + store(application); // store missing information even if we fail deployment below } + // TODO jvenstad: Use code from DeploymentTrigger? Also, validate application version. // Validate the change being deployed if (!canDeployDirectly) { validateChange(application, zone, version); @@ -550,6 +553,11 @@ public class ApplicationController { } public void notifyJobCompletion(JobReport report) { + log.log(Level.INFO, String.format("Notified of %s of %s %d for '%s'.", + report.jobError().map(error -> error + " failure").orElse("success"), + report.jobType(), + report.buildNumber(), + report.applicationId())); if ( ! get(report.applicationId()).isPresent()) { log.log(Level.WARNING, "Ignoring completion of job of project '" + report.projectId() + "': Unknown application '" + report.applicationId() + "'"); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java index 10fc1b41733..d5d7cf1c0ef 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java @@ -80,6 +80,7 @@ public class JobStatus { ", but that has neither been triggered nor deployed"); } else { + // TODO jvenstad: This is wrong, because triggering versions are not necessarily the same as deployed versions! version = lastTriggered.get().version(); applicationVersion = lastTriggered.get().applicationVersion(); reason = lastTriggered.get().reason(); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java index f59e0fbce5c..1208eac74df 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java @@ -12,6 +12,7 @@ import com.yahoo.vespa.athenz.api.NToken; import com.yahoo.vespa.hosted.controller.api.identifiers.UserId; import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsKeystore; import com.yahoo.vespa.hosted.controller.athenz.config.AthenzConfig; +import com.yahoo.yolean.chain.After; import java.security.Principal; import java.util.Optional; @@ -29,6 +30,7 @@ import static com.yahoo.vespa.hosted.controller.restapi.filter.SecurityFilterUti * @author bjorncs */ // TODO Remove this filter once migrated to Okta +@After({"AccessControlRequestFilter", "BouncerFilter"}) public class UserAuthWithAthenzPrincipalFilter extends AthenzPrincipalFilter { private static final Logger log = Logger.getLogger(UserAuthWithAthenzPrincipalFilter.class.getName()); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentQueue.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentQueue.java index 385de7b5a30..9e33da674aa 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentQueue.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentQueue.java @@ -33,6 +33,7 @@ public class DeploymentQueue { /** Add the given application to the queue of the given job type -- in front if first, at the back otherwise. */ public void addJob(ApplicationId applicationId, JobType jobType, boolean first) { + // TODO jvenstad: Replace with direct triggering. locked(jobType, queue -> { if ( ! queue.contains(applicationId)) { if (first) 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 0f27131aaad..f6f65df56b7 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 @@ -22,11 +22,13 @@ import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; import java.time.Clock; import java.time.Duration; import java.time.Instant; -import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.logging.Logger; +import java.util.stream.Stream; /** * Responsible for scheduling deployment jobs in a build system and keeping @@ -85,17 +87,12 @@ public class DeploymentTrigger { // Handle successful starting and ending if (report.jobType() == JobType.component) { if (report.success()) { - if ( ! acceptNewApplicationVersionNow(application)) { - applications().store(application.withOutstandingChange(Change.of(applicationVersion))); - return; - } - // Note that in case of an ongoing upgrade this may result in both the upgrade and application - // change being deployed together - application = application.withChange(application.change().with(applicationVersion)); - } - else { // don't re-trigger component on failure - applications().store(application); - return; + if ( ! acceptNewApplicationVersionNow(application)) + application = application.withOutstandingChange(Change.of(applicationVersion)); + else + // Note that in case of an ongoing upgrade this may result in both the upgrade and application + // change being deployed together + application = application.withChange(application.change().with(applicationVersion)); } } else if (report.jobType().isProduction() && deploymentComplete(application)) { @@ -104,29 +101,10 @@ public class DeploymentTrigger { application = application.withChange(Change.empty()); } - // Trigger next - if (report.success()) - application = trigger(order.nextAfter(report.jobType(), application), application, - report.jobType().jobName() + " completed"); - else if (retryBecauseOutOfCapacity(application, report.jobType())) - application = trigger(report.jobType(), application, true, - "Retrying on out of capacity"); - else if (retryBecauseNewFailure(application, report.jobType())) - application = trigger(report.jobType(), application, false, - "Immediate retry on failure"); - applications().store(application); }); } - /** Returns whether all production zones listed in deployment spec has this change (or a newer version, if upgrade) */ - private boolean deploymentComplete(LockedApplication application) { - return order.jobsFrom(application.deploymentSpec()).stream() - .filter(JobType::isProduction) - .filter(job -> job.zone(controller.system()).isPresent()) - .allMatch(job -> changeDeployed(application, job)); - } - /** * Find jobs that can and should run but are currently not. */ @@ -137,87 +115,42 @@ public class DeploymentTrigger { applications().lockIfPresent(application.id(), this::triggerReadyJobs); } - /** Find the next step to trigger if any, and triggers it */ - public void triggerReadyJobs(LockedApplication application) { - if ( ! application.change().isPresent()) return; - List<JobType> jobs = order.jobsFrom(application.deploymentSpec()); - - // Should the first step be triggered? - if ( ! jobs.isEmpty() && jobs.get(0).equals(JobType.systemTest) ) { - JobStatus systemTestStatus = application.deploymentJobs().jobStatus().get(JobType.systemTest); - if (application.change().platform().isPresent()) { - Version target = application.change().platform().get(); - if (systemTestStatus == null - || ! systemTestStatus.lastTriggered().isPresent() - || ! systemTestStatus.isSuccess() - || ! systemTestStatus.lastTriggered().get().version().equals(target) - || systemTestStatus.isHanging(jobTimeoutLimit())) { - application = trigger(JobType.systemTest, application, false, "Upgrade to " + target); - controller.applications().store(application); - } - } - else { - JobStatus componentStatus = application.deploymentJobs().jobStatus().get(JobType.component); - if (componentStatus != null && changesAvailable(application, componentStatus, systemTestStatus)) { - application = trigger(JobType.systemTest, application, false, "Available change in component"); - controller.applications().store(application); - } - } - } - - // Find next steps to trigger based on the state of the previous step - for (JobType jobType : jobs) { - JobStatus jobStatus = application.deploymentJobs().jobStatus().get(jobType); - if (jobStatus == null) continue; // job has never run - if (jobStatus.isRunning(jobTimeoutLimit())) continue; - - // Collect the subset of next jobs which have not run with the last changes - List<JobType> nextToTrigger = new ArrayList<>(); - for (JobType nextJobType : order.nextAfter(jobType, application)) { - JobStatus nextStatus = application.deploymentJobs().jobStatus().get(nextJobType); - if (changesAvailable(application, jobStatus, nextStatus) || nextStatus.isHanging(jobTimeoutLimit())) - nextToTrigger.add(nextJobType); - } - // Trigger them in parallel - application = trigger(nextToTrigger, application, "Available change in " + jobType.jobName()); - controller.applications().store(application); - } - } - /** - * Returns true if the previous job has completed successfully with a application version and/or Vespa version - * which is newer (different) than the one last completed successfully in next + * Trigger a job for an application, if allowed + * + * @param triggering the triggering to execute, i.e., application, job type and reason + * @param concurrentlyWith production jobs that may run concurrently with the job to trigger + * @param force true to disable checks which should normally prevent this triggering from happening + * @return the application in the triggered state, if actually triggered. This *must* be stored by the caller */ - private boolean changesAvailable(Application application, JobStatus previous, JobStatus next) { - if ( ! application.change().isPresent()) return false; - if (next == null) return true; + public LockedApplication trigger(Triggering triggering, Collection<JobType> concurrentlyWith, boolean force) { + if (triggering.jobType == null) return triggering.application; // we are passed null when the last job has been reached - if (next.type().isTest()) { - // Is it not yet this job's turn to upgrade? - if ( ! lastSuccessfulIs(application.change(), previous.type(), application)) - return false; + List<JobType> runningProductionJobs = JobList.from(triggering.application) + .production() + .running(jobTimeoutLimit()) + .mapToList(JobStatus::type); + if ( ! force && triggering.jobType().isProduction() && ! concurrentlyWith.containsAll(runningProductionJobs)) + return triggering.application; - // Has the upgrade test already been done? - if (lastSuccessfulIs(application.change(), next.type(), application)) - return false; - } - else if (next.type().isProduction()) { - // Is the target version tested? - if ( ! lastSuccessfulIs(application.change(), JobType.stagingTest, application)) - return false; - - // Is the previous a job production which neither succeed with the target version, nor has a higher version? - if (previous.type().isProduction() && ! changeDeployed(application, previous.type())) - return false; - - // Did the next job already succeed on the target version, or does it already have a higher version? - if (changeDeployed(application, next.type())) - return false; + // Never allow untested changes to go through + // Note that this may happen because a new change catches up and prevents an older one from continuing + if ( ! triggering.application.deploymentJobs().isDeployableTo(triggering.jobType.environment(), triggering.application.change())) { + log.warning(String.format("Want to trigger %s for %s with reason %s, but change is untested", triggering.jobType, + triggering.application, triggering.reason)); + return triggering.application; } - else - throw new IllegalStateException("Unclassified type of next job: " + next); - return true; + if ( ! force && ! allowedTriggering(triggering.jobType, triggering.application)) return triggering.application; + log.info(triggering.toString()); + deploymentQueue.addJob(triggering.application.id(), triggering.jobType, triggering.retry); + // TODO jvenstad: Let triggering set only time of triggering (and reason, for debugging?) when build system is polled for job status. + return triggering.application.withJobTriggering(triggering.jobType, + clock.instant(), + triggering.application.deployVersionFor(triggering.jobType, controller), + triggering.application.deployApplicationVersionFor(triggering.jobType, controller, false) + .orElse(ApplicationVersion.unknown), + triggering.reason); } /** @@ -234,7 +167,7 @@ public class DeploymentTrigger { application = application.withChange(change); if (change.application().isPresent()) application = application.withOutstandingChange(Change.empty()); - application = trigger(JobType.systemTest, application, false, change.toString()); + applications().store(application); }); } @@ -257,83 +190,52 @@ public class DeploymentTrigger { //--- End of methods which triggers deployment jobs ---------------------------- - private ApplicationController applications() { return controller.applications(); } + /** Find the next step to trigger if any, and triggers it */ + private void triggerReadyJobs(LockedApplication application) { + List<JobType> jobs = order.jobsFrom(application.deploymentSpec()); - /** Retry immediately only if this job just started failing. Otherwise retry periodically */ - private boolean retryBecauseNewFailure(Application application, JobType jobType) { - JobStatus jobStatus = application.deploymentJobs().jobStatus().get(jobType); - return (jobStatus != null && jobStatus.firstFailing().get().at().isAfter(clock.instant().minus(Duration.ofSeconds(10)))); - } + // Should the first step be triggered? + if ( ! jobs.isEmpty() && jobs.get(0).equals(JobType.systemTest) ) { + JobStatus systemTestStatus = application.deploymentJobs().jobStatus().get(JobType.systemTest); + if (application.change().platform().isPresent()) { + Version target = application.change().platform().get(); + if (systemTestStatus == null + || ! systemTestStatus.lastTriggered().isPresent() + || ! systemTestStatus.isSuccess() + || ! systemTestStatus.lastTriggered().get().version().equals(target) + || systemTestStatus.isHanging(jobTimeoutLimit())) { + application = trigger(new Triggering(application, JobType.systemTest, false, "Upgrade to " + target), Collections.emptySet(), false); + applications().store(application); + } + } + } - /** Decide whether to retry due to capacity restrictions */ - private boolean retryBecauseOutOfCapacity(Application application, JobType jobType) { - JobStatus jobStatus = application.deploymentJobs().jobStatus().get(jobType); - if (jobStatus == null || ! jobStatus.jobError().equals(Optional.of(JobError.outOfCapacity))) return false; - // Retry the job if it failed recently - return jobStatus.firstFailing().get().at().isAfter(clock.instant().minus(Duration.ofMinutes(15))); + // Find next steps to trigger based on the state of the previous step + for (JobType jobType : (Iterable<JobType>) Stream.concat(Stream.of(JobType.component), jobs.stream())::iterator) { + JobStatus jobStatus = application.deploymentJobs().jobStatus().get(jobType); + if (jobStatus == null) continue; // job has never run + + // Collect the subset of next jobs which have not run with the last changes + // TODO jvenstad: Change to be step-centric. + List<JobType> nextJobs = order.nextAfter(jobType, application); + for (JobType nextJobType : nextJobs) { + JobStatus nextStatus = application.deploymentJobs().jobStatus().get(nextJobType); + if (changesAvailable(application, jobStatus, nextStatus) || nextStatus.isHanging(jobTimeoutLimit())) { + boolean isRetry = nextStatus != null && nextStatus.jobError().filter(JobError.outOfCapacity::equals).isPresent(); + application = trigger(new Triggering(application, nextJobType, isRetry, isRetry ? "Retrying on out of capacity" : "Available change in " + jobType.jobName()), nextJobs, false); + } + } + applications().store(application); + } } + private ApplicationController applications() { return controller.applications(); } + /** Returns whether the given job type should be triggered according to deployment spec */ private boolean hasJob(JobType jobType, Application application) { if ( ! jobType.isProduction()) return true; // Deployment spec only determines this for production jobs. return application.deploymentSpec().includes(jobType.environment(), jobType.region(controller.system())); } - - /** - * Trigger a job for an application - * - * @param jobType the type of the job to trigger, or null to trigger nothing - * @param application the application to trigger the job for - * @param first whether to put the job at the front of the build system queue (or the back) - * @param reason describes why the job is triggered - * @return the application in the triggered state, which *must* be stored by the caller - */ - private LockedApplication trigger(JobType jobType, LockedApplication application, boolean first, String reason) { - if (jobType.isProduction() && isRunningProductionJob(application)) return application; - return triggerAllowParallel(jobType, application, first, false, reason); - } - - private LockedApplication trigger(List<JobType> jobs, LockedApplication application, String reason) { - if (jobs.stream().anyMatch(JobType::isProduction) && isRunningProductionJob(application)) return application; - for (JobType job : jobs) - application = triggerAllowParallel(job, application, false, false, reason); - return application; - } - - /** - * Trigger a job for an application, if allowed - * - * @param jobType the type of the job to trigger, or null to trigger nothing - * @param application the application to trigger the job for - * @param first whether to trigger the job before other jobs - * @param force true to disable checks which should normally prevent this triggering from happening - * @param reason describes why the job is triggered - * @return the application in the triggered state, if actually triggered. This *must* be stored by the caller - */ - public LockedApplication triggerAllowParallel(JobType jobType, LockedApplication application, - boolean first, boolean force, String reason) { - if (jobType == null) return application; // we are passed null when the last job has been reached - // Never allow untested changes to go through - // Note that this may happen because a new change catches up and prevents an older one from continuing - if ( ! application.deploymentJobs().isDeployableTo(jobType.environment(), application.change())) { - log.warning(String.format("Want to trigger %s for %s with reason %s, but change is untested", jobType, - application, reason)); - return application; - } - - if ( ! force && ! allowedTriggering(jobType, application)) return application; - log.info(String.format("Triggering %s for %s, %s: %s", jobType, application, - application.change().isPresent() ? "deploying " + application.change() : "restarted deployment", - reason)); - deploymentQueue.addJob(application.id(), jobType, first); - return application.withJobTriggering(jobType, - clock.instant(), - application.deployVersionFor(jobType, controller), - application.deployApplicationVersionFor(jobType, controller, false) - .orElse(ApplicationVersion.unknown), - reason); - } - /** Create application version from job report */ private ApplicationVersion applicationVersionFrom(JobReport report) { return report.sourceRevision().map(sr -> ApplicationVersion.from(sr, report.buildNumber())) @@ -361,11 +263,48 @@ public class DeploymentTrigger { return true; } - private boolean isRunningProductionJob(Application application) { - return ! JobList.from(application) - .production() - .running(jobTimeoutLimit()) - .isEmpty(); + /** + * Returns true if the previous job has completed successfully with a application version and/or Vespa version + * which is newer (different) than the one last completed successfully in next + */ + private boolean changesAvailable(Application application, JobStatus previous, JobStatus next) { + if ( ! application.change().isPresent()) return false; + if (next == null) return true; + + if (next.type().isTest()) { + // Is it not yet this job's turn to upgrade? + if ( ! lastSuccessfulIs(application.change(), previous.type(), application)) + return false; + + // Has the upgrade test already been done? + if (lastSuccessfulIs(application.change(), next.type(), application)) + return false; + } + else if (next.type().isProduction()) { + // Is the target version tested? + if ( ! lastSuccessfulIs(application.change(), JobType.stagingTest, application)) + return false; + + // Is the previous a job production which neither succeed with the target version, nor has a higher version? + if (previous.type().isProduction() && ! changeDeployed(application, previous.type())) + return false; + + // Did the next job already succeed on the target version, or does it already have a higher version? + if (changeDeployed(application, next.type())) + return false; + } + else + throw new IllegalStateException("Unclassified type of next job: " + next); + + return true; + } + + /** Returns whether all production zones listed in deployment spec has this change (or a newer version, if upgrade) */ + private boolean deploymentComplete(LockedApplication application) { + return order.jobsFrom(application.deploymentSpec()).stream() + .filter(JobType::isProduction) + .filter(job -> job.zone(controller.system()).isPresent()) + .allMatch(job -> changeDeployed(application, job)); } /** @@ -428,4 +367,45 @@ public class DeploymentTrigger { return true; } + + public static class Triggering { + + private final LockedApplication application; // TODO jvenstad: Consider passing an ID instead. + private final JobType jobType; + private final boolean retry; + private final String reason; + + public Triggering(LockedApplication application, JobType jobType, boolean retry, String reason) { + this.application = application; + this.jobType = jobType; + this.retry = retry; + this.reason = reason; + } + + public LockedApplication application() { + return application; + } + + public JobType jobType() { + return jobType; + } + + public boolean isRetry() { + return retry; + } + + public String reason() { + return reason; + } + + public String toString() { + return String.format("Triggering %s for %s, %s: %s", + jobType, + application, + application.change().isPresent() ? "deploying " + application.change() : "restarted deployment", + reason); + } + + } + } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java index e72ec5224f1..d92e07d475f 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java @@ -53,7 +53,7 @@ public class ControllerMaintenance extends AbstractComponent { outstandingChangeDeployer = new OutstandingChangeDeployer(controller, maintenanceInterval, jobControl); versionStatusUpdater = new VersionStatusUpdater(controller, Duration.ofMinutes(3), jobControl); upgrader = new Upgrader(controller, maintenanceInterval, jobControl, curator); - readyJobsTrigger = new ReadyJobsTrigger(controller, maintenanceInterval, jobControl); + readyJobsTrigger = new ReadyJobsTrigger(controller, Duration.ofSeconds(30), jobControl); clusterInfoMaintainer = new ClusterInfoMaintainer(controller, Duration.ofHours(2), jobControl); clusterUtilizationMaintainer = new ClusterUtilizationMaintainer(controller, Duration.ofHours(2), jobControl); deploymentMetricsMaintainer = new DeploymentMetricsMaintainer(controller, Duration.ofMinutes(10), jobControl); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AccessControlRequestFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AccessControlRequestFilter.java index 8dace5d56dc..8df4124028e 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AccessControlRequestFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AccessControlRequestFilter.java @@ -11,6 +11,7 @@ import com.yahoo.jdisc.http.filter.SecurityRequestFilter; import com.yahoo.vespa.hosted.controller.restapi.filter.config.HttpAccessControlConfig; import com.yahoo.yolean.chain.After; import com.yahoo.yolean.chain.Before; +import com.yahoo.yolean.chain.Provides; import java.util.Collections; import java.util.Set; @@ -38,7 +39,8 @@ import static com.yahoo.vespa.hosted.controller.restapi.filter.AccessControlHead * @author gv */ @After({"InputValidationFilter","RemoteIPFilter", "DoNotTrackRequestFilter", "CookieDataRequestFilter"}) -@Before("BouncerFilter") +@Before({"BouncerFilter", "ControllerAuthorizationFilter"}) +@Provides("AccessControlRequestFilter") public class AccessControlRequestFilter implements SecurityRequestFilter { private final Set<String> allowedUrls; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java index f718d4fa8bd..e6623fd6508 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java @@ -23,6 +23,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.athenz.AthenzClientFact import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsException; import com.yahoo.vespa.hosted.controller.restapi.Path; import com.yahoo.yolean.chain.After; +import com.yahoo.yolean.chain.Provides; import javax.ws.rs.ForbiddenException; import javax.ws.rs.InternalServerErrorException; @@ -47,6 +48,7 @@ import static com.yahoo.vespa.hosted.controller.restapi.filter.SecurityFilterUti * @author bjorncs */ @After("com.yahoo.vespa.hosted.controller.athenz.filter.UserAuthWithAthenzPrincipalFilter") +@Provides("ControllerAuthorizationFilter") public class ControllerAuthorizationFilter implements SecurityRequestFilter { private static final List<Method> WHITELISTED_METHODS = Arrays.asList(GET, OPTIONS, HEAD); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java index 3a8d74d2c2e..d7f224d15aa 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java @@ -11,6 +11,7 @@ import com.yahoo.slime.Slime; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.api.integration.BuildService.BuildJob; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType; +import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger; import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse; import com.yahoo.vespa.hosted.controller.restapi.Path; import com.yahoo.vespa.hosted.controller.restapi.SlimeJsonResponse; @@ -18,6 +19,7 @@ import com.yahoo.vespa.hosted.controller.versions.VespaVersion; import com.yahoo.yolean.Exceptions; import java.io.InputStream; +import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.Scanner; @@ -89,10 +91,9 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler { controller.applications().lockOrThrow(applicationId, application -> { // Since this is a manual operation we likely want it to trigger as soon as possible so we add it at to the // front of the queue - application = controller.applications().deploymentTrigger().triggerAllowParallel( - jobType, application, true, true, - "Triggered from screwdriver/v1" - ); + application = controller.applications().deploymentTrigger().trigger( + new DeploymentTrigger.Triggering(application, jobType, true, "Triggered from screwdriver/v1"), Collections.emptySet(), true + ); controller.applications().store(application); }); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/AthenzFilterMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/AthenzFilterMock.java index 02a7f63fbb8..0a32406be60 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/AthenzFilterMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/AthenzFilterMock.java @@ -16,7 +16,7 @@ import static com.yahoo.vespa.hosted.controller.restapi.filter.SecurityFilterUti /** * @author bjorncs */ -@Before("com.yahoo.vespa.hosted.controller.restapi.filter.ControllerAuthorizationFilter") +@Before("ControllerAuthorizationFilter") public class AthenzFilterMock implements SecurityRequestFilter { public static final String IDENTITY_HEADER_NAME = "Athenz-Identity"; 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 bba82ce2980..5c24b70fd65 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 @@ -142,18 +142,18 @@ public class ControllerTest { tester.clock().advance(Duration.ofHours(1)); - // Need to complete the job, or new jobs won't start. - tester.jobCompletion(productionCorpUsEast1).application(app1).unsuccessful().submit(); - // system and staging test job - succeeding - tester.jobCompletion(component).application(app1).submit(); + tester.jobCompletion(component).application(app1).nextBuildNumber().uploadArtifact(applicationPackage).submit(); + applicationVersion = tester.application("app1").change().application().get(); tester.deployAndNotify(app1, applicationPackage, true, false, systemTest); assertStatus(JobStatus.initial(systemTest) .withTriggering(version1, applicationVersion, "", tester.clock().instant().minus(Duration.ofMillis(1))) - .withCompletion(42, Optional.empty(), tester.clock().instant(), tester.controller()), app1.id(), tester.controller()); + .withCompletion(42, Optional.empty(), tester.clock().instant(), tester.controller()), + app1.id(), tester.controller()); tester.deployAndNotify(app1, applicationPackage, true, stagingTest); // production job succeeding now + tester.jobCompletion(productionCorpUsEast1).application(app1).unsuccessful().submit(); tester.deployAndNotify(app1, applicationPackage, true, productionCorpUsEast1); expectedJobStatus = expectedJobStatus .withTriggering(version1, applicationVersion, "", tester.clock().instant().minus(Duration.ofMillis(1))) @@ -186,7 +186,7 @@ public class ControllerTest { JobStatus jobStatus = applications.require(app1.id()).deploymentJobs().jobStatus().get(productionCorpUsEast1); assertNotNull("Deployment job was not removed", jobStatus); assertEquals(42, jobStatus.lastCompleted().get().id()); - assertEquals("staging-test completed", jobStatus.lastCompleted().get().reason()); + assertEquals("Available change in staging-test", jobStatus.lastCompleted().get().reason()); // prod zone removal is allowed with override applicationPackage = new ApplicationPackageBuilder() @@ -278,6 +278,7 @@ public class ControllerTest { // Version upgrade changes system version applications.deploymentTrigger().triggerChange(app1.id(), Change.of(newSystemVersion)); + tester.deploymentTrigger().triggerReadyJobs(); tester.deployAndNotify(app1, applicationPackage, true, systemTest); tester.deployAndNotify(app1, applicationPackage, true, stagingTest); tester.deployAndNotify(app1, applicationPackage, true, productionUsWest1); @@ -392,7 +393,7 @@ public class ControllerTest { // Initial failure tester.clock().advance(Duration.ofMillis(1000)); initialFailure = tester.clock().instant(); - tester.jobCompletion(component).application(app).submit(); + tester.jobCompletion(component).application(app).nextBuildNumber().uploadArtifact(applicationPackage).submit(); tester.deployAndNotify(app, applicationPackage, false, systemTest); assertEquals("Failure age is right at initial failure", initialFailure.plus(Duration.ofMillis(2)), firstFailing(app, tester).get().at()); @@ -448,17 +449,6 @@ public class ControllerTest { tester.deployAndNotify(app3, applicationPackage, true, stagingTest); tester.deployAndNotify(app3, applicationPackage, true, productionCorpUsEast1); - // app1: 15 minutes pass, staging-test job is still failing due out of capacity, but is no longer re-queued by - // out of capacity retry mechanism - tester.clock().advance(Duration.ofMinutes(15)); - tester.jobCompletion(stagingTest).application(app1).error(JobError.outOfCapacity).submit(); // Clear the previous staging test - tester.jobCompletion(component).application(app1).nextBuildNumber().uploadArtifact(applicationPackage).submit(); - tester.deployAndNotify(app1, applicationPackage, true, false, systemTest); - tester.deploy(stagingTest, app1, applicationPackage); - assertEquals(1, deploymentQueue.takeJobsToRun().size()); - tester.jobCompletion(stagingTest).application(app1).error(JobError.outOfCapacity).submit(); - assertTrue("No jobs queued", deploymentQueue.jobs().isEmpty()); - // app2 and app3: New change triggers system-test jobs // Provide a changed application package, too, or the deployment is a no-op. tester.jobCompletion(component).application(app2).nextBuildNumber().uploadArtifact(applicationPackage).submit(); @@ -467,17 +457,9 @@ public class ControllerTest { tester.jobCompletion(component).application(app3).nextBuildNumber().uploadArtifact(applicationPackage).submit(); tester.deployAndNotify(app3, applicationPackage2, true, systemTest); - assertEquals(2, deploymentQueue.jobs().size()); - - // app1: 4 hours pass in total, staging-test job for app1 is re-queued by periodic trigger mechanism and added at the - // back of the queue - tester.clock().advance(Duration.ofHours(3)); - tester.clock().advance(Duration.ofMinutes(50)); - tester.readyJobTrigger().maintain(); - + assertEquals(Collections.singletonList(new BuildService.BuildJob(project1, stagingTest.jobName())), deploymentQueue.takeJobsToRun()); assertEquals(Collections.singletonList(new BuildService.BuildJob(project2, stagingTest.jobName())), deploymentQueue.takeJobsToRun()); assertEquals(Collections.singletonList(new BuildService.BuildJob(project3, stagingTest.jobName())), deploymentQueue.takeJobsToRun()); - assertEquals(Collections.singletonList(new BuildService.BuildJob(project1, stagingTest.jobName())), deploymentQueue.takeJobsToRun()); assertEquals(Collections.emptyList(), deploymentQueue.takeJobsToRun()); } @@ -576,6 +558,7 @@ public class ControllerTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // Test environments pass tester.deploy(DeploymentJobs.JobType.systemTest, application, applicationPackage); @@ -661,7 +644,7 @@ public class ControllerTest { .environment(Environment.prod) .allow(ValidationId.deploymentRemoval) .build(); - tester.jobCompletion(component).application(app1).uploadArtifact(applicationPackage).submit(); + tester.jobCompletion(component).application(app1).nextBuildNumber().uploadArtifact(applicationPackage).submit(); tester.deployAndNotify(app1, applicationPackage, true, systemTest); tester.applications().deactivate(app1, ZoneId.from(Environment.test, RegionName.from("us-east-1"))); tester.applications().deactivate(app1, ZoneId.from(Environment.staging, RegionName.from("us-east-3"))); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/BuildJob.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/BuildJob.java index bb8840a7b1f..f1e6108710d 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/BuildJob.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/BuildJob.java @@ -104,6 +104,7 @@ public class BuildJob { /** Upload given application package to artifact repository as part of this job */ public BuildJob uploadArtifact(ApplicationPackage applicationPackage) { Objects.requireNonNull(job, "job cannot be null"); + Objects.requireNonNull(applicationId, "applicationId cannot be null"); if (job != DeploymentJobs.JobType.component) { throw new IllegalStateException(job + " cannot upload artifact"); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index 92bf22df535..6bc544605b6 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -101,6 +101,7 @@ public class DeploymentTester { configServer().setDefaultVersion(version); updateVersionStatus(version); upgrader().maintain(); + readyJobTrigger().maintain(); } public Version defaultVespaVersion() { @@ -275,7 +276,7 @@ public class DeploymentTester { assertEquals(job.jobName(), buildJob.jobName()); } if (expectOnlyTheseJobs) - assertEquals(jobs.length, countJobsOf(application)); + assertEquals("Unexpected job queue: " + jobsOf(application), jobs.length, jobsOf(application).size()); deploymentQueue().removeJobs(application.id()); } @@ -286,15 +287,17 @@ public class DeploymentTester { throw new IllegalArgumentException(jobType + " is not scheduled for " + application); } - private int countJobsOf(Application application) { - return (int) deploymentQueue().jobs().stream() - .filter(job -> job.projectId() == application.deploymentJobs().projectId().get()) - .count(); + private List<JobType> jobsOf(Application application) { + return deploymentQueue().jobs().stream() + .filter(job -> job.projectId() == application.deploymentJobs().projectId().get()) + .map(buildJob -> JobType.fromJobName(buildJob.jobName())) + .collect(Collectors.toList()); } private void notifyJobCompletion(DeploymentJobs.JobReport report) { clock().advance(Duration.ofMillis(1)); applications().notifyJobCompletion(report); + applications().deploymentTrigger().triggerReadyJobs(); } public static ApplicationPackage applicationPackage(String upgradePolicy) { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java index ce765249b97..364cb66c3d1 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java @@ -55,6 +55,7 @@ public class DeploymentTriggerTest { Version version = new Version(5, 1); tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // Deploy completely once tester.jobCompletion(component).application(app).uploadArtifact(applicationPackage).submit(); @@ -66,17 +67,11 @@ public class DeploymentTriggerTest { version = new Version(5, 2); tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // system-test fails and is retried tester.deployAndNotify(app, applicationPackage, false, JobType.systemTest); - assertEquals("Retried immediately", 1, tester.deploymentQueue().jobs().size()); - tester.clock().advance(Duration.ofHours(1)); - tester.deployAndNotify(app, applicationPackage, false, JobType.systemTest); - tester.clock().advance(Duration.ofHours(1)); - assertEquals("Nothing scheduled", 0, tester.deploymentQueue().jobs().size()); - tester.readyJobTrigger().maintain(); // Causes retry of systemTests - - assertEquals("Scheduled retry", 1, tester.deploymentQueue().jobs().size()); + assertEquals("Job is retried on failure", 1, tester.deploymentQueue().jobs().size()); tester.deployAndNotify(app, applicationPackage, true, JobType.systemTest); // staging-test times out and is retried @@ -390,28 +385,22 @@ public class DeploymentTriggerTest { tester.upgradeSystem(version1); tester.completeUpgradeWithError(application, version1, applicationPackage, productionEuWest1); - // Exhaust the retry, so productionEuWest1 is no longer running. - tester.clock().advance(Duration.ofHours(1)); - tester.deployAndNotify(application, Optional.empty(), false, true, productionEuWest1); - assertTrue(tester.deploymentQueue().jobs().isEmpty()); - // Deploy the new application version, even though the platform version is already deployed in us-central-1. // Let it fail in us-central-1 after deployment, so we can test this zone is later skipped. - tester.completeDeploymentWithError(application, applicationPackage, BuildJob.defaultBuildNumber + 1, productionUsCentral1); + tester.jobCompletion(component).application(application).nextBuildNumber().uploadArtifact(applicationPackage).submit(); + tester.deployAndNotify(application, applicationPackage, true, false, systemTest); + tester.deployAndNotify(application, applicationPackage, true, false, stagingTest); + tester.jobCompletion(productionEuWest1).application(application).unsuccessful().submit(); tester.deploy(productionUsCentral1, application, Optional.empty(), false); + // Deploying before notifying here makes the job not re-trigger, but instead triggers the next job (because of triggerReadyJobs() in notification.) + tester.deployAndNotify(application, applicationPackage, false, productionUsCentral1); assertEquals(ApplicationVersion.from(BuildJob.defaultSourceRevision, BuildJob.defaultBuildNumber + 1), app.get().deployments().get(ZoneId.from("prod.us-central-1")).applicationVersion()); - // Exhaust the automatic retry. - tester.clock().advance(Duration.ofHours(1)); - tester.deployAndNotify(application, Optional.empty(), false, true, productionUsCentral1); - assertTrue(tester.deploymentQueue().jobs().isEmpty()); - - // Let the ReadyJobTrigger get what it thinks is the next job -- should be the last job. - tester.readyJobTrigger().maintain(); assertEquals(Collections.singletonList(new BuildService.BuildJob(1, productionEuWest1.jobName())), tester.deploymentQueue().jobs()); + tester.deploy(productionEuWest1, application, Optional.empty(), false); tester.deployAndNotify(application, Optional.empty(), false, true, productionEuWest1); assertFalse(app.get().change().isPresent()); @@ -438,11 +427,6 @@ public class DeploymentTriggerTest { tester.deployAndNotify(application, Optional.empty(), false, true, productionUsCentral1); tester.deploy(productionUsCentral1, application, Optional.empty(), false); - // Exhaust the automatic retry. - tester.clock().advance(Duration.ofHours(1)); - tester.deployAndNotify(application, Optional.empty(), false, true, productionUsCentral1); - assertTrue(tester.deploymentQueue().jobs().isEmpty()); - ApplicationVersion appVersion1 = ApplicationVersion.from(BuildJob.defaultSourceRevision, BuildJob.defaultBuildNumber + 1); assertEquals(appVersion1, app.get().deployments().get(ZoneId.from("prod.us-central-1")).applicationVersion()); @@ -450,13 +434,14 @@ public class DeploymentTriggerTest { tester.deploymentTrigger().cancelChange(application.id(), true); assertEquals(Change.of(appVersion1), app.get().change()); - // Now cancel the change -- this should not normally happen. + // Now cancel the change as is done through the web API. tester.deploymentTrigger().cancelChange(application.id(), false); assertEquals(Change.empty(), app.get().change()); // A new version is released, which should now deploy the currently deployed application version to avoid downgrades. Version version1 = new Version("6.2"); tester.upgradeSystem(version1); + tester.jobCompletion(productionUsCentral1).application(application).unsuccessful().submit(); tester.completeUpgrade(application, version1, applicationPackage); assertEquals(appVersion1, app.get().deployments().get(ZoneId.from("prod.us-central-1")).applicationVersion()); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java index 155c9f14f77..f088c4216ba 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java @@ -134,7 +134,7 @@ public class DeploymentIssueReporterTest { // app3 now has a new failure past max failure age; see that a new issue is filed. - tester.jobCompletion(component).application(app3).submit(); + tester.jobCompletion(component).application(app3).nextBuildNumber().uploadArtifact(applicationPackage).submit(); tester.deployAndNotify(app3, applicationPackage, false, systemTest); tester.clock().advance(maxInactivity.plus(maxFailureAge)); @@ -148,6 +148,7 @@ public class DeploymentIssueReporterTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); tester.completeUpgradeWithError(app2, version, canaryPackage, systemTest); tester.updateVersionStatus(version); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java index 5ed4c3a186e..f6216856317 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java @@ -74,7 +74,7 @@ public class DnsMaintainerTest { .environment(Environment.prod) .allow(ValidationId.deploymentRemoval) .build(); - tester.jobCompletion(component).application(application).uploadArtifact(applicationPackage).submit(); + tester.jobCompletion(component).application(application).nextBuildNumber().uploadArtifact(applicationPackage).submit(); tester.deployAndNotify(application, applicationPackage, true, systemTest); tester.applications().deactivate(application, ZoneId.from(Environment.test, RegionName.from("us-east-1"))); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/FailureRedeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/FailureRedeployerTest.java index 092cdcd6984..429a0da6543 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/FailureRedeployerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/FailureRedeployerTest.java @@ -23,6 +23,7 @@ import java.util.Collections; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.component; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionUsEast3; +import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.systemTest; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -54,6 +55,7 @@ public class FailureRedeployerTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // Test environments pass tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.systemTest); @@ -68,14 +70,14 @@ public class FailureRedeployerTest { // Another version is released, which cancels any pending upgrades to lower versions version = Version.fromString("5.2"); tester.updateVersionStatus(version); - tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.productionUsEast3); // Finish previous production job. tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Application starts upgrading to new version", 1, tester.deploymentQueue().jobs().size()); assertEquals("Application has pending upgrade to " + version, version, tester.application(app.id()).change().platform().get()); - // Failure redeployer does not retry failing job for prod.us-east-3 as there's an ongoing deployment + // Failure re-deployer does not retry failing job for prod.us-east-3, since it no longer has an available change tester.clock().advance(Duration.ofMinutes(1)); - tester.readyJobTrigger().maintain(); + tester.jobCompletion(DeploymentJobs.JobType.productionUsEast3).application(app).unsuccessful().submit(); assertFalse("Job is not retried", tester.deploymentQueue().jobs().stream() .anyMatch(j -> j.jobName().equals(DeploymentJobs.JobType.productionUsEast3.jobName()))); @@ -83,16 +85,8 @@ public class FailureRedeployerTest { tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.systemTest); tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.stagingTest); - // Production job fails again and exhausts all immediate retries + // Production job fails again, and is retried tester.deployAndNotify(app, applicationPackage, false, DeploymentJobs.JobType.productionUsEast3); - tester.deploymentQueue().takeJobsToRun(); - tester.clock().advance(Duration.ofMinutes(10)); - tester.jobCompletion(DeploymentJobs.JobType.productionUsEast3).application(app).unsuccessful().submit(); - assertTrue("Retries exhausted", tester.deploymentQueue().jobs().isEmpty()); - assertTrue("Failure is recorded", tester.application(app.id()).deploymentJobs().hasFailures()); - - // Failure redeployer retries job - tester.clock().advance(Duration.ofMinutes(5)); tester.readyJobTrigger().maintain(); assertEquals("Job is retried", Collections.singletonList(new BuildService.BuildJob(app.deploymentJobs().projectId().get(), productionUsEast3.jobName())), tester.deploymentQueue().jobs()); @@ -154,29 +148,25 @@ public class FailureRedeployerTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Application has pending upgrade to " + version, version, tester.application(app.id()).change().platform().get()); - // system-test fails and exhausts all immediate retries + // system-test fails and is left with a retry tester.deployAndNotify(app, applicationPackage, false, DeploymentJobs.JobType.systemTest); - tester.deploymentQueue().takeJobsToRun(); - tester.clock().advance(Duration.ofMinutes(10)); - tester.jobCompletion(DeploymentJobs.JobType.systemTest).application(app).unsuccessful().submit(); - assertTrue("Retries exhausted", tester.deploymentQueue().jobs().isEmpty()); // Another version is released version = Version.fromString("5.2"); tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); + + // Job is left "running", so needs to time out before it can be retried. + tester.clock().advance(Duration.ofHours(13)); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Application has pending upgrade to " + version, version, tester.application(app.id()).change().platform().get()); - // Consume system-test job for 5.2 - tester.deploymentQueue().takeJobsToRun(); - - // Failure re-deployer does not retry failing system-test job as it failed for an older change - tester.clock().advance(Duration.ofMinutes(5)); - tester.readyJobTrigger().maintain(); - assertTrue("No jobs retried", tester.deploymentQueue().jobs().isEmpty()); + // Cancellation of outdated version and triggering on a new version is done by the upgrader. + assertEquals(version, tester.application(app.id()).deploymentJobs().jobStatus().get(systemTest).lastTriggered().get().version()); } @Test @@ -207,6 +197,7 @@ public class FailureRedeployerTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // Test environments pass tester.deploy(DeploymentJobs.JobType.systemTest, application, applicationPackage); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java index 12fb2b6c862..a4e464a065f 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java @@ -42,6 +42,7 @@ public class OutstandingChangeDeployerTest { Version version = new Version(6, 2); tester.deploymentTrigger().triggerChange(tester.application("app1").id(), Change.of(version)); + tester.deploymentTrigger().triggerReadyJobs(); assertEquals(Change.of(version), tester.application("app1").change()); assertFalse(tester.application("app1").outstandingChange().isPresent()); @@ -59,6 +60,7 @@ public class OutstandingChangeDeployerTest { assertEquals(1, tester.deploymentQueue().jobs().size()); deployer.maintain(); + tester.deploymentTrigger().triggerReadyJobs(); assertEquals("No effect as job is in progress", 1, tester.deploymentQueue().jobs().size()); assertEquals("1.0.43-cafed00d", app.outstandingChange().application().get().id()); @@ -68,6 +70,7 @@ public class OutstandingChangeDeployerTest { assertEquals("Upgrade done", 0, tester.deploymentQueue().jobs().size()); deployer.maintain(); + tester.deploymentTrigger().triggerReadyJobs(); app = tester.application("app1"); assertEquals("1.0.43-cafed00d", app.change().application().get().id()); List<BuildService.BuildJob> jobs = tester.deploymentQueue().jobs(); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java index 088895408fc..b7eff6d8448 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java @@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs; import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder; +import com.yahoo.vespa.hosted.controller.deployment.BuildJob; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import com.yahoo.vespa.hosted.controller.versions.VespaVersion; import org.junit.Test; @@ -20,6 +21,8 @@ import java.time.Duration; import java.time.Instant; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.component; +import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionEuWest1; +import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionUsEast3; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionUsWest1; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.stagingTest; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.systemTest; @@ -41,6 +44,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("No applications: Nothing to do", 0, tester.deploymentQueue().jobs().size()); // Setup applications @@ -52,6 +56,7 @@ public class UpgraderTest { Application conservative0 = tester.createAndDeploy("conservative0", 6, "conservative"); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("All already on the right version: Nothing to do", 0, tester.deploymentQueue().jobs().size()); // --- 5.1 is released - everything goes smoothly @@ -59,6 +64,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("New system version: Should upgrade Canaries", 2, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(canary0, version, "canary"); @@ -66,6 +72,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("One canary pending; nothing else", 1, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(canary1, version, "canary"); @@ -73,6 +80,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(VespaVersion.Confidence.normal, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Canaries done: Should upgrade defaults", 3, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(default0, version, "default"); @@ -82,11 +90,13 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(VespaVersion.Confidence.high, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Normals done: Should upgrade conservatives", 1, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(conservative0, version, "conservative"); tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Nothing to do", 0, tester.deploymentQueue().jobs().size()); // --- 5.2 is released - which fails a Canary @@ -94,31 +104,25 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("New system version: Should upgrade Canaries", 2, tester.deploymentQueue().jobs().size()); tester.completeUpgradeWithError(canary0, version, "canary", DeploymentJobs.JobType.stagingTest); assertEquals("Other Canary was cancelled", 2, tester.deploymentQueue().jobs().size()); - // TODO: Cancelled would mean it was triggerd, removed from the build system, but never reported in. - // Thus, the expected number of jobs should be 1, above: the retrying canary0. - // Further, canary1 should be retried after the timeout period of 12 hours, but verifying this is - // not possible when jobs are consumed form the build system on notification, rather than on deploy. tester.updateVersionStatus(version); assertEquals(VespaVersion.Confidence.broken, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Version broken, but Canaries should keep trying", 2, tester.deploymentQueue().jobs().size()); - // Exhaust canary retries. - tester.jobCompletion(systemTest).application(canary1).unsuccessful().submit(); - tester.clock().advance(Duration.ofHours(1)); - tester.deployAndNotify(canary0, DeploymentTester.applicationPackage("canary"), false, DeploymentJobs.JobType.stagingTest); - tester.jobCompletion(systemTest).application(canary1).unsuccessful().submit(); - // --- A new version is released - which repairs the Canary app and fails a default + tester.clock().advance(Duration.ofHours(13)); version = Version.fromString("5.3"); tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("New system version: Should upgrade Canaries", 2, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(canary0, version, "canary"); @@ -126,6 +130,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("One canary pending; nothing else", 1, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(canary1, version, "canary"); @@ -133,6 +138,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(VespaVersion.Confidence.normal, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Canaries done: Should upgrade defaults", 3, tester.deploymentQueue().jobs().size()); @@ -146,22 +152,27 @@ public class UpgraderTest { assertEquals("Upgrade with error should retry", 1, tester.deploymentQueue().jobs().size()); - // Finish previous run, with exhausted retry. - tester.clock().advance(Duration.ofHours(1)); - tester.jobCompletion(stagingTest).application(default0).unsuccessful().submit(); // --- Failing application is repaired by changing the application, causing confidence to move above 'high' threshold // Deploy application change - tester.deployCompletely("default0"); + tester.deploymentQueue().takeJobsToRun(); + tester.jobCompletion(component).application(default0).nextBuildNumber().uploadArtifact(DeploymentTester.applicationPackage("default")).submit(); + tester.jobCompletion(stagingTest).application(default0).unsuccessful().submit(); + tester.deployAndNotify(default0, "default", true, systemTest); + tester.deployAndNotify(default0, "default", true, stagingTest); + tester.deployAndNotify(default0, "default", true, productionUsWest1); + tester.deployAndNotify(default0, "default", true, productionUsEast3); tester.updateVersionStatus(version); assertEquals(VespaVersion.Confidence.high, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Normals done: Should upgrade conservatives", 1, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(conservative0, version, "conservative"); tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Applications are on 5.3 - nothing to do", 0, tester.deploymentQueue().jobs().size()); // --- Starting upgrading to a new version which breaks, causing upgrades to commence on the previous version @@ -170,11 +181,13 @@ public class UpgraderTest { Application default4 = tester.createAndDeploy("default4", 5, "default"); tester.updateVersionStatus(version54); tester.upgrader().maintain(); // cause canary upgrades to 5.4 + tester.readyJobTrigger().maintain(); tester.completeUpgrade(canary0, version54, "canary"); tester.completeUpgrade(canary1, version54, "canary"); tester.updateVersionStatus(version54); assertEquals(VespaVersion.Confidence.normal, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Upgrade of defaults are scheduled", 5, tester.deploymentQueue().jobs().size()); assertEquals(version54, tester.application(default0.id()).change().platform().get()); @@ -188,11 +201,13 @@ public class UpgraderTest { Version version55 = Version.fromString("5.5"); tester.updateVersionStatus(version55); tester.upgrader().maintain(); // cause canary upgrades to 5.5 + tester.readyJobTrigger().maintain(); tester.completeUpgrade(canary0, version55, "canary"); tester.completeUpgrade(canary1, version55, "canary"); tester.updateVersionStatus(version55); assertEquals(VespaVersion.Confidence.normal, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Upgrade of defaults are scheduled", 5, tester.deploymentQueue().jobs().size()); assertEquals(version55, tester.application(default0.id()).change().platform().get()); @@ -205,13 +220,12 @@ public class UpgraderTest { tester.completeUpgrade(default2, version54, "default"); tester.completeUpgradeWithError(default3, version54, "default", DeploymentJobs.JobType.stagingTest); - // Exhaust immediate retries for upgrade - tester.clock().advance(Duration.ofHours(1)); - tester.jobCompletion(stagingTest).application(default3).unsuccessful().submit(); tester.completeUpgradeWithError(default4, version54, "default", DeploymentJobs.JobType.productionUsWest1); // State: Default applications started upgrading to 5.5 + tester.clock().advance(Duration.ofHours(13)); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); tester.completeUpgradeWithError(default0, version55, "default", DeploymentJobs.JobType.stagingTest); tester.completeUpgradeWithError(default1, version55, "default", DeploymentJobs.JobType.stagingTest); tester.completeUpgradeWithError(default2, version55, "default", DeploymentJobs.JobType.stagingTest); @@ -224,6 +238,7 @@ public class UpgraderTest { tester.jobCompletion(DeploymentJobs.JobType.productionUsWest1).application(default3).unsuccessful().submit(); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Upgrade of defaults are scheduled on 5.4 instead, since 5.5 broken: " + "This is default3 since it failed upgrade on both 5.4 and 5.5", 1, tester.deploymentQueue().jobs().size()); @@ -235,12 +250,14 @@ public class UpgraderTest { // --- Setup DeploymentTester tester = new DeploymentTester(); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("No system version: Nothing to do", 0, tester.deploymentQueue().jobs().size()); Version version = Version.fromString("5.0"); // (lower than the hardcoded version in the config server client) tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("No applications: Nothing to do", 0, tester.deploymentQueue().jobs().size()); // Setup applications @@ -258,6 +275,7 @@ public class UpgraderTest { Application default9 = tester.createAndDeploy("default9", 12, "default"); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("All already on the right version: Nothing to do", 0, tester.deploymentQueue().jobs().size()); // --- A new version is released @@ -265,6 +283,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("New system version: Should upgrade Canaries", 2, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(canary0, version, "canary"); @@ -272,6 +291,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("One canary pending; nothing else", 1, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(canary1, version, "canary"); @@ -279,6 +299,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(VespaVersion.Confidence.normal, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Canaries done: Should upgrade defaults", 10, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(default0, version, "default"); @@ -290,6 +311,7 @@ public class UpgraderTest { // > 40% and at least 4 failed - version is broken tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals(VespaVersion.Confidence.broken, tester.controller().versionStatus().systemVersion().get().confidence()); assertEquals("Upgrades are cancelled", 0, tester.deploymentQueue().jobs().size()); } @@ -312,6 +334,7 @@ public class UpgraderTest { tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.productionUsEast3); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Application is on expected version: Nothing to do", 0, tester.deploymentQueue().jobs().size()); @@ -320,16 +343,13 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // system-test completes successfully tester.deployAndNotify(app, applicationPackage, true, systemTest); - // staging-test fails multiple times, exhausts retries and failure is recorded + // staging-test fails and failure is recorded tester.deployAndNotify(app, applicationPackage, false, DeploymentJobs.JobType.stagingTest); - tester.deploymentQueue().takeJobsToRun(); - tester.clock().advance(Duration.ofMinutes(10)); - tester.jobCompletion(stagingTest).application(app).unsuccessful().submit(); - assertTrue("Retries exhausted", tester.deploymentQueue().jobs().isEmpty()); assertTrue("Failure is recorded", tester.application(app.id()).deploymentJobs().hasFailures()); assertTrue("Application has pending change", tester.application(app.id()).change().isPresent()); @@ -340,12 +360,14 @@ public class UpgraderTest { // Upgrade is scheduled. system-tests starts, but does not complete tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertTrue("Application still has failures", tester.application(app.id()).deploymentJobs().hasFailures()); assertEquals(1, tester.deploymentQueue().jobs().size()); tester.deploymentQueue().takeJobsToRun(); // Upgrader runs again, nothing happens as there's already a job in progress for this change tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertTrue("No more jobs triggered at this time", tester.deploymentQueue().jobs().isEmpty()); } @@ -369,6 +391,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // Canaries upgrade and raise confidence tester.completeUpgrade(canary0, version, "canary"); @@ -378,6 +401,7 @@ public class UpgraderTest { // Applications with default policy start upgrading tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Upgrade scheduled for remaining apps", 5, tester.deploymentQueue().jobs().size()); // 4/5 applications fail and lowers confidence @@ -388,6 +412,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(VespaVersion.Confidence.broken, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // 5th app passes system-test, but does not trigger next job as upgrade is cancelled assertFalse("No change present", tester.applications().require(default4.id()).change().isPresent()); @@ -423,6 +448,7 @@ public class UpgraderTest { tester.updateVersionStatus(v1); assertEquals(v1, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // Canaries upgrade and raise confidence of V+1 (other apps are not upgraded) tester.completeUpgrade(canary0, v1, "canary"); @@ -435,6 +461,7 @@ public class UpgraderTest { tester.updateVersionStatus(v2); assertEquals(v2, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // We "manually" cancel upgrades to V1 so that we can use the applications to make V2 fail instead // But we keep one (default4) to avoid V1 being garbage collected @@ -452,6 +479,7 @@ public class UpgraderTest { // Applications with default policy start upgrading to V2 tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Upgrade scheduled for remaining apps", 5, tester.deploymentQueue().jobs().size()); // 4/5 applications fail (in the last prod zone) and lowers confidence @@ -465,6 +493,7 @@ public class UpgraderTest { assertEquals(v2, tester.application("default0").deployments().get(ZoneId.from("prod.us-west-1")).version()); assertEquals(v0, tester.application("default0").deployments().get(ZoneId.from("prod.us-east-3")).version()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Upgrade to 5.1 scheduled for apps not completely on 5.1 or 5.2", 5, tester.deploymentQueue().jobs().size()); tester.deploymentTrigger().triggerReadyJobs(); @@ -513,6 +542,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // Canaries upgrade and raise confidence tester.completeUpgrade(canary0, version, "canary"); @@ -522,6 +552,7 @@ public class UpgraderTest { // All applications upgrade successfully tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); tester.completeUpgrade(default0, version, "default"); tester.completeUpgrade(default1, version, "default"); tester.completeUpgrade(default2, version, "default"); @@ -564,16 +595,19 @@ public class UpgraderTest { // Application is not upgraded at this time tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertTrue("No jobs scheduled", tester.deploymentQueue().jobs().isEmpty()); // One hour passes, time is 19:00, still no upgrade tester.clock().advance(Duration.ofHours(1)); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertTrue("No jobs scheduled", tester.deploymentQueue().jobs().isEmpty()); // Two hours pass in total, time is 20:00 and application upgrades tester.clock().advance(Duration.ofHours(1)); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertFalse("Job is scheduled", tester.deploymentQueue().jobs().isEmpty()); tester.completeUpgrade(app, version, applicationPackage); assertTrue("All jobs consumed", tester.deploymentQueue().jobs().isEmpty()); @@ -607,6 +641,7 @@ public class UpgraderTest { // Application upgrade starts tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); tester.deployAndNotify(app, applicationPackage, true, systemTest); tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.stagingTest); clock.advance(Duration.ofHours(1)); // Entering block window after prod job is triggered @@ -637,9 +672,6 @@ public class UpgraderTest { public void testBlockVersionChangeHalfwayThoughThenNewVersion() { ManualClock clock = new ManualClock(Instant.parse("2017-09-29T16:00:00.00Z")); // Friday, 16:00 DeploymentTester tester = new DeploymentTester(new ControllerTester(clock)); - ReadyJobsTrigger readyJobsTrigger = new ReadyJobsTrigger(tester.controller(), - Duration.ofHours(1), - new JobControl(tester.controllerTester().curator())); Version version = Version.fromString("5.0"); tester.updateVersionStatus(version); @@ -662,6 +694,7 @@ public class UpgraderTest { // Application upgrade starts tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); tester.deployAndNotify(app, applicationPackage, true, systemTest); tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.stagingTest); tester.deployAndNotify(app, applicationPackage, true, productionUsWest1); @@ -674,14 +707,14 @@ public class UpgraderTest { version = Version.fromString("5.2"); tester.updateVersionStatus(version); tester.upgrader().maintain(); - readyJobsTrigger.maintain(); + tester.readyJobTrigger().maintain(); assertTrue("Nothing is scheduled", tester.deploymentQueue().jobs().isEmpty()); // Monday morning: We are not blocked tester.clock().advance(Duration.ofDays(1)); // Sunday, 17:00 tester.clock().advance(Duration.ofHours(17)); // Monday, 10:00 tester.upgrader().maintain(); - readyJobsTrigger.maintain(); + tester.readyJobTrigger().maintain(); // We proceed with the new version in the expected order, not starting with the previously blocked version: // Test jobs are run with the new version, but not production as we are in the block window tester.deployAndNotify(app, applicationPackage, true, systemTest); @@ -729,6 +762,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); // Canaries upgrade and raise confidence tester.completeUpgrade(canary0, version, canaryApplicationPackage); @@ -739,6 +773,7 @@ public class UpgraderTest { // Applications with default policy start upgrading tester.clock().advance(Duration.ofMinutes(1)); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Upgrade scheduled for remaining apps", 5, tester.deploymentQueue().jobs().size()); // 4/5 applications fail, confidence is lowered and upgrade is cancelled @@ -750,9 +785,9 @@ public class UpgraderTest { assertEquals(VespaVersion.Confidence.broken, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); - // Exhaust retries and finish runs - tester.clock().advance(Duration.ofHours(1)); + // Finish runs tester.jobCompletion(systemTest).application(default0).unsuccessful().submit(); tester.jobCompletion(systemTest).application(default1).unsuccessful().submit(); tester.jobCompletion(systemTest).application(default2).unsuccessful().submit(); @@ -779,6 +814,7 @@ public class UpgraderTest { assertEquals(VespaVersion.Confidence.normal, tester.controller().versionStatus().systemVersion().get().confidence()); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); assertEquals("Upgrade scheduled for previously failing apps", 4, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(default0, version, defaultApplicationPackageV2); @@ -821,6 +857,7 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); upgrader.maintain(); + tester.readyJobTrigger().maintain(); assertEquals(2, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(canary0, version, "canary"); @@ -829,16 +866,19 @@ public class UpgraderTest { // Next run upgrades a subset upgrader.maintain(); + tester.readyJobTrigger().maintain(); assertEquals(2, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(default0, version, "default"); tester.completeUpgrade(default2, version, "default"); // Remaining applications upgraded upgrader.maintain(); + tester.readyJobTrigger().maintain(); assertEquals(2, tester.deploymentQueue().jobs().size()); tester.completeUpgrade(default1, version, "default"); tester.completeUpgrade(default3, version, "default"); upgrader.maintain(); + tester.readyJobTrigger().maintain(); assertTrue("All jobs consumed", tester.deploymentQueue().jobs().isEmpty()); } @@ -859,13 +899,10 @@ public class UpgraderTest { version = Version.fromString("5.1"); tester.updateVersionStatus(version); tester.upgrader().maintain(); + tester.readyJobTrigger().maintain(); tester.deployAndNotify(app, applicationPackage, true, systemTest); tester.deployAndNotify(app, applicationPackage, true, stagingTest); - - // Production job fails and exhausts retries, new application changes are now accepted - tester.deployAndNotify(app, applicationPackage, false, productionUsWest1); - tester.clock().advance(Duration.ofHours(1)); tester.deployAndNotify(app, applicationPackage, false, productionUsWest1); // New application change @@ -879,8 +916,9 @@ public class UpgraderTest { app.change().application().get().id().equals(applicationVersion)); // Deployment completes - tester.deployAndNotify(app, applicationPackage, true, systemTest); - tester.deployAndNotify(app, applicationPackage, true, stagingTest); + tester.deployAndNotify(app, applicationPackage, true, false, systemTest); + tester.deployAndNotify(app, applicationPackage, true, false, stagingTest); + tester.jobCompletion(productionUsWest1).application(app).unsuccessful().submit(); tester.deployAndNotify(app, applicationPackage, true, productionUsWest1); assertTrue("All jobs consumed", tester.deploymentQueue().jobs().isEmpty()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/application-without-project-id.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/application-without-project-id.json index 63832531c7d..31949cce282 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/application-without-project-id.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/application-without-project-id.json @@ -1,6 +1,6 @@ { "id": "tenant1:app1:default", - "deploymentSpecField": "<deployment version='1.0'>\n <test />\n <staging />\n <prod>\n <region active=\"true\">cd-us-central-1</region>\n <region active=\"true\">cd-us-central-2</region>\n </prod>\n</deployment>\n", + "deploymentSpecField": "<deployment version='1.0'>\n <test />\n <staging />\n <prod>\n <region active=\"true\">us-central-1</region>\n <region active=\"true\">us-west-1</region>\n </prod>\n</deployment>\n", "validationOverrides": "<deployment version='1.0'/>", "deployments": [], "deploymentJobs": { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java index b810c3adeb5..8e60e63e873 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java @@ -126,6 +126,7 @@ public class ContainerControllerTester { throw new RuntimeException(e); } controller().applications().notifyJobCompletion(jobReport); + controller().applications().deploymentTrigger().triggerReadyJobs(); } private AthenzDomain addTenantAthenzDomain(String domainName, String userName) { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index 0ae9cf767d0..b9eef2069d9 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.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.restapi.application; +import com.google.common.base.Functions; import com.yahoo.application.container.handler.Request; import com.yahoo.application.container.handler.Response; import com.yahoo.component.Version; @@ -173,22 +174,6 @@ public class ApplicationApiTest extends ControllerContainerTest { addUserToHostedOperatorRole(HostedAthenzIdentities.from(HOSTED_VESPA_OPERATOR)); - // POST triggering of a full deployment to an application (if version is omitted, current system version is used) - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/deploying", POST) - .userIdentity(HOSTED_VESPA_OPERATOR) - .data("6.1.0"), - new File("application-deployment.json")); - - // DELETE (cancel) ongoing change - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/deploying", DELETE) - .userIdentity(HOSTED_VESPA_OPERATOR), - new File("application-deployment-cancelled.json")); - - // DELETE (cancel) again is a no-op - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/deploying", DELETE) - .userIdentity(HOSTED_VESPA_OPERATOR), - new File("application-deployment-cancelled-no-op.json")); - // POST (deploy) an application to a zone - manual user deployment HttpEntity entity = createApplicationDeployData(applicationPackage, Optional.empty()); tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/dev/region/us-west-1/instance/default/deploy", POST) @@ -285,6 +270,21 @@ public class ApplicationApiTest extends ControllerContainerTest { .recursive("true"), new File("application1-recursive.json")); + // DELETE (cancel) ongoing change + tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/deploying", DELETE) + .userIdentity(HOSTED_VESPA_OPERATOR), + new File("application-deployment-cancelled.json")); + + // DELETE (cancel) again is a no-op + tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/deploying", DELETE) + .userIdentity(HOSTED_VESPA_OPERATOR), + new File("application-deployment-cancelled-no-op.json")); + + // POST triggering of a full deployment to an application (if version is omitted, current system version is used) + tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/deploying", POST) + .userIdentity(HOSTED_VESPA_OPERATOR) + .data("6.1.0"), + new File("application-deployment.json")); // POST a 'restart application' command tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/corp-us-east-1/instance/default/restart", POST) @@ -792,7 +792,7 @@ public class ApplicationApiTest extends ControllerContainerTest { Version vespaVersion = new Version("6.1"); // system version from mock config server client - BuildJob job = new BuildJob(this::notifyCompletion, tester.artifactRepository()) + BuildJob job = new BuildJob(report -> notifyCompletion(report, tester), tester.artifactRepository()) .application(app) .projectId(projectId); job.type(DeploymentJobs.JobType.component).uploadArtifact(applicationPackage).submit(); @@ -846,7 +846,7 @@ public class ApplicationApiTest extends ControllerContainerTest { .build(); // Report job failing with out of capacity - BuildJob job = new BuildJob(this::notifyCompletion, tester.artifactRepository()) + BuildJob job = new BuildJob(report -> notifyCompletion(report, tester), tester.artifactRepository()) .application(app) .projectId(projectId); job.type(DeploymentJobs.JobType.component).uploadArtifact(applicationPackage).submit(); @@ -866,12 +866,13 @@ public class ApplicationApiTest extends ControllerContainerTest { assertEquals(DeploymentJobs.JobError.outOfCapacity, jobStatus.jobError().get()); } - private void notifyCompletion(DeploymentJobs.JobReport report) { + private void notifyCompletion(DeploymentJobs.JobReport report, ContainerControllerTester tester) { assertResponse(request("/application/v4/tenant/tenant1/application/application1/jobreport", POST) .userIdentity(HOSTED_VESPA_OPERATOR) .data(asJson(report)) .get(), 200, "{\"message\":\"ok\"}"); + tester.controller().applications().deploymentTrigger().triggerReadyJobs(); } private static byte[] asJson(DeploymentJobs.JobReport report) { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-deployment-cancelled.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-deployment-cancelled.json index 3b6d8ed71e9..bc09003d86f 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-deployment-cancelled.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-deployment-cancelled.json @@ -1 +1 @@ -{"message":"Cancelled upgrade to 6.1 for application 'tenant1.application1'"} +{"message":"Cancelled application change to 1.0.42-commit1 for application 'tenant1.application1'"} diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-without-change-multiple-deployments.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-without-change-multiple-deployments.json index 9cd9329e36f..06d562d40f2 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-without-change-multiple-deployments.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-without-change-multiple-deployments.json @@ -48,7 +48,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" }, "lastCompleted": { @@ -62,7 +62,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" }, "lastSuccess": { @@ -76,7 +76,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" } }, @@ -94,7 +94,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" }, "lastCompleted": { @@ -108,7 +108,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" }, "lastSuccess": { @@ -122,7 +122,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" } }, @@ -140,7 +140,7 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", "at": "(ignore)" }, "lastCompleted": { @@ -154,7 +154,7 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", "at": "(ignore)" }, "lastSuccess": { @@ -168,7 +168,7 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", "at": "(ignore)" } }, @@ -186,7 +186,7 @@ "gitCommit": "commit1" } }, - "reason": "production-us-west-1 completed", + "reason": "Available change in production-us-west-1", "at": "(ignore)" }, "lastCompleted": { @@ -200,7 +200,7 @@ "gitCommit": "commit1" } }, - "reason": "production-us-west-1 completed", + "reason": "Available change in production-us-west-1", "at": "(ignore)" }, "lastSuccess": { @@ -214,7 +214,7 @@ "gitCommit": "commit1" } }, - "reason": "production-us-west-1 completed", + "reason": "Available change in production-us-west-1", "at": "(ignore)" } } 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 3d8d8a4b34d..a067d19fb6a 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 @@ -58,7 +58,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" }, "lastCompleted": { @@ -72,7 +72,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" }, "lastSuccess": { @@ -86,7 +86,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" } }, @@ -104,7 +104,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" }, "lastCompleted": { @@ -118,7 +118,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" }, "lastSuccess": { @@ -132,7 +132,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" } }, @@ -150,7 +150,7 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", "at": "(ignore)" }, "lastCompleted": { @@ -164,7 +164,7 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", "at": "(ignore)" }, "firstFailing": { @@ -178,7 +178,25 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", + "at": "(ignore)" + } + }, + { + "type": "production-us-east-3", + "success": false, + "lastTriggered": { + "id": -1, + "version": "(ignore)", + "revision": { + "hash": "(ignore)", + "source": { + "gitRepository": "repository1", + "gitBranch": "master", + "gitCommit": "commit1" + } + }, + "reason": "Available change in production-corp-us-east-1", "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 16bd5c2e720..3c4cc32111c 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 @@ -58,7 +58,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" }, "lastCompleted": { @@ -72,7 +72,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" }, "lastSuccess": { @@ -86,7 +86,7 @@ "gitCommit": "commit1" } }, - "reason": "component completed", + "reason": "Available change in component", "at": "(ignore)" } }, @@ -104,7 +104,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" }, "lastCompleted": { @@ -118,7 +118,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" }, "lastSuccess": { @@ -132,7 +132,7 @@ "gitCommit": "commit1" } }, - "reason": "system-test completed", + "reason": "Available change in system-test", "at": "(ignore)" } }, @@ -150,7 +150,7 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", "at": "(ignore)" }, "lastCompleted": { @@ -164,7 +164,7 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", "at": "(ignore)" }, "firstFailing": { @@ -178,7 +178,25 @@ "gitCommit": "commit1" } }, - "reason": "staging-test completed", + "reason": "Available change in staging-test", + "at": "(ignore)" + } + }, + { + "type": "production-us-east-3", + "success": false, + "lastTriggered": { + "id": -1, + "version": "(ignore)", + "revision": { + "hash": "(ignore)", + "source": { + "gitRepository": "repository1", + "gitBranch": "master", + "gitCommit": "commit1" + } + }, + "reason": "Available change in production-corp-us-east-1", "at": "(ignore)" } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java index 4de3b9abd5b..743baf76759 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java @@ -69,6 +69,7 @@ public class DeploymentApiTest extends ControllerContainerTest { // Applications upgrade, 1/2 succeed tester.upgrader().maintain(); + tester.controller().applications().deploymentTrigger().triggerReadyJobs(); deployCompletely(failingApplication, applicationPackage, projectId, false); deployCompletely(productionApplication, applicationPackage, projectId, true); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java index 27e26e3267a..14f5d00ec88 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/versions/VersionStatusTest.java @@ -156,12 +156,6 @@ public class VersionStatusTest { assertEquals("One canary failed: Broken", Confidence.broken, confidence(tester.controller(), version1)); - // Finish running jobs - tester.deployAndNotify(canary2, DeploymentTester.applicationPackage("canary"), false, systemTest); - tester.clock().advance(Duration.ofHours(1)); - tester.deployAndNotify(canary1, DeploymentTester.applicationPackage("canary"), false, productionUsWest1); - tester.deployAndNotify(canary2, DeploymentTester.applicationPackage("canary"), false, systemTest); - // New version is released Version version2 = new Version("5.2"); tester.upgradeSystem(version2); @@ -170,6 +164,7 @@ public class VersionStatusTest { // All canaries upgrade successfully tester.completeUpgrade(canary0, version2, "canary"); + tester.jobCompletion(productionUsWest1).application(canary1).unsuccessful().submit(); tester.completeUpgrade(canary1, version2, "canary"); assertEquals("Confidence for remains unchanged for version1: Broken", @@ -178,6 +173,7 @@ public class VersionStatusTest { Confidence.low, confidence(tester.controller(), version2)); // Remaining canary upgrades to version2 which raises confidence to normal and more apps upgrade + tester.jobCompletion(systemTest).application(canary2).unsuccessful().submit(); tester.completeUpgrade(canary2, version2, "canary"); tester.upgradeSystem(version2); assertEquals("Canaries have upgraded: Normal", diff --git a/docker/run/run-vespa-internal.sh b/docker/run/run-vespa-internal.sh index dce0d4dff97..cf00c8a32dc 100755 --- a/docker/run/run-vespa-internal.sh +++ b/docker/run/run-vespa-internal.sh @@ -14,7 +14,7 @@ cd $DIR # Workaround until we figure out why rpm does not set the ownership. chown -R vespa:vespa /opt/vespa -export VESPA_CONFIG_SERVERS=$(hostname) +export VESPA_CONFIGSERVERS=$(hostname) /opt/vespa/bin/vespa-start-configserver # Give config server some time to come up before starting services diff --git a/eval/src/tests/tensor/dense_dot_product_function/dense_dot_product_function_test.cpp b/eval/src/tests/tensor/dense_dot_product_function/dense_dot_product_function_test.cpp index 37f9602565d..60830e4abd7 100644 --- a/eval/src/tests/tensor/dense_dot_product_function/dense_dot_product_function_test.cpp +++ b/eval/src/tests/tensor/dense_dot_product_function/dense_dot_product_function_test.cpp @@ -123,7 +123,11 @@ EvalFixture::ParamRepo make_params() { .add("v07_x3_a", spec({x(3)}, MyVecSeq(8.0)), "any") .add("v08_x3_u", spec({x(3)}, MyVecSeq(9.0)), "tensor(x[])") .add("v09_x4_u", spec({x(4)}, MyVecSeq(3.0)), "tensor(x[])") - .add("m01_x3y3", spec({x(3),y(3)}, MyVecSeq(0))); + .add("m01_x3y3", spec({x(3),y(3)}, MyVecSeq(1.0))) + .add("m02_x2y3", spec({x(2),y(3)}, MyVecSeq(2.0))) + .add("m03_x3y2", spec({x(3),y(2)}, MyVecSeq(3.0))) + .add("m04_xuy3", spec({x(3),y(3)}, MyVecSeq(4.0)), "tensor(x[],y[3])") + .add("m05_x3yu", spec({x(3),y(3)}, MyVecSeq(5.0)), "tensor(x[3],y[])"); } EvalFixture::ParamRepo param_repo = make_params(); @@ -183,6 +187,59 @@ TEST("require that expressions similar to dot product are not optimized") { // TEST_DO(assertNotOptimized("reduce(join(v02_x3,v03_x3,f(x,y)(y*x)),sum)")); } +TEST("require that multi-dimensional dot product can be optimized") { + TEST_DO(assertOptimized("reduce(m01_x3y3*m02_x2y3,sum)")); + TEST_DO(assertOptimized("reduce(m02_x2y3*m01_x3y3,sum)")); + TEST_DO(assertOptimized("reduce(m01_x3y3*m04_xuy3,sum)")); + TEST_DO(assertOptimized("reduce(m04_xuy3*m01_x3y3,sum)")); + TEST_DO(assertOptimized("reduce(m04_xuy3*m04_xuy3,sum)")); +} + +TEST("require that result must be double to trigger optimization") { + TEST_DO(assertOptimized("reduce(m01_x3y3*m01_x3y3,sum,x,y)")); + TEST_DO(assertNotOptimized("reduce(m01_x3y3*m01_x3y3,sum,x)")); + TEST_DO(assertNotOptimized("reduce(m01_x3y3*m01_x3y3,sum,y)")); +} + +TEST("require that additional dimensions must have matching size") { + TEST_DO(assertOptimized("reduce(m01_x3y3*m01_x3y3,sum)")); + TEST_DO(assertNotOptimized("reduce(m01_x3y3*m03_x3y2,sum)")); + TEST_DO(assertNotOptimized("reduce(m03_x3y2*m01_x3y3,sum)")); + TEST_DO(assertNotOptimized("reduce(m01_x3y3*m05_x3yu,sum)")); + TEST_DO(assertNotOptimized("reduce(m05_x3yu*m01_x3y3,sum)")); +} + +void verify_compatible(const vespalib::string &a, const vespalib::string &b) { + auto a_type = ValueType::from_spec(a); + auto b_type = ValueType::from_spec(b); + EXPECT_TRUE(!a_type.is_error()); + EXPECT_TRUE(!b_type.is_error()); + EXPECT_TRUE(DenseDotProductFunction::compatible_types(ValueType::double_type(), a_type, b_type)); + EXPECT_TRUE(DenseDotProductFunction::compatible_types(ValueType::double_type(), b_type, a_type)); +} + +void verify_not_compatible(const vespalib::string &a, const vespalib::string &b) { + auto a_type = ValueType::from_spec(a); + auto b_type = ValueType::from_spec(b); + EXPECT_TRUE(!a_type.is_error()); + EXPECT_TRUE(!b_type.is_error()); + EXPECT_TRUE(!DenseDotProductFunction::compatible_types(ValueType::double_type(), a_type, b_type)); + EXPECT_TRUE(!DenseDotProductFunction::compatible_types(ValueType::double_type(), b_type, a_type)); +} + +TEST("require that type compatibility test is appropriate") { + TEST_DO(verify_compatible("tensor(x[5])", "tensor(x[5])")); + TEST_DO(verify_not_compatible("tensor(x[5])", "tensor(y[5])")); + TEST_DO(verify_compatible("tensor(x[5])", "tensor(x[3])")); + TEST_DO(verify_compatible("tensor(x[])", "tensor(x[3])")); + TEST_DO(verify_compatible("tensor(x[3],y[7],z[9])", "tensor(x[5],y[7],z[9])")); + TEST_DO(verify_compatible("tensor(x[3],y[7],z[9])", "tensor(x[],y[7],z[9])")); + TEST_DO(verify_not_compatible("tensor(x[5],y[7],z[9])", "tensor(x[5],y[5],z[9])")); + TEST_DO(verify_not_compatible("tensor(x[5],y[],z[9])", "tensor(x[5],y[7],z[9])")); + TEST_DO(verify_not_compatible("tensor(x[5],y[7],z[9])", "tensor(x[5],y[7],z[5])")); + TEST_DO(verify_not_compatible("tensor(x[5],y[7],z[])", "tensor(x[5],y[7],z[9])")); +} + //----------------------------------------------------------------------------- TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/eval/src/vespa/eval/tensor/dense/dense_dot_product_function.cpp b/eval/src/vespa/eval/tensor/dense/dense_dot_product_function.cpp index ae217935fd9..859a7092ce2 100644 --- a/eval/src/vespa/eval/tensor/dense/dense_dot_product_function.cpp +++ b/eval/src/vespa/eval/tensor/dense/dense_dot_product_function.cpp @@ -33,17 +33,6 @@ void my_dot_product_op(eval::InterpretedFunction::State &state, uint64_t param) state.pop_pop_push(state.stash.create<eval::DoubleValue>(result)); } -bool is1dDenseTensor(const ValueType &type) { - return (type.is_dense() && (type.dimensions().size() == 1)); -} - -bool isDenseDotProduct(const ValueType &res, const ValueType &lhsType, const ValueType &rhsType) { - return (res.is_double() && - is1dDenseTensor(lhsType) && - is1dDenseTensor(rhsType) && - (lhsType.dimensions()[0].name == rhsType.dimensions()[0].name)); -} - } // namespace vespalib::tensor::<unnamed> DenseDotProductFunction::DenseDotProductFunction(const eval::TensorFunction &lhs_in, @@ -59,16 +48,38 @@ DenseDotProductFunction::compile_self(Stash &) const return eval::InterpretedFunction::Instruction(my_dot_product_op, (uint64_t)(_hwAccelerator.get())); } +bool +DenseDotProductFunction::compatible_types(const ValueType &res, const ValueType &lhs, const ValueType &rhs) +{ + if (!res.is_double() || !lhs.is_dense() || !rhs.is_dense() || + (lhs.dimensions().size() != rhs.dimensions().size()) || + (lhs.dimensions().empty())) + { + return false; + } + for (size_t i = 0; i < lhs.dimensions().size(); ++i) { + const auto &ldim = lhs.dimensions()[i]; + const auto &rdim = rhs.dimensions()[i]; + bool first = (i == 0); + bool name_mismatch = (ldim.name != rdim.name); + bool size_mismatch = ((ldim.size != rdim.size) || !ldim.is_bound()); + if (name_mismatch || (!first && size_mismatch)) { + return false; + } + } + return true; +} + const TensorFunction & DenseDotProductFunction::optimize(const eval::TensorFunction &expr, Stash &stash) { - const Reduce *reduce = as<Reduce>(expr); + auto reduce = as<Reduce>(expr); if (reduce && (reduce->aggr() == Aggr::SUM)) { - const Join *join = as<Join>(reduce->child()); + auto join = as<Join>(reduce->child()); if (join && (join->function() == Mul::f)) { const TensorFunction &lhs = join->lhs(); const TensorFunction &rhs = join->rhs(); - if (isDenseDotProduct(expr.result_type(), lhs.result_type(), rhs.result_type())) { + if (compatible_types(expr.result_type(), lhs.result_type(), rhs.result_type())) { return stash.create<DenseDotProductFunction>(lhs, rhs); } } diff --git a/eval/src/vespa/eval/tensor/dense/dense_dot_product_function.h b/eval/src/vespa/eval/tensor/dense/dense_dot_product_function.h index 46b04a446d4..d6181d33887 100644 --- a/eval/src/vespa/eval/tensor/dense/dense_dot_product_function.h +++ b/eval/src/vespa/eval/tensor/dense/dense_dot_product_function.h @@ -14,12 +14,13 @@ class DenseDotProductFunction : public eval::tensor_function::Op2 { private: hwaccelrated::IAccelrated::UP _hwAccelerator; - + using ValueType = eval::ValueType; public: DenseDotProductFunction(const eval::TensorFunction &lhs_in, const eval::TensorFunction &rhs_in); eval::InterpretedFunction::Instruction compile_self(Stash &stash) const override; bool result_is_mutable() const override { return true; } + static bool compatible_types(const ValueType &res, const ValueType &lhs, const ValueType &rhs); static const eval::TensorFunction &optimize(const eval::TensorFunction &expr, Stash &stash); }; diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/CompressedFileReference.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/CompressedFileReference.java index c4cd2a073c1..a1ad5c8a200 100644 --- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/CompressedFileReference.java +++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/CompressedFileReference.java @@ -64,9 +64,11 @@ public class CompressedFileReference { static void decompress(File inputFile, File outputDir) throws IOException { log.log(LogLevel.DEBUG, () -> "Decompressing '" + inputFile + "' into '" + outputDir + "'"); - ArchiveInputStream ais = new TarArchiveInputStream(new GZIPInputStream(new FileInputStream(inputFile))); - decompress(ais, outputDir); - ais.close(); + try (ArchiveInputStream ais = new TarArchiveInputStream(new GZIPInputStream(new FileInputStream(inputFile)))) { + decompress(ais, outputDir); + } catch (IllegalArgumentException e) { + throw new RuntimeException("Unable to decompress '" + inputFile.getAbsolutePath() + "': " + e.getMessage()); + } } private static void decompress(ArchiveInputStream archiveInputStream, File outputFile) throws IOException { @@ -95,7 +97,8 @@ public class CompressedFileReference { entries++; } if (entries == 0) { - log.log(LogLevel.WARNING, "Not able to read any entries from " + outputFile.getName()); + throw new IllegalArgumentException("Not able to read any entries from stream (" + + archiveInputStream.getBytesRead() + " bytes read from stream)"); } } diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileDownloader.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileDownloader.java index f2a82ac6ead..c9f29197854 100644 --- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileDownloader.java +++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileDownloader.java @@ -78,10 +78,6 @@ public class FileDownloader { } } - void receiveFile(FileReferenceData fileReferenceData) { - fileReferenceDownloader.receiveFile(fileReferenceData); - } - double downloadStatus(FileReference fileReference) { return fileReferenceDownloader.downloadStatus(fileReference.value()); } diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java index 83cf4e1ad80..26a1cad2220 100644 --- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java +++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java @@ -185,38 +185,6 @@ public class FileReceiver { return methods; } - void receiveFile(FileReferenceData fileReferenceData) { - long xxHashFromContent = fileReferenceData.xxhash(); - if (xxHashFromContent != fileReferenceData.xxhash()) { - throw new RuntimeException("xxhash from content (" + xxHashFromContent + ") is not equal to xxhash in request (" + fileReferenceData.xxhash() + ")"); - } - - File fileReferenceDir = new File(downloadDirectory, fileReferenceData.fileReference().value()); - // file might be a directory (and then type is compressed) - File file = new File(fileReferenceDir, fileReferenceData.filename()); - try { - File tempDownloadedDir = Files.createTempDirectory(tmpDirectory.toPath(), "downloaded").toFile(); - File tempFile = new File(tempDownloadedDir, fileReferenceData.filename()); - Files.write(tempFile.toPath(), fileReferenceData.content().array()); - - // Unpack if necessary - if (fileReferenceData.type() == FileReferenceData.Type.compressed) { - File decompressedDir = Files.createTempDirectory(tempDownloadedDir.toPath(), "decompressed").toFile(); - log.log(LogLevel.DEBUG, () -> "Compressed file, unpacking " + tempFile + " to " + decompressedDir); - CompressedFileReference.decompress(tempFile, decompressedDir); - moveFileToDestination(decompressedDir, fileReferenceDir); - } else { - log.log(LogLevel.DEBUG, () -> "Uncompressed file, moving to " + file.getAbsolutePath()); - Files.createDirectories(fileReferenceDir.toPath()); - moveFileToDestination(tempFile, file); - } - downloader.completedDownloading(fileReferenceData.fileReference(), file); - } catch (IOException e) { - log.log(LogLevel.ERROR, "Failed writing file: " + e.getMessage(), e); - throw new RuntimeException("Failed writing file: ", e); - } - } - private static void moveFileToDestination(File tempFile, File destination) { try { Files.move(tempFile.toPath(), destination.toPath()); diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java index 1008df229f1..04e5f1a9577 100644 --- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java +++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java @@ -5,7 +5,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.ListenableFuture; import com.yahoo.concurrent.DaemonThreadFactory; import com.yahoo.config.FileReference; -import com.yahoo.jrt.ErrorCode; import com.yahoo.jrt.Int32Value; import com.yahoo.jrt.Request; import com.yahoo.jrt.StringValue; @@ -43,14 +42,13 @@ public class FileReferenceDownloader { private final Map<FileReference, Double> downloadStatus = new HashMap<>(); // between 0 and 1 private final Duration downloadTimeout; private final Duration sleepBetweenRetries; - private final FileReceiver fileReceiver; - FileReferenceDownloader(File downloadDirectory, File tmpDirectory, ConnectionPool connectionPool, - Duration timeout, Duration sleepBetweenRetries) { + FileReferenceDownloader(File downloadDirectory, File tmpDirectory, ConnectionPool connectionPool, Duration timeout, Duration sleepBetweenRetries) { this.connectionPool = connectionPool; this.downloadTimeout = timeout; this.sleepBetweenRetries = sleepBetweenRetries; - this.fileReceiver = new FileReceiver(connectionPool.getSupervisor(), this, downloadDirectory, tmpDirectory); + // Needed to receive RPC calls receiveFile* from server after asking for files + new FileReceiver(connectionPool.getSupervisor(), this, downloadDirectory, tmpDirectory); } private void startDownload(Duration timeout, FileReferenceDownload fileReferenceDownload) { @@ -86,10 +84,6 @@ public class FileReferenceDownloader { downloadExecutor.submit(() -> startDownload(downloadTimeout, fileReferenceDownload)); } - void receiveFile(FileReferenceData fileReferenceData) { - fileReceiver.receiveFile(fileReferenceData); - } - void completedDownloading(FileReference fileReference, File file) { synchronized (downloads) { FileReferenceDownload download = downloads.get(fileReference); @@ -130,11 +124,8 @@ public class FileReferenceDownloader { } } else { log.log(LogLevel.DEBUG, "Request failed. Req: " + request + "\nSpec: " + connection.getAddress() + - ", error code: " + request.errorCode()); - if (request.isError() && request.errorCode() == ErrorCode.CONNECTION || request.errorCode() == ErrorCode.TIMEOUT) { - log.log(LogLevel.DEBUG, "Mark connection " + connection.getAddress() + " with error"); - connectionPool.setError(connection, request.errorCode()); - } + ", error code: " + request.errorCode() + ", set error for connection and use another for next request"); + connectionPool.setError(connection, request.errorCode()); return false; } } diff --git a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java index e0ef2ecf7e4..4351b796624 100644 --- a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java +++ b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java @@ -13,11 +13,14 @@ import com.yahoo.jrt.Transport; import com.yahoo.text.Utf8; import com.yahoo.vespa.config.Connection; import com.yahoo.vespa.config.ConnectionPool; +import net.jpountz.xxhash.XXHash64; +import net.jpountz.xxhash.XXHashFactory; import org.junit.Before; import org.junit.Test; import java.io.File; import java.io.IOException; +import java.nio.ByteBuffer; import java.nio.file.Files; import java.nio.file.Path; import java.time.Duration; @@ -225,7 +228,11 @@ public class FileDownloaderTest { } private void receiveFile(FileReference fileReference, String filename, FileReferenceData.Type type, byte[] content) { - fileDownloader.receiveFile(new FileReferenceDataBlob(fileReference, filename, type, content)); + XXHash64 hasher = XXHashFactory.fastestInstance().hash64(); + FileReceiver.Session session = + new FileReceiver.Session(downloadDir, tempDir, 1, fileReference, type, filename, content.length); + session.addPart(0, content); + session.close(hasher.hash(ByteBuffer.wrap(content), 0)); } private static class MockConnection implements ConnectionPool, com.yahoo.vespa.config.Connection { diff --git a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReceiverTest.java b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReceiverTest.java index 78fc094a9ef..8dda0bcce66 100644 --- a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReceiverTest.java +++ b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileReceiverTest.java @@ -14,14 +14,10 @@ import org.junit.rules.TemporaryFolder; import static org.junit.Assert.assertEquals; import java.io.File; -import java.io.FileOutputStream; -import java.io.FileReader; import java.io.FileWriter; import java.io.IOException; import java.nio.ByteBuffer; import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; public class FileReceiverTest { private File root; @@ -92,7 +88,7 @@ public class FileReceiverTest { assertEquals(all, Utf8.toString(allReadBytes)); } - private void transferCompressedData(FileReference ref, String fileName, byte[] data) throws IOException { + private void transferCompressedData(FileReference ref, String fileName, byte[] data) { FileReceiver.Session session = new FileReceiver.Session(root, tempDir, 1, ref, FileReferenceData.Type.compressed, fileName, data.length); session.addPart(0, data); diff --git a/node-admin/src/main/application/services.xml b/node-admin/src/main/application/services.xml index afe2888c5ef..96fe82a5b94 100644 --- a/node-admin/src/main/application/services.xml +++ b/node-admin/src/main/application/services.xml @@ -4,10 +4,6 @@ <jdisc id="node-admin" jetty="true" version="1.0"> <!-- Please update container test when changing this file --> <accesslog type="vespa" fileNamePattern="logs/vespa/node-admin/access.log.%Y%m%d%H%M%S" rotationScheme="date" symlinkName="access.log" /> - <handler id="com.yahoo.vespa.hosted.node.admin.restapi.RestApiHandler" bundle="node-admin"> - <binding>http://*/rest/*</binding> - </handler> - <component id="node-admin" class="com.yahoo.vespa.hosted.node.admin.provider.NodeAdminProvider" bundle="node-admin"/> <component id="docker-api" class="com.yahoo.vespa.hosted.dockerapi.DockerImpl" bundle="docker-api"/> <component id="metrics-wrapper" class="com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper" bundle="docker-api"/> diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminConfig.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminConfig.java deleted file mode 100644 index c3a1f47dfc5..00000000000 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminConfig.java +++ /dev/null @@ -1,33 +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.hosted.node.admin.nodeadmin; - -import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; -import com.fasterxml.jackson.databind.ObjectMapper; - -import java.io.File; -import java.io.IOException; - -@JsonIgnoreProperties(ignoreUnknown = true) -public class NodeAdminConfig { - private static final ObjectMapper mapper = new ObjectMapper(); - - /** - * If null, the default admin component will be used. - */ - @JsonProperty("main-component") - public String mainComponent = null; - - public static NodeAdminConfig fromFile(File file) { - if (!file.exists()) { - return new NodeAdminConfig(); - } - - try { - return mapper.readValue(file, NodeAdminConfig.class); - } catch (IOException e) { - throw new RuntimeException("Failed to read " + file + " as a " + - NodeAdminConfig.class.getName(), e); - } - } -} diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminMain.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminMain.java deleted file mode 100644 index 4b806c905d9..00000000000 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminMain.java +++ /dev/null @@ -1,105 +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.hosted.node.admin.nodeadmin; - -import com.yahoo.component.ComponentId; -import com.yahoo.component.provider.ComponentRegistry; -import com.yahoo.concurrent.classlock.ClassLocking; -import com.yahoo.log.LogLevel; -import com.yahoo.vespa.hosted.dockerapi.Docker; -import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper; -import com.yahoo.vespa.hosted.node.admin.component.AdminComponent; -import com.yahoo.vespa.hosted.node.admin.component.Environment; -import com.yahoo.vespa.hosted.node.admin.config.ConfigServerConfig; -import com.yahoo.vespa.hosted.node.admin.component.DockerAdminComponent; -import com.yahoo.vespa.hosted.node.admin.configserver.RealConfigServerClients; -import com.yahoo.vespa.hosted.node.admin.provider.NodeAdminStateUpdater; - -import java.io.File; -import java.util.logging.Logger; -import java.util.stream.Collectors; - -/** - * NodeAdminMain is the main component of the node admin JDisc application: - * - It will read config and check its environment to figure out its responsibilities - * - It will "start" (only) the necessary components. - * - Other components MUST NOT try to start (typically in constructor) since the features - * they provide is NOT WANTED and possibly destructive, and/or the environment may be - * incompatible. For instance, trying to contact the Docker daemon too early will be - * fatal: the node admin may not have installed and started the docker daemon. - */ -public class NodeAdminMain implements AutoCloseable { - private static final Logger logger = Logger.getLogger(NodeAdminMain.class.getName()); - - private final ComponentRegistry<AdminComponent> adminRegistry; - private final ConfigServerConfig configServerConfig; - private final Docker docker; - private final MetricReceiverWrapper metricReceiver; - private final ClassLocking classLocking; - - private AdminComponent mainAdminComponent = null; - - public NodeAdminMain(ComponentRegistry<AdminComponent> adminRegistry, - ConfigServerConfig configServerConfig, - Docker docker, - MetricReceiverWrapper metricReceiver, - ClassLocking classLocking) { - this.adminRegistry = adminRegistry; - this.configServerConfig = configServerConfig; - this.docker = docker; - this.metricReceiver = metricReceiver; - this.classLocking = classLocking; - } - - public static NodeAdminConfig getConfig() { - return NodeAdminConfig.fromFile(new File("/etc/vespa/node-admin.json")); - } - - public void start() { - NodeAdminConfig config = getConfig(); - mainAdminComponent = selectAdminComponent(config); - mainAdminComponent.enable(); - } - - private AdminComponent selectAdminComponent(NodeAdminConfig config) { - if (config.mainComponent == null) { - return new DockerAdminComponent(configServerConfig, - docker, - metricReceiver, - classLocking, - new RealConfigServerClients(new Environment(configServerConfig))); - } - - logger.log(LogLevel.INFO, () -> { - String registeredComponentsList = adminRegistry - .allComponentsById().keySet().stream() - .map(ComponentId::stringValue) - .collect(Collectors.joining(", ")); - - return String.format( - "Components registered = '%s', enabled = '%s'", - registeredComponentsList, - config.mainComponent); - }); - - AdminComponent component = adminRegistry.getComponent(config.mainComponent); - if (component == null) { - throw new IllegalArgumentException("There is no component named '" + - config.mainComponent + "'"); - } - - return component; - } - - @Override - public void close() { - if (mainAdminComponent != null) { - mainAdminComponent.disable(); - mainAdminComponent = null; - } - } - - public NodeAdminStateUpdater getNodeAdminStateUpdater() { - assert mainAdminComponent != null : "start() hasn't been called yet"; - return mainAdminComponent.getNodeAdminStateUpdater(); - } -} diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/NodeAdminProvider.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/NodeAdminProvider.java index fd8ca74644d..d49251d3754 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/NodeAdminProvider.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/NodeAdminProvider.java @@ -2,35 +2,38 @@ package com.yahoo.vespa.hosted.node.admin.provider; import com.google.inject.Inject; -import com.yahoo.component.provider.ComponentRegistry; import com.yahoo.concurrent.classlock.ClassLocking; import com.yahoo.container.di.componentgraph.Provider; import com.yahoo.vespa.hosted.dockerapi.Docker; import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper; +import com.yahoo.vespa.hosted.node.admin.component.DockerAdminComponent; +import com.yahoo.vespa.hosted.node.admin.component.Environment; import com.yahoo.vespa.hosted.node.admin.config.ConfigServerConfig; -import com.yahoo.vespa.hosted.node.admin.component.AdminComponent; -import com.yahoo.vespa.hosted.node.admin.nodeadmin.NodeAdminMain; +import com.yahoo.vespa.hosted.node.admin.configserver.RealConfigServerClients; public class NodeAdminProvider implements Provider<NodeAdminStateUpdater> { - private final NodeAdminMain nodeAdminMain; + private final DockerAdminComponent dockerAdmin; @Inject - public NodeAdminProvider(ComponentRegistry<AdminComponent> adminRegistry, - ConfigServerConfig configServerConfig, + public NodeAdminProvider(ConfigServerConfig configServerConfig, Docker docker, MetricReceiverWrapper metricReceiver, ClassLocking classLocking) { - nodeAdminMain = new NodeAdminMain(adminRegistry, configServerConfig, docker, metricReceiver, classLocking); - nodeAdminMain.start(); + dockerAdmin = new DockerAdminComponent(configServerConfig, + docker, + metricReceiver, + classLocking, + new RealConfigServerClients(new Environment(configServerConfig))); + dockerAdmin.enable(); } @Override public NodeAdminStateUpdater get() { - return nodeAdminMain.getNodeAdminStateUpdater(); + return dockerAdmin.getNodeAdminStateUpdater(); } @Override public void deconstruct() { - nodeAdminMain.close(); + dockerAdmin.disable(); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index d957f4c0e25..0c256f9bee8 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -79,21 +79,21 @@ public class NodeRepository extends AbstractComponent { private final DockerImage dockerImage; /** - * Creates a node repository form a zookeeper provider. + * Creates a node repository from a zookeeper provider. * This will use the system time to make time-sensitive decisions */ @Inject public NodeRepository(NodeRepositoryConfig config, NodeFlavors flavors, Curator curator, Zone zone) { - this(flavors, curator, Clock.systemUTC(), zone, new DnsNameResolver(), new DockerImage(config.dockerImage())); + this(flavors, curator, Clock.systemUTC(), zone, new DnsNameResolver(), new DockerImage(config.dockerImage()), config.useCuratorClientCache()); } /** - * Creates a node repository form a zookeeper provider and a clock instance + * Creates a node repository from a zookeeper provider and a clock instance * which will be used for time-sensitive decisions. */ public NodeRepository(NodeFlavors flavors, Curator curator, Clock clock, Zone zone, NameResolver nameResolver, - DockerImage dockerImage) { - this.db = new CuratorDatabaseClient(flavors, curator, clock, zone); + DockerImage dockerImage, boolean useCuratorClientCache) { + this.db = new CuratorDatabaseClient(flavors, curator, clock, zone, useCuratorClientCache); this.curator = curator; this.clock = clock; this.flavors = flavors; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java index 65d34eb8e83..f7d547f1076 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java @@ -38,7 +38,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent { private final NodeFailer nodeFailer; private final PeriodicApplicationMaintainer periodicApplicationMaintainer; private final OperatorChangeApplicationMaintainer operatorChangeApplicationMaintainer; - private final ZooKeeperAccessMaintainer zooKeeperAccessMaintainer; private final ReservationExpirer reservationExpirer; private final InactiveExpirer inactiveExpirer; private final RetiredExpirer retiredExpirer; @@ -70,7 +69,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent { nodeFailer = new NodeFailer(deployer, hostLivenessTracker, serviceMonitor, nodeRepository, durationFromEnv("fail_grace").orElse(defaults.failGrace), clock, orchestrator, throttlePolicyFromEnv("throttle_policy").orElse(defaults.throttlePolicy), metric, jobControl, configserverConfig); periodicApplicationMaintainer = new PeriodicApplicationMaintainer(deployer, nodeRepository, durationFromEnv("periodic_redeploy_interval").orElse(defaults.periodicRedeployInterval), jobControl); operatorChangeApplicationMaintainer = new OperatorChangeApplicationMaintainer(deployer, nodeRepository, clock, durationFromEnv("operator_change_redeploy_interval").orElse(defaults.operatorChangeRedeployInterval), jobControl); - zooKeeperAccessMaintainer = new ZooKeeperAccessMaintainer(nodeRepository, curator, durationFromEnv("zookeeper_access_maintenance_interval").orElse(defaults.zooKeeperAccessMaintenanceInterval), jobControl); reservationExpirer = new ReservationExpirer(nodeRepository, clock, durationFromEnv("reservation_expiry").orElse(defaults.reservationExpiry), jobControl); retiredExpirer = new RetiredExpirer(nodeRepository, orchestrator, deployer, clock, durationFromEnv("retired_interval").orElse(defaults.retiredInterval), durationFromEnv("retired_expiry").orElse(defaults.retiredExpiry), jobControl); inactiveExpirer = new InactiveExpirer(nodeRepository, clock, durationFromEnv("inactive_expiry").orElse(defaults.inactiveExpiry), jobControl); @@ -91,7 +89,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent { nodeFailer.deconstruct(); periodicApplicationMaintainer.deconstruct(); operatorChangeApplicationMaintainer.deconstruct(); - zooKeeperAccessMaintainer.deconstruct(); reservationExpirer.deconstruct(); inactiveExpirer.deconstruct(); retiredExpirer.deconstruct(); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainer.java deleted file mode 100644 index b392d670a77..00000000000 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainer.java +++ /dev/null @@ -1,49 +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.hosted.provision.maintenance; - -import com.yahoo.vespa.curator.Curator; -import com.yahoo.vespa.hosted.provision.Node; -import com.yahoo.vespa.hosted.provision.NodeRepository; -import com.yahoo.vespa.zookeeper.ZooKeeperServer; - -import java.time.Duration; -import java.util.HashSet; -import java.util.Set; - -/** - * Maintains the list of hosts that should be allowed to access ZooKeeper in this runtime. - * These are the zookeeper servers and all nodes in node repository. This is maintained in the background - * because nodes could be added or removed on another server. - * - * We could limit access to the <i>active</i> subset of nodes, but that - * does not seem to have any particular operational or security benefits and might make it more problematic - * for this job to be behind actual changes to the active set of nodes. - * - * @author bratseth - */ -public class ZooKeeperAccessMaintainer extends Maintainer { - - private final Curator curator; - - public ZooKeeperAccessMaintainer(NodeRepository nodeRepository, Curator curator, Duration maintenanceInterval, - JobControl jobControl) { - super(nodeRepository, maintenanceInterval, jobControl); - this.curator = curator; - } - - @Override - protected void maintain() { - Set<String> hosts = new HashSet<>(); - - for (Node node : nodeRepository().getNodes()) - hosts.add(node.hostname()); - - if ( ! hosts.isEmpty()) { // no nodes -> not a hosted instance: Pass an empty list to deactivate restriction - for (String hostPort : curator.zooKeeperEnsembleConnectionSpec().split(",")) - hosts.add(hostPort.split(":")[0]); - } - - ZooKeeperServer.setAllowedClientHostnames(hosts); - } - -} diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java index 12a0f64aee1..40580de666d 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java @@ -59,11 +59,10 @@ public class CuratorDatabaseClient { private final Zone zone; - public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, Zone zone) { + public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, Zone zone, boolean useCache) { this.nodeSerializer = new NodeSerializer(flavors); this.zone = zone; - boolean useCache = zone.system().equals(SystemName.cd); - this.curatorDatabase = new CuratorDatabase(curator, root, useCache); + this.curatorDatabase = new CuratorDatabase(curator, root, useCache || zone.system().equals(SystemName.cd)); this.clock = clock; initZK(); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/AuthorizationFilter.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/AuthorizationFilter.java index 12de9aeef30..fcefe73a8b9 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/AuthorizationFilter.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/AuthorizationFilter.java @@ -2,7 +2,6 @@ package com.yahoo.vespa.hosted.provision.restapi.v2.filter; import com.google.inject.Inject; -import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.Zone; import com.yahoo.jdisc.handler.ResponseHandler; import com.yahoo.jdisc.http.filter.DiscFilterRequest; @@ -34,7 +33,7 @@ public class AuthorizationFilter implements SecurityRequestFilter { @Inject public AuthorizationFilter(Zone zone, NodeRepository nodeRepository) { - this(new Authorizer(zone.system(), nodeRepository), rejectActionIn(zone.system())); + this(new Authorizer(zone.system(), nodeRepository), AuthorizationFilter::logAndReject); } AuthorizationFilter(BiPredicate<Principal, URI> authorizer, @@ -63,17 +62,6 @@ public class AuthorizationFilter implements SecurityRequestFilter { } } - private static BiConsumer<ErrorResponse, ResponseHandler> rejectActionIn(SystemName system) { - if (system == SystemName.cd) { - return AuthorizationFilter::logAndReject; - } - return AuthorizationFilter::log; - } - - private static void log(ErrorResponse response, @SuppressWarnings("unused") ResponseHandler handler) { - log.warning("Would reject request: " + response.getStatus() + " - " + response.message()); - } - private static void logAndReject(ErrorResponse response, ResponseHandler handler) { log.warning(response.message()); FilterUtils.write(response, handler); @@ -81,8 +69,7 @@ public class AuthorizationFilter implements SecurityRequestFilter { /** Read common name (CN) from certificate */ private static Optional<String> commonName(X509Certificate certificate) { - return X509CertificateUtils.getCommonNames(certificate).stream() - .findFirst(); + return X509CertificateUtils.getCommonNames(certificate).stream().findFirst(); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/LocalhostFilter.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/LocalhostFilter.java index 368b464afff..f9900f9b0ec 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/LocalhostFilter.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/LocalhostFilter.java @@ -10,7 +10,7 @@ import com.yahoo.vespa.hosted.provision.restapi.v2.ErrorResponse; import java.net.InetAddress; /** - * A security filter that only allows local requests. + * A security filter that only allows self-originating requests. * * @author mpolden */ diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java index 5386e3f07ff..a21d03ff603 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java @@ -48,7 +48,8 @@ public class MockNodeRepository extends NodeRepository { new MockNameResolver() .addRecord("test-container-1", "::2") .mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), + true); this.flavors = flavors; curator.setZooKeeperEnsembleConnectionSpec("cfg1:1234,cfg2:1234,cfg3:1234"); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java index 583d5658ef4..1a42a71863d 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java @@ -35,7 +35,8 @@ public class NodeRepositoryTester { curator.setZooKeeperEnsembleConnectionSpec("server1:1234,server2:5678"); nodeRepository = new NodeRepository(nodeFlavors, curator, clock, Zone.defaultZone(), new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), + true); } public NodeRepository nodeRepository() { return nodeRepository; } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java index a7873842580..84569077053 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java @@ -218,7 +218,8 @@ public class FailedExpirerTest { Zone zone = new Zone(system, environment, RegionName.defaultName()); this.nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone, new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-image")); + new DockerImage("docker-image"), + true); this.provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, Zone.defaultZone()); this.expirer = new FailedExpirer(nodeRepository, zone, clock, Duration.ofMinutes(30), new JobControl(nodeRepository.database())); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MaintenanceTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MaintenanceTester.java index 16465c620de..740e4ff1ca4 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MaintenanceTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MaintenanceTester.java @@ -35,7 +35,8 @@ public class MaintenanceTester { private final NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default"); public final NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone, new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), + true); public NodeRepository nodeRepository() { return nodeRepository; } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java index f7592bd7422..e4560ef685d 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java @@ -81,7 +81,7 @@ public class NodeFailTester { clock = new ManualClock(); curator = new MockCurator(); nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone, new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), true); provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone); hostLivenessTracker = new TestHostLivenessTracker(clock); orchestrator = new OrchestratorMock(); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java index 4e9a8f4b608..ec71b36064f 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirerTester.java @@ -72,7 +72,7 @@ public class NodeRetirerTester { NodeRetirerTester(NodeFlavors nodeFlavors) { Curator curator = new MockCurator(); nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone, new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), true); jobControl = new JobControl(nodeRepository.database()); NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone); deployer = new MockDeployer(provisioner, apps); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java index f7abd823c77..898054f23ff 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java @@ -56,7 +56,8 @@ public class OperatorChangeApplicationMaintainerTest { Zone zone = new Zone(Environment.prod, RegionName.from("us-east")); this.nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone, new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), + true); this.fixture = new Fixture(zone, nodeRepository, nodeFlavors, curator); createReadyNodes(15, nodeRepository, nodeFlavors); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java index fe87f34a3a5..ed360abc5ea 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java @@ -57,7 +57,8 @@ public class PeriodicApplicationMaintainerTest { Zone zone = new Zone(Environment.prod, RegionName.from("us-east")); this.nodeRepository = new NodeRepository(nodeFlavors, curator, new ManualClock(), zone, new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), + true); this.fixture = new Fixture(zone, nodeRepository, nodeFlavors, curator); } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirerTest.java index 5c9607e3545..0301c570ad1 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirerTest.java @@ -42,7 +42,8 @@ public class ReservationExpirerTest { NodeFlavors flavors = FlavorConfigBuilder.createDummies("default"); NodeRepository nodeRepository = new NodeRepository(flavors, curator, clock, Zone.defaultZone(), new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), + true); NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, flavors, Zone.defaultZone()); List<Node> nodes = new ArrayList<>(2); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java index f49185b04ec..ca4929ece14 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java @@ -59,7 +59,7 @@ public class RetiredExpirerTest { private final NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default"); private final NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone, new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), true); private final NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone); private final Orchestrator orchestrator = mock(Orchestrator.class); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java deleted file mode 100644 index 93cf19f5450..00000000000 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java +++ /dev/null @@ -1,68 +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.hosted.provision.maintenance; - -import com.yahoo.config.provision.NodeType; -import com.yahoo.vespa.hosted.provision.NodeRepositoryTester; -import com.yahoo.vespa.hosted.provision.node.Agent; -import com.yahoo.vespa.zookeeper.ZooKeeperServer; -import org.junit.Test; - -import java.time.Duration; -import java.util.Arrays; -import java.util.HashSet; -import java.util.Set; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; - -/** - * @author bratseth - */ -public class ZooKeeperAccessMaintainerTest { - - @Test - public void test() { - NodeRepositoryTester tester = new NodeRepositoryTester(); - tester.curator().setZooKeeperEnsembleConnectionSpec("server1:1234,server2:5678"); - ZooKeeperAccessMaintainer maintainer = new ZooKeeperAccessMaintainer(tester.nodeRepository(), - tester.curator(), Duration.ofHours(1), new JobControl(tester.nodeRepository().database())); - assertTrue(ZooKeeperServer.getAllowedClientHostnames().isEmpty()); - maintainer.maintain(); - assertTrue("We don't restrict to only config servers", ZooKeeperServer.getAllowedClientHostnames().isEmpty()); - - tester.addNode("id1", "host1", "default", NodeType.tenant); - tester.addNode("id2", "host2", "default", NodeType.tenant); - tester.addNode("id3", "host3", "default", NodeType.tenant); - maintainer.maintain(); - - assertEquals(3, tester.getNodes(NodeType.tenant).size()); - assertEquals(0, tester.getNodes(NodeType.proxy).size()); - assertEquals(asSet("host1,host2,host3,server1,server2"), ZooKeeperServer.getAllowedClientHostnames()); - - tester.addNode("proxy1", "host4", "default", NodeType.proxy); - tester.addNode("proxy2", "host5", "default", NodeType.proxy); - maintainer.maintain(); - - assertEquals(3, tester.getNodes(NodeType.tenant).size()); - assertEquals(2, tester.getNodes(NodeType.proxy).size()); - assertEquals(asSet("host1,host2,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames()); - - tester.nodeRepository().park("host2", Agent.system, "Parking to unit test"); - tester.nodeRepository().removeRecursively("host2"); - maintainer.maintain(); - - assertEquals(2, tester.getNodes(NodeType.tenant).size()); - assertEquals(2, tester.getNodes(NodeType.proxy).size()); - assertEquals(asSet("host1,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames()); - - tester.addNode("docker-host-1", "host6", "default", NodeType.host); - tester.addNode("docker-host-2", "host7", "default", NodeType.host); - maintainer.maintain(); - assertEquals(asSet("host1,host3,host4,host5,host6,host7,server1,server2"), ZooKeeperServer.getAllowedClientHostnames()); - } - - private Set<String> asSet(String s) { - return new HashSet<>(Arrays.asList(s.split(","))); - } - -} 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 066227e007f..622732460c0 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 @@ -54,7 +54,8 @@ public class MetricsReporterTest { Curator curator = new MockCurator(); NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, Clock.systemUTC(), Zone.defaultZone(), new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), + true); Node node = nodeRepository.createNode("openStackId", "hostname", Optional.empty(), nodeFlavors.getFlavorOrThrow("default"), NodeType.tenant); nodeRepository.addNodes(Collections.singletonList(node)); Node hostNode = nodeRepository.createNode("openStackId2", "parent", Optional.empty(), nodeFlavors.getFlavorOrThrow("default"), NodeType.proxy); @@ -112,7 +113,8 @@ public class MetricsReporterTest { Curator curator = new MockCurator(); NodeRepository nodeRepository = new NodeRepository(nodeFlavors, curator, Clock.systemUTC(), Zone.defaultZone(), new MockNameResolver().mockAnyLookup(), - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), + true); // Allow 4 containers Set<String> additionalIps = new HashSet<>(); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java index 9a1db707f13..050fb274fc7 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java @@ -26,8 +26,7 @@ public class CuratorDatabaseClientTest { private final Curator curator = new MockCurator(); private final CuratorDatabaseClient zkClient = new CuratorDatabaseClient( - FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), Zone.defaultZone() - ); + FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), Zone.defaultZone(), true); @Test public void can_read_stored_host_information() throws Exception { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java index ecb8a8692c4..7e982d17603 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java @@ -84,7 +84,7 @@ public class ProvisioningTester { this.clock = new ManualClock(); this.curator = curator; this.nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone, nameResolver, - new DockerImage("docker-registry.domain.tld:8080/dist/vespa")); + new DockerImage("docker-registry.domain.tld:8080/dist/vespa"), true); this.orchestrator = mock(Orchestrator.class); doThrow(new RuntimeException()).when(orchestrator).acquirePermissionToRemove(any()); this.provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/AuthorizationFilterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/AuthorizationFilterTest.java index 1d59ed52b67..c6203c76347 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/AuthorizationFilterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/AuthorizationFilterTest.java @@ -22,7 +22,7 @@ public class AuthorizationFilterTest { @Before public void before() { - tester = filterTester(SystemName.cd); + tester = filterTester(SystemName.main); } @Test @@ -43,14 +43,6 @@ public class AuthorizationFilterTest { tester.assertSuccess(new Request(Method.GET, "/nodes/v2/node/foo").commonName("foo")); } - // TODO: Remove once filter applies to all systems - @Test - public void filter_does_nothing_in_main_system() { - FilterTester tester = filterTester(SystemName.main); - tester.assertSuccess(new Request(Method.GET, "/").commonName("foo")); - tester.assertSuccess(new Request(Method.GET, "/nodes/v2/node/bar").commonName("foo")); - } - private static FilterTester filterTester(SystemName system) { Zone zone = new Zone(system, Environment.prod, RegionName.defaultName()); return new FilterTester(new AuthorizationFilter(zone, new MockNodeRepository(new MockCurator(), diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/FilterTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/FilterTester.java index 5cd01755c26..3fdff46933c 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/FilterTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/FilterTester.java @@ -87,8 +87,7 @@ public class FilterTester { Instant now = Instant.now(); X500Principal subject = new X500Principal("CN=" + commonName); return X509CertificateBuilder - .fromKeypair( - keyPair, subject, now, now.plus(Duration.ofDays(30)), SHA256_WITH_RSA, now.toEpochMilli()) + .fromKeypair(keyPair, subject, now, now.plus(Duration.ofDays(30)), SHA256_WITH_RSA, now.toEpochMilli()) .setBasicConstraints(true, true) .build(); } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/LocalhostFilterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/LocalhostFilterTest.java index b4e446f6818..cb1ac2ade72 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/LocalhostFilterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/LocalhostFilterTest.java @@ -30,7 +30,7 @@ public class LocalhostFilterTest { tester.assertSuccess(new Request(Method.GET, "/").remoteAddr("127.127.0.1")); tester.assertSuccess(new Request(Method.GET, "/").remoteAddr("0:0:0:0:0:0:0:1")); - // Allow requests originating from same host + // Allow requests originating from self tester.assertSuccess(new Request(Method.GET, "/").localAddr("1.3.3.7").remoteAddr("1.3.3.7")); } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json index 979d846704e..28e28f9678e 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/maintenance.json @@ -34,9 +34,6 @@ "name":"MetricsReporter" }, { - "name":"ZooKeeperAccessMaintainer" - }, - { "name":"NodeFailer" } ], diff --git a/storage/src/tests/common/testnodestateupdater.cpp b/storage/src/tests/common/testnodestateupdater.cpp index 0547980b847..c4afda1a5ad 100644 --- a/storage/src/tests/common/testnodestateupdater.cpp +++ b/storage/src/tests/common/testnodestateupdater.cpp @@ -24,7 +24,13 @@ TestNodeStateUpdater::getClusterStateBundle() const void TestNodeStateUpdater::setClusterState(lib::ClusterState::CSP c) { - _clusterStateBundle = std::make_shared<const lib::ClusterStateBundle>(*c); + setClusterStateBundle(std::make_shared<const lib::ClusterStateBundle>(*c)); +} + +void +TestNodeStateUpdater::setClusterStateBundle(std::shared_ptr<const lib::ClusterStateBundle> clusterStateBundle) +{ + _clusterStateBundle = std::move(clusterStateBundle); for (uint32_t i = 0; i < _listeners.size(); ++i) { _listeners[i]->handleNewState(); } diff --git a/storage/src/tests/common/testnodestateupdater.h b/storage/src/tests/common/testnodestateupdater.h index 5e3001abee4..e0c636d2715 100644 --- a/storage/src/tests/common/testnodestateupdater.h +++ b/storage/src/tests/common/testnodestateupdater.h @@ -42,6 +42,7 @@ public: } void setClusterState(lib::ClusterState::CSP c); + void setClusterStateBundle(std::shared_ptr<const lib::ClusterStateBundle> clusterStateBundle); size_t explicit_node_state_reply_send_invocations() const noexcept { return _explicit_node_state_reply_send_invocations; diff --git a/storage/src/tests/storageserver/bouncertest.cpp b/storage/src/tests/storageserver/bouncertest.cpp index 43683132bc9..0882d7c07c8 100644 --- a/storage/src/tests/storageserver/bouncertest.cpp +++ b/storage/src/tests/storageserver/bouncertest.cpp @@ -9,6 +9,7 @@ #include <tests/common/teststorageapp.h> #include <tests/common/testhelper.h> #include <tests/common/dummystoragelink.h> +#include <vespa/document/bucket/fixed_bucket_spaces.h> #include <vespa/document/test/make_document_bucket.h> #include <vespa/storageapi/message/persistence.h> #include <vespa/config/common/exceptions.h> @@ -38,6 +39,7 @@ struct BouncerTest : public CppUnit::TestFixture { void readOnlyOperationsAreNotRejected(); void internalOperationsAreNotRejected(); void outOfBoundsConfigValuesThrowException(); + void abort_request_when_derived_bucket_space_node_state_is_marked_down(); CPPUNIT_TEST_SUITE(BouncerTest); CPPUNIT_TEST(testFutureTimestamp); @@ -50,6 +52,7 @@ struct BouncerTest : public CppUnit::TestFixture { CPPUNIT_TEST(readOnlyOperationsAreNotRejected); CPPUNIT_TEST(internalOperationsAreNotRejected); CPPUNIT_TEST(outOfBoundsConfigValuesThrowException); + CPPUNIT_TEST(abort_request_when_derived_bucket_space_node_state_is_marked_down); CPPUNIT_TEST_SUITE_END(); using Priority = api::StorageMessage::Priority; @@ -64,7 +67,12 @@ struct BouncerTest : public CppUnit::TestFixture { api::Timestamp timestamp, Priority priority = 0); + std::shared_ptr<api::StorageCommand> createDummyFeedMessage( + api::Timestamp timestamp, + document::BucketSpace bucketSpace); + void assertMessageBouncedWithRejection(); + void assertMessageBouncedWithAbort(); void assertMessageNotBounced(); }; @@ -120,6 +128,18 @@ BouncerTest::createDummyFeedMessage(api::Timestamp timestamp, return cmd; } +std::shared_ptr<api::StorageCommand> +BouncerTest::createDummyFeedMessage(api::Timestamp timestamp, + document::BucketSpace bucketSpace) +{ + auto cmd = std::make_shared<api::RemoveCommand>( + document::Bucket(bucketSpace, document::BucketId(0)), + document::DocumentId("doc:foo:bar"), + timestamp); + cmd->setPriority(Priority(0)); + return cmd; +} + void BouncerTest::testFutureTimestamp() { @@ -189,6 +209,17 @@ BouncerTest::assertMessageBouncedWithRejection() } void +BouncerTest::assertMessageBouncedWithAbort() +{ + CPPUNIT_ASSERT_EQUAL(size_t(1), _upper->getNumReplies()); + CPPUNIT_ASSERT_EQUAL(size_t(0), _upper->getNumCommands()); + CPPUNIT_ASSERT_EQUAL(api::ReturnCode::ABORTED, + static_cast<api::RemoveReply&>(*_upper->getReply(0)). + getResult().getResult()); + CPPUNIT_ASSERT_EQUAL(size_t(0), _lower->getNumCommands()); +} + +void BouncerTest::assertMessageNotBounced() { CPPUNIT_ASSERT_EQUAL(size_t(0), _upper->getNumReplies()); @@ -283,5 +314,32 @@ BouncerTest::outOfBoundsConfigValuesThrowException() } catch (config::InvalidConfigException) {} } + +namespace { + +std::shared_ptr<const lib::ClusterStateBundle> +makeClusterStateBundle(const vespalib::string &baselineState, const std::map<document::BucketSpace, vespalib::string> &derivedStates) +{ + lib::ClusterStateBundle::BucketSpaceStateMapping derivedBucketSpaceStates; + for (const auto &entry : derivedStates) { + derivedBucketSpaceStates[entry.first] = std::make_shared<const lib::ClusterState>(entry.second); + } + return std::make_shared<const lib::ClusterStateBundle>(lib::ClusterState(baselineState), std::move(derivedBucketSpaceStates)); +} + +} + +void +BouncerTest::abort_request_when_derived_bucket_space_node_state_is_marked_down() +{ + auto state = makeClusterStateBundle("distributor:3 storage:3", {{ document::FixedBucketSpaces::default_space(), "distributor:3 storage:3 .2.s:d" }}); + _node->getNodeStateUpdater().setClusterStateBundle(state); + _upper->sendDown(createDummyFeedMessage(11 * 1000000, document::FixedBucketSpaces::default_space())); + assertMessageBouncedWithAbort(); + _upper->reset(); + _upper->sendDown(createDummyFeedMessage(11 * 1000000, document::FixedBucketSpaces::global_space())); + assertMessageNotBounced(); +} + } // storage diff --git a/storage/src/vespa/storage/storageserver/bouncer.cpp b/storage/src/vespa/storage/storageserver/bouncer.cpp index 65e04ad7fdb..2f8838d14eb 100644 --- a/storage/src/vespa/storage/storageserver/bouncer.cpp +++ b/storage/src/vespa/storage/storageserver/bouncer.cpp @@ -20,7 +20,8 @@ Bouncer::Bouncer(StorageComponentRegister& compReg, const config::ConfigUri & co _config(new vespa::config::content::core::StorBouncerConfig()), _component(compReg, "bouncer"), _lock(), - _nodeState("s:i"), + _baselineNodeState("s:i"), + _derivedNodeStates(), _clusterState(&lib::State::UP), _configFetcher(configUri.getContext()), _metrics(std::make_unique<BouncerMetrics>()) @@ -55,7 +56,7 @@ Bouncer::print(std::ostream& out, bool verbose, const std::string& indent) const { (void) verbose; (void) indent; - out << "Bouncer(" << _nodeState << ")"; + out << "Bouncer(" << _baselineNodeState << ")"; } void @@ -233,7 +234,7 @@ Bouncer::onDown(const std::shared_ptr<api::StorageMessage>& msg) int feedPriorityLowerBound; { vespalib::LockGuard lock(_lock); - state = &_nodeState.getState(); + state = &getDerivedNodeState(msg->getBucket().getBucketSpace()).getState(); maxClockSkewInSeconds = _config->maxClockSkewSeconds; abortLoadWhenClusterDown = _config->stopExternalLoadWhenClusterDown; isInAvailableState = state->oneOf( @@ -283,25 +284,47 @@ Bouncer::onDown(const std::shared_ptr<api::StorageMessage>& msg) return false; } +namespace { + +lib::NodeState +deriveNodeState(const lib::NodeState &reportedNodeState, + const lib::NodeState ¤tNodeState) +{ + // If current node state is more strict than our own reported state, + // set node state to our current state + if (reportedNodeState.getState().maySetWantedStateForThisNodeState(currentNodeState.getState())) { + return currentNodeState; + } + return reportedNodeState; +} + +} + void Bouncer::handleNewState() { vespalib::LockGuard lock(_lock); - _nodeState = *_component.getStateUpdater().getReportedNodeState(); + const auto reportedNodeState = *_component.getStateUpdater().getReportedNodeState(); const auto clusterStateBundle = _component.getStateUpdater().getClusterStateBundle(); const auto &clusterState = *clusterStateBundle->getBaselineClusterState(); _clusterState = &clusterState.getClusterState(); - if (_config->useWantedStateIfPossible) { - // If current node state is more strict than our own reported state, - // set node state to our current state - lib::NodeState currState = clusterState. - getNodeState(lib::Node(_component.getNodeType(), - _component.getIndex())); - if (_nodeState.getState().maySetWantedStateForThisNodeState( - currState.getState())) - { - _nodeState = currState; - } + const lib::Node node(_component.getNodeType(), _component.getIndex()); + _baselineNodeState = deriveNodeState(reportedNodeState, clusterState.getNodeState(node)); + _derivedNodeStates.clear(); + for (const auto &derivedClusterState : clusterStateBundle->getDerivedClusterStates()) { + _derivedNodeStates[derivedClusterState.first] = + deriveNodeState(reportedNodeState, derivedClusterState.second->getNodeState(node)); + } +} + +const lib::NodeState & +Bouncer::getDerivedNodeState(document::BucketSpace bucketSpace) const +{ + auto itr = _derivedNodeStates.find(bucketSpace); + if (itr != _derivedNodeStates.end()) { + return itr->second; + } else { + return _baselineNodeState; } } diff --git a/storage/src/vespa/storage/storageserver/bouncer.h b/storage/src/vespa/storage/storageserver/bouncer.h index b46bf3fedc6..83c0c82ff57 100644 --- a/storage/src/vespa/storage/storageserver/bouncer.h +++ b/storage/src/vespa/storage/storageserver/bouncer.h @@ -18,6 +18,7 @@ #include <vespa/storage/common/storagelink.h> #include <vespa/storage/config/config-stor-bouncer.h> #include <vespa/vespalib/util/sync.h> +#include <unordered_map> namespace config { class ConfigUri; } @@ -32,7 +33,9 @@ class Bouncer : public StorageLink, std::unique_ptr<vespa::config::content::core::StorBouncerConfig> _config; StorageComponent _component; vespalib::Lock _lock; - lib::NodeState _nodeState; + lib::NodeState _baselineNodeState; + using BucketSpaceNodeStateMapping = std::unordered_map<document::BucketSpace, lib::NodeState, document::BucketSpace::hash>; + BucketSpaceNodeStateMapping _derivedNodeStates; const lib::State* _clusterState; config::ConfigFetcher _configFetcher; std::unique_ptr<BouncerMetrics> _metrics; @@ -84,6 +87,7 @@ private: bool onDown(const std::shared_ptr<api::StorageMessage>&) override; void handleNewState() override; + const lib::NodeState &getDerivedNodeState(document::BucketSpace bucketSpace) const; }; diff --git a/storage/src/vespa/storage/storageserver/statemanager.cpp b/storage/src/vespa/storage/storageserver/statemanager.cpp index 0efae3c5f8f..e643350aa65 100644 --- a/storage/src/vespa/storage/storageserver/statemanager.cpp +++ b/storage/src/vespa/storage/storageserver/statemanager.cpp @@ -2,26 +2,30 @@ #include "statemanager.h" #include <vespa/defaults.h> -#include <fstream> +#include <vespa/document/bucket/fixed_bucket_spaces.h> #include <vespa/metrics/jsonwriter.h> #include <vespa/metrics/metricmanager.h> +#include <vespa/storage/common/bucketoperationlogger.h> #include <vespa/storage/config/config-stor-server.h> -#include <vespa/storageapi/messageapi/storagemessage.h> #include <vespa/storage/storageserver/storagemetricsset.h> -#include <vespa/storage/common/bucketoperationlogger.h> +#include <vespa/storageapi/messageapi/storagemessage.h> #include <vespa/vdslib/state/cluster_state_bundle.h> -#include <sys/types.h> -#include <unistd.h> -#include <vespa/vespalib/util/stringfmt.h> -#include <vespa/vespalib/util/exceptions.h> #include <vespa/vespalib/io/fileutil.h> #include <vespa/vespalib/stllike/asciistream.h> +#include <vespa/vespalib/util/exceptions.h> +#include <vespa/vespalib/util/stringfmt.h> + +#include <fstream> +#include <sys/types.h> +#include <unistd.h> #include <vespa/log/log.h> LOG_SETUP(".state.manager"); namespace storage { +using lib::ClusterStateBundle; + StateManager::StateManager(StorageComponentRegister& compReg, metrics::MetricManager& metricManager, std::unique_ptr<HostInfo> hostInfo, @@ -352,6 +356,73 @@ StateManager::enableNextClusterState() _systemStateHistory.emplace_back(_component.getClock().getTimeInMillis(), _systemState); } +namespace { + +using BucketSpaceToTransitionString = std::unordered_map<document::BucketSpace, + vespalib::string, + document::BucketSpace::hash>; + +void +considerInsertDerivedTransition(const lib::State ¤tBaseline, + const lib::State &newBaseline, + const lib::State ¤tDerived, + const lib::State &newDerived, + const document::BucketSpace &bucketSpace, + BucketSpaceToTransitionString &transitions) +{ + bool considerDerivedTransition = ((currentDerived != newDerived) && + ((currentDerived != currentBaseline) || (newDerived != newBaseline))); + if (considerDerivedTransition && (transitions.find(bucketSpace) == transitions.end())) { + transitions[bucketSpace] = vespalib::make_string("%s space: '%s' to '%s'", + document::FixedBucketSpaces::to_string(bucketSpace).c_str(), + currentDerived.getName().c_str(), + newDerived.getName().c_str()); + } +} + +BucketSpaceToTransitionString +calculateDerivedClusterStateTransitions(const ClusterStateBundle ¤tState, + const ClusterStateBundle &newState, + const lib::Node node) +{ + BucketSpaceToTransitionString result; + const lib::State ¤tBaseline = currentState.getBaselineClusterState()->getNodeState(node).getState(); + const lib::State &newBaseline = newState.getBaselineClusterState()->getNodeState(node).getState(); + for (const auto &entry : currentState.getDerivedClusterStates()) { + const lib::State ¤tDerived = entry.second->getNodeState(node).getState(); + const lib::State &newDerived = newState.getDerivedClusterState(entry.first)->getNodeState(node).getState(); + considerInsertDerivedTransition(currentBaseline, newBaseline, currentDerived, newDerived, entry.first, result); + } + for (const auto &entry : newState.getDerivedClusterStates()) { + const lib::State &newDerived = entry.second->getNodeState(node).getState(); + const lib::State ¤tDerived = currentState.getDerivedClusterState(entry.first)->getNodeState(node).getState(); + considerInsertDerivedTransition(currentBaseline, newBaseline, currentDerived, newDerived, entry.first, result); + } + return result; +} + +vespalib::string +transitionsToString(const BucketSpaceToTransitionString &transitions) +{ + if (transitions.empty()) { + return ""; + } + vespalib::asciistream stream; + stream << "["; + bool first = true; + for (const auto &entry : transitions) { + if (!first) { + stream << ", "; + } + stream << entry.second; + first = false; + } + stream << "] "; + return stream.str(); +} + +} + void StateManager::logNodeClusterStateTransition( const ClusterStateBundle& currentState, @@ -360,11 +431,13 @@ StateManager::logNodeClusterStateTransition( lib::Node self(thisNode()); const lib::State& before(currentState.getBaselineClusterState()->getNodeState(self).getState()); const lib::State& after(newState.getBaselineClusterState()->getNodeState(self).getState()); - if (before != after) { - LOG(info, "Transitioning from state '%s' to '%s' " + auto derivedTransitions = calculateDerivedClusterStateTransitions(currentState, newState, self); + if ((before != after) || !derivedTransitions.empty()) { + LOG(info, "Transitioning from baseline state '%s' to '%s' %s" "(cluster state version %u)", before.getName().c_str(), after.getName().c_str(), + transitionsToString(derivedTransitions).c_str(), newState.getVersion()); } } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java index a0c8b845aca..d7f42c7e6e9 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/RestrictedServerCnxnFactory.java @@ -71,9 +71,9 @@ public class RestrictedServerCnxnFactory extends NIOServerCnxnFactory { return ZooKeeperServer.getAllowedClientHostnames(); } - private Set<String> toHostnameSet(String hosatnamesString) { + private Set<String> toHostnameSet(String hostnamesString) { Set<String> hostnames = new HashSet<>(); - for (String hostname : StringUtilities.split(hosatnamesString)) { + for (String hostname : StringUtilities.split(hostnamesString)) { if ( ! hostname.trim().isEmpty()) hostnames.add(hostname.trim()); } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java index 74f9d01b833..352635ac920 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/zookeeper/ZooKeeperServer.java @@ -3,6 +3,7 @@ package com.yahoo.vespa.zookeeper; import com.google.common.collect.ImmutableSet; import com.google.inject.Inject; +import com.yahoo.cloud.config.ConfigserverConfig; import com.yahoo.cloud.config.ZookeeperServerConfig; import com.yahoo.component.AbstractComponent; import com.yahoo.log.LogLevel; @@ -10,40 +11,41 @@ import static com.yahoo.vespa.defaults.Defaults.getDefaults; import java.io.FileWriter; import java.io.IOException; -import java.util.Collection; import java.util.List; -import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; /** * Writes zookeeper config and starts zookeeper server. * - * @author lulf - * @since 5.3 + * @author Ulf Lilleengen */ public class ZooKeeperServer extends AbstractComponent implements Runnable { /** * The set of hosts which can access the ZooKeeper server in this VM, or empty * to allow access from anywhere. - * This belongs logically to the server instance but must be static to make it accessible + * This belongs logically to the server instance and is final, but must be static to make it accessible * from RestrictedServerCnxnFactory, which is created by ZK through reflection. */ - private static volatile ImmutableSet<String> allowedClientHostnames = ImmutableSet.of(); + private static ImmutableSet<String> allowedClientHostnames = ImmutableSet.of(); private static final java.util.logging.Logger log = java.util.logging.Logger.getLogger(ZooKeeperServer.class.getName()); private static final String ZOOKEEPER_JMX_LOG4J_DISABLE = "zookeeper.jmx.log4j.disable"; static final String ZOOKEEPER_JUTE_MAX_BUFFER = "jute.maxbuffer"; private final Thread zkServerThread; - private final ZookeeperServerConfig config; + private final ZookeeperServerConfig zookeeperServerConfig; - ZooKeeperServer(ZookeeperServerConfig config, boolean startServer) { - this.config = config; + ZooKeeperServer(ZookeeperServerConfig zookeeperServerConfig, ConfigserverConfig configserverConfig, boolean startServer) { + this.zookeeperServerConfig = zookeeperServerConfig; System.setProperty("zookeeper.jmx.log4j.disable", "true"); - System.setProperty(ZOOKEEPER_JUTE_MAX_BUFFER, "" + config.juteMaxBuffer()); + System.setProperty(ZOOKEEPER_JUTE_MAX_BUFFER, "" + zookeeperServerConfig.juteMaxBuffer()); System.setProperty("zookeeper.serverCnxnFactory", "com.yahoo.vespa.zookeeper.RestrictedServerCnxnFactory"); - writeConfigToDisk(config); + if (configserverConfig.hostedVespa()) // restrict access to config servers only + allowedClientHostnames = ImmutableSet.copyOf(zookeeperServerHostnames(zookeeperServerConfig)); + + writeConfigToDisk(zookeeperServerConfig); zkServerThread = new Thread(this, "zookeeper server"); if (startServer) { zkServerThread.start(); @@ -51,15 +53,10 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable { } @Inject - public ZooKeeperServer(ZookeeperServerConfig config) { - this(config, true); - } - - /** Restrict access to this ZooKeeper server to the given client hosts */ - public static void setAllowedClientHostnames(Collection<String> hostnames) { - allowedClientHostnames = ImmutableSet.copyOf(hostnames); + public ZooKeeperServer(ZookeeperServerConfig zookeeperServerConfig, ConfigserverConfig configserverConfig) { + this(zookeeperServerConfig, configserverConfig, true); } - + /** Returns the hosts which are allowed to access this ZooKeeper server, or empty to allow access from anywhere */ public static ImmutableSet<String> getAllowedClientHostnames() { return allowedClientHostnames; } @@ -130,10 +127,9 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable { @Override public void run() { System.setProperty(ZOOKEEPER_JMX_LOG4J_DISABLE, "true"); - String[] args = new String[]{getDefaults().underVespaHome(config.zooKeeperConfigFile())}; + String[] args = new String[]{getDefaults().underVespaHome(zookeeperServerConfig.zooKeeperConfigFile())}; log.log(LogLevel.DEBUG, "Starting ZooKeeper server with config: " + args[0]); - log.log(LogLevel.INFO, "Trying to establish ZooKeeper quorum (from " + - config.server().stream().map(ZookeeperServerConfig.Server::hostname).collect(Collectors.toList()) + ")"); + log.log(LogLevel.INFO, "Trying to establish ZooKeeper quorum (from " + zookeeperServerHostnames(zookeeperServerConfig) + ")"); org.apache.zookeeper.server.quorum.QuorumPeerMain.main(args); } @@ -143,6 +139,10 @@ public class ZooKeeperServer extends AbstractComponent implements Runnable { super.deconstruct(); } - public ZookeeperServerConfig getConfig() { return config; } + public ZookeeperServerConfig getZookeeperServerConfig() { return zookeeperServerConfig; } + + private static Set<String> zookeeperServerHostnames(ZookeeperServerConfig zookeeperServerConfig) { + return zookeeperServerConfig.server().stream().map(ZookeeperServerConfig.Server::hostname).collect(Collectors.toSet()); + } } diff --git a/zkfacade/src/test/java/com/yahoo/vespa/zookeeper/ZooKeeperServerTest.java b/zkfacade/src/test/java/com/yahoo/vespa/zookeeper/ZooKeeperServerTest.java index 8dd33f3d744..626e5bf0627 100644 --- a/zkfacade/src/test/java/com/yahoo/vespa/zookeeper/ZooKeeperServerTest.java +++ b/zkfacade/src/test/java/com/yahoo/vespa/zookeeper/ZooKeeperServerTest.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.zookeeper; +import com.yahoo.cloud.config.ConfigserverConfig; import com.yahoo.cloud.config.ZookeeperServerConfig; import com.yahoo.io.IOUtils; import org.junit.Rule; @@ -53,11 +54,11 @@ public class ZooKeeperServerTest { } private void createServer(ZookeeperServerConfig.Builder builder) { - new ZooKeeperServer(new ZookeeperServerConfig(builder), false); + new ZooKeeperServer(new ZookeeperServerConfig(builder), new ConfigserverConfig(new ConfigserverConfig.Builder()), false); } @Test(expected = RuntimeException.class) - public void require_that_this_id_must_be_present_amongst_servers() throws IOException { + public void require_that_this_id_must_be_present_amongst_servers() { ZookeeperServerConfig.Builder builder = new ZookeeperServerConfig.Builder(); builder.server(newServer(2, "bar", 234, 432)); builder.server(newServer(3, "baz", 345, 543)); |