diff options
119 files changed, 1743 insertions, 772 deletions
diff --git a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/AthenzSslKeyStoreConfigurator.java b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/AthenzSslKeyStoreConfigurator.java new file mode 100644 index 00000000000..67f07875243 --- /dev/null +++ b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/AthenzSslKeyStoreConfigurator.java @@ -0,0 +1,118 @@ +// 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.athenz.instanceproviderservice; + +import com.google.inject.Inject; +import com.yahoo.component.AbstractComponent; +import com.yahoo.config.provision.SystemName; +import com.yahoo.config.provision.Zone; +import com.yahoo.jdisc.http.SecretStore; +import com.yahoo.jdisc.http.ssl.SslKeyStoreConfigurator; +import com.yahoo.jdisc.http.ssl.SslKeyStoreContext; +import com.yahoo.log.LogLevel; +import com.yahoo.vespa.hosted.athenz.instanceproviderservice.config.AthenzProviderServiceConfig; +import com.yahoo.vespa.hosted.athenz.instanceproviderservice.impl.AthenzCertificateClient; +import com.yahoo.vespa.hosted.athenz.instanceproviderservice.impl.SecretStoreKeyProvider; + +import java.security.KeyStore; +import java.security.PrivateKey; +import java.security.cert.Certificate; +import java.security.cert.X509Certificate; +import java.time.Duration; +import java.time.Instant; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.logging.Logger; + +import static com.yahoo.vespa.hosted.athenz.instanceproviderservice.impl.Utils.getZoneConfig; + +/** + * @author bjorncs + */ +// TODO Cache certificate on disk +public class AthenzSslKeyStoreConfigurator extends AbstractComponent implements SslKeyStoreConfigurator { + private static final Logger log = Logger.getLogger(AthenzSslKeyStoreConfigurator.class.getName()); + // TODO Make expiry and update frequency configurable parameters + private static final Duration CERTIFICATE_EXPIRY_TIME = Duration.ofDays(30); + private static final Duration CERTIFICATE_UPDATE_PERIOD = Duration.ofDays(7); + + private final ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor(); + private final AthenzCertificateClient certificateClient; + private final SecretStoreKeyProvider keyProvider; + private final AthenzProviderServiceConfig.Zones zoneConfig; + private final AtomicBoolean alreadyConfigured = new AtomicBoolean(); + private final Zone zone; + + @Inject + public AthenzSslKeyStoreConfigurator(SecretStore secretStore, + AthenzProviderServiceConfig config, + Zone zone) { + AthenzProviderServiceConfig.Zones zoneConfig = getZoneConfig(config, zone); + this.certificateClient = new AthenzCertificateClient(config, zoneConfig); + this.keyProvider = new SecretStoreKeyProvider(secretStore, zoneConfig.secretName()); + this.zoneConfig = zoneConfig; + this.zone = zone; + } + + @Override + public void configure(SslKeyStoreContext sslKeyStoreContext) { + // TODO Remove this when main is ready + if (zone.system() != SystemName.cd) { + return; + } + if (alreadyConfigured.getAndSet(true)) { // For debugging purpose of SslKeyStoreConfigurator interface + throw new IllegalStateException("Already configured. configure() can only be called once."); + } + AthenzCertificateUpdater updater = new AthenzCertificateUpdater(sslKeyStoreContext); + scheduler.scheduleAtFixedRate(updater, /*initialDelay*/0, CERTIFICATE_UPDATE_PERIOD.toMinutes(), TimeUnit.MINUTES); + } + + @Override + public void deconstruct() { + try { + scheduler.shutdownNow(); + scheduler.awaitTermination(30, TimeUnit.SECONDS); + } catch (InterruptedException e) { + throw new RuntimeException("Failed to shutdown Athenz certificate updater on time", e); + } + } + + private class AthenzCertificateUpdater implements Runnable { + + private final SslKeyStoreContext sslKeyStoreContext; + + AthenzCertificateUpdater(SslKeyStoreContext sslKeyStoreContext) { + this.sslKeyStoreContext = sslKeyStoreContext; + } + + @Override + public void run() { + try { + log.log(LogLevel.INFO, "Updating Athenz certificate from ZTS"); + PrivateKey privateKey = keyProvider.getPrivateKey(zoneConfig.secretVersion()); + X509Certificate certificate = certificateClient.updateCertificate(privateKey, CERTIFICATE_EXPIRY_TIME); + verifyActualExperiy(certificate); + + String dummyPassword = "athenz"; + KeyStore keyStore = KeyStore.getInstance("JKS"); + keyStore.load(null); + keyStore.setKeyEntry("athenz", privateKey, dummyPassword.toCharArray(), new Certificate[]{certificate}); + sslKeyStoreContext.updateKeyStore(keyStore, dummyPassword); + log.log(LogLevel.INFO, "Athenz certificate reload successfully completed"); + } catch (Throwable e) { + log.log(LogLevel.ERROR, "Failed to update certificate from ZTS: " + e.getMessage(), e); + } + } + + private void verifyActualExperiy(X509Certificate certificate) { + Instant notAfter = certificate.getNotAfter().toInstant(); + Instant notBefore = certificate.getNotBefore().toInstant(); + if (!notBefore.plus(CERTIFICATE_EXPIRY_TIME).equals(notAfter)) { + Duration actualExpiry = Duration.between(notBefore, notAfter); + log.log(LogLevel.WARNING, + String.format("Expected expiry %s, got %s", CERTIFICATE_EXPIRY_TIME, actualExpiry)); + } + } + } +} diff --git a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/impl/Utils.java b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/impl/Utils.java index d81ec183fd4..ad54aa341bf 100644 --- a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/impl/Utils.java +++ b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/impl/Utils.java @@ -3,6 +3,8 @@ package com.yahoo.vespa.hosted.athenz.instanceproviderservice.impl; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import com.yahoo.config.provision.Zone; +import com.yahoo.vespa.hosted.athenz.instanceproviderservice.config.AthenzProviderServiceConfig; /** * @author bjorncs @@ -20,4 +22,10 @@ public class Utils { mapper.registerModule(new JavaTimeModule()); return mapper; } + + public static AthenzProviderServiceConfig.Zones getZoneConfig(AthenzProviderServiceConfig config, Zone zone) { + String key = zone.environment().value() + "." + zone.region().value(); + return config.zones(key); + } + } diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java index eef90975035..521e72ae580 100644 --- a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java +++ b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java @@ -6,6 +6,7 @@ import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.config.application.api.DeployLogger; import com.yahoo.config.application.api.FileRegistry; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.Rotation; import com.yahoo.config.provision.Zone; @@ -42,10 +43,18 @@ public interface ModelContext { boolean multitenant(); ApplicationId applicationId(); List<ConfigServerSpec> configServerSpecs(); - URI loadBalancerAddress(); + HostName loadBalancerName(); boolean hostedVespa(); Zone zone(); Set<Rotation> rotations(); + + /* + * DEPRECATED + * TODO: Remove when 6.172 and earlier are no longer in use + */ + default URI loadBalancerAddress() { + return URI.create("http://localhost"); + } } } diff --git a/config-model-fat/pom.xml b/config-model-fat/pom.xml index 4e7af809ce7..a4bbba0e6e8 100644 --- a/config-model-fat/pom.xml +++ b/config-model-fat/pom.xml @@ -240,11 +240,6 @@ <artifactId>filedistributionmanager</artifactId> <version>${project.version}</version> </dependency> - <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>filedistribution</artifactId> - <version>${project.version}</version> - </dependency> <dependency> <groupId>com.yahoo.vespa</groupId> <artifactId>searchsummary</artifactId> diff --git a/config-model/pom.xml b/config-model/pom.xml index 4e79da3279d..0fdc09e1a61 100644 --- a/config-model/pom.xml +++ b/config-model/pom.xml @@ -276,11 +276,6 @@ </dependency> <dependency> <groupId>com.yahoo.vespa</groupId> - <artifactId>filedistribution</artifactId> - <version>${project.version}</version> - </dependency> - <dependency> - <groupId>com.yahoo.vespa</groupId> <artifactId>searchsummary</artifactId> <version>${project.version}</version> </dependency> diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/DeployProperties.java b/config-model/src/main/java/com/yahoo/config/model/deploy/DeployProperties.java index 942320ecd40..3e96b225226 100644 --- a/config-model/src/main/java/com/yahoo/config/model/deploy/DeployProperties.java +++ b/config-model/src/main/java/com/yahoo/config/model/deploy/DeployProperties.java @@ -3,10 +3,10 @@ package com.yahoo.config.model.deploy; import com.yahoo.config.model.api.ConfigServerSpec; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.Version; import com.yahoo.config.provision.Zone; -import java.net.URI; import java.util.ArrayList; import java.util.List; @@ -21,7 +21,7 @@ public class DeployProperties { private final boolean multitenant; private final ApplicationId applicationId; private final List<ConfigServerSpec> serverSpecs = new ArrayList<>(); - private final URI loadBalancerAddress; + private final HostName loadBalancerName; private final boolean hostedVespa; private final Version vespaVersion; private final Zone zone; @@ -29,11 +29,11 @@ public class DeployProperties { private DeployProperties(boolean multitenant, ApplicationId applicationId, List<ConfigServerSpec> configServerSpecs, - URI loadBalancerAddress, + HostName loadBalancerName, boolean hostedVespa, Version vespaVersion, Zone zone) { - this.loadBalancerAddress = loadBalancerAddress; + this.loadBalancerName = loadBalancerName; this.vespaVersion = vespaVersion; this.zone = zone; this.multitenant = multitenant || hostedVespa || Boolean.getBoolean("multitenant"); @@ -55,8 +55,8 @@ public class DeployProperties { return serverSpecs; } - public URI loadBalancerAddress() { - return loadBalancerAddress; + public HostName loadBalancerName() { + return loadBalancerName; } public boolean hostedVespa() { @@ -75,7 +75,7 @@ public class DeployProperties { private ApplicationId applicationId = ApplicationId.defaultId(); private boolean multitenant = false; private List<ConfigServerSpec> configServerSpecs = new ArrayList<>(); - private URI loadBalancerAddress; + private HostName loadBalancerName; private boolean hostedVespa = false; private Version vespaVersion = Version.fromIntValues(1, 0, 0); private Zone zone = Zone.defaultZone(); @@ -95,8 +95,8 @@ public class DeployProperties { return this; } - public Builder loadBalancerAddress(URI loadBalancerAddress) { - this.loadBalancerAddress = loadBalancerAddress; + public Builder loadBalancerName(HostName loadBalancerName) { + this.loadBalancerName = loadBalancerName; return this; } @@ -116,7 +116,7 @@ public class DeployProperties { } public DeployProperties build() { - return new DeployProperties(multitenant, applicationId, configServerSpecs, loadBalancerAddress, hostedVespa, vespaVersion, zone); + return new DeployProperties(multitenant, applicationId, configServerSpecs, loadBalancerName, hostedVespa, vespaVersion, zone); } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java index 56db1542de8..74512e70ebe 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/VespaModelFactory.java @@ -147,7 +147,7 @@ public class VespaModelFactory implements ModelFactory { return new DeployProperties.Builder() .applicationId(properties.applicationId()) .configServerSpecs(properties.configServerSpecs()) - .loadBalancerAddress(properties.loadBalancerAddress()) + .loadBalancerName(properties.loadBalancerName()) .multitenant(properties.multitenant()) .hostedVespa(properties.hostedVespa()) .vespaVersion(getVersion()) 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 5915f0cea0b..8991bfa6215 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 @@ -201,7 +201,7 @@ public class Container extends AbstractService implements } private void initDefaultJettyConnector() { - defaultHttpServer.addConnector(new ConnectorFactory("SearchServer", getSearchPort(), null)); + defaultHttpServer.addConnector(new ConnectorFactory("SearchServer", getSearchPort())); } private boolean hasDocproc() { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/Identity.java b/config-model/src/main/java/com/yahoo/vespa/model/container/Identity.java index d3038a32bfe..bc7a6e20361 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/Identity.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/Identity.java @@ -1,13 +1,11 @@ // 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.container; +import com.yahoo.config.provision.HostName; import com.yahoo.container.core.identity.IdentityConfig; import com.yahoo.container.jdisc.athenz.impl.AthenzIdentityProviderImpl; import com.yahoo.vespa.model.container.component.SimpleComponent; -import java.net.URI; -import java.util.Optional; - /** * @author mortent */ @@ -16,26 +14,21 @@ public class Identity extends SimpleComponent implements IdentityConfig.Producer private final String domain; private final String service; - private final URI loadBalancerAddress; + private final HostName loadBalancerName; - public Identity(String domain, String service, URI loadBalancerAddress) { + public Identity(String domain, String service, HostName loadBalancerName) { super(CLASS); this.domain = domain; this.service = service; - this.loadBalancerAddress = loadBalancerAddress; + this.loadBalancerName = loadBalancerName; } @Override public void getConfig(IdentityConfig.Builder builder) { builder.domain(domain); builder.service(service); - // Load balancer address might not have been set // Current interpretation of loadbalancer address is: hostname. // Config should be renamed or send the uri - builder.loadBalancerAddress( - Optional.ofNullable(loadBalancerAddress) - .map(URI::getHost) - .orElse("") - ); + builder.loadBalancerAddress(loadBalancerName.value()); } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/ConnectorFactory.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/ConnectorFactory.java index 9d3d8b32ddb..22c42056b3f 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/ConnectorFactory.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/ConnectorFactory.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.model.container.http; import com.yahoo.component.ComponentId; import com.yahoo.container.bundle.BundleInstantiationSpecification; import com.yahoo.jdisc.http.ConnectorConfig; +import com.yahoo.jdisc.http.ssl.DefaultSslKeyStoreConfigurator; import com.yahoo.osgi.provider.model.ComponentModel; import com.yahoo.text.XML; import com.yahoo.vespa.model.container.component.SimpleComponent; @@ -13,31 +14,50 @@ import static com.yahoo.component.ComponentSpecification.fromString; import static com.yahoo.jdisc.http.ConnectorConfig.Ssl.KeyStoreType; /** - * @author <a href="mailto:einarmr@yahoo-inc.com">Einar M R Rosenvinge</a> - * @since 5.21.0 + * @author Einar M R Rosenvinge + * @author bjorncs */ public class ConnectorFactory extends SimpleComponent implements ConnectorConfig.Producer { private final String name; - private volatile int listenPort; + private final int listenPort; private final Element legacyConfig; - public ConnectorFactory(final String name, final int listenPort, final Element legacyConfig) { + public ConnectorFactory(String name, int listenPort) { + this(name, listenPort, null, null); + } + + public ConnectorFactory(String name, int listenPort, Element legacyConfig, Element sslKeystoreConfigurator) { super(new ComponentModel( new BundleInstantiationSpecification(new ComponentId(name), fromString("com.yahoo.jdisc.http.server.jetty.ConnectorFactory"), - fromString("jdisc_http_service")) - - )); - - + fromString("jdisc_http_service")))); this.name = name; this.listenPort = listenPort; this.legacyConfig = legacyConfig; + SimpleComponent sslKeyStoreConfigurator = getSslKeyStoreConfigurator(name, sslKeystoreConfigurator); + addChild(sslKeyStoreConfigurator); + inject(sslKeyStoreConfigurator); } @Override public void getConfig(ConnectorConfig.Builder connectorBuilder) { + configureWithLegacyHttpConfig(legacyConfig, connectorBuilder); + connectorBuilder.listenPort(listenPort); + connectorBuilder.name(name); + } + + public String getName() { + return name; + } + + public int getListenPort() { + return listenPort; + } + + // TODO Remove support for legacy config in Vespa 7 + @Deprecated + private static void configureWithLegacyHttpConfig(Element legacyConfig, ConnectorConfig.Builder connectorBuilder) { if (legacyConfig != null) { { Element tcpKeepAliveEnabled = XML.getChild(legacyConfig, "tcpKeepAliveEnabled"); @@ -85,9 +105,7 @@ public class ConnectorFactory extends SimpleComponent implements ConnectorConfig Element ssl = XML.getChild(legacyConfig, "ssl"); Element sslEnabled = XML.getChild(ssl, "enabled"); - if (ssl != null && - sslEnabled != null && - Boolean.parseBoolean(XML.getValue(sslEnabled).trim())) { + if (ssl != null && sslEnabled != null && Boolean.parseBoolean(XML.getValue(sslEnabled).trim())) { ConnectorConfig.Ssl.Builder sslBuilder = new ConnectorConfig.Ssl.Builder(); sslBuilder.enabled(true); { @@ -129,21 +147,18 @@ public class ConnectorFactory extends SimpleComponent implements ConnectorConfig connectorBuilder.ssl(sslBuilder); } } - - connectorBuilder.listenPort(listenPort); - connectorBuilder.name(name); - } - - public String getName() { - return name; } - public int getListenPort() { - return listenPort; - } - - public void setListenPort(int httpPort) { - this.listenPort = httpPort; + private static SimpleComponent getSslKeyStoreConfigurator(String name, Element sslKeystoreConfigurator) { + String idSpec = "ssl-keystore-configurator@" + name; + if (sslKeystoreConfigurator != null) { + String className = sslKeystoreConfigurator.getAttribute("class"); + String bundleName = sslKeystoreConfigurator.getAttribute("bundle"); + return new SimpleComponent(new ComponentModel(idSpec, className, bundleName)); + } else { + return new SimpleComponent( + new ComponentModel(idSpec, DefaultSslKeyStoreConfigurator.class.getName(), "jdisc_http_service")); + } } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyConnectorBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyConnectorBuilder.java index 6271ff817bb..f2012a609a7 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyConnectorBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/JettyConnectorBuilder.java @@ -33,6 +33,8 @@ public class JettyConnectorBuilder extends VespaDomBuilder.DomConfigProducerBuil legacyServerConfig = null; } } - return new ConnectorFactory(name, port, legacyServerConfig); + Element sslKeystoreConfigurator = XML.getChild(serverSpec, "ssl-keystore-configurator"); + return new ConnectorFactory(name, port, legacyServerConfig, sslKeystoreConfigurator); } + } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index f8d69d1b0ac..d59846cd5e2 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -7,6 +7,7 @@ import com.yahoo.config.application.Xml; import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.config.application.api.DeployLogger; import com.yahoo.config.model.ConfigModelContext; +import com.yahoo.config.model.api.ConfigServerSpec; import com.yahoo.config.model.application.provider.IncludeDirs; import com.yahoo.config.model.builder.xml.ConfigModelBuilder; import com.yahoo.config.model.builder.xml.ConfigModelId; @@ -15,6 +16,7 @@ import com.yahoo.config.provision.Capacity; import com.yahoo.config.provision.ClusterMembership; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.NodeType; import com.yahoo.container.jdisc.config.MetricDefaultsConfig; import com.yahoo.search.rendering.RendererRegistry; @@ -57,7 +59,6 @@ import com.yahoo.vespa.model.content.StorageGroup; import org.w3c.dom.Element; import org.w3c.dom.Node; -import java.net.URI; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -166,7 +167,10 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { // Athenz copper argos // NOTE: Must be done after addNodes() - addIdentity(spec, cluster, context.getDeployState().getProperties().loadBalancerAddress()); + addIdentity(spec, + cluster, + context.getDeployState().getProperties().configServerSpecs(), + context.getDeployState().getProperties().loadBalancerName()); //TODO: overview handler, see DomQrserverClusterBuilder } @@ -694,13 +698,22 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { } } - private void addIdentity(Element element, ContainerCluster cluster, URI loadBalancerAddress) { + private void addIdentity(Element element, ContainerCluster cluster, List<ConfigServerSpec> configServerSpecs, HostName loadBalancerName) { Element identityElement = XML.getChild(element, "identity"); if(identityElement != null) { String domain = XML.getValue(XML.getChild(identityElement, "domain")); String service = XML.getValue(XML.getChild(identityElement, "service")); - Identity identity = new Identity(domain.trim(), service.trim(), loadBalancerAddress); + // Set lbaddress, or use first hostname if not specified. + HostName lbName = Optional.ofNullable(loadBalancerName) + .orElseGet( + () -> HostName.from(configServerSpecs.stream() + .findFirst() + .map(ConfigServerSpec::getHostName) + .orElse("unknown") // Currently unable to test this, hence the unknown + )); + + Identity identity = new Identity(domain.trim(), service.trim(), lbName); cluster.addComponent(identity); cluster.getContainers().forEach(container -> { diff --git a/config-model/src/main/resources/schema/containercluster.rnc b/config-model/src/main/resources/schema/containercluster.rnc index af9b89b8553..47cf0638d72 100644 --- a/config-model/src/main/resources/schema/containercluster.rnc +++ b/config-model/src/main/resources/schema/containercluster.rnc @@ -62,6 +62,7 @@ Filtering = element filtering { HttpServer = element server { attribute port { xsd:nonNegativeInteger } & ComponentId & + element ssl-keystore-configurator { BundleSpec }? & # FOR INTERNAL USE ONLY - SUBJECT TO CHANGE GenericConfig* } diff --git a/config-model/src/test/java/com/yahoo/config/model/MockModelContext.java b/config-model/src/test/java/com/yahoo/config/model/MockModelContext.java index ff37fb1fad3..58f83d1e4e6 100644 --- a/config-model/src/test/java/com/yahoo/config/model/MockModelContext.java +++ b/config-model/src/test/java/com/yahoo/config/model/MockModelContext.java @@ -2,19 +2,23 @@ package com.yahoo.config.model; import com.yahoo.component.Version; -import com.yahoo.config.model.api.*; import com.yahoo.config.application.api.ApplicationPackage; -import com.yahoo.config.model.application.provider.BaseDeployLogger; import com.yahoo.config.application.api.DeployLogger; import com.yahoo.config.application.api.FileRegistry; +import com.yahoo.config.model.api.ConfigDefinitionRepo; +import com.yahoo.config.model.api.ConfigServerSpec; +import com.yahoo.config.model.api.HostProvisioner; +import com.yahoo.config.model.api.Model; +import com.yahoo.config.model.api.ModelContext; +import com.yahoo.config.model.application.provider.BaseDeployLogger; import com.yahoo.config.model.application.provider.MockFileRegistry; import com.yahoo.config.model.application.provider.StaticConfigDefinitionRepo; import com.yahoo.config.model.test.MockApplicationPackage; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.Rotation; import com.yahoo.config.provision.Zone; -import java.net.URI; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -96,7 +100,7 @@ public class MockModelContext implements ModelContext { } @Override - public URI loadBalancerAddress() { + public HostName loadBalancerName() { return null; } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/VespaModelFactoryTest.java b/config-model/src/test/java/com/yahoo/vespa/model/VespaModelFactoryTest.java index 873883716e4..9a507827d17 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/VespaModelFactoryTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/VespaModelFactoryTest.java @@ -18,6 +18,7 @@ import com.yahoo.config.provision.ApplicationName; import com.yahoo.config.provision.Capacity; import com.yahoo.config.provision.ClusterMembership; import com.yahoo.config.provision.ClusterSpec; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.HostSpec; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.ProvisionLogger; @@ -27,7 +28,6 @@ import com.yahoo.config.provision.Zone; import org.junit.Before; import org.junit.Test; -import java.net.URI; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -196,7 +196,7 @@ public class VespaModelFactoryTest { } @Override - public URI loadBalancerAddress() { + public HostName loadBalancerName() { return null; } }; diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JettyContainerModelBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JettyContainerModelBuilderTest.java index 4621b5ebe50..1e24b055095 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JettyContainerModelBuilderTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JettyContainerModelBuilderTest.java @@ -3,17 +3,24 @@ package com.yahoo.vespa.model.container.xml; import com.yahoo.config.model.builder.xml.test.DomBuilderTest; import com.yahoo.container.ComponentsConfig; +import com.yahoo.container.bundle.BundleInstantiationSpecification; import com.yahoo.container.jdisc.FilterBindingsProvider; import com.yahoo.jdisc.http.ConnectorConfig; +import com.yahoo.jdisc.http.ssl.DefaultSslKeyStoreConfigurator; import com.yahoo.vespa.model.container.ContainerCluster; +import com.yahoo.vespa.model.container.component.SimpleComponent; +import com.yahoo.vespa.model.container.http.ConnectorFactory; import com.yahoo.vespa.model.container.http.JettyHttpServer; import org.junit.Test; import org.w3c.dom.Element; +import org.xml.sax.SAXException; +import java.io.IOException; import java.util.List; import static com.yahoo.jdisc.http.ConnectorConfig.Ssl.KeyStoreType; import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.nullValue; @@ -182,6 +189,42 @@ public class JettyContainerModelBuilderTest extends ContainerModelBuilderTestBas is(not(nullValue()))); } + @Test + public void ssl_keystore_configurator_can_be_overriden() throws IOException, SAXException { + Element clusterElem = DomBuilderTest.parse( + "<jdisc id='default' version='1.0' jetty='true'>", + " <http>", + " <server port='9000' id='foo'>", + " <ssl-keystore-configurator class='com.yahoo.MySslKeyStoreConfigurator' bundle='mybundle'/>", + " </server>", + " <server port='9001' id='bar'/>", + " </http>", + nodesXml, + "</jdisc>"); + createModel(root, clusterElem); + ContainerCluster cluster = (ContainerCluster) root.getChildren().get("default"); + List<ConnectorFactory> connectorFactories = cluster.getChildrenByTypeRecursive(ConnectorFactory.class); + + { + ConnectorFactory firstConnector = connectorFactories.get(0); + assertThat(firstConnector.getInjectedComponentIds(), hasItem("ssl-keystore-configurator@foo")); + assertThat(firstConnector.getInjectedComponentIds().size(), equalTo(1)); + SimpleComponent sslKeystoreConfigurator = firstConnector.getChildrenByTypeRecursive(SimpleComponent.class).get(0); + BundleInstantiationSpecification spec = sslKeystoreConfigurator.model.bundleInstantiationSpec; + assertThat(spec.classId.toString(), is("com.yahoo.MySslKeyStoreConfigurator")); + assertThat(spec.bundle.toString(), is("mybundle")); + } + { + ConnectorFactory secondFactory = connectorFactories.get(1); + assertThat(secondFactory.getInjectedComponentIds(), hasItem("ssl-keystore-configurator@bar")); + assertThat(secondFactory.getInjectedComponentIds().size(), equalTo(1)); + SimpleComponent sslKeystoreConfigurator = secondFactory.getChildrenByTypeRecursive(SimpleComponent.class).get(0); + BundleInstantiationSpecification spec = sslKeystoreConfigurator.model.bundleInstantiationSpec; + assertThat(spec.classId.toString(), is(DefaultSslKeyStoreConfigurator.class.getName())); + assertThat(spec.bundle.toString(), is("jdisc_http_service")); + } + } + private void assertJettyServerInConfig() { ContainerCluster cluster = (ContainerCluster) root.getChildren().get("default"); List<JettyHttpServer> jettyServers = cluster.getChildrenByTypeRecursive(JettyHttpServer.class); diff --git a/config-model/src/test/schema-test-files/services.xml b/config-model/src/test/schema-test-files/services.xml index 18468dcc433..98637c03020 100644 --- a/config-model/src/test/schema-test-files/services.xml +++ b/config-model/src/test/schema-test-files/services.xml @@ -110,7 +110,9 @@ </request-chain> </filtering> - <server port="4080" id="myServer" /> + <server port="4080" id="myServer"> + <ssl-keystore-configurator class="com.yahoo.MySslKeyStoreConfigurator" bundle="mybundle" /> + </server> <server port="4081" id="anotherServer"> <config name="container.jdisc.config.http-server"> <maxChunkSize>9999</maxChunkSize> diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/HostName.java b/config-provisioning/src/main/java/com/yahoo/config/provision/HostName.java new file mode 100644 index 00000000000..510122c2342 --- /dev/null +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/HostName.java @@ -0,0 +1,52 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.config.provision; + +import java.util.Objects; + +/** + * Represents a host name + * + * @author mortent + */ +public class HostName implements Comparable<HostName> { + + private final String name; + + private HostName(String name) { + this.name = name; + } + + public String value() { return name; } + + /** + * Create a {@link HostName} with a given name. + * + * @param name Name + * @return instance of {@link HostName}. + */ + public static HostName from(String name) { + return new HostName(name); + } + + @Override + public int hashCode() { + return name.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof HostName)) return false; + return Objects.equals(((HostName)obj).value(), value()); + } + + @Override + public String toString() { + return name; + } + + @Override + public int compareTo(HostName that) { + return this.name.compareTo(that.name); + } + +} diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ZKTenantApplications.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ZKTenantApplications.java index 648e6bb7180..c425dc9f22d 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/ZKTenantApplications.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/ZKTenantApplications.java @@ -47,6 +47,7 @@ public class ZKTenantApplications implements TenantApplications, PathChildrenCac private ZKTenantApplications(Curator curator, Path applicationsPath, ReloadHandler reloadHandler, TenantName tenant) { this.curator = curator; this.applicationsPath = applicationsPath; + curator.create(applicationsPath); this.reloadHandler = reloadHandler; this.tenant = tenant; this.directoryCache = curator.createDirectoryCache(applicationsPath.getAbsolute(), false, false, pathChildrenExecutor); @@ -54,9 +55,9 @@ public class ZKTenantApplications implements TenantApplications, PathChildrenCac this.directoryCache.addListener(this); } - public static TenantApplications create(Curator curator, Path applicationsPath, ReloadHandler reloadHandler, TenantName tenant) { + public static TenantApplications create(Curator curator, ReloadHandler reloadHandler, TenantName tenant) { try { - return new ZKTenantApplications(curator, applicationsPath, reloadHandler, tenant); + return new ZKTenantApplications(curator, Tenants.getApplicationsPath(tenant), reloadHandler, tenant); } catch (Exception e) { throw new RuntimeException(Tenants.logPre(tenant) + "Error creating application repo", e); } @@ -143,7 +144,7 @@ public class ZKTenantApplications implements TenantApplications, PathChildrenCac private void applicationRemoved(ApplicationId applicationId) { reloadHandler.removeApplication(applicationId); - log.log(LogLevel.DEBUG, Tenants.logPre(applicationId) + "Application removed: " + applicationId); + log.log(LogLevel.INFO, Tenants.logPre(applicationId) + "Application removed: " + applicationId); } private void applicationAdded(ApplicationId applicationId) { diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java index 1b96ba46907..c0c9c309576 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java @@ -2,16 +2,20 @@ package com.yahoo.vespa.config.server.deploy; import com.yahoo.component.Version; -import com.yahoo.config.model.api.*; import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.config.application.api.DeployLogger; import com.yahoo.config.application.api.FileRegistry; +import com.yahoo.config.model.api.ConfigDefinitionRepo; +import com.yahoo.config.model.api.ConfigServerSpec; +import com.yahoo.config.model.api.HostProvisioner; +import com.yahoo.config.model.api.Model; +import com.yahoo.config.model.api.ModelContext; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.Rotation; import com.yahoo.config.provision.Zone; import java.io.File; -import java.net.URI; import java.util.List; import java.util.Optional; import java.util.Set; @@ -133,7 +137,7 @@ public class ModelContextImpl implements ModelContext { private final ApplicationId applicationId; private final boolean multitenant; private final List<ConfigServerSpec> configServerSpecs; - private final URI loadBalancerAddress; + private final HostName loadBalancerName; private final boolean hostedVespa; private final Zone zone; private final Set<Rotation> rotations; @@ -141,14 +145,14 @@ public class ModelContextImpl implements ModelContext { public Properties(ApplicationId applicationId, boolean multitenant, List<ConfigServerSpec> configServerSpecs, - URI loadBalancerAddress, + HostName loadBalancerName, boolean hostedVespa, Zone zone, Set<Rotation> rotations) { this.applicationId = applicationId; this.multitenant = multitenant; this.configServerSpecs = configServerSpecs; - this.loadBalancerAddress = loadBalancerAddress; + this.loadBalancerName = loadBalancerName; this.hostedVespa = hostedVespa; this.zone = zone; this.rotations = rotations; @@ -170,8 +174,8 @@ public class ModelContextImpl implements ModelContext { } @Override - public URI loadBalancerAddress() { - return loadBalancerAddress; + public HostName loadBalancerName() { + return loadBalancerName; } @Override diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java index fac73dcac77..d869b5a2901 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/modelfactory/ModelsBuilder.java @@ -6,10 +6,11 @@ import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.config.model.api.HostProvisioner; import com.yahoo.config.model.api.ModelContext; import com.yahoo.config.model.api.ModelFactory; +import com.yahoo.config.provision.AllocatedHosts; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationLockException; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.OutOfCapacityException; -import com.yahoo.config.provision.AllocatedHosts; import com.yahoo.config.provision.Rotation; import com.yahoo.config.provision.Version; import com.yahoo.config.provision.Zone; @@ -19,7 +20,6 @@ import com.yahoo.vespa.config.server.deploy.ModelContextImpl; import com.yahoo.vespa.config.server.http.UnknownVespaVersionException; import com.yahoo.vespa.config.server.provision.StaticProvisioner; -import java.net.URI; import java.time.Instant; import java.util.ArrayList; import java.util.Collections; @@ -174,7 +174,7 @@ public abstract class ModelsBuilder<MODELRESULT extends ModelResult> { return new ModelContextImpl.Properties(applicationId, configserverConfig.multitenant(), ConfigServerSpec.fromConfig(configserverConfig), - URI.create(configserverConfig.loadBalancerAddress()), + HostName.from(configserverConfig.loadBalancerAddress()), configserverConfig.hostedVespa(), zone, rotations); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionFactory.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionFactory.java index 298acaca901..e96ddb4b094 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionFactory.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionFactory.java @@ -6,6 +6,7 @@ import com.yahoo.path.Path; import com.yahoo.config.model.api.ConfigDefinitionRepo; import com.yahoo.cloud.config.ConfigserverConfig; import com.yahoo.vespa.config.server.GlobalComponentRegistry; +import com.yahoo.vespa.config.server.tenant.Tenants; import com.yahoo.vespa.config.server.zookeeper.ConfigCurator; import com.yahoo.vespa.curator.Curator; @@ -19,20 +20,19 @@ public class RemoteSessionFactory { private final GlobalComponentRegistry componentRegistry; private final Curator curator; private final ConfigCurator configCurator; - private final Path sessionDirPath; + private final Path sessionsPath; private final ConfigDefinitionRepo defRepo; private final TenantName tenant; private final ConfigserverConfig configserverConfig; private final Clock clock; public RemoteSessionFactory(GlobalComponentRegistry componentRegistry, - Path sessionsPath, TenantName tenant, Clock clock) { this.componentRegistry = componentRegistry; this.curator = componentRegistry.getCurator(); this.configCurator = componentRegistry.getConfigCurator(); - this.sessionDirPath = sessionsPath; + this.sessionsPath = Tenants.getSessionsPath(tenant); this.tenant = tenant; this.defRepo = componentRegistry.getConfigDefinitionRepo(); this.configserverConfig = componentRegistry.getConfigserverConfig(); @@ -40,7 +40,7 @@ public class RemoteSessionFactory { } public RemoteSession createSession(long sessionId) { - Path sessionPath = sessionDirPath.append(String.valueOf(sessionId)); + Path sessionPath = this.sessionsPath.append(String.valueOf(sessionId)); SessionZooKeeperClient sessionZKClient = new SessionZooKeeperClient(curator, configCurator, sessionPath, diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java index 659a44bb339..2269a7ed997 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java @@ -8,10 +8,12 @@ import java.util.logging.Logger; import com.google.common.collect.HashMultiset; import com.google.common.collect.Multiset; +import com.yahoo.config.provision.TenantName; import com.yahoo.log.LogLevel; import com.yahoo.path.Path; import com.yahoo.transaction.NestedTransaction; import com.yahoo.vespa.config.server.application.ApplicationSet; +import com.yahoo.vespa.config.server.tenant.Tenants; import com.yahoo.vespa.curator.Curator; import com.yahoo.yolean.Exceptions; import com.yahoo.vespa.config.server.ReloadHandler; @@ -49,19 +51,19 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod * @param curator a {@link Curator} instance. * @param remoteSessionFactory a {@link com.yahoo.vespa.config.server.session.RemoteSessionFactory} * @param reloadHandler a {@link com.yahoo.vespa.config.server.ReloadHandler} - * @param sessionsPath a {@link com.yahoo.path.Path} to the sessions dir. - * @param applicationRepo an {@link TenantApplications} object. + * @param tenant a {@link TenantName} instance. + * @param applicationRepo a {@link TenantApplications} instance. * @param executorService an {@link ExecutorService} to run callbacks from ZooKeeper. */ public RemoteSessionRepo(Curator curator, - RemoteSessionFactory remoteSessionFactory, - ReloadHandler reloadHandler, - Path sessionsPath, - TenantApplications applicationRepo, - MetricUpdater metricUpdater, - ExecutorService executorService) { + RemoteSessionFactory remoteSessionFactory, + ReloadHandler reloadHandler, + TenantName tenant, + TenantApplications applicationRepo, + MetricUpdater metricUpdater, + ExecutorService executorService) { this.curator = curator; - this.sessionsPath = sessionsPath; + this.sessionsPath = Tenants.getSessionsPath(tenant); this.applicationRepo = applicationRepo; this.remoteSessionFactory = remoteSessionFactory; this.reloadHandler = reloadHandler; diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionFactoryImpl.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionFactoryImpl.java index 1d5025f2e61..fdc681b5fb6 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionFactoryImpl.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionFactoryImpl.java @@ -16,6 +16,7 @@ import com.yahoo.config.provision.TenantName; import com.yahoo.vespa.config.server.application.TenantApplications; import com.yahoo.vespa.config.server.deploy.TenantFileSystemDirs; import com.yahoo.vespa.config.server.host.HostValidator; +import com.yahoo.vespa.config.server.tenant.Tenant; import com.yahoo.vespa.config.server.tenant.Tenants; import com.yahoo.vespa.config.server.zookeeper.SessionCounter; import com.yahoo.vespa.config.server.zookeeper.ConfigCurator; @@ -57,7 +58,6 @@ public class SessionFactoryImpl implements SessionFactory, LocalSessionLoader { public SessionFactoryImpl(GlobalComponentRegistry globalComponentRegistry, SessionCounter sessionCounter, - Path sessionsPath, TenantApplications applicationRepo, TenantFileSystemDirs tenantFileSystemDirs, HostValidator<ApplicationId> hostRegistry, @@ -68,7 +68,7 @@ public class SessionFactoryImpl implements SessionFactory, LocalSessionLoader { this.curator = globalComponentRegistry.getCurator(); this.configCurator = globalComponentRegistry.getConfigCurator(); this.sessionCounter = sessionCounter; - this.sessionsPath = sessionsPath; + this.sessionsPath = Tenants.getSessionsPath(tenant); this.applicationRepo = applicationRepo; this.tenantFileSystemDirs = tenantFileSystemDirs; this.superModelGenerationCounter = globalComponentRegistry.getSuperModelGenerationCounter(); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionPreparer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionPreparer.java index 6154be52bcc..531085883c4 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionPreparer.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionPreparer.java @@ -11,6 +11,7 @@ import com.yahoo.config.model.api.ConfigDefinitionRepo; import com.yahoo.config.model.api.ModelContext; import com.yahoo.config.provision.AllocatedHosts; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.OutOfCapacityException; import com.yahoo.config.provision.Rotation; import com.yahoo.config.provision.Version; @@ -35,7 +36,6 @@ import org.xml.sax.SAXException; import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.TransformerException; import java.io.IOException; -import java.net.URI; import java.time.Instant; import java.util.List; import java.util.Map; @@ -152,7 +152,7 @@ public class SessionPreparer { this.properties = new ModelContextImpl.Properties(params.getApplicationId(), configserverConfig.multitenant(), ConfigServerSpec.fromConfig(configserverConfig), - URI.create(configserverConfig.loadBalancerAddress()), + HostName.from(configserverConfig.loadBalancerAddress()), configserverConfig.hostedVespa(), zone, rotationsSet); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TenantBuilder.java b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TenantBuilder.java index 084d35a42d4..61145c2a138 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TenantBuilder.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TenantBuilder.java @@ -34,7 +34,6 @@ public class TenantBuilder { private final Path tenantPath; private final GlobalComponentRegistry componentRegistry; private final TenantName tenant; - private final Path sessionsPath; private RemoteSessionRepo remoteSessionRepo; private LocalSessionRepo localSessionRepo; private SessionFactory sessionFactory; @@ -47,15 +46,14 @@ public class TenantBuilder { private TenantFileSystemDirs tenantFileSystemDirs; private HostValidator<ApplicationId> hostValidator; - private TenantBuilder(GlobalComponentRegistry componentRegistry, TenantName tenant, Path zkPath) { + private TenantBuilder(GlobalComponentRegistry componentRegistry, TenantName tenant) { this.componentRegistry = componentRegistry; - this.tenantPath = zkPath; + this.tenantPath = Tenants.getTenantPath(tenant); this.tenant = tenant; - this.sessionsPath = tenantPath.append(Tenant.SESSIONS); } - public static TenantBuilder create(GlobalComponentRegistry componentRegistry, TenantName tenant, Path zkPath) { - return new TenantBuilder(componentRegistry, tenant, zkPath); + public static TenantBuilder create(GlobalComponentRegistry componentRegistry, TenantName tenant) { + return new TenantBuilder(componentRegistry, tenant); } public TenantBuilder withSessionFactory(SessionFactory sessionFactory) { @@ -123,7 +121,7 @@ public class TenantBuilder { private void createSessionFactory() { if (sessionFactory == null || localSessionLoader == null) { - SessionFactoryImpl impl = new SessionFactoryImpl(componentRegistry, sessionCounter, sessionsPath, + SessionFactoryImpl impl = new SessionFactoryImpl(componentRegistry, sessionCounter, applicationRepo, tenantFileSystemDirs, hostValidator, tenant); if (sessionFactory == null) { sessionFactory = impl; @@ -136,13 +134,13 @@ public class TenantBuilder { private void createApplicationRepo() { if (applicationRepo == null) { - applicationRepo = ZKTenantApplications.create(componentRegistry.getCurator(), tenantPath.append(Tenant.APPLICATIONS), reloadHandler, tenant); + applicationRepo = ZKTenantApplications.create(componentRegistry.getCurator(), reloadHandler, tenant); } } private void createSessionCounter() { if (sessionCounter == null) { - sessionCounter = new SessionCounter(componentRegistry.getCurator(), tenantPath, sessionsPath); + sessionCounter = new SessionCounter(componentRegistry.getCurator(), tenant); } } @@ -167,7 +165,7 @@ public class TenantBuilder { private void createRemoteSessionFactory(Clock clock) { if (remoteSessionFactory == null) { - remoteSessionFactory = new RemoteSessionFactory(componentRegistry, sessionsPath, tenant, clock); + remoteSessionFactory = new RemoteSessionFactory(componentRegistry, tenant, clock); } } @@ -176,7 +174,7 @@ public class TenantBuilder { remoteSessionRepo = new RemoteSessionRepo(componentRegistry.getCurator(), remoteSessionFactory, reloadHandler, - sessionsPath, + tenant, applicationRepo, componentRegistry.getMetrics().getOrCreateMetricUpdater(Metrics.createDimensions(tenant)), createSingleThreadedExecutorService(RemoteSessionRepo.class.getName())); diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/Tenants.java b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/Tenants.java index 528a30e0191..d2cf17a38d4 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/Tenants.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/Tenants.java @@ -189,7 +189,7 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen if (tenants.containsKey(tenantName)) return; try { - Tenant tenant = TenantBuilder.create(globalComponentRegistry, tenantName, getTenantPath(tenantName)).build(); + Tenant tenant = TenantBuilder.create(globalComponentRegistry, tenantName).build(); notifyNewTenant(tenant); tenants.put(tenantName, tenant); } catch (Exception e) { @@ -351,6 +351,7 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen /** * Gets zookeeper path for tenant data + * * @param tenantName tenant name * @return a {@link com.yahoo.path.Path} to the zookeeper data for a tenant */ @@ -358,4 +359,24 @@ public class Tenants implements ConnectionStateListener, PathChildrenCacheListen return tenantsPath.append(tenantName.value()); } + /** + * Gets zookeeper path for session data for a tenant + * + * @param tenantName tenant name + * @return a {@link com.yahoo.path.Path} to the zookeeper sessions data for a tenant + */ + public static Path getSessionsPath(TenantName tenantName) { + return getTenantPath(tenantName).append(Tenant.SESSIONS); + } + + /** + * Gets zookeeper path for application data for a tenant + * + * @param tenantName tenant name + * @return a {@link com.yahoo.path.Path} to the zookeeper application data for a tenant + */ + public static Path getApplicationsPath(TenantName tenantName) { + return getTenantPath(tenantName).append(Tenant.APPLICATIONS); + } + } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/SessionCounter.java b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/SessionCounter.java index 2d95a013da9..4df292dd204 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/SessionCounter.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/zookeeper/SessionCounter.java @@ -1,7 +1,8 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.server.zookeeper; -import com.yahoo.path.Path; +import com.yahoo.config.provision.TenantName; +import com.yahoo.vespa.config.server.tenant.Tenants; import com.yahoo.vespa.curator.Curator; /** @@ -12,8 +13,10 @@ import com.yahoo.vespa.curator.Curator; */ public class SessionCounter extends InitializedCounter { - public SessionCounter(Curator curator, Path rootPath, Path sessionsDir) { - super(curator, rootPath.append("sessionCounter").getAbsolute(), sessionsDir.getAbsolute()); + public SessionCounter(Curator curator, TenantName tenantName) { + super(curator, + Tenants.getTenantPath(tenantName).append("sessionCounter").getAbsolute(), + Tenants.getSessionsPath(tenantName).getAbsolute()); } /** diff --git a/configserver/src/main/resources/configserver-app/services.xml b/configserver/src/main/resources/configserver-app/services.xml index daa65f996ae..635ce07e727 100644 --- a/configserver/src/main/resources/configserver-app/services.xml +++ b/configserver/src/main/resources/configserver-app/services.xml @@ -1,13 +1,13 @@ <?xml version="1.0" encoding="utf-8" ?> <!-- Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. --> <services version="1.0" xmlns:preprocess="properties"> - <preprocess:include file='controller/admin.xml' required='false' /> <jdisc id="configserver" jetty="true" version="1.0"> <config name="container.handler.threadpool"> <maxthreads>100</maxthreads> <!-- Reduced thread count to minimize memory consumption --> </config> <accesslog type="vespa" fileNamePattern="logs/vespa/configserver/access.log.%Y%m%d%H%M%S" rotationScheme="date" symlinkName="access.log" /> + <preprocess:include file='access-logging.xml' required='false' /> <component id="com.yahoo.vespa.config.server.ConfigServerBootstrap" bundle="configserver" /> <component id="com.yahoo.vespa.config.server.monitoring.Metrics" bundle="configserver" /> <component id="com.yahoo.vespa.zookeeper.ZooKeeperServer" bundle="zkfacade" /> diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/TenantApplicationsTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/TenantApplicationsTest.java index 1a14ac1761c..08cfa74da3b 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/TenantApplicationsTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/TenantApplicationsTest.java @@ -3,11 +3,11 @@ package com.yahoo.vespa.config.server.application; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.TenantName; -import com.yahoo.path.Path; import com.yahoo.text.Utf8; import com.yahoo.vespa.config.server.MockReloadHandler; import com.yahoo.vespa.config.server.TestWithCurator; +import com.yahoo.vespa.config.server.tenant.Tenants; import org.junit.Test; import java.util.Arrays; @@ -18,60 +18,61 @@ import static org.hamcrest.Matchers.is; import static org.junit.Assert.*; /** - * @author lulf + * @author Ulf Lilleengen * @since 5.1 */ public class TenantApplicationsTest extends TestWithCurator { + private static final TenantName tenantName = TenantName.from("tenant"); + @Test public void require_that_applications_are_read_from_zookeeper() throws Exception { - curatorFramework.create().creatingParentsIfNeeded().forPath("/foo:dev:baz", Utf8.toAsciiBytes(3)); - curatorFramework.create().creatingParentsIfNeeded().forPath("/bar:test:bim", Utf8.toAsciiBytes(4)); + writeApplicationData(createApplicationId("foo"), 3L); + writeApplicationData(createApplicationId("bar"), 4L); TenantApplications repo = createZKAppRepo(); List<ApplicationId> applications = repo.listApplications(); assertThat(applications.size(), is(2)); - assertThat(applications.get(0).application().value(), is("dev")); - assertThat(applications.get(1).application().value(), is("test")); + assertThat(applications.get(0).application().value(), is("foo")); + assertThat(applications.get(1).application().value(), is("bar")); assertThat(repo.getSessionIdForApplication(applications.get(0)), is(3L)); assertThat(repo.getSessionIdForApplication(applications.get(1)), is(4L)); } @Test public void require_that_invalid_entries_are_skipped() throws Exception { - curatorFramework.create().creatingParentsIfNeeded().forPath("/foo:dev:baz"); - curatorFramework.create().creatingParentsIfNeeded().forPath("/invalid"); + writeApplicationData(createApplicationId("foo"), 3L); + writeApplicationData("invalid", 3L); TenantApplications repo = createZKAppRepo(); List<ApplicationId> applications = repo.listApplications(); assertThat(applications.size(), is(1)); - assertThat(applications.get(0).application().value(), is("dev")); + assertThat(applications.get(0).application().value(), is("foo")); } @Test(expected = IllegalArgumentException.class) public void require_that_requesting_session_for_unknown_application_throws_exception() throws Exception { - curatorFramework.create().creatingParentsIfNeeded().forPath("/foo:dev:baz:bim"); TenantApplications repo = createZKAppRepo(); - repo.getSessionIdForApplication(new ApplicationId.Builder() - .tenant("exist") - .applicationName("tenant").instanceName("here").build()); + repo.getSessionIdForApplication(createApplicationId("nonexistent")); } @Test(expected = IllegalArgumentException.class) public void require_that_requesting_session_for_empty_application_throws_exception() throws Exception { - curatorFramework.create().creatingParentsIfNeeded().forPath("/foo:dev:baz:bim"); + ApplicationId baz = createApplicationId("baz"); + // No data in node + curatorFramework.create().creatingParentsIfNeeded() + .forPath(Tenants.getApplicationsPath(tenantName).append(baz.serializedForm()).getAbsolute()); TenantApplications repo = createZKAppRepo(); - repo.getSessionIdForApplication(new ApplicationId.Builder() - .tenant("tenant") - .applicationName("foo").instanceName("bim").build()); + repo.getSessionIdForApplication(baz); } @Test public void require_that_application_ids_can_be_written() throws Exception { TenantApplications repo = createZKAppRepo(); - repo.createPutApplicationTransaction(createAppplicationId("myapp"), 3l).commit(); - String path = "/mytenant:myapp:myinst"; + ApplicationId myapp = createApplicationId("myapp"); + repo.createPutApplicationTransaction(myapp, 3l).commit(); + String path = Tenants.getApplicationsPath(tenantName).append(myapp.serializedForm()).getAbsolute(); assertTrue(curatorFramework.checkExists().forPath(path) != null); assertThat(Utf8.toString(curatorFramework.getData().forPath(path)), is("3")); - repo.createPutApplicationTransaction(createAppplicationId("myapp"), 5l).commit(); + repo.createPutApplicationTransaction(myapp, 5l).commit(); assertTrue(curatorFramework.checkExists().forPath(path) != null); assertThat(Utf8.toString(curatorFramework.getData().forPath(path)), is("5")); } @@ -79,8 +80,8 @@ public class TenantApplicationsTest extends TestWithCurator { @Test public void require_that_application_ids_can_be_deleted() throws Exception { TenantApplications repo = createZKAppRepo(); - ApplicationId id1 = createAppplicationId("myapp"); - ApplicationId id2 = createAppplicationId("myapp2"); + ApplicationId id1 = createApplicationId("myapp"); + ApplicationId id2 = createApplicationId("myapp2"); repo.createPutApplicationTransaction(id1, 1).commit(); repo.createPutApplicationTransaction(id2, 1).commit(); assertThat(repo.listApplications().size(), is(2)); @@ -95,8 +96,8 @@ public class TenantApplicationsTest extends TestWithCurator { TenantApplications zkRepo = createZKAppRepo(); TenantApplications memRepo = new MemoryTenantApplications(); for (TenantApplications repo : Arrays.asList(zkRepo, memRepo)) { - ApplicationId id1 = createAppplicationId("myapp"); - ApplicationId id2 = createAppplicationId("myapp2"); + ApplicationId id1 = createApplicationId("myapp"); + ApplicationId id2 = createApplicationId("myapp2"); repo.createPutApplicationTransaction(id1, 4).commit(); repo.createPutApplicationTransaction(id2, 5).commit(); List<ApplicationId> lst = repo.listApplications(); @@ -122,21 +123,19 @@ public class TenantApplicationsTest extends TestWithCurator { @Test public void require_that_reload_handler_is_called_when_apps_are_removed() throws Exception { - curatorFramework.create().creatingParentsIfNeeded().forPath("/foo:test:baz", Utf8.toAsciiBytes(3)); - curatorFramework.create().creatingParentsIfNeeded().forPath("/bar:dev:bim", Utf8.toAsciiBytes(4)); + ApplicationId foo = createApplicationId("foo"); + writeApplicationData(foo, 3L); + writeApplicationData(createApplicationId("bar"), 4L); MockReloadHandler reloadHandler = new MockReloadHandler(); TenantApplications repo = createZKAppRepo(reloadHandler); assertNull(reloadHandler.lastRemoved); - repo.deleteApplication(new ApplicationId.Builder() - .tenant("foo") - .applicationName("test").instanceName("baz").build()) - .commit(); + repo.deleteApplication(foo).commit(); long endTime = System.currentTimeMillis() + 60_000; while (System.currentTimeMillis() < endTime && reloadHandler.lastRemoved == null) { Thread.sleep(100); } assertNotNull(reloadHandler.lastRemoved); - assertThat(reloadHandler.lastRemoved.serializedForm(), is("foo:test:baz")); + assertThat(reloadHandler.lastRemoved.serializedForm(), is(foo.serializedForm())); } private TenantApplications createZKAppRepo() { @@ -144,12 +143,26 @@ public class TenantApplicationsTest extends TestWithCurator { } private TenantApplications createZKAppRepo(MockReloadHandler reloadHandler) { - return ZKTenantApplications.create(curator, Path.createRoot(), reloadHandler, TenantName.from("mytenant")); + return ZKTenantApplications.create(curator, reloadHandler, tenantName); } - private static ApplicationId createAppplicationId(String name) { + private static ApplicationId createApplicationId(String name) { return new ApplicationId.Builder() - .tenant("mytenant") - .applicationName(name).instanceName("myinst").build(); + .tenant(tenantName.value()) + .applicationName(name) + .instanceName("myinst") + .build(); + } + + private void writeApplicationData(ApplicationId applicationId, long sessionId) throws Exception { + writeApplicationData(applicationId.serializedForm(), sessionId); + } + + private void writeApplicationData(String applicationId, long sessionId) throws Exception { + curatorFramework + .create() + .creatingParentsIfNeeded() + .forPath(Tenants.getApplicationsPath(tenantName).append(applicationId).getAbsolute(), + Utf8.toAsciiBytes(sessionId)); } } diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/TestTenantBuilder.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/TestTenantBuilder.java index 959dfab1bee..16ce605d4d1 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/TestTenantBuilder.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/TestTenantBuilder.java @@ -4,8 +4,8 @@ package com.yahoo.vespa.config.server.http.v2; import com.google.common.base.Function; import com.google.common.collect.Collections2; import com.yahoo.config.provision.TenantName; -import com.yahoo.path.Path; -import com.yahoo.vespa.config.server.*; +import com.yahoo.vespa.config.server.GlobalComponentRegistry; +import com.yahoo.vespa.config.server.TestComponentRegistry; import com.yahoo.vespa.config.server.application.MemoryTenantApplications; import com.yahoo.vespa.config.server.session.LocalSessionRepo; import com.yahoo.vespa.config.server.session.RemoteSessionRepo; @@ -18,7 +18,7 @@ import java.util.*; /** * Test utility for creating tenants used for testing and setup wiring of tenant stuff. * - * @author lulf + * @author Ulf Lilleengen * @since 5.1 */ public class TestTenantBuilder { @@ -32,7 +32,7 @@ public class TestTenantBuilder { public TenantBuilder createTenant(TenantName tenantName) { MemoryTenantApplications applicationRepo = new MemoryTenantApplications(); - TenantBuilder builder = TenantBuilder.create(componentRegistry, tenantName, Path.createRoot().append(tenantName.value())) + TenantBuilder builder = TenantBuilder.create(componentRegistry, tenantName) .withSessionFactory(new SessionCreateHandlerTest.MockSessionFactory()) .withLocalSessionRepo(new LocalSessionRepo(componentRegistry.getClock())) .withRemoteSessionRepo(new RemoteSessionRepo()) diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionRepoTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionRepoTest.java index 5753b2959f7..3d34d08edeb 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionRepoTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionRepoTest.java @@ -2,10 +2,11 @@ package com.yahoo.vespa.config.server.session; import com.yahoo.config.model.application.provider.FilesApplicationPackage; -import com.yahoo.path.Path; import com.yahoo.test.ManualClock; -import com.yahoo.vespa.config.server.*; import com.yahoo.config.provision.TenantName; +import com.yahoo.vespa.config.server.GlobalComponentRegistry; +import com.yahoo.vespa.config.server.TestComponentRegistry; +import com.yahoo.vespa.config.server.TestWithCurator; import com.yahoo.vespa.config.server.application.MemoryTenantApplications; import com.yahoo.vespa.config.server.deploy.TenantFileSystemDirs; import com.yahoo.io.IOUtils; @@ -20,13 +21,12 @@ import java.io.File; import java.time.Duration; import java.time.Instant; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; /** - * @author lulf + * @author Ulf Lilleengen * @since 5.1 */ public class LocalSessionRepoTest extends TestWithCurator { @@ -51,10 +51,7 @@ public class LocalSessionRepoTest extends TestWithCurator { } clock = new ManualClock(Instant.ofEpochSecond(1)); LocalSessionLoader loader = new SessionFactoryImpl(globalComponentRegistry, - new SessionCounter(globalComponentRegistry.getCurator(), - Path.fromString("counter"), - Path.fromString("sessions")), - Path.createRoot(), + new SessionCounter(globalComponentRegistry.getCurator(), tenantName), new MemoryTenantApplications(), tenantFileSystemDirs, new HostRegistry<>(), tenantName); diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/RemoteSessionRepoTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/RemoteSessionRepoTest.java index 462062ce8a8..878339bd703 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/RemoteSessionRepoTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/RemoteSessionRepoTest.java @@ -2,18 +2,22 @@ package com.yahoo.vespa.config.server.session; import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.*; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.TenantName; import com.yahoo.path.Path; import com.yahoo.text.Utf8; import com.yahoo.transaction.Transaction; -import com.yahoo.vespa.config.server.*; +import com.yahoo.vespa.config.server.TestComponentRegistry; +import com.yahoo.vespa.config.server.TestWithCurator; import com.yahoo.vespa.config.server.application.TenantApplications; import com.yahoo.vespa.config.server.tenant.Tenant; import com.yahoo.vespa.config.server.tenant.TenantBuilder; +import com.yahoo.vespa.config.server.tenant.Tenants; import com.yahoo.vespa.curator.Curator; import org.junit.Before; import org.junit.Test; @@ -27,33 +31,35 @@ import java.util.concurrent.TimeUnit; import java.util.function.LongPredicate; /** - * @author lulf + * @author Ulf Lilleengen * @since 5.1 */ public class RemoteSessionRepoTest extends TestWithCurator { + private static final TenantName tenantName = TenantName.defaultName(); + private RemoteSessionRepo remoteSessionRepo; @Before public void setupFacade() throws Exception { - createSession(2l, false); - createSession(3l, false); - curator.create(Path.fromString("/applications")); - curator.create(Path.fromString("/sessions")); - Tenant tenant = TenantBuilder.create(new TestComponentRegistry.Builder().curator(curator).build(), - TenantName.defaultName(), - Path.createRoot()).build(); + Tenant tenant = TenantBuilder.create(new TestComponentRegistry.Builder() + .curator(curator) + .build(), + tenantName) + .build(); this.remoteSessionRepo = tenant.getRemoteSessionRepo(); + curator.create(Tenants.getTenantPath(tenantName).append("/applications")); + curator.create(Tenants.getSessionsPath(tenantName)); + createSession(1l, false); + createSession(2l, false); } private void createSession(long sessionId, boolean wait) { - createSession("", sessionId, wait); + createSession(sessionId, wait, tenantName); } - - private void createSession(String root, long sessionId, boolean wait) { - Path sessionsPath = Path.fromString(root).append("sessions"); - curator.create(sessionsPath); + private void createSession(long sessionId, boolean wait, TenantName tenantName) { + Path sessionsPath = Tenants.getSessionsPath(tenantName); SessionZooKeeperClient zkc = new SessionZooKeeperClient(curator, sessionsPath.append(String.valueOf(sessionId))); zkc.createNewSession(System.currentTimeMillis(), TimeUnit.MILLISECONDS); if (wait) { @@ -64,27 +70,28 @@ public class RemoteSessionRepoTest extends TestWithCurator { @Test public void testInitialize() { + assertSessionExists(1l); assertSessionExists(2l); - assertSessionExists(3l); } @Test public void testCreateSession() throws Exception { - createSession(0l, true); - assertSessionExists(0l); + createSession(3l, true); + assertSessionExists(3l); } @Test public void testSessionStateChange() throws Exception { - Path session = Path.fromString("/sessions/0"); - createSession(0l, true); - assertSessionStatus(0l, Session.Status.NEW); - assertStatusChange(0l, Session.Status.PREPARE); - assertStatusChange(0l, Session.Status.ACTIVATE); + long sessionId = 3L; + createSession(sessionId, true); + assertSessionStatus(sessionId, Session.Status.NEW); + assertStatusChange(sessionId, Session.Status.PREPARE); + assertStatusChange(sessionId, Session.Status.ACTIVATE); + Path session = Tenants.getSessionsPath(tenantName).append("" + sessionId); curator.delete(session); - assertSessionRemoved(0l); - assertNull(remoteSessionRepo.getSession(0l)); + assertSessionRemoved(sessionId); + assertNull(remoteSessionRepo.getSession(sessionId)); } // If reading a session throws an exception it should be handled and not prevent other applications @@ -93,25 +100,25 @@ public class RemoteSessionRepoTest extends TestWithCurator { // throw an exception). @Test public void testBadApplicationRepoOnActivate() throws Exception { + long sessionId = 3L; TenantApplications applicationRepo = new FailingTenantApplications(); - curator.framework().create().forPath("/mytenant"); - Tenant tenant = TenantBuilder.create(new TestComponentRegistry.Builder().curator(curator).build(), - TenantName.from("mytenant"), - Path.fromString("mytenant")) + TenantName mytenant = TenantName.from("mytenant"); + Tenant tenant = TenantBuilder.create(new TestComponentRegistry.Builder().curator(curator).build(), mytenant) .withApplicationRepo(applicationRepo) .build(); + curator.create(Tenants.getSessionsPath(mytenant)); remoteSessionRepo = tenant.getRemoteSessionRepo(); assertThat(remoteSessionRepo.listSessions().size(), is(0)); - createSession("/mytenant", 2l, true); + createSession(sessionId, true, mytenant); assertThat(remoteSessionRepo.listSessions().size(), is(1)); } private void assertStatusChange(long sessionId, Session.Status status) throws Exception { - Path statePath = Path.fromString("/sessions/" + sessionId).append(ConfigCurator.SESSIONSTATE_ZK_SUBPATH); + Path statePath = Tenants.getSessionsPath(tenantName).append("" + sessionId).append(ConfigCurator.SESSIONSTATE_ZK_SUBPATH); curator.create(statePath); curatorFramework.setData().forPath(statePath.getAbsolute(), Utf8.toBytes(status.toString())); System.out.println("Setting status " + status + " for " + sessionId); - assertSessionStatus(0l, status); + assertSessionStatus(sessionId, status); } private void assertSessionRemoved(long sessionId) { diff --git a/container-accesslogging/src/main/java/com/yahoo/container/logging/JSONFormatter.java b/container-accesslogging/src/main/java/com/yahoo/container/logging/JSONFormatter.java index a328f0d34bd..4e9ccc341b8 100644 --- a/container-accesslogging/src/main/java/com/yahoo/container/logging/JSONFormatter.java +++ b/container-accesslogging/src/main/java/com/yahoo/container/logging/JSONFormatter.java @@ -11,6 +11,7 @@ import java.io.IOException; import java.math.BigDecimal; import java.math.RoundingMode; import java.net.URI; +import java.time.Instant; import java.util.List; import java.util.Map; import java.util.Objects; @@ -47,6 +48,8 @@ public class JSONFormatter { generator.writeStartObject(); generator.writeStringField("ip", accessLogEntry.getIpV4Address()); generator.writeNumberField("time", toTimestampInSeconds(accessLogEntry.getTimeStampMillis())); + generator.writeStringField("time-iso8601", + Instant.ofEpochMilli(accessLogEntry.getTimeStampMillis()).toString()); generator.writeNumberField("duration", durationAsSeconds(accessLogEntry.getDurationBetweenRequestResponseMillis())); generator.writeNumberField("responsesize", accessLogEntry.getReturnedContentSize()); diff --git a/container-accesslogging/src/test/java/com/yahoo/container/logging/JSONLogTestCase.java b/container-accesslogging/src/test/java/com/yahoo/container/logging/JSONLogTestCase.java index ae27d7b1814..7f81a3568dd 100644 --- a/container-accesslogging/src/test/java/com/yahoo/container/logging/JSONLogTestCase.java +++ b/container-accesslogging/src/test/java/com/yahoo/container/logging/JSONLogTestCase.java @@ -40,6 +40,7 @@ public class JSONLogTestCase extends junit.framework.TestCase { String expectedOutput = "{\"ip\":\"152.200.54.243\"," + "\"time\":920880005.023," + + "\"time-iso8601\":\"1999-03-08T08:00:05.023Z\"," + "\"duration\":0.122," + "\"responsesize\":9875," + "\"code\":200," + @@ -68,6 +69,7 @@ public class JSONLogTestCase extends junit.framework.TestCase { String expectedOutput = "{\"ip\":\"152.200.54.243\"," + "\"time\":920880005.023," + + "\"time-iso8601\":\"1999-03-08T08:00:05.023Z\"," + "\"duration\":0.122," + "\"responsesize\":9875," + "\"code\":200," + @@ -100,6 +102,7 @@ public class JSONLogTestCase extends junit.framework.TestCase { String expectedOutput = "{\"ip\":\"152.200.54.243\"," + "\"time\":920880005.023," + + "\"time-iso8601\":\"1999-03-08T08:00:05.023Z\"," + "\"duration\":0.122," + "\"responsesize\":9875," + "\"code\":200," + @@ -125,6 +128,7 @@ public class JSONLogTestCase extends junit.framework.TestCase { expectedOutput = "{\"ip\":\"152.200.54.243\"," + "\"time\":920880005.023," + + "\"time-iso8601\":\"1999-03-08T08:00:05.023Z\"," + "\"duration\":0.122," + "\"responsesize\":9875," + "\"code\":200," + @@ -171,6 +175,7 @@ public class JSONLogTestCase extends junit.framework.TestCase { String expectedOutput = "{\"ip\":\"152.200.54.243\"," + "\"time\":920880005.023," + + "\"time-iso8601\":\"1999-03-08T08:00:05.023Z\"," + "\"duration\":0.122," + "\"responsesize\":9875," + "\"code\":200," + diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/OwnershipIssues.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/OwnershipIssues.java new file mode 100644 index 00000000000..8ded0c5fb52 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/OwnershipIssues.java @@ -0,0 +1,48 @@ +package com.yahoo.vespa.hosted.controller.api.integration.organization; + +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; + +import java.util.Optional; + +/** + * Periodically issues ownership confirmation requests for given applications, and escalates the issues if needed. + * + * Even machines wrought from cold steel occasionally require the gentle touch only a fleshling can provide. + * By making humans regularly acknowledge their dedication to given applications, this class provides the machine + * with reassurance that any misbehaving applications will swiftly be dealt with. + * Ignored confirmation requests are periodically redirected to humans of higher rank, until they are acknowledged. + * + * @author jvenstad + */ +public interface OwnershipIssues { + + /** + * Ensure ownership of the given application has been recently confirmed by the given property. + * + * @param issueId ID of the previous ownership issue filed for the given application. + * @param applicationId ID of the application for which to file an issue. + * @param propertyId ID of the property responsible for the given application. + * @return ID of the created issue, if one was created. + */ + Optional<IssueId> confirmOwnership(Optional<IssueId> issueId, ApplicationId applicationId, PropertyId propertyId); + + /** + * Ensure ownership of the given application has been recently confirmed by the given user. + * + * @param issueId ID of the previous ownership issue filed for the given application. + * @param applicationId ID of the application for which to file an issue. + * @param owner ID of the user responsible for the given application. + * @return ID of the created issue, if one was created. + */ + Optional<IssueId> confirmOwnership(Optional<IssueId> issueId, ApplicationId applicationId, User owner); + + /** + * Make sure the given ownership confirmation request is acted upon, unless it is already acknowledged. + * + * @param issueId ID of the ownership issue to escalate. + * @param propertyId ID of the property responsible for the issue, if any. + */ + void ensureResponse(IssueId issueId, Optional<PropertyId> propertyId); + +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/DummyOwnershipIssues.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/DummyOwnershipIssues.java new file mode 100644 index 00000000000..0cf103739d1 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/DummyOwnershipIssues.java @@ -0,0 +1,28 @@ +package com.yahoo.vespa.hosted.controller.api.integration.stubs; + +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.OwnershipIssues; +import com.yahoo.vespa.hosted.controller.api.integration.organization.User; + +import java.util.Optional; + +public class DummyOwnershipIssues implements OwnershipIssues { + + @Override + public Optional<IssueId> confirmOwnership(Optional<IssueId> issueId, ApplicationId applicationId, PropertyId propertyId) { + return Optional.empty(); + } + + @Override + public Optional<IssueId> confirmOwnership(Optional<IssueId> issueId, ApplicationId applicationId, User owner) { + return Optional.empty(); + } + + @Override + public void ensureResponse(IssueId issueId, Optional<PropertyId> propertyId) { + + } + +} diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java index a025ce1bd32..7154e10eca0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java @@ -8,6 +8,7 @@ import com.yahoo.config.application.api.ValidationOverrides; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.Zone; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; import com.yahoo.vespa.hosted.controller.application.Change; import com.yahoo.vespa.hosted.controller.application.Change.VersionChange; import com.yahoo.vespa.hosted.controller.application.Deployment; @@ -38,26 +39,27 @@ public class Application { private final DeploymentJobs deploymentJobs; private final Optional<Change> deploying; private final boolean outstandingChange; + private final Optional<IssueId> ownershipIssueId; /** Creates an empty application */ public Application(ApplicationId id) { this(id, DeploymentSpec.empty, ValidationOverrides.empty, ImmutableMap.of(), new DeploymentJobs(Optional.empty(), Collections.emptyList(), Optional.empty()), - Optional.empty(), false); + Optional.empty(), false, Optional.empty()); } /** Used from persistence layer: Do not use */ public Application(ApplicationId id, DeploymentSpec deploymentSpec, ValidationOverrides validationOverrides, - List<Deployment> deployments, - DeploymentJobs deploymentJobs, Optional<Change> deploying, boolean outstandingChange) { + List<Deployment> deployments, DeploymentJobs deploymentJobs, Optional<Change> deploying, + boolean outstandingChange, Optional<IssueId> ownershipIssueId) { this(id, deploymentSpec, validationOverrides, deployments.stream().collect(Collectors.toMap(Deployment::zone, d -> d)), - deploymentJobs, deploying, outstandingChange); + deploymentJobs, deploying, outstandingChange, ownershipIssueId); } Application(ApplicationId id, DeploymentSpec deploymentSpec, ValidationOverrides validationOverrides, Map<Zone, Deployment> deployments, DeploymentJobs deploymentJobs, Optional<Change> deploying, - boolean outstandingChange) { + boolean outstandingChange, Optional<IssueId> ownershipIssueId) { Objects.requireNonNull(id, "id cannot be null"); Objects.requireNonNull(deploymentSpec, "deploymentSpec cannot be null"); Objects.requireNonNull(validationOverrides, "validationOverrides cannot be null"); @@ -71,6 +73,7 @@ public class Application { this.deploymentJobs = deploymentJobs; this.deploying = deploying; this.outstandingChange = outstandingChange; + this.ownershipIssueId = ownershipIssueId; } public ApplicationId id() { return id; } @@ -159,4 +162,8 @@ public class Application { return "application '" + id + "'"; } + public Optional<IssueId> ownershipIssueId() { + return ownershipIssueId; + } + } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java index d47197cd6d5..3ef904ebf85 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java @@ -38,20 +38,20 @@ public class LockedApplication extends Application { LockedApplication(Application application, Lock lock) { super(application.id(), application.deploymentSpec(), application.validationOverrides(), application.deployments(), application.deploymentJobs(), application.deploying(), - application.hasOutstandingChange()); + application.hasOutstandingChange(), application.ownershipIssueId()); this.lock = Objects.requireNonNull(lock, "lock cannot be null"); } public LockedApplication withProjectId(long projectId) { return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments(), deploymentJobs().withProjectId(projectId), deploying(), - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication with(IssueId issueId) { return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments(), deploymentJobs().with(issueId), deploying(), - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication withJobCompletion(DeploymentJobs.JobReport report, Instant notificationTime, @@ -60,7 +60,7 @@ public class LockedApplication extends Application { deployments(), deploymentJobs().withCompletion(report, notificationTime, controller), - deploying(), hasOutstandingChange()), lock); + deploying(), hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication withJobTriggering(DeploymentJobs.JobType type, Optional<Change> change, @@ -72,7 +72,7 @@ public class LockedApplication extends Application { deployRevisionFor(type, controller), reason, triggerTime), - deploying(), hasOutstandingChange()), lock); + deploying(), hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication with(Deployment deployment) { @@ -80,13 +80,13 @@ public class LockedApplication extends Application { deployments.put(deployment.zone(), deployment); return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments, deploymentJobs(), deploying(), - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication with(DeploymentJobs deploymentJobs) { return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments(), deploymentJobs, deploying(), - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication withoutDeploymentIn(Zone zone) { @@ -94,38 +94,44 @@ public class LockedApplication extends Application { deployments.remove(zone); return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments, deploymentJobs(), deploying(), - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication withoutDeploymentJob(DeploymentJobs.JobType jobType) { DeploymentJobs deploymentJobs = deploymentJobs().without(jobType); return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments(), deploymentJobs, deploying(), - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication with(DeploymentSpec deploymentSpec) { return new LockedApplication(new Application(id(), deploymentSpec, validationOverrides(), deployments(), deploymentJobs(), deploying(), - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication with(ValidationOverrides validationOverrides) { return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides, deployments(), deploymentJobs(), deploying(), - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication withDeploying(Optional<Change> deploying) { return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments(), deploymentJobs(), deploying, - hasOutstandingChange()), lock); + hasOutstandingChange(), ownershipIssueId()), lock); } public LockedApplication withOutstandingChange(boolean outstandingChange) { - return new LockedApplication(new Application(id(), deploymentSpec(), - validationOverrides(), deployments(), - deploymentJobs(), deploying(), outstandingChange), lock); + return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), + deployments(), deploymentJobs(), deploying(), + outstandingChange, ownershipIssueId()), lock); + } + + public LockedApplication withOwnershipIssueId(IssueId issueId) { + return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), + deployments(), deploymentJobs(), deploying(), + hasOutstandingChange(), Optional.of(issueId)), lock); } private Version deployVersionFor(DeploymentJobs.JobType jobType, Controller controller) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java new file mode 100644 index 00000000000..5d9cf291af7 --- /dev/null +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java @@ -0,0 +1,85 @@ +package com.yahoo.vespa.hosted.controller.maintenance; + +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.vespa.curator.Lock; +import com.yahoo.vespa.hosted.controller.Application; +import com.yahoo.vespa.hosted.controller.Controller; +import com.yahoo.vespa.hosted.controller.api.Tenant; +import com.yahoo.vespa.hosted.controller.api.application.v4.model.TenantType; +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; +import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.OwnershipIssues; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.User; + +import java.time.Duration; +import java.util.NoSuchElementException; +import java.util.Optional; +import java.util.logging.Level; + +public class ApplicationOwnershipConfirmer extends Maintainer { + + private final OwnershipIssues ownershipIssues; + + public ApplicationOwnershipConfirmer(Controller controller, Duration interval, JobControl jobControl, OwnershipIssues ownershipIssues) { + super(controller, interval, jobControl); + this.ownershipIssues = ownershipIssues; + } + + @Override + protected void maintain() { + confirmApplicationOwnerships(); + ensureConfirmationResponses(); + } + + /** File an ownership issue with the owners of all applications we know about. */ + private void confirmApplicationOwnerships() { + for (Application application : controller().applications().asList()) { + try { + Tenant tenant = ownerOf(application.id()); + Optional<IssueId> ourIssueId = application.ownershipIssueId(); + ourIssueId = tenant.tenantType() == TenantType.USER + ? ownershipIssues.confirmOwnership(ourIssueId, application.id(), userFor(tenant)) + : ownershipIssues.confirmOwnership(ourIssueId, application.id(), propertyIdFor(tenant)); + ourIssueId.ifPresent(issueId -> store(issueId, application.id())); + } + catch (RuntimeException e) { // Catch errors due to wrong data in the controller, or issues client timeout. + log.log(Level.WARNING, "Exception caught when attempting to file an issue for " + application.id(), e); + } + } + } + + /** Escalate ownership issues which have not been closed before a defined amount of time has passed. */ + private void ensureConfirmationResponses() { + for (Application application : controller().applications().asList()) + application.ownershipIssueId().ifPresent(issueId -> { + try { + ownershipIssues.ensureResponse(issueId, ownerOf(application.id()).getPropertyId()); + } + catch (RuntimeException e) { + log.log(Level.WARNING, "Exception caught when attempting to escalate issue with id " + issueId, e); + } + }); + } + + private Tenant ownerOf(ApplicationId applicationId) { + return controller().tenants().tenant(new TenantId(applicationId.tenant().value())) + .orElseThrow(() -> new IllegalStateException("No tenant found for application " + applicationId)); + } + + protected User userFor(Tenant tenant) { + return User.from(tenant.getId().id().replaceFirst("by-", "")); + } + + protected PropertyId propertyIdFor(Tenant tenant) { + return tenant.getPropertyId() + .orElseThrow(() -> new NoSuchElementException("No PropertyId is listed for non-user tenant " + tenant)); + } + + protected void store(IssueId issueId, ApplicationId applicationId) { + try (Lock lock = controller().applications().lock(applicationId)) { + controller().applications().get(applicationId, lock) + .ifPresent(application -> controller().applications().store(application.withOwnershipIssueId(issueId))); + } + } +} 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 01edc269116..1969b41fd63 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 @@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.yahoo.component.AbstractComponent; import com.yahoo.jdisc.Metric; import com.yahoo.vespa.hosted.controller.Controller; +import com.yahoo.vespa.hosted.controller.api.integration.organization.OwnershipIssues; import com.yahoo.vespa.hosted.controller.api.integration.organization.DeploymentIssues; import com.yahoo.vespa.hosted.controller.api.integration.chef.Chef; import com.yahoo.vespa.hosted.controller.maintenance.config.MaintainerConfig; @@ -32,11 +33,12 @@ public class ControllerMaintenance extends AbstractComponent { private final ClusterInfoMaintainer clusterInfoMaintainer; private final ClusterUtilizationMaintainer clusterUtilizationMaintainer; private final DeploymentMetricsMaintainer deploymentMetricsMaintainer; + private final ApplicationOwnershipConfirmer applicationOwnershipConfirmer; @SuppressWarnings("unused") // instantiated by Dependency Injection public ControllerMaintenance(MaintainerConfig maintainerConfig, Controller controller, CuratorDb curator, JobControl jobControl, Metric metric, Chef chefClient, - DeploymentIssues deploymentIssues) { + DeploymentIssues deploymentIssues, OwnershipIssues ownershipIssues) { Duration maintenanceInterval = Duration.ofMinutes(maintainerConfig.intervalMinutes()); this.jobControl = jobControl; deploymentExpirer = new DeploymentExpirer(controller, maintenanceInterval, jobControl); @@ -49,6 +51,7 @@ public class ControllerMaintenance extends AbstractComponent { clusterInfoMaintainer = new ClusterInfoMaintainer(controller, Duration.ofHours(2), jobControl); clusterUtilizationMaintainer = new ClusterUtilizationMaintainer(controller, Duration.ofHours(2), jobControl); deploymentMetricsMaintainer = new DeploymentMetricsMaintainer(controller, Duration.ofMinutes(10), jobControl); + applicationOwnershipConfirmer = new ApplicationOwnershipConfirmer(controller, Duration.ofHours(1), jobControl, ownershipIssues); } public Upgrader upgrader() { return upgrader; } @@ -68,6 +71,7 @@ public class ControllerMaintenance extends AbstractComponent { clusterUtilizationMaintainer.deconstruct(); clusterInfoMaintainer.deconstruct(); deploymentMetricsMaintainer.deconstruct(); + applicationOwnershipConfirmer.deconstruct(); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java index b4708dccb6b..4ab1426539d 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java @@ -67,7 +67,7 @@ public class DeploymentIssueReporter extends Maintainer { if (failingApplications.contains(application.id())) fileDeploymentIssueFor(application.id()); else - storeIssueId(application.id(), null); + store(application.id(), null); } /** @@ -111,7 +111,7 @@ public class DeploymentIssueReporter extends Maintainer { IssueId issueId = tenant.tenantType() == TenantType.USER ? deploymentIssues.fileUnlessOpen(ourIssueId, applicationId, userFor(tenant)) : deploymentIssues.fileUnlessOpen(ourIssueId, applicationId, propertyIdFor(tenant)); - storeIssueId(applicationId, issueId); + store(applicationId, issueId); } catch (RuntimeException e) { // Catch errors due to wrong data in the controller, or issues client timeout. log.log(Level.WARNING, "Exception caught when attempting to file an issue for " + applicationId, e); @@ -130,7 +130,7 @@ public class DeploymentIssueReporter extends Maintainer { })); } - private void storeIssueId(ApplicationId id, IssueId issueId) { + private void store(ApplicationId id, IssueId issueId) { try (Lock lock = controller().applications().lock(id)) { controller().applications().get(id, lock).ifPresent( application -> controller().applications().store(application.with(issueId)) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java index 1d19d8ca522..ebab2054d4f 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java @@ -30,7 +30,6 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { private final ScheduledExecutorService service; public Maintainer(Controller controller, Duration interval, JobControl jobControl) { - initId(new ComponentId(name())); this.controller = controller; this.maintenanceInterval = interval; this.jobControl = jobControl; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java index 130d6f92d59..d294ad5dad8 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java @@ -51,6 +51,7 @@ public class ApplicationSerializer { private final String deploymentJobsField = "deploymentJobs"; private final String deployingField = "deployingField"; private final String outstandingChangeField = "outstandingChangeField"; + private final String ownershipIssueIdField = "ownershipIssueId"; // Deployment fields private final String zoneField = "zone"; @@ -123,6 +124,7 @@ public class ApplicationSerializer { toSlime(application.deploymentJobs(), root.setObject(deploymentJobsField)); toSlime(application.deploying(), root); root.setBool(outstandingChangeField, application.hasOutstandingChange()); + application.ownershipIssueId().ifPresent(issueId -> root.setString(ownershipIssueIdField, issueId.value())); return slime; } @@ -257,9 +259,10 @@ public class ApplicationSerializer { DeploymentJobs deploymentJobs = deploymentJobsFromSlime(root.field(deploymentJobsField)); Optional<Change> deploying = changeFromSlime(root.field(deployingField)); boolean outstandingChange = root.field(outstandingChangeField).asBool(); + Optional<IssueId> ownershipIssueId = optionalString(root.field(ownershipIssueIdField)).map(IssueId::from); return new Application(id, deploymentSpec, validationOverrides, deployments, - deploymentJobs, deploying, outstandingChange); + deploymentJobs, deploying, outstandingChange, ownershipIssueId); } private List<Deployment> deploymentsFromSlime(Inspector array) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerCuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerCuratorDb.java deleted file mode 100644 index 53c152308d9..00000000000 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerCuratorDb.java +++ /dev/null @@ -1,69 +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.controller.persistence; - -import com.google.inject.Inject; -import com.yahoo.cloud.config.ClusterInfoConfig; -import com.yahoo.cloud.config.ZookeeperServerConfig; -import com.yahoo.net.HostName; -import com.yahoo.vespa.curator.Curator; -import com.yahoo.vespa.zookeeper.ZooKeeperServer; - -import java.util.stream.Collectors; - -/** - * A CuratorDb that configures its own ZooKeeper cluster - * - * @author bratseth - */ -// TODO: Remove when multi controller is enabled -@Deprecated -public class ControllerCuratorDb extends CuratorDb { - - /** Use a nonstandard zk port to avoid interfering with connection to the config server zk cluster */ - private static final int zooKeeperPort = 2281; - - @SuppressWarnings("unused") // This server is used (only) from the curator instance of this over the network */ - private final ZooKeeperServer zooKeeperServer; - - /** Create a curator db which also set up a ZooKeeper server (such that this instance is both client and server) */ - @Inject - public ControllerCuratorDb(ClusterInfoConfig clusterInfo) { - super(new Curator(toConnectionSpec(clusterInfo))); - this.zooKeeperServer = new ZooKeeperServer(toZookeeperServerConfig(clusterInfo)); - } - - private static ZookeeperServerConfig toZookeeperServerConfig(ClusterInfoConfig clusterInfo) { - ZookeeperServerConfig.Builder b = new ZookeeperServerConfig.Builder(); - b.zooKeeperConfigFile("conf/zookeeper/controller-zookeeper.cfg"); - b.dataDir("var/controller-zookeeper"); - b.clientPort(zooKeeperPort); - b.myidFile("var/controller-zookeeper/myid"); - b.myid(myIndex(clusterInfo)); - - for (ClusterInfoConfig.Services clusterMember : clusterInfo.services()) { - ZookeeperServerConfig.Server.Builder server = new ZookeeperServerConfig.Server.Builder(); - server.id(clusterMember.index()); - server.hostname(clusterMember.hostname()); - server.quorumPort(zooKeeperPort + 1); - server.electionPort(zooKeeperPort + 2); - b.server(server); - } - return new ZookeeperServerConfig(b); - } - - private static Integer myIndex(ClusterInfoConfig clusterInfo) { - String hostname = HostName.getLocalhost(); - return clusterInfo.services().stream() - .filter(service -> service.hostname().equals(hostname)) - .map(ClusterInfoConfig.Services::index) - .findFirst() - .orElseThrow(() -> new IllegalStateException("Unable to find index for this node by hostname '" + - hostname + "'")); - } - - private static String toConnectionSpec(ClusterInfoConfig clusterInfo) { - return clusterInfo.services().stream() - .map(member -> member.hostname() + ":" + zooKeeperPort) - .collect(Collectors.joining(",")); - } -} diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java new file mode 100644 index 00000000000..25daf81b319 --- /dev/null +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java @@ -0,0 +1,107 @@ +package com.yahoo.vespa.hosted.controller.maintenance; + +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.vespa.hosted.controller.Application; +import com.yahoo.vespa.hosted.controller.ControllerTester; +import com.yahoo.vespa.hosted.controller.api.Tenant; +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; +import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.OwnershipIssues; +import com.yahoo.vespa.hosted.controller.api.integration.organization.User; +import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb; +import org.junit.Before; +import org.junit.Test; + +import java.time.Duration; +import java.util.Optional; +import java.util.function.Supplier; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * @#author jvenstad + */ +public class ApplicationOwnershipConfirmerTest { + + private MockOwnershipIssues issues; + private ApplicationOwnershipConfirmer confirmer; + private ControllerTester tester; + + @Before + public void setup() { + tester = new ControllerTester(); + issues = new MockOwnershipIssues(); + confirmer = new ApplicationOwnershipConfirmer(tester.controller(), Duration.ofDays(1), new JobControl(new MockCuratorDb()), issues); + } + + + @Test + public void testConfirmation() { + TenantId property = tester.createTenant("tenant", "domain", 1L); + ApplicationId propertyAppId = tester.createApplication(property, "application", "default", 1).id(); + Supplier<Application> propertyApp = () -> tester.controller().applications().require(propertyAppId); + + TenantId user = new TenantId("by-user"); + tester.controller().tenants().addTenant(Tenant.createUserTenant(new TenantId("by-user")), Optional.empty()); + assertTrue(tester.controller().tenants().tenant(user).isPresent()); + ApplicationId userAppId = tester.createApplication(user, "application", "default", 1).id(); + Supplier<Application> userApp = () -> tester.controller().applications().require(userAppId); + + assertFalse("No issue is initially stored for a new application.", propertyApp.get().ownershipIssueId().isPresent()); + assertFalse("No issue is initially stored for a new application.", userApp.get().ownershipIssueId().isPresent()); + assertFalse("No escalation has been attempted for a new application", issues.escalatedForProperty || issues.escalatedForUser); + + // Set response from the issue mock, which will be obtained by the maintainer on issue filing. + Optional<IssueId> issueId = Optional.of(IssueId.from("1")); + issues.response = issueId; + confirmer.maintain(); + confirmer.maintain(); + + assertEquals("Confirmation issue has been filed for property owned application.", propertyApp.get().ownershipIssueId(), issueId); + assertEquals("Confirmation issue has been filed for user owned application.", userApp.get().ownershipIssueId(), issueId); + assertTrue("Both applications have had their responses ensured.", issues.escalatedForProperty && issues.escalatedForUser); + + // No new issue is created, so return empty now. + issues.response = Optional.empty(); + confirmer.maintain(); + confirmer.maintain(); + + assertEquals("Confirmation issue reference is not updated when no issue id is returned.", propertyApp.get().ownershipIssueId(), issueId); + + // Time has passed, and a new confirmation issue is in order. + Optional<IssueId> issueId2 = Optional.of(IssueId.from("2")); + issues.response = issueId2; + confirmer.maintain(); + confirmer.maintain(); + + assertEquals("A new confirmation issue id is stored when something is returned to the maintainer.", propertyApp.get().ownershipIssueId(), issueId2); + } + + private class MockOwnershipIssues implements OwnershipIssues { + + private Optional<IssueId> response; + private boolean escalatedForProperty = false; + private boolean escalatedForUser = false; + + @Override + public Optional<IssueId> confirmOwnership(Optional<IssueId> issueId, ApplicationId applicationId, PropertyId propertyId) { + return response; + } + + @Override + public Optional<IssueId> confirmOwnership(Optional<IssueId> issueId, ApplicationId applicationId, User owner) { + return response; + } + + @Override + public void ensureResponse(IssueId issueId, Optional<PropertyId> propertyId) { + if (propertyId.isPresent()) escalatedForProperty = true; + else escalatedForUser = true; + } + + } + +} 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 41cf7a331bb..e57edcf6da0 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 @@ -54,7 +54,7 @@ public class DeploymentIssueReporterTest { public void setup() { tester = new DeploymentTester(); issues = new MockDeploymentIssues(); - reporter = new DeploymentIssueReporter(tester.controller(), issues, Duration.ofMinutes(5), new JobControl(new MockCuratorDb())); + reporter = new DeploymentIssueReporter(tester.controller(), issues, Duration.ofDays(1), new JobControl(new MockCuratorDb())); } @Test diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java index daccab8efbf..c737308e247 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java @@ -13,6 +13,7 @@ import com.yahoo.slime.Slime; import com.yahoo.vespa.config.SlimeUtils; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.ControllerTester; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; import com.yahoo.vespa.hosted.controller.application.ApplicationRevision; import com.yahoo.vespa.hosted.controller.application.Change; import com.yahoo.vespa.hosted.controller.application.ClusterInfo; @@ -82,7 +83,8 @@ public class ApplicationSerializerTest { validationOverrides, deployments, deploymentJobs, Optional.of(new Change.VersionChange(Version.fromString("6.7"))), - true); + true, + Optional.of(IssueId.from("1234"))); Application serialized = applicationSerializer.fromSlime(applicationSerializer.toSlime(original)); @@ -108,6 +110,8 @@ public class ApplicationSerializerTest { assertEquals(original.hasOutstandingChange(), serialized.hasOutstandingChange()); + assertEquals(original.ownershipIssueId(), serialized.ownershipIssueId()); + assertEquals(original.deploying(), serialized.deploying()); // Test cluster utilization diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java index e6c0ce9027d..25b7d51b84f 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java @@ -43,6 +43,7 @@ public class ControllerContainerTest { " <component id='com.yahoo.vespa.hosted.controller.api.integration.github.GitHubMock'/>" + " <component id='com.yahoo.vespa.hosted.controller.api.integration.routing.MemoryGlobalRoutingService'/>" + " <component id='com.yahoo.vespa.hosted.controller.api.integration.stubs.LoggingDeploymentIssues'/>" + + " <component id='com.yahoo.vespa.hosted.controller.api.integration.stubs.DummyOwnershipIssues'/>" + " <component id='com.yahoo.vespa.hosted.controller.api.integration.organization.MockOrganization'/>" + " <component id='com.yahoo.vespa.hosted.controller.ConfigServerClientMock'/>" + " <component id='com.yahoo.vespa.hosted.controller.ZoneRegistryMock'/>" + diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/responses/maintenance.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/responses/maintenance.json index 33b9d4c70d5..354bab4379c 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/responses/maintenance.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/responses/maintenance.json @@ -1,6 +1,9 @@ { "jobs": [ { + "name": "ApplicationOwnershipConfirmer" + }, + { "name": "ClusterInfoMaintainer" }, { @@ -34,4 +37,4 @@ "inactive": [ "DeploymentExpirer" ] -}
\ No newline at end of file +} diff --git a/document/src/vespa/document/bucket/bucket.cpp b/document/src/vespa/document/bucket/bucket.cpp index d2855c0b3a1..b79d45eee34 100644 --- a/document/src/vespa/document/bucket/bucket.cpp +++ b/document/src/vespa/document/bucket/bucket.cpp @@ -23,4 +23,9 @@ vespalib::asciistream& operator<<(vespalib::asciistream& os, const Bucket& id) return os << "Bucket(" << id.getBucketSpace() << ", " << id.getBucketId() << ")"; } +std::ostream& operator<<(std::ostream& os, const Bucket& id) +{ + return os << id.toString(); +} + } diff --git a/document/src/vespa/document/bucket/bucket.h b/document/src/vespa/document/bucket/bucket.h index 5aa9b360c42..44068e1c443 100644 --- a/document/src/vespa/document/bucket/bucket.h +++ b/document/src/vespa/document/bucket/bucket.h @@ -45,5 +45,6 @@ private: }; vespalib::asciistream& operator<<(vespalib::asciistream&, const Bucket&); +std::ostream& operator<<(std::ostream&, const Bucket&); } diff --git a/document/src/vespa/document/test/make_document_bucket.cpp b/document/src/vespa/document/test/make_document_bucket.cpp index f4d22f7b83f..a24f757c99f 100644 --- a/document/src/vespa/document/test/make_document_bucket.cpp +++ b/document/src/vespa/document/test/make_document_bucket.cpp @@ -1,12 +1,13 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "make_document_bucket.h" +#include "make_bucket_space.h" namespace document::test { Bucket makeDocumentBucket(BucketId bucketId) { - return Bucket(BucketSpace::placeHolder(), bucketId); + return Bucket(makeBucketSpace(), bucketId); } } diff --git a/fileacquirer/pom.xml b/fileacquirer/pom.xml index 4a7d22a10f5..eb040eddffb 100644 --- a/fileacquirer/pom.xml +++ b/fileacquirer/pom.xml @@ -21,11 +21,6 @@ <artifactId>config</artifactId> <version>${project.version}</version> </dependency> - <dependency> - <groupId>com.yahoo.vespa</groupId> - <artifactId>filedistribution</artifactId> - <version>${project.version}</version> - </dependency> </dependencies> <build> <plugins> diff --git a/jdisc_core_test/test_bundles/app-h-log/src/main/java/com/yahoo/jdisc/bundle/ApplicationH.java b/jdisc_core_test/test_bundles/app-h-log/src/main/java/com/yahoo/jdisc/bundle/ApplicationH.java index ae37e4e6cfc..f014a16ddb3 100644 --- a/jdisc_core_test/test_bundles/app-h-log/src/main/java/com/yahoo/jdisc/bundle/ApplicationH.java +++ b/jdisc_core_test/test_bundles/app-h-log/src/main/java/com/yahoo/jdisc/bundle/ApplicationH.java @@ -8,7 +8,7 @@ import com.yahoo.jdisc.application.ContainerActivator; import com.yahoo.jdisc.service.CurrentContainer; /** - * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a> + * @author Simon Thoresen */ public class ApplicationH extends AbstractApplication { diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java index 7ec51f35b74..8255e16e0ee 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactory.java @@ -5,9 +5,9 @@ import com.google.inject.Inject; import com.yahoo.jdisc.Metric; import com.yahoo.jdisc.http.ConnectorConfig; import com.yahoo.jdisc.http.ConnectorConfig.Ssl; -import com.yahoo.jdisc.http.ConnectorConfig.Ssl.PemKeyStore; import com.yahoo.jdisc.http.SecretStore; -import com.yahoo.jdisc.http.ssl.pem.PemSslKeyStore; +import com.yahoo.jdisc.http.ssl.DefaultSslKeyStoreContext; +import com.yahoo.jdisc.http.ssl.SslKeyStoreConfigurator; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.HttpConnectionFactory; @@ -17,16 +17,7 @@ import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.SslConnectionFactory; import org.eclipse.jetty.util.ssl.SslContextFactory; -import java.io.IOException; -import java.io.UncheckedIOException; import java.nio.channels.ServerSocketChannel; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.security.KeyStore; -import java.util.logging.Logger; - -import static com.yahoo.jdisc.http.ConnectorConfig.Ssl.KeyStoreType.Enum.JKS; -import static com.yahoo.jdisc.http.ConnectorConfig.Ssl.KeyStoreType.Enum.PEM; /** * @author Einar M R Rosenvinge @@ -34,14 +25,17 @@ import static com.yahoo.jdisc.http.ConnectorConfig.Ssl.KeyStoreType.Enum.PEM; */ public class ConnectorFactory { - private final static Logger log = Logger.getLogger(ConnectorFactory.class.getName()); private final ConnectorConfig connectorConfig; private final SecretStore secretStore; + private final SslKeyStoreConfigurator sslKeyStoreConfigurator; @Inject - public ConnectorFactory(ConnectorConfig connectorConfig, SecretStore secretStore) { + public ConnectorFactory(ConnectorConfig connectorConfig, + SecretStore secretStore, + SslKeyStoreConfigurator sslKeyStoreConfigurator) { this.connectorConfig = connectorConfig; this.secretStore = secretStore; + this.sslKeyStoreConfigurator = sslKeyStoreConfigurator; if (connectorConfig.ssl().enabled()) validateSslConfig(connectorConfig); @@ -50,30 +44,6 @@ public class ConnectorFactory { // TODO: can be removed when we have dedicated SSL config in services.xml private static void validateSslConfig(ConnectorConfig config) { ConnectorConfig.Ssl ssl = config.ssl(); - - if (ssl.keyStoreType() == JKS) { - if (!ssl.pemKeyStore().keyPath().isEmpty() || ! ssl.pemKeyStore().certificatePath().isEmpty()) { - throw new IllegalArgumentException("pemKeyStore attributes can not be set when keyStoreType is JKS."); - } - if (ssl.keyDbKey().isEmpty()) { - throw new IllegalArgumentException("Missing password for JKS keystore"); - } - } - if (ssl.keyStoreType() == PEM) { - if (! ssl.keyStorePath().isEmpty()) { - throw new IllegalArgumentException("keyStorePath can not be set when keyStoreType is PEM"); - } - if (!ssl.keyDbKey().isEmpty()) { - // TODO Make an error once there are separate passwords for truststore and keystore - log.warning("Encrypted PEM key stores are not supported. Password is only applied to truststore"); - } - if (ssl.pemKeyStore().certificatePath().isEmpty()) { - throw new IllegalArgumentException("Missing certificate path."); - } - if (ssl.pemKeyStore().keyPath().isEmpty()) { - throw new IllegalArgumentException("Missing key path."); - } - } if (!ssl.trustStorePath().isEmpty() && ssl.useTrustStorePassword() && ssl.keyDbKey().isEmpty()) { throw new IllegalArgumentException("Missing password for JKS truststore"); } @@ -128,6 +98,9 @@ public class ConnectorFactory { Ssl sslConfig = connectorConfig.ssl(); SslContextFactory factory = new SslContextFactory(); + + sslKeyStoreConfigurator.configure(new DefaultSslKeyStoreContext(factory)); + switch (sslConfig.clientAuth()) { case NEED_AUTH: factory.setNeedClientAuth(true); @@ -172,16 +145,6 @@ public class ConnectorFactory { } String keyDbPassword = sslConfig.keyDbKey(); - switch (sslConfig.keyStoreType()) { - case PEM: - factory.setKeyStore(createPemKeyStore(sslConfig.pemKeyStore())); - break; - case JKS: - factory.setKeyStorePath(sslConfig.keyStorePath()); - factory.setKeyStoreType(sslConfig.keyStoreType().toString()); - factory.setKeyStorePassword(secretStore.getSecret(keyDbPassword)); - break; - } if (!sslConfig.trustStorePath().isEmpty()) { factory.setTrustStorePath(sslConfig.trustStorePath()); @@ -196,17 +159,4 @@ public class ConnectorFactory { return new SslConnectionFactory(factory, HttpVersion.HTTP_1_1.asString()); } - private static KeyStore createPemKeyStore(PemKeyStore pemKeyStore) { - try { - Path certificatePath = Paths.get(pemKeyStore.certificatePath()); - Path keyPath = Paths.get(pemKeyStore.keyPath()); - return new PemSslKeyStore(certificatePath, keyPath) - .loadJavaKeyStore(); - } catch (IOException e) { - throw new UncheckedIOException(e); - } catch (Exception e) { - throw new RuntimeException("Failed setting up key store for " + pemKeyStore.keyPath() + ", " + pemKeyStore.certificatePath(), e); - } - } - } diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java index 7f169c7c8d0..5cabe8acd27 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestDispatch.java @@ -12,6 +12,7 @@ import com.yahoo.jdisc.handler.OverloadException; import com.yahoo.jdisc.handler.RequestHandler; import com.yahoo.jdisc.http.HttpHeaders; import com.yahoo.jdisc.http.HttpRequest; +import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.server.HttpConnection; import javax.servlet.AsyncContext; @@ -122,7 +123,11 @@ class HttpRequestDispatch { boolean reportedError = false; if (error != null) { - if (!(error instanceof OverloadException || error instanceof BindingNotFoundException)) { + if (error instanceof EofException) { + log.log(Level.FINE, + "Network connection was unexpectedly terminated: " + parent.servletRequest.getRequestURI(), + error); + } else if (!(error instanceof OverloadException || error instanceof BindingNotFoundException)) { log.log(Level.WARNING, "Request failed: " + parent.servletRequest.getRequestURI(), error); } reportedError = true; diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/DefaultSslKeyStoreConfigurator.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/DefaultSslKeyStoreConfigurator.java new file mode 100644 index 00000000000..fb0a5869bb3 --- /dev/null +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/DefaultSslKeyStoreConfigurator.java @@ -0,0 +1,95 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.jdisc.http.ssl; + +import com.google.inject.Inject; +import com.yahoo.jdisc.http.ConnectorConfig; +import com.yahoo.jdisc.http.SecretStore; +import com.yahoo.jdisc.http.ssl.pem.PemSslKeyStore; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.logging.Logger; + +/** + * @author bjorncs + */ +public class DefaultSslKeyStoreConfigurator implements SslKeyStoreConfigurator { + + private static final Logger log = Logger.getLogger(DefaultSslKeyStoreConfigurator.class.getName()); + + private final SecretStore secretStore; + private final ConnectorConfig.Ssl config; + + @Inject + public DefaultSslKeyStoreConfigurator(ConnectorConfig config, SecretStore secretStore) { + validateConfig(config.ssl()); + this.secretStore = secretStore; + this.config = config.ssl(); + } + + private static void validateConfig(ConnectorConfig.Ssl config) { + if (!config.enabled()) return; + switch (config.keyStoreType()) { + case JKS: + validateJksConfig(config); + break; + case PEM: + validatePemConfig(config); + break; + } + } + + @Override + public void configure(SslKeyStoreContext context) { + if (!config.enabled()) return; + switch (config.keyStoreType()) { + case JKS: + context.updateKeyStore(config.keyStorePath(), "JKS", secretStore.getSecret(config.keyDbKey())); + break; + case PEM: + context.updateKeyStore(createPemKeyStore(config.pemKeyStore())); + break; + } + } + + private static void validateJksConfig(ConnectorConfig.Ssl ssl) { + if (!ssl.pemKeyStore().keyPath().isEmpty() || ! ssl.pemKeyStore().certificatePath().isEmpty()) { + throw new IllegalArgumentException("pemKeyStore attributes can not be set when keyStoreType is JKS."); + } + if (ssl.keyDbKey().isEmpty()) { + throw new IllegalArgumentException("Missing password for JKS keystore"); + } + } + + private static void validatePemConfig(ConnectorConfig.Ssl ssl) { + if (! ssl.keyStorePath().isEmpty()) { + throw new IllegalArgumentException("keyStorePath can not be set when keyStoreType is PEM"); + } + if (!ssl.keyDbKey().isEmpty()) { + // TODO Make an error once there are separate passwords for truststore and keystore + log.warning("Encrypted PEM key stores are not supported. Password is only applied to truststore"); + } + if (ssl.pemKeyStore().certificatePath().isEmpty()) { + throw new IllegalArgumentException("Missing certificate path."); + } + if (ssl.pemKeyStore().keyPath().isEmpty()) { + throw new IllegalArgumentException("Missing key path."); + } + } + + private static KeyStore createPemKeyStore(ConnectorConfig.Ssl.PemKeyStore pemKeyStore) { + try { + Path certificatePath = Paths.get(pemKeyStore.certificatePath()); + Path keyPath = Paths.get(pemKeyStore.keyPath()); + return new PemSslKeyStore(certificatePath, keyPath).loadJavaKeyStore(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } catch (Exception e) { + throw new RuntimeException("Failed setting up key store for " + pemKeyStore.keyPath() + ", " + pemKeyStore.certificatePath(), e); + } + } + +} diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/DefaultSslKeyStoreContext.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/DefaultSslKeyStoreContext.java new file mode 100644 index 00000000000..8a95893eaeb --- /dev/null +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/DefaultSslKeyStoreContext.java @@ -0,0 +1,51 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.jdisc.http.ssl; + +import org.eclipse.jetty.util.ssl.SslContextFactory; + +import java.security.KeyStore; +import java.util.function.Consumer; + +/** + * @author bjorncs + */ +public class DefaultSslKeyStoreContext implements SslKeyStoreContext { + + private final SslContextFactory sslContextFactory; + + public DefaultSslKeyStoreContext(SslContextFactory sslContextFactory) { + this.sslContextFactory = sslContextFactory; + } + + @Override + public void updateKeyStore(KeyStore keyStore) { + updateKeyStore(keyStore, null); + } + + @Override + public void updateKeyStore(KeyStore keyStore, String password) { + updateKeyStore(sslContextFactory -> { + sslContextFactory.setKeyStore(keyStore); + if (password != null) { + sslContextFactory.setKeyStorePassword(null); + } + }); + } + + @Override + public void updateKeyStore(String keyStorePath, String keyStoreType, String keyStorePassword) { + updateKeyStore(sslContextFactory -> { + sslContextFactory.setKeyStorePath(keyStorePath); + sslContextFactory.setKeyStoreType(keyStoreType); + sslContextFactory.setKeyStorePassword(keyStorePassword); + }); + } + + private void updateKeyStore(Consumer<SslContextFactory> reloader) { + try { + sslContextFactory.reload(reloader); + } catch (Exception e) { + throw new RuntimeException("Could not update keystore: " + e.getMessage(), e); + } + } +} diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStore.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStore.java deleted file mode 100644 index c282c94c1bd..00000000000 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStore.java +++ /dev/null @@ -1,12 +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.jdisc.http.ssl; - -import java.security.KeyStore; - -/** - * - * @author bjorncs - */ -public interface SslKeyStore { - KeyStore loadJavaKeyStore() throws Exception; -} diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStoreConfigurator.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStoreConfigurator.java new file mode 100644 index 00000000000..619f4a636ed --- /dev/null +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStoreConfigurator.java @@ -0,0 +1,14 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.jdisc.http.ssl; + +/** + * An interface for an component that can configure an {@link SslKeyStoreContext}. The implementor can assume that + * the {@link SslKeyStoreContext} instance is thread-safe and be updated at any time + * during and after the call to{@link #configure(SslKeyStoreContext)}. + * Modifying the {@link SslKeyStoreContext} instance will trigger a hot reload of the keystore in JDisc. + * + * @author bjorncs + */ +public interface SslKeyStoreConfigurator { + void configure(SslKeyStoreContext context); +} diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStoreContext.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStoreContext.java new file mode 100644 index 00000000000..2a25f6d78b5 --- /dev/null +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/SslKeyStoreContext.java @@ -0,0 +1,16 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.jdisc.http.ssl; + +import java.security.KeyStore; + +/** + * An interface to update the keystore in JDisc. Any update will trigger a hot reload and new connections will + * immediately see the new certificate chain. + * + * @author bjorncs + */ +public interface SslKeyStoreContext { + void updateKeyStore(KeyStore keyStore); + void updateKeyStore(KeyStore keyStore, String password); + void updateKeyStore(String keyStorePath, String keyStoreType, String keyStorePassword); +} diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/package-info.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/package-info.java index c47d36991d4..5f817d4cfc2 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/package-info.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/package-info.java @@ -1,4 +1,8 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +/** + * @author bjorncs + */ @ExportPackage package com.yahoo.jdisc.http.ssl; + import com.yahoo.osgi.annotation.ExportPackage; diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/pem/PemSslKeyStore.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/pem/PemSslKeyStore.java index 9f0a635f7c1..2ae1894a8d4 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/pem/PemSslKeyStore.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/pem/PemSslKeyStore.java @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.jdisc.http.ssl.pem; -import com.yahoo.jdisc.http.ssl.SslKeyStore; import com.yahoo.jdisc.http.ssl.pem.PemKeyStore.KeyStoreLoadParameter; import com.yahoo.jdisc.http.ssl.pem.PemKeyStore.TrustStoreLoadParameter; @@ -21,7 +20,7 @@ import java.security.cert.CertificateException; * @author Tony Vaagenes * @author bjorncs */ -public class PemSslKeyStore implements SslKeyStore { +public class PemSslKeyStore { static { Security.addProvider(new PemKeyStoreProvider()); @@ -40,9 +39,8 @@ public class PemSslKeyStore implements SslKeyStore { this.loadParameter = new TrustStoreLoadParameter(certificatePath); } - @Override - public KeyStore loadJavaKeyStore() throws KeyStoreException, IOException, NoSuchAlgorithmException, CertificateException { - //cached since Reader(in loadParameter) can only be used one time. + public KeyStore loadJavaKeyStore() + throws KeyStoreException, CertificateException, NoSuchAlgorithmException, IOException { if (keyStore == null) { keyStore = KeyStore.getInstance(KEY_STORE_TYPE); keyStore.load(loadParameter); diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/jks/JksKeyStore.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/JksKeyStore.java index 9cb040fb97d..1c7a917c688 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/ssl/jks/JksKeyStore.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/JksKeyStore.java @@ -1,24 +1,19 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.jdisc.http.ssl.jks; +package com.yahoo.jdisc.http; -import com.yahoo.jdisc.http.ssl.SslKeyStore; - -import java.io.IOException; import java.io.InputStream; import java.nio.file.Files; import java.nio.file.Path; import java.security.KeyStore; -import java.security.KeyStoreException; -import java.security.NoSuchAlgorithmException; -import java.security.cert.CertificateException; /** * @author Tony Vaagenes * @author bjorncs */ -public class JksKeyStore implements SslKeyStore { +public class JksKeyStore { private static final String KEY_STORE_TYPE = "JKS"; + private final Path keyStoreFile; private final String keyStorePassword; @@ -35,8 +30,7 @@ public class JksKeyStore implements SslKeyStore { return keyStorePassword; } - @Override - public KeyStore loadJavaKeyStore() throws KeyStoreException, IOException, NoSuchAlgorithmException, CertificateException { + public KeyStore loadJavaKeyStore() throws Exception { try(InputStream stream = Files.newInputStream(keyStoreFile)) { KeyStore keystore = KeyStore.getInstance(KEY_STORE_TYPE); keystore.load(stream, keyStorePassword != null ? keyStorePassword.toCharArray() : null); diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/SslContextFactory.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/SslContextFactory.java index 5dd5dca1667..d86516df453 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/SslContextFactory.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/SslContextFactory.java @@ -1,8 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.jdisc.http; -import com.yahoo.jdisc.http.ssl.jks.JksKeyStore; - import javax.net.ssl.KeyManagerFactory; import javax.net.ssl.SSLContext; import javax.net.ssl.TrustManagerFactory; diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/guiceModules/ConnectorFactoryRegistryModule.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/guiceModules/ConnectorFactoryRegistryModule.java index 1200a06be2c..0d8f433cc39 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/guiceModules/ConnectorFactoryRegistryModule.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/guiceModules/ConnectorFactoryRegistryModule.java @@ -11,6 +11,7 @@ import com.yahoo.jdisc.http.ConnectorConfig.Builder; import com.yahoo.jdisc.http.SecretStore; import com.yahoo.jdisc.http.server.jetty.ConnectorFactory; import com.yahoo.jdisc.http.server.jetty.TestDrivers; +import com.yahoo.jdisc.http.ssl.DefaultSslKeyStoreConfigurator; /** * Guice module for test ConnectorFactories @@ -46,7 +47,9 @@ public class ConnectorFactoryRegistryModule implements Module { private static class StaticKeyDbConnectorFactory extends ConnectorFactory { public StaticKeyDbConnectorFactory(ConnectorConfig connectorConfig) { - super(connectorConfig, new MockSecretStore()); + super(connectorConfig, + new MockSecretStore(), + new DefaultSslKeyStoreConfigurator(connectorConfig, new MockSecretStore())); } } diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java index fceec51231a..781bc6a7b5f 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/ConnectorFactoryTest.java @@ -4,6 +4,7 @@ package com.yahoo.jdisc.http.server.jetty; import com.yahoo.jdisc.Metric; import com.yahoo.jdisc.http.ConnectorConfig; import com.yahoo.jdisc.http.SecretStore; +import com.yahoo.jdisc.http.ssl.DefaultSslKeyStoreConfigurator; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.AbstractHandler; @@ -23,7 +24,7 @@ import static com.yahoo.jdisc.http.ConnectorConfig.Ssl.KeyStoreType.Enum.PEM; import static org.hamcrest.CoreMatchers.equalTo; /** - * @author <a href="mailto:einarmr@yahoo-inc.com">Einar M R Rosenvinge</a> + * @author Einar M R Rosenvinge */ public class ConnectorFactoryTest { @@ -37,8 +38,7 @@ public class ConnectorFactoryTest { .pemKeyStore( new Ssl.PemKeyStore.Builder() .keyPath("nonEmpty")))); - - ConnectorFactory willThrowException = new ConnectorFactory(config, new ThrowingSecretStore()); + ConnectorFactory willThrowException = createConnectorFactory(config); } @Test(expectedExceptions = IllegalArgumentException.class) @@ -49,16 +49,15 @@ public class ConnectorFactoryTest { .enabled(true) .keyStoreType(PEM) .keyStorePath("nonEmpty"))); - - ConnectorFactory willThrowException = new ConnectorFactory(config, new ThrowingSecretStore()); + ConnectorFactory willThrowException = createConnectorFactory(config); } @Test public void requireThatNoPreBoundChannelWorks() throws Exception { Server server = new Server(); try { - ConnectorFactory factory = new ConnectorFactory(new ConnectorConfig(new ConnectorConfig.Builder()), - new ThrowingSecretStore()); + ConnectorConfig config = new ConnectorConfig(new ConnectorConfig.Builder()); + ConnectorFactory factory = createConnectorFactory(config); JDiscServerConnector connector = (JDiscServerConnector)factory.createConnector(new DummyMetric(), server, null); server.addConnector(connector); @@ -85,8 +84,10 @@ public class ConnectorFactoryTest { ServerSocketChannel serverChannel = ServerSocketChannel.open(); serverChannel.socket().bind(new InetSocketAddress(0)); - ConnectorFactory factory = new ConnectorFactory(new ConnectorConfig(new ConnectorConfig.Builder()), new ThrowingSecretStore()); - JDiscServerConnector connector = (JDiscServerConnector) factory.createConnector(new DummyMetric(), server, serverChannel); + ConnectorConfig config = new ConnectorConfig(new ConnectorConfig.Builder()); + ConnectorFactory factory = createConnectorFactory(config); + JDiscServerConnector connector = + (JDiscServerConnector) factory.createConnector(new DummyMetric(), server, serverChannel); server.addConnector(connector); server.setHandler(new HelloWorldHandler()); server.start(); @@ -104,6 +105,11 @@ public class ConnectorFactoryTest { } } + private static ConnectorFactory createConnectorFactory(ConnectorConfig config) { + ThrowingSecretStore secretStore = new ThrowingSecretStore(); + return new ConnectorFactory(config, secretStore, new DefaultSslKeyStoreConfigurator(config, secretStore)); + } + private static class HelloWorldHandler extends AbstractHandler { @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDriver.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDriver.java index 525cde9d8b3..bcc23facd95 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDriver.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/TestDriver.java @@ -7,7 +7,7 @@ import com.yahoo.jdisc.application.ContainerBuilder; import com.yahoo.jdisc.handler.RequestHandler; import com.yahoo.jdisc.http.ConnectorConfig; import com.yahoo.jdisc.http.SslContextFactory; -import com.yahoo.jdisc.http.ssl.jks.JksKeyStore; +import com.yahoo.jdisc.http.JksKeyStore; import javax.net.ssl.SSLContext; import java.io.IOException; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java index 1c81d97ddea..266d91e7e3e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.provision.maintenance; import com.yahoo.config.provision.Deployer; import com.yahoo.config.provision.Deployment; -import com.yahoo.config.provision.Flavor; import com.yahoo.config.provision.HostLivenessTracker; import com.yahoo.config.provision.NodeType; import com.yahoo.transaction.Mutex; @@ -24,12 +23,12 @@ import java.time.Clock; import java.time.Duration; import java.time.Instant; import java.util.ArrayList; -import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.logging.Level; import java.util.logging.Logger; -import java.util.stream.Collectors; /** * Maintains information in the node repo about when this node last responded to ping @@ -76,21 +75,15 @@ public class NodeFailer extends Maintainer { @Override protected void maintain() { // Ready nodes - updateNodeLivenessEventsForReadyNodes(); - for (Node node : readyNodesWhichAreDead()) { - // Docker hosts and nodes do not run Vespa services - if (node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER || node.type() == NodeType.host) continue; - if ( ! throttle(node)) nodeRepository().fail(node.hostname(), - Agent.system, "Not receiving config requests from node"); - } - - for (Node node : readyNodesWithHardwareFailure()) - if ( ! throttle(node)) nodeRepository().fail(node.hostname(), - Agent.system, "Node has hardware failure"); + try (Mutex lock = nodeRepository().lockUnallocated()) { + updateNodeLivenessEventsForReadyNodes(); - for (Node node: readyNodesWithHardwareDivergence()) - if ( ! throttle(node)) nodeRepository().fail(node.hostname(), - Agent.system, "Node hardware diverges from spec"); + getReadyNodesByFailureReason().forEach((node, reason) -> { + if (!throttle(node)) { + nodeRepository().fail(node.hostname(), Agent.system, reason); + } + }); + } // Active nodes for (Node node : determineActiveNodeDownStatus()) { @@ -103,59 +96,55 @@ public class NodeFailer extends Maintainer { private void updateNodeLivenessEventsForReadyNodes() { // Update node last request events through ZooKeeper to collect request to all config servers. // We do this here ("lazily") to avoid writing to zk for each config request. - try (Mutex lock = nodeRepository().lockUnallocated()) { - for (Node node : nodeRepository().getNodes(Node.State.ready)) { - Optional<Instant> lastLocalRequest = hostLivenessTracker.lastRequestFrom(node.hostname()); - if ( ! lastLocalRequest.isPresent()) continue; - - Optional<History.Event> recordedRequest = node.history().event(History.Event.Type.requested); - if ( ! recordedRequest.isPresent() || recordedRequest.get().at().isBefore(lastLocalRequest.get())) { - History updatedHistory = node.history().with(new History.Event(History.Event.Type.requested, - Agent.system, - lastLocalRequest.get())); - nodeRepository().write(node.with(updatedHistory)); - } + for (Node node : nodeRepository().getNodes(Node.State.ready)) { + Optional<Instant> lastLocalRequest = hostLivenessTracker.lastRequestFrom(node.hostname()); + if ( ! lastLocalRequest.isPresent()) continue; + + Optional<History.Event> recordedRequest = node.history().event(History.Event.Type.requested); + if ( ! recordedRequest.isPresent() || recordedRequest.get().at().isBefore(lastLocalRequest.get())) { + History updatedHistory = node.history().with(new History.Event(History.Event.Type.requested, + Agent.system, + lastLocalRequest.get())); + nodeRepository().write(node.with(updatedHistory)); } } } - private List<Node> readyNodesWhichAreDead() { - // Allow requests some time to be registered in case all config servers have been down - if (constructionTime.isAfter(clock.instant().minus(nodeRequestInterval).minus(nodeRequestInterval) )) - return Collections.emptyList(); - - // Nodes are taken as dead if they have not made a config request since this instant. - // Add 10 minutes to the down time limit to allow nodes to make a request that infrequently. - Instant oldestAcceptableRequestTime = clock.instant().minus(downTimeLimit).minus(nodeRequestInterval); - - return nodeRepository().getNodes(Node.State.ready).stream() - .filter(node -> wasMadeReadyBefore(oldestAcceptableRequestTime, node)) - .filter(node -> ! hasRecordedRequestAfter(oldestAcceptableRequestTime, node)) - .collect(Collectors.toList()); + private Map<Node, String> getReadyNodesByFailureReason() { + Instant oldestAcceptableRequestTime = + // Allow requests some time to be registered in case all config servers have been down + constructionTime.isAfter(clock.instant().minus(nodeRequestInterval.multipliedBy(2))) ? + Instant.EPOCH : + + // Nodes are taken as dead if they have not made a config request since this instant. + // Add 10 minutes to the down time limit to allow nodes to make a request that infrequently. + clock.instant().minus(downTimeLimit).minus(nodeRequestInterval); + + Map<Node, String> nodesByFailureReason = new HashMap<>(); + for (Node node : nodeRepository().getNodes(Node.State.ready)) { + if (! hasNodeRequestedConfigAfter(node, oldestAcceptableRequestTime)) { + nodesByFailureReason.put(node, "Not receiving config requests from node"); + } else if (node.status().hardwareFailureDescription().isPresent()) { + nodesByFailureReason.put(node, "Node has hardware failure"); + } else if (node.status().hardwareDivergence().isPresent()) { + nodesByFailureReason.put(node, "Node has hardware divergence"); + } + } + return nodesByFailureReason; } - private List<Node> readyNodesWithHardwareDivergence() { - return nodeRepository().getNodes(Node.State.ready).stream() - .filter(node -> node.status().hardwareDivergence().isPresent()) - .collect(Collectors.toList()); + private boolean hasNodeRequestedConfigAfter(Node node, Instant instant) { + return !wasMadeReadyBefore(node, instant) || hasRecordedRequestAfter(node, instant); } - private boolean wasMadeReadyBefore(Instant instant, Node node) { + private boolean wasMadeReadyBefore(Node node, Instant instant) { Optional<History.Event> readiedEvent = node.history().event(History.Event.Type.readied); - if ( ! readiedEvent.isPresent()) return false; - return readiedEvent.get().at().isBefore(instant); + return readiedEvent.map(event -> event.at().isBefore(instant)).orElse(false); } - private boolean hasRecordedRequestAfter(Instant instant, Node node) { + private boolean hasRecordedRequestAfter(Node node, Instant instant) { Optional<History.Event> lastRequest = node.history().event(History.Event.Type.requested); - if ( ! lastRequest.isPresent()) return false; - return lastRequest.get().at().isAfter(instant); - } - - private List<Node> readyNodesWithHardwareFailure() { - return nodeRepository().getNodes(Node.State.ready).stream() - .filter(node -> node.status().hardwareFailureDescription().isPresent()) - .collect(Collectors.toList()); + return lastRequest.map(event -> event.at().isAfter(instant)).orElse(false); } private boolean applicationSuspended(Node node) { @@ -272,18 +261,18 @@ public class NodeFailer extends Maintainer { private boolean throttle(Node node) { if (throttlePolicy == ThrottlePolicy.disabled) return false; Instant startOfThrottleWindow = clock.instant().minus(throttlePolicy.throttleWindow); - List<Node> nodes = nodeRepository().getNodes().stream() - // Do not consider Docker containers when throttling - .filter(n -> n.flavor().getType() != Flavor.Type.DOCKER_CONTAINER) - .collect(Collectors.toList()); + List<Node> nodes = nodeRepository().getNodes(); long recentlyFailedNodes = nodes.stream() .map(n -> n.history().event(History.Event.Type.failed)) .filter(Optional::isPresent) .map(Optional::get) .filter(failedEvent -> failedEvent.at().isAfter(startOfThrottleWindow)) .count(); - boolean throttle = recentlyFailedNodes >= Math.max(nodes.size() * throttlePolicy.fractionAllowedToFail, - throttlePolicy.minimumAllowedToFail); + int allowedFailedNodes = (int) Math.max(nodes.size() * throttlePolicy.fractionAllowedToFail, + throttlePolicy.minimumAllowedToFail); + + boolean throttle = allowedFailedNodes < recentlyFailedNodes || + (allowedFailedNodes == recentlyFailedNodes && node.type() != NodeType.host); if (throttle) { log.info(String.format("Want to fail node %s, but throttling is in effect: %s", node.hostname(), throttlePolicy.toHumanReadableString())); 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 6bcb9426373..0e0195a5bed 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 @@ -185,8 +185,8 @@ public class NodeFailTester { } public void allNodesMakeAConfigRequestExcept(List<Node> deadNodes) { - for (Node node : nodeRepository.getNodes(NodeType.tenant)) { - if ( ! deadNodes.contains(node) && node.flavor().getType() != Flavor.Type.DOCKER_CONTAINER) + for (Node node : nodeRepository.getNodes()) { + if ( ! deadNodes.contains(node)) hostLivenessTracker.receivedRequestFrom(node.hostname()); } } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java index c78663415ef..b9b871dfd1f 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java @@ -5,7 +5,6 @@ import com.yahoo.config.provision.Flavor; import com.yahoo.config.provision.NodeType; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeRepository; -import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.orchestrator.ApplicationIdNotFoundException; import com.yahoo.vespa.orchestrator.ApplicationStateChangeDeniedException; import org.junit.Test; @@ -66,7 +65,7 @@ public class NodeFailerTest { assertEquals( 4, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).size()); } - // Failures are detected on two ready nodes, which are then failed + // Hardware failures are detected on two ready nodes, which are then failed Node readyFail1 = tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).get(2); Node readyFail2 = tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).get(3); tester.nodeRepository.write(readyFail1.with(readyFail1.status().withHardwareFailureDescription(Optional.of("memory_mcelog")))); @@ -166,18 +165,16 @@ public class NodeFailerTest { tester.createReadyNodes(1, 16, "docker"); // For a day all nodes work so nothing happens - for (int minutes = 0; minutes < 24 * 60; minutes +=5 ) { - tester.clock.advance(Duration.ofMinutes(5)); + for (int minutes = 0, interval = 30; minutes < 24 * 60; minutes += interval) { + tester.clock.advance(Duration.ofMinutes(interval)); tester.allNodesMakeAConfigRequestExcept(); tester.failer.run(); assertEquals( 5, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).size()); } List<Node> ready = tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready); - List<Node> readyHosts = tester.nodeRepository.getNodes(NodeType.host, Node.State.ready); - // Two ready nodes die and a ready docker node "dies" - // (Vespa does not run when in ready state for docker node, so it does not make config requests) + // Two ready nodes and a ready docker node die, but only 2 of those are failed out tester.clock.advance(Duration.ofMinutes(180)); Node dockerNode = ready.stream().filter(node -> node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER).findFirst().get(); List<Node> otherNodes = ready.stream() @@ -188,18 +185,13 @@ public class NodeFailerTest { assertEquals( 3, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).size()); assertEquals( 2, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.failed).size()); - // Another ready node die + // Another ready node dies and the node that died earlier, are allowed to fail tester.clock.advance(Duration.ofDays(1)); tester.allNodesMakeAConfigRequestExcept(otherNodes.get(0), otherNodes.get(2), dockerNode, otherNodes.get(3)); tester.failer.run(); - assertEquals( 2, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).size()); - assertEquals(ready.get(1), tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).get(0)); - assertEquals( 3, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.failed).size()); - - // Ready Docker hosts do not make config requests - tester.allNodesMakeAConfigRequestExcept(readyHosts.get(0), readyHosts.get(1), readyHosts.get(2)); - tester.failer.run(); - assertEquals(3, tester.nodeRepository.getNodes(NodeType.host, Node.State.ready).size()); + assertEquals( 1, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).size()); + assertEquals(otherNodes.get(1), tester.nodeRepository.getNodes(NodeType.tenant, Node.State.ready).get(0)); + assertEquals( 4, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.failed).size()); } @Test @@ -207,8 +199,8 @@ public class NodeFailerTest { NodeFailTester tester = NodeFailTester.withTwoApplicationsOnDocker(7); // For a day all nodes work so nothing happens - for (int minutes = 0; minutes < 24 * 60; minutes += 5 ) { - tester.clock.advance(Duration.ofMinutes(5)); + for (int minutes = 0, interval = 30; minutes < 24 * 60; minutes += interval) { + tester.clock.advance(Duration.ofMinutes(interval)); tester.allNodesMakeAConfigRequestExcept(); tester.failer.run(); assertEquals(8, tester.nodeRepository.getNodes(NodeType.tenant, Node.State.active).size()); @@ -247,10 +239,10 @@ public class NodeFailerTest { Node downTenant1 = tester.nodeRepository.getNodes(NodeType.tenant, Node.State.active).get(0); tester.serviceMonitor.setHostDown(downTenant1.hostname()); - // nothing happens the first 45 minutes - for (int minutes = 0; minutes < 45; minutes += 5 ) { + // nothing happens during the entire day because of the failure throttling + for (int minutes = 0, interval = 30; minutes < 24 * 60; minutes += interval) { tester.failer.run(); - tester.clock.advance(Duration.ofMinutes(5)); + tester.clock.advance(Duration.ofMinutes(interval)); tester.allNodesMakeAConfigRequestExcept(); assertEquals(3 + 1, tester.nodeRepository.getNodes(Node.State.failed).size()); } @@ -374,14 +366,12 @@ public class NodeFailerTest { public void node_failing_throttle() { // Throttles based on a absolute number in small zone { - NodeFailTester tester = NodeFailTester.withNoApplications(); - List<Node> readyNodes = tester.createReadyNodes(50); - List<Node> readyDockerNodes = tester.createReadyNodes(50, 50, "docker"); + // 50 regular tenant nodes, 10 hosts with each 3 tenant nodes, total 90 nodes + NodeFailTester tester = NodeFailTester.withTwoApplicationsOnDocker(10); + List<Node> readyNodes = tester.createReadyNodes(50, 30); + List<Node> hosts = tester.nodeRepository.getNodes(NodeType.host); List<Node> deadNodes = readyNodes.subList(0, 4); - // Fail 10 Docker containers, should not impact throttling policy - readyDockerNodes.subList(0, 10) - .forEach(node -> tester.nodeRepository.fail(node.hostname(), Agent.system, "Failed in test")); // 2 hours pass, 4 nodes die for (int minutes = 0, interval = 30; minutes < 2 * 60; minutes += interval) { @@ -391,7 +381,7 @@ public class NodeFailerTest { // 2 nodes are failed (the minimum amount that are always allowed to fail) tester.failer.run(); - assertEquals(2, getNonDockerFailedNodes(tester.nodeRepository).size()); + assertEquals(2, tester.nodeRepository.getNodes(Node.State.failed).size()); // 6 more hours pass, no more nodes are failed for (int minutes = 0, interval = 30; minutes < 6 * 60; minutes += interval) { @@ -399,15 +389,39 @@ public class NodeFailerTest { tester.allNodesMakeAConfigRequestExcept(deadNodes); } tester.failer.run(); - assertEquals(2, getNonDockerFailedNodes(tester.nodeRepository).size()); + assertEquals(2, tester.nodeRepository.getNodes(Node.State.failed).size()); - // 18 more hours pass, it's now 24 hours since the first 2 failed. The remaining 2 are failed - for (int minutes = 0, interval = 30; minutes < 18 * 60; minutes += interval) { + // 2 docker hosts now fail, 1 of them (with all its children is allowed to fail) + hosts.subList(0, 2).forEach(host -> { + tester.serviceMonitor.setHostDown(host.hostname()); + deadNodes.add(host); + }); + tester.failer.run(); + tester.clock.advance(Duration.ofMinutes(61)); + tester.allNodesMakeAConfigRequestExcept(deadNodes); + + tester.failer.run(); + assertEquals(6, tester.nodeRepository.getNodes(Node.State.failed).size()); + + // 24 more hours pass without any other nodes being failed out + for (int minutes = 0, interval = 30; minutes <= 23 * 60; minutes += interval) { tester.clock.advance(Duration.ofMinutes(interval)); tester.allNodesMakeAConfigRequestExcept(deadNodes); } tester.failer.run(); - assertEquals(4, getNonDockerFailedNodes(tester.nodeRepository).size()); + assertEquals(6, tester.nodeRepository.getNodes(Node.State.failed).size()); + + // Next, the 2 ready nodes that were dead from the start are failed out, and finally + // the second host and all its children are failed + tester.clock.advance(Duration.ofMinutes(30)); + tester.failer.run(); + assertEquals(12, tester.nodeRepository.getNodes(Node.State.failed).size()); + + // Nothing else to fail + tester.clock.advance(Duration.ofHours(25)); + tester.allNodesMakeAConfigRequestExcept(deadNodes); + tester.failer.run(); + assertEquals(12, tester.nodeRepository.getNodes(Node.State.failed).size()); } // Throttles based on percentage in large zone @@ -443,12 +457,6 @@ public class NodeFailerTest { } } - /** Get all failed nodes that are not Docker containers */ - private static List<Node> getNonDockerFailedNodes(NodeRepository nodeRepository) { - return nodeRepository.getNodes(Node.State.failed).stream() - .filter(node -> node.flavor().getType() != Flavor.Type.DOCKER_CONTAINER) - .collect(Collectors.toList()); - } /** * Selects the first parent host that: diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/DoubleCompatibleValue.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/DoubleCompatibleValue.java index 0ed2bdd6331..ea750295423 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/DoubleCompatibleValue.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/DoubleCompatibleValue.java @@ -44,6 +44,26 @@ public abstract class DoubleCompatibleValue extends Value { } @Override + public Value and(Value value) { + return new BooleanValue(asBoolean() && value.asBoolean()); + } + + @Override + public Value or(Value value) { + return new BooleanValue(asBoolean() || value.asBoolean()); + } + + @Override + public Value not() { + return new BooleanValue(!asBoolean()); + } + + @Override + public Value power(Value value) { + return new DoubleValue(Function.pow.evaluate(asDouble(), value.asDouble())); + } + + @Override public Value compare(TruthOperator operator, Value value) { return new BooleanValue(operator.evaluate(asDouble(), value.asDouble())); } diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/StringValue.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/StringValue.java index 5374a9d3ce6..ac8aba6a617 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/StringValue.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/StringValue.java @@ -54,22 +54,42 @@ public class StringValue extends Value { @Override public Value subtract(Value value) { - throw new UnsupportedOperationException("String values ('" + value + "') does not support subtraction"); + throw new UnsupportedOperationException("String values ('" + value + "') do not support subtraction"); } @Override public Value multiply(Value value) { - throw new UnsupportedOperationException("String values ('" + value + "') does not support multiplication"); + throw new UnsupportedOperationException("String values ('" + value + "') do not support multiplication"); } @Override public Value divide(Value value) { - throw new UnsupportedOperationException("String values ('" + value + "') does not support division"); + throw new UnsupportedOperationException("String values ('" + value + "') do not support division"); } @Override public Value modulo(Value value) { - throw new UnsupportedOperationException("String values ('" + value + "') does not support modulo"); + throw new UnsupportedOperationException("String values ('" + value + "') do not support modulo"); + } + + @Override + public Value and(Value value) { + throw new UnsupportedOperationException("String values ('" + value + "') do not support and"); + } + + @Override + public Value or(Value value) { + throw new UnsupportedOperationException("String values ('" + value + "') do not support or"); + } + + @Override + public Value not() { + throw new UnsupportedOperationException("String values ('" + value + "') do not support not"); + } + + @Override + public Value power(Value value) { + throw new UnsupportedOperationException("String values ('" + value + "') do not support ^"); } @Override diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/TensorValue.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/TensorValue.java index b283603e713..49c3ccb7b01 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/TensorValue.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/TensorValue.java @@ -89,6 +89,34 @@ public class TensorValue extends Value { return new TensorValue(value.map((value) -> value % argument.asDouble())); } + @Override + public Value and(Value argument) { + if (argument instanceof TensorValue) + return new TensorValue(value.join(((TensorValue)argument).value, (a, b) -> ((a!=0.0) && (b!=0.0)) ? 1.0 : 0.0 )); + else + return new TensorValue(value.map((value) -> ((value!=0.0) && argument.asBoolean()) ? 1 : 0)); + } + + @Override + public Value or(Value argument) { + if (argument instanceof TensorValue) + return new TensorValue(value.join(((TensorValue)argument).value, (a, b) -> ((a!=0.0) || (b!=0.0)) ? 1.0 : 0.0 )); + else + return new TensorValue(value.map((value) -> ((value!=0.0) || argument.asBoolean()) ? 1 : 0)); + } + + @Override + public Value not() { + return new TensorValue(value.map((value) -> (value==0.0) ? 1.0 : 0.0)); + } + + @Override + public Value power(Value argument) { + if (argument instanceof TensorValue) + return new TensorValue(value.pow(((TensorValue)argument).value)); + else + return new TensorValue(value.map((value) -> Math.pow(value, argument.asDouble()))); + } private Tensor asTensor(Value value, String operationName) { if ( ! (value instanceof TensorValue)) diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/Value.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/Value.java index f42082321b3..b2ccbe572d0 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/Value.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/evaluation/Value.java @@ -43,6 +43,14 @@ public abstract class Value { public abstract Value modulo(Value value); + public abstract Value and(Value value); + + public abstract Value or(Value value); + + public abstract Value not(); + + public abstract Value power(Value value); + /** Perform the comparison specified by the operator between this value and the given value */ public abstract Value compare(TruthOperator operator, Value value); diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticNode.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticNode.java index 91d8abec1be..518a15bcc87 100755 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticNode.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticNode.java @@ -77,7 +77,7 @@ public final class ArithmeticNode extends CompositeNode { Iterator<ExpressionNode> child = children.iterator(); Deque<ValueItem> stack = new ArrayDeque<>(); - stack.push(new ValueItem(ArithmeticOperator.PLUS, child.next().evaluate(context))); + stack.push(new ValueItem(ArithmeticOperator.OR, child.next().evaluate(context))); for (Iterator<ArithmeticOperator> it = operators.iterator(); it.hasNext() && child.hasNext();) { ArithmeticOperator op = it.next(); if (!stack.isEmpty()) { diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticOperator.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticOperator.java index 2187a96ba4d..a715490e95a 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticOperator.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/ArithmeticOperator.java @@ -14,22 +14,31 @@ import java.util.List; */ public enum ArithmeticOperator { - PLUS(0, "+") { public Value evaluate(Value x, Value y) { + OR(0, "||") { public Value evaluate(Value x, Value y) { + return x.or(y); + }}, + AND(1, "&&") { public Value evaluate(Value x, Value y) { + return x.and(y); + }}, + PLUS(2, "+") { public Value evaluate(Value x, Value y) { return x.add(y); }}, - MINUS(1, "-") { public Value evaluate(Value x, Value y) { + MINUS(3, "-") { public Value evaluate(Value x, Value y) { return x.subtract(y); }}, - MULTIPLY(2, "*") { public Value evaluate(Value x, Value y) { + MULTIPLY(4, "*") { public Value evaluate(Value x, Value y) { return x.multiply(y); }}, - DIVIDE(3, "/") { public Value evaluate(Value x, Value y) { + DIVIDE(5, "/") { public Value evaluate(Value x, Value y) { return x.divide(y); }}, - MODULO(4, "%") { public Value evaluate(Value x, Value y) { + MODULO(6, "%") { public Value evaluate(Value x, Value y) { return x.modulo(y); + }}, + POWER(7, "^") { public Value evaluate(Value x, Value y) { + return x.power(y); }}; - + /** A list of all the operators in this in order of decreasing precedence */ public static final List<ArithmeticOperator> operatorsByPrecedence = operatorsByPrecedence(); @@ -55,11 +64,14 @@ public enum ArithmeticOperator { private static List<ArithmeticOperator> operatorsByPrecedence() { List<ArithmeticOperator> operators = new ArrayList<>(); + operators.add(POWER); operators.add(MODULO); operators.add(DIVIDE); operators.add(MULTIPLY); operators.add(MINUS); operators.add(PLUS); + operators.add(AND); + operators.add(OR); return Collections.unmodifiableList(operators); } diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/NotNode.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/NotNode.java new file mode 100644 index 00000000000..8c459a032bd --- /dev/null +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/NotNode.java @@ -0,0 +1,50 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.searchlib.rankingexpression.rule; + +import com.yahoo.searchlib.rankingexpression.evaluation.Context; +import com.yahoo.searchlib.rankingexpression.evaluation.Value; + +import java.util.Collections; +import java.util.Deque; +import java.util.List; + +/** + * A node which flips the logical value produced from the nested expression. + * + * @author lesters + */ +public class NotNode extends BooleanNode { + + private final ExpressionNode value; + + public NotNode(ExpressionNode value) { + this.value = value; + } + + public ExpressionNode getValue() { + return value; + } + + @Override + public List<ExpressionNode> children() { + return Collections.singletonList(value); + } + + @Override + public String toString(SerializationContext context, Deque<String> path, CompositeNode parent) { + return "!" + value.toString(context, path, parent); + } + + @Override + public Value evaluate(Context context) { + return value.evaluate(context).not(); + } + + @Override + public NotNode setChildren(List<ExpressionNode> children) { + if (children.size() != 1) throw new IllegalArgumentException("Expected 1 children but got " + children.size()); + return new NotNode(children.get(0)); + } + +} + diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/SetMembershipNode.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/SetMembershipNode.java index f8e44f1087c..f6b1a1a8979 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/SetMembershipNode.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/SetMembershipNode.java @@ -4,9 +4,14 @@ package com.yahoo.searchlib.rankingexpression.rule; import com.google.common.collect.ImmutableList; import com.yahoo.searchlib.rankingexpression.evaluation.BooleanValue; import com.yahoo.searchlib.rankingexpression.evaluation.Context; +import com.yahoo.searchlib.rankingexpression.evaluation.TensorValue; import com.yahoo.searchlib.rankingexpression.evaluation.Value; +import com.yahoo.tensor.Tensor; -import java.util.*; +import java.util.ArrayList; +import java.util.Deque; +import java.util.List; +import java.util.function.Predicate; /** * A node which returns true or false depending on a set membership test @@ -55,11 +60,30 @@ public class SetMembershipNode extends BooleanNode { @Override public Value evaluate(Context context) { Value value = testValue.evaluate(context); + if (value instanceof TensorValue) { + return evaluateTensor(((TensorValue) value).asTensor(), context); + } + return evaluateValue(value, context); + } + + private Value evaluateValue(Value value, Context context) { + return new BooleanValue(testMembership(value::equals, context)); + } + + private Value evaluateTensor(Tensor tensor, Context context) { + return new TensorValue(tensor.map((value) -> contains(value, context) ? 1.0 : 0.0)); + } + + private boolean contains(double value, Context context) { + return testMembership((setValue) -> setValue.asDouble() == value, context); + } + + private boolean testMembership(Predicate<Value> test, Context context) { for (ExpressionNode setValue : setValues) { - if (setValue.evaluate(context).equals(value)) - return new BooleanValue(true); + if (test.test(setValue.evaluate(context))) + return true; } - return new BooleanValue(false); + return false; } @Override diff --git a/searchlib/src/main/javacc/RankingExpressionParser.jj b/searchlib/src/main/javacc/RankingExpressionParser.jj index 01fed00202c..7821ab88b86 100755 --- a/searchlib/src/main/javacc/RankingExpressionParser.jj +++ b/searchlib/src/main/javacc/RankingExpressionParser.jj @@ -66,6 +66,7 @@ TOKEN : <MUL: "*"> | <DOT: "."> | <MOD: "%"> | + <POWOP: "^"> | <DOLLAR: "$"> | <COMMA: ","> | @@ -86,6 +87,10 @@ TOKEN : <IN: "in"> | <F: "f"> | + <NOT: "!"> | + <AND: "&&"> | + <OR: "||"> | + <ABS: "abs"> | <ACOS: "acos"> | <ASIN: "asin"> | @@ -200,11 +205,14 @@ ExpressionNode arithmeticExpression() : ArithmeticOperator arithmetic() : { } { - ( <ADD> { return ArithmeticOperator.PLUS; } | - <SUB> { return ArithmeticOperator.MINUS; } | - <DIV> { return ArithmeticOperator.DIVIDE; } | - <MUL> { return ArithmeticOperator.MULTIPLY; } | - <MOD> { return ArithmeticOperator.MODULO; } ) + ( <ADD> { return ArithmeticOperator.PLUS; } | + <SUB> { return ArithmeticOperator.MINUS; } | + <DIV> { return ArithmeticOperator.DIVIDE; } | + <MUL> { return ArithmeticOperator.MULTIPLY; } | + <MOD> { return ArithmeticOperator.MODULO; } | + <AND> { return ArithmeticOperator.AND; } | + <OR> { return ArithmeticOperator.OR; } | + <POWOP> { return ArithmeticOperator.POWER; } ) { return null; } } @@ -224,16 +232,23 @@ ExpressionNode value() : { ExpressionNode ret; boolean neg = false; + boolean not = false; } { - ( [ LOOKAHEAD(2) <SUB> { neg = true; } ] - ( ret = constantPrimitive() | - LOOKAHEAD(2) ret = ifExpression() | - LOOKAHEAD(4) ret = function() | - ret = feature() | - ret = queryFeature() | + ( + [ <NOT> { not = true; } ] + [ LOOKAHEAD(2) <SUB> { neg = true; } ] + ( ret = constantPrimitive() | + LOOKAHEAD(2) ret = ifExpression() | + LOOKAHEAD(4) ret = function() | + ret = feature() | + ret = queryFeature() | ( <LBRACE> ret = expression() <RBRACE> { ret = new EmbracedNode(ret); } ) ) ) - { return neg ? new NegativeNode(ret) : ret; } + { + ret = not ? new NotNode(ret) : ret; + ret = neg ? new NegativeNode(ret) : ret; + return ret; + } } IfNode ifExpression() : diff --git a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java index 5d357777657..82e5d0cfe5b 100644 --- a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java +++ b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java @@ -29,6 +29,7 @@ public class EvaluationTestCase { tester.assertEvaluates(0.75, "0.5 + 0.25"); tester.assertEvaluates(0.75, "one_half + a_quarter"); tester.assertEvaluates(1.25, "0.5 - 0.25 + one"); + tester.assertEvaluates(9.0, "3 ^ 2"); // String tester.assertEvaluates(1, "if(\"a\"==\"a\",1,0)"); @@ -37,6 +38,9 @@ public class EvaluationTestCase { tester.assertEvaluates(26, "2*3+4*5"); tester.assertEvaluates(1, "2/6+4/6"); tester.assertEvaluates(2 * 3 * 4 + 3 * 4 * 5 - 4 * 200 / 10, "2*3*4+3*4*5-4*200/10"); + tester.assertEvaluates(3, "1 + 10 % 6 / 2"); + tester.assertEvaluates(10.0, "3 ^ 2 + 1"); + tester.assertEvaluates(18.0, "2 * 3 ^ 2"); // Conditionals tester.assertEvaluates(2 * (3 * 4 + 3) * (4 * 5 - 4 * 200) / 10, "2*(3*4+3)*(4*5-4*200)/10"); @@ -89,6 +93,38 @@ public class EvaluationTestCase { } @Test + public void testBooleanEvaluation() { + EvaluationTester tester = new EvaluationTester(); + + // and + tester.assertEvaluates(false, "0 && 0"); + tester.assertEvaluates(false, "0 && 1"); + tester.assertEvaluates(false, "1 && 0"); + tester.assertEvaluates(true, "1 && 1"); + tester.assertEvaluates(true, "1 && 2"); + tester.assertEvaluates(true, "1 && 0.1"); + + // or + tester.assertEvaluates(false, "0 || 0"); + tester.assertEvaluates(true, "0 || 0.1"); + tester.assertEvaluates(true, "0 || 1"); + tester.assertEvaluates(true, "1 || 0"); + tester.assertEvaluates(true, "1 || 1"); + + // not + tester.assertEvaluates(true, "!0"); + tester.assertEvaluates(false, "!1"); + tester.assertEvaluates(false, "!2"); + tester.assertEvaluates(true, "!0 && 1"); + + // precedence + tester.assertEvaluates(0, "2 * (0 && 1)"); + tester.assertEvaluates(2, "2 * (1 && 1)"); + tester.assertEvaluates(true, "2 + 0 && 1"); + tester.assertEvaluates(true, "1 && 0 + 2"); + } + + @Test public void testTensorEvaluation() { EvaluationTester tester = new EvaluationTester(); tester.assertEvaluates("{}", "tensor0", "{}"); @@ -107,6 +143,16 @@ public class EvaluationTestCase { "min(tensor0, 0)", "{ {d1:0}:-10, {d1:1}:0, {d1:2}:10 }"); tester.assertEvaluates("{ {d1:0}:0, {d1:1}:0, {d1:2 }:10 }", "max(tensor0, 0)", "{ {d1:0}:-10, {d1:1}:0, {d1:2}:10 }"); + // operators + tester.assertEvaluates("{ {d1:0}:1, {d1:1}:1, {d1:2 }:1 }", + "tensor0 % 2 == map(tensor0, f(x) (x % 2))", "{ {d1:0}:2, {d1:1}:3, {d1:2}:4 }"); + tester.assertEvaluates("{ {d1:0}:1, {d1:1}:1, {d1:2 }:1 }", + "tensor0 || 1 == map(tensor0, f(x) (x || 1))", "{ {d1:0}:2, {d1:1}:3, {d1:2}:4 }"); + tester.assertEvaluates("{ {d1:0}:1, {d1:1}:1, {d1:2 }:1 }", + "tensor0 && 1 == map(tensor0, f(x) (x && 1))", "{ {d1:0}:2, {d1:1}:3, {d1:2}:4 }"); + tester.assertEvaluates("{ {d1:0}:1, {d1:1}:1, {d1:2 }:1 }", + "!tensor0 == map(tensor0, f(x) (!x))", "{ {d1:0}:0, {d1:1}:1, {d1:2}:0 }"); + // -- explicitly implemented functions (not foolproof tests as we don't bother testing float value equivalence) tester.assertEvaluates("{ {x:0}:1, {x:1}:2 }", "abs(tensor0)", "{ {x:0}:1, {x:1}:-2 }"); tester.assertEvaluates("{ {x:0}:0, {x:1}:0 }", "acos(tensor0)", "{ {x:0}:1, {x:1}:1 }"); @@ -122,8 +168,9 @@ public class EvaluationTestCase { tester.assertEvaluates("{ {x:0}:0, {x:1}:0 }", "isNan(tensor0)", "{ {x:0}:1, {x:1}:2 }"); tester.assertEvaluates("{ {x:0}:0, {x:1}:0 }", "log(tensor0)", "{ {x:0}:1, {x:1}:1 }"); tester.assertEvaluates("{ {x:0}:0, {x:1}:1 }", "log10(tensor0)", "{ {x:0}:1, {x:1}:10 }"); - tester.assertEvaluates("{ {x:0}:0, {x:1}:2 }", "fmod(tensor0, 3)", "{ {x:0}:3, {x:1}:8 }"); + tester.assertEvaluates("{ {x:0}:0, {x:1}:2 }", "fmod(tensor0, 3)","{ {x:0}:3, {x:1}:8 }"); tester.assertEvaluates("{ {x:0}:1, {x:1}:8 }", "pow(tensor0, 3)", "{ {x:0}:1, {x:1}:2 }"); + tester.assertEvaluates("{ {x:0}:8, {x:1}:16 }", "ldexp(tensor0,3.1)","{ {x:0}:1, {x:1}:2 }"); tester.assertEvaluates("{ {x:0}:1, {x:1}:2 }", "relu(tensor0)", "{ {x:0}:1, {x:1}:2 }"); tester.assertEvaluates("{ {x:0}:1, {x:1}:2 }", "round(tensor0)", "{ {x:0}:1, {x:1}:1.8 }"); tester.assertEvaluates("{ {x:0}:0.5, {x:1}:0.5 }", "sigmoid(tensor0)","{ {x:0}:0, {x:1}:0 }"); @@ -201,6 +248,16 @@ public class EvaluationTestCase { "max(tensor0, tensor1)", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:5 }"); tester.assertEvaluates("{ {x:0,y:0}:3, {x:1,y:0}:5 }", "min(tensor0, tensor1)", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:5 }"); + tester.assertEvaluates("{ {x:0,y:0}:243, {x:1,y:0}:16807 }", + "pow(tensor0, tensor1)", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:5 }"); + tester.assertEvaluates("{ {x:0,y:0}:243, {x:1,y:0}:16807 }", + "tensor0 ^ tensor1", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:5 }"); + tester.assertEvaluates("{ {x:0,y:0}:3, {x:1,y:0}:2 }", + "fmod(tensor0, tensor1)", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:5 }"); + tester.assertEvaluates("{ {x:0,y:0}:3, {x:1,y:0}:2 }", + "tensor0 % tensor1", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:5 }"); + tester.assertEvaluates("{ {x:0,y:0}:96, {x:1,y:0}:224 }", + "ldexp(tensor0, tensor1)", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:5.1 }"); tester.assertEvaluates("{ {x:0,y:0,z:0}:7, {x:0,y:0,z:1}:13, {x:1,y:0,z:0}:21, {x:1,y:0,z:1}:39, {x:0,y:1,z:0}:55, {x:0,y:1,z:1}:0, {x:1,y:1,z:0}:0, {x:1,y:1,z:1}:0 }", "tensor0 * tensor1", "{ {x:0,y:0}:1, {x:1,y:0}:3, {x:0,y:1}:5, {x:1,y:1}:0 }", "{ {y:0,z:0}:7, {y:1,z:0}:11, {y:0,z:1}:13, {y:1,z:1}:0 }"); tester.assertEvaluates("{ {x:0,y:1,z:0}:35, {x:0,y:1,z:1}:65 }", @@ -225,8 +282,13 @@ public class EvaluationTestCase { "tensor0 <= tensor1", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:5 }"); tester.assertEvaluates("{ {x:0,y:0}:0, {x:1,y:0}:1 }", "tensor0 == tensor1", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:7 }"); + tester.assertEvaluates("{ {x:0,y:0}:0, {x:1,y:0}:1 }", + "tensor0 ~= tensor1", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:7 }"); tester.assertEvaluates("{ {x:0,y:0}:1, {x:1,y:0}:0 }", "tensor0 != tensor1", "{ {x:0}:3, {x:1}:7 }", "{ {y:0}:7 }"); + tester.assertEvaluates("{ {x:0}:1, {x:1}:0 }", + "tensor0 in [1,2,3]", "{ {x:0}:3, {x:1}:7 }"); + // TODO // argmax // argmin diff --git a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTester.java b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTester.java index d67c9dfd9dc..ee2b1c147e3 100644 --- a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTester.java +++ b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTester.java @@ -58,10 +58,18 @@ public class EvaluationTester { return assertEvaluates(value, expressionString, defaultContext); } + public RankingExpression assertEvaluates(boolean value, String expressionString) { + return assertEvaluates(value, expressionString, defaultContext); + } + public RankingExpression assertEvaluates(double value, String expressionString, Context context) { return assertEvaluates(new DoubleValue(value), expressionString, context, ""); } + public RankingExpression assertEvaluates(boolean value, String expressionString, Context context) { + return assertEvaluates(new BooleanValue(value), expressionString, context, ""); + } + public RankingExpression assertEvaluates(Value value, String expressionString, Context context, String explanation) { try { RankingExpression expression = new RankingExpression(expressionString); diff --git a/searchlib/src/test/java/com/yahoo/searchlib/tensor/TensorConformanceTest.java b/searchlib/src/test/java/com/yahoo/searchlib/tensor/TensorConformanceTest.java index 27aaeb776e4..dde9d4bf21e 100644 --- a/searchlib/src/test/java/com/yahoo/searchlib/tensor/TensorConformanceTest.java +++ b/searchlib/src/test/java/com/yahoo/searchlib/tensor/TensorConformanceTest.java @@ -18,6 +18,7 @@ import org.junit.Test; import java.io.BufferedReader; import java.io.File; +import java.io.FileNotFoundException; import java.io.FileReader; import java.io.IOException; import java.util.ArrayList; @@ -51,32 +52,32 @@ public class TensorConformanceTest { count++; } } - if (failList.size() > 0) { - System.out.println("Conformance test fails:"); - System.out.println(failList); - } - - // Disable this for now: - //assertEquals(0, failList.size()); + assertEquals(failList.size() + " conformance test fails: " + failList, 0, failList.size()); } - private boolean testCase(String test, int count) throws IOException { + private boolean testCase(String test, int count) { try { ObjectMapper mapper = new ObjectMapper(); JsonNode node = mapper.readTree(test); + if (node.has("num_tests")) { Assert.assertEquals(node.get("num_tests").asInt(), count); - } else if (node.has("expression")) { - String expression = node.get("expression").asText(); - MapContext context = getInput(node.get("inputs")); - Tensor expect = getTensor(node.get("result").get("expect").asText()); - Tensor result = evaluate(expression, context); - boolean equals = Tensor.equals(result, expect); - if (!equals) { - System.out.println(count + " : Tensors not equal. Result: " + result.toString() + " Expected: " + expect.toString() + " -> expression \"" + expression + "\""); - } - return Tensor.equals(result, expect); + return true; + } + if (!node.has("expression")) { + return true; // ignore } + + String expression = node.get("expression").asText(); + MapContext context = getInput(node.get("inputs")); + Tensor expect = getTensor(node.get("result").get("expect").asText()); + Tensor result = evaluate(expression, context); + boolean equals = Tensor.equals(result, expect); + if (!equals) { + System.out.println(count + " : Tensors not equal. Result: " + result.toString() + " Expected: " + expect.toString() + " -> expression \"" + expression + "\""); + } + return equals; + } catch (Exception e) { System.out.println(count + " : " + e.toString()); } @@ -133,22 +134,5 @@ public class TensorConformanceTest { throw new IllegalArgumentException("Hex contains illegal characters"); } - private static String valueType(Value value) { - if (value instanceof StringValue) { - return "string"; - } - if (value instanceof BooleanValue) { - return "boolean"; - } - if (value instanceof DoubleCompatibleValue) { - return "double"; - } - if (value instanceof TensorValue) { - return ((TensorValue)value).asTensor().type().toString(); - } - return "unknown"; - } - - } diff --git a/searchlib/src/tests/rankingexpression/rankingexpressionlist b/searchlib/src/tests/rankingexpression/rankingexpressionlist index 327f2b161cd..77b2294c668 100644 --- a/searchlib/src/tests/rankingexpression/rankingexpressionlist +++ b/searchlib/src/tests/rankingexpression/rankingexpressionlist @@ -160,3 +160,7 @@ mysum ( mysum(4, 4), value( 4 ), value(4) ); mysum(mysum(4,4),value(4),value(4) "1008\x1977" "100819\x77" if(1.09999~=1.1,2,3); if (1.09999 ~= 1.1, 2, 3) +10 % 3 +1 && 0 || 1 +!a && (a || a) +10 ^ 3 diff --git a/storage/src/tests/distributor/simplemaintenancescannertest.cpp b/storage/src/tests/distributor/simplemaintenancescannertest.cpp index a46419b71a4..66a2d3efa6c 100644 --- a/storage/src/tests/distributor/simplemaintenancescannertest.cpp +++ b/storage/src/tests/distributor/simplemaintenancescannertest.cpp @@ -1,6 +1,9 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#include <vespa/document/test/make_bucket_space.h> #include <vespa/vdstestlib/cppunit/macros.h> +#include <vespa/storage/distributor/distributor_bucket_space_repo.h> +#include <vespa/storage/distributor/distributor_bucket_space.h> #include <vespa/storage/distributor/maintenance/simplemaintenancescanner.h> #include <vespa/storage/distributor/maintenance/simplebucketprioritydatabase.h> #include <vespa/storage/bucketdb/mapbucketdatabase.h> @@ -11,11 +14,13 @@ namespace storage::distributor { using document::BucketId; +using document::test::makeBucketSpace; typedef MaintenancePriority Priority; class SimpleMaintenanceScannerTest : public CppUnit::TestFixture { CPPUNIT_TEST_SUITE(SimpleMaintenanceScannerTest); CPPUNIT_TEST(testPrioritizeSingleBucket); + CPPUNIT_TEST(testPrioritizeSingleBucketAltBucketSpace); CPPUNIT_TEST(testPrioritizeMultipleBuckets); CPPUNIT_TEST(testPendingMaintenanceOperationStatistics); CPPUNIT_TEST(perNodeMaintenanceStatsAreTracked); @@ -27,10 +32,11 @@ class SimpleMaintenanceScannerTest : public CppUnit::TestFixture { std::string dumpPriorityDbToString(const BucketPriorityDatabase&) const; std::unique_ptr<MockMaintenancePriorityGenerator> _priorityGenerator; - std::unique_ptr<MapBucketDatabase> _bucketDb; + std::unique_ptr<DistributorBucketSpaceRepo> _bucketSpaceRepo; std::unique_ptr<SimpleBucketPriorityDatabase> _priorityDb; std::unique_ptr<SimpleMaintenanceScanner> _scanner; + void addBucketToDb(document::BucketSpace bucketSpace, int bucketNum); void addBucketToDb(int bucketNum); bool scanEntireDatabase(int expected); @@ -39,6 +45,7 @@ class SimpleMaintenanceScannerTest : public CppUnit::TestFixture { public: void testPrioritizeSingleBucket(); + void testPrioritizeSingleBucketAltBucketSpace(); void testPrioritizeMultipleBuckets(); void testPendingMaintenanceOperationStatistics(); void perNodeMaintenanceStatsAreTracked(); @@ -53,16 +60,23 @@ void SimpleMaintenanceScannerTest::setUp() { _priorityGenerator.reset(new MockMaintenancePriorityGenerator()); - _bucketDb.reset(new MapBucketDatabase()); + _bucketSpaceRepo = std::make_unique<DistributorBucketSpaceRepo>(); _priorityDb.reset(new SimpleBucketPriorityDatabase()); - _scanner.reset(new SimpleMaintenanceScanner(*_priorityDb, *_priorityGenerator, *_bucketDb)); + _scanner.reset(new SimpleMaintenanceScanner(*_priorityDb, *_priorityGenerator, *_bucketSpaceRepo)); } void -SimpleMaintenanceScannerTest::addBucketToDb(int bucketNum) +SimpleMaintenanceScannerTest::addBucketToDb(document::BucketSpace bucketSpace, int bucketNum) { BucketDatabase::Entry entry(BucketId(16, bucketNum), BucketInfo()); - _bucketDb->update(entry); + auto &bucketDb(_bucketSpaceRepo->get(bucketSpace).getBucketDatabase()); + bucketDb.update(entry); +} + +void +SimpleMaintenanceScannerTest::addBucketToDb(int bucketNum) +{ + addBucketToDb(makeBucketSpace(), bucketNum); } std::string @@ -80,7 +94,27 @@ SimpleMaintenanceScannerTest::testPrioritizeSingleBucket() addBucketToDb(1); std::string expected("PrioritizedBucket(Bucket(BucketSpace(0x0000000000000000), BucketId(0x4000000000000001)), pri VERY_HIGH)\n"); - CPPUNIT_ASSERT(!_scanner->scanNext().isDone()); + auto scanResult = _scanner->scanNext(); + CPPUNIT_ASSERT(!scanResult.isDone()); + CPPUNIT_ASSERT_EQUAL(makeBucketSpace().getId(), scanResult.getBucketSpace().getId()); + CPPUNIT_ASSERT_EQUAL(expected, _priorityDb->toString()); + + CPPUNIT_ASSERT(_scanner->scanNext().isDone()); + CPPUNIT_ASSERT_EQUAL(expected, _priorityDb->toString()); +} + +void +SimpleMaintenanceScannerTest::testPrioritizeSingleBucketAltBucketSpace() +{ + document::BucketSpace bucketSpace(4); + _bucketSpaceRepo->add(bucketSpace, std::make_unique<DistributorBucketSpace>()); + _scanner->reset(); + addBucketToDb(bucketSpace, 1); + std::string expected("PrioritizedBucket(Bucket(BucketSpace(0x0000000000000004), BucketId(0x4000000000000001)), pri VERY_HIGH)\n"); + + auto scanResult = _scanner->scanNext(); + CPPUNIT_ASSERT(!scanResult.isDone()); + CPPUNIT_ASSERT_EQUAL(bucketSpace.getId(), scanResult.getBucketSpace().getId()); CPPUNIT_ASSERT_EQUAL(expected, _priorityDb->toString()); CPPUNIT_ASSERT(_scanner->scanNext().isDone()); diff --git a/storage/src/tests/distributor/statecheckerstest.cpp b/storage/src/tests/distributor/statecheckerstest.cpp index e71525d5dd6..865d869761d 100644 --- a/storage/src/tests/distributor/statecheckerstest.cpp +++ b/storage/src/tests/distributor/statecheckerstest.cpp @@ -826,7 +826,7 @@ StateCheckersTest::testSynchronizeAndMove() runAndVerify<SynchronizeAndMoveStateChecker>( CheckerParams() .expect("[Moving bucket to ideal node 3] " - "(scheduling pri VERY_LOW)") + "(scheduling pri LOW)") .bucketInfo("0=1,1=1,2=1") .clusterState("distributor:1 storage:4") .includeSchedulingPriority(true)); @@ -843,7 +843,7 @@ StateCheckersTest::testSynchronizeAndMove() CheckerParams() .expect("[Moving bucket to ideal node 1]" "[Moving bucket to ideal node 3] (pri 165) " - "(scheduling pri VERY_LOW)") + "(scheduling pri LOW)") .clusterState("distributor:1 storage:5") .bucketInfo("0=1,4=1,5=1") .includeMessagePriority(true) @@ -1540,7 +1540,7 @@ StateCheckersTest::testGarbageCollection() void StateCheckersTest::gc_ops_are_prioritized_with_low_priority_category() { CPPUNIT_ASSERT_EQUAL( std::string("[Needs garbage collection: Last check at 3, current time 4000, " - "configured interval 300] (scheduling pri LOW)"), + "configured interval 300] (scheduling pri VERY_LOW)"), testGarbageCollection(3, 4000, 300, 1, false, true)); } diff --git a/storage/src/tests/storageserver/mergethrottlertest.cpp b/storage/src/tests/storageserver/mergethrottlertest.cpp index 7d04714e9a8..3d469fc4252 100644 --- a/storage/src/tests/storageserver/mergethrottlertest.cpp +++ b/storage/src/tests/storageserver/mergethrottlertest.cpp @@ -313,7 +313,7 @@ MergeThrottlerTest::testChain() _servers[i]->setClusterState(lib::ClusterState("distributor:100 storage:100 version:123")); } - BucketId bid(14, 0x1337); + Bucket bucket(makeDocumentBucket(BucketId(14, 0x1337))); // Use different node permutations to ensure it works no matter which node is // set as the executor. More specifically, _all_ permutations. @@ -321,15 +321,11 @@ MergeThrottlerTest::testChain() uint16_t lastNodeIdx = _storageNodeCount - 1; uint16_t executorNode = indices[0]; - //std::cout << "\n----\n"; std::vector<MergeBucketCommand::Node> nodes; for (int i = 0; i < _storageNodeCount; ++i) { nodes.push_back(MergeBucketCommand::Node(indices[i], (i + executorNode) % 2 == 0)); - //std::cout << indices[i] << " "; } - //std::cout << "\n"; - std::shared_ptr<MergeBucketCommand> cmd( - new MergeBucketCommand(makeDocumentBucket(bid), nodes, UINT_MAX, 123)); + auto cmd = std::make_shared<MergeBucketCommand>(bucket, nodes, UINT_MAX, 123); cmd->setPriority(7); cmd->setTimeout(54321); StorageMessageAddress address("storage", lib::NodeType::STORAGE, 0); @@ -351,8 +347,6 @@ MergeThrottlerTest::testChain() _topLinks[i]->sendDown(fwd); _topLinks[i]->waitForMessage(MessageType::MERGEBUCKET, _messageWaitTime); - //std::cout << "fwd " << i << " -> " << i+1 << "\n"; - // Forwarded merge should not be sent down. Should not be necessary // to lock throttler here, since it should be sleeping like a champion CPPUNIT_ASSERT_EQUAL(std::size_t(0), _bottomLinks[i]->getNumCommands()); @@ -363,7 +357,6 @@ MergeThrottlerTest::testChain() CPPUNIT_ASSERT_EQUAL(uint16_t(i + 1), fwd->getAddress()->getIndex()); CPPUNIT_ASSERT_EQUAL(distributorIndex, dynamic_cast<const StorageCommand&>(*fwd).getSourceIndex()); { - //uint16_t chain[] = { 0 }; std::vector<uint16_t> chain; for (int j = 0; j <= i; ++j) { chain.push_back(j); @@ -416,10 +409,10 @@ MergeThrottlerTest::testChain() // The MergeBucketCommand that is kept in the executor node should // be the one from the node it initially got it from, NOT the one // from the last node, since the chain has looped - CPPUNIT_ASSERT(_throttlers[executorNode]->getActiveMerges().find(bid) + CPPUNIT_ASSERT(_throttlers[executorNode]->getActiveMerges().find(bucket) != _throttlers[executorNode]->getActiveMerges().end()); CPPUNIT_ASSERT_EQUAL(static_cast<StorageMessage*>(fwdToExec.get()), - _throttlers[executorNode]->getActiveMerges().find(bid)->second.getMergeCmd().get()); + _throttlers[executorNode]->getActiveMerges().find(bucket)->second.getMergeCmd().get()); } // Send reply up from persistence layer to simulate a completed @@ -440,7 +433,7 @@ MergeThrottlerTest::testChain() // Merge should not be removed yet from executor, since it's pending an unwind CPPUNIT_ASSERT_EQUAL(std::size_t(1), _throttlers[executorNode]->getActiveMerges().size()); CPPUNIT_ASSERT_EQUAL(static_cast<StorageMessage*>(fwdToExec.get()), - _throttlers[executorNode]->getActiveMerges().find(bid)->second.getMergeCmd().get()); + _throttlers[executorNode]->getActiveMerges().find(bucket)->second.getMergeCmd().get()); } // MergeBucketReply waiting to be sent back to node 2. NOTE: we don't have any // transport context stuff set up here to perform the reply mapping, so we @@ -452,8 +445,6 @@ MergeThrottlerTest::testChain() // eg: 0 -> 2 -> 1 -> 0. Or: 2 -> 1 -> 0 if no cycle for (int i = (executorNode != lastNodeIdx ? _storageNodeCount - 1 : _storageNodeCount - 2); i >= 0; --i) { - //std::cout << "unwind " << i << "\n"; - _topLinks[i]->sendDown(unwind); _topLinks[i]->waitForMessage(MessageType::MERGEBUCKET_REPLY, _messageWaitTime); @@ -469,7 +460,7 @@ MergeThrottlerTest::testChain() CPPUNIT_ASSERT_EQUAL(ReturnCode::OK, mbr.getResult().getResult()); CPPUNIT_ASSERT_EQUAL(vespalib::string("Great success! :D-|-<"), mbr.getResult().getMessage()); - CPPUNIT_ASSERT_EQUAL(bid, mbr.getBucketId()); + CPPUNIT_ASSERT_EQUAL(bucket, mbr.getBucket()); } while (std::next_permutation(indices, indices + _storageNodeCount)); diff --git a/storage/src/vespa/storage/common/bucketmessages.cpp b/storage/src/vespa/storage/common/bucketmessages.cpp index 1d9d64ad24f..3157bad49e5 100644 --- a/storage/src/vespa/storage/common/bucketmessages.cpp +++ b/storage/src/vespa/storage/common/bucketmessages.cpp @@ -39,6 +39,12 @@ ReadBucketListReply::ReadBucketListReply(const ReadBucketList& cmd) ReadBucketListReply::~ReadBucketListReply() { } +document::Bucket +ReadBucketListReply::getBucket() const +{ + return document::Bucket(_bucketSpace, document::BucketId()); +} + void ReadBucketListReply::print(std::ostream& out, bool verbose, const std::string& indent) const { diff --git a/storage/src/vespa/storage/common/bucketmessages.h b/storage/src/vespa/storage/common/bucketmessages.h index 0ff7a22aa4d..941928b1064 100644 --- a/storage/src/vespa/storage/common/bucketmessages.h +++ b/storage/src/vespa/storage/common/bucketmessages.h @@ -55,6 +55,7 @@ public: document::BucketSpace getBucketSpace() const { return _bucketSpace; } spi::PartitionId getPartition() const { return _partition; } + document::Bucket getBucket() const override; spi::BucketIdListResult::List& getBuckets() { return _buckets; } const spi::BucketIdListResult::List& getBuckets() const { diff --git a/storage/src/vespa/storage/common/content_bucket_space_repo.cpp b/storage/src/vespa/storage/common/content_bucket_space_repo.cpp index 04e2c4c27d3..3761b6fc20d 100644 --- a/storage/src/vespa/storage/common/content_bucket_space_repo.cpp +++ b/storage/src/vespa/storage/common/content_bucket_space_repo.cpp @@ -21,6 +21,16 @@ ContentBucketSpaceRepo::get(BucketSpace bucketSpace) const return *itr->second; } +ContentBucketSpaceRepo::BucketSpaces +ContentBucketSpaceRepo::getBucketSpaces() const +{ + BucketSpaces result; + for (const auto &elem : _map) { + result.push_back(elem.first); + } + return result; +} + size_t ContentBucketSpaceRepo::getBucketMemoryUsage() const { diff --git a/storage/src/vespa/storage/common/content_bucket_space_repo.h b/storage/src/vespa/storage/common/content_bucket_space_repo.h index 390cfc15f5d..0d4ddb86bcf 100644 --- a/storage/src/vespa/storage/common/content_bucket_space_repo.h +++ b/storage/src/vespa/storage/common/content_bucket_space_repo.h @@ -13,6 +13,7 @@ namespace storage { class ContentBucketSpaceRepo { public: using BucketSpaceMap = std::unordered_map<document::BucketSpace, ContentBucketSpace::UP, document::BucketSpace::hash>; + using BucketSpaces = std::vector<document::BucketSpace>; private: BucketSpaceMap _map; @@ -23,6 +24,7 @@ public: BucketSpaceMap::const_iterator begin() const { return _map.begin(); } BucketSpaceMap::const_iterator end() const { return _map.end(); } + BucketSpaces getBucketSpaces() const; size_t getBucketMemoryUsage() const; template <typename Functor> diff --git a/storage/src/vespa/storage/distributor/distributor.cpp b/storage/src/vespa/storage/distributor/distributor.cpp index c9930b7299c..665f2edd569 100644 --- a/storage/src/vespa/storage/distributor/distributor.cpp +++ b/storage/src/vespa/storage/distributor/distributor.cpp @@ -87,7 +87,7 @@ Distributor::Distributor(DistributorComponentRegister& compReg, _bucketPriorityDb(new SimpleBucketPriorityDatabase()), _scanner(new SimpleMaintenanceScanner( *_bucketPriorityDb, _idealStateManager, - getDefaultBucketSpace().getBucketDatabase())), + *_bucketSpaceRepo)), _throttlingStarter(new ThrottlingOperationStarter( _maintenanceOperationOwner)), _blockingStarter(new BlockingOperationStarter(_pendingMessageTracker, diff --git a/storage/src/vespa/storage/distributor/distributor.h b/storage/src/vespa/storage/distributor/distributor.h index 3cb3408a951..bb50e69c70e 100644 --- a/storage/src/vespa/storage/distributor/distributor.h +++ b/storage/src/vespa/storage/distributor/distributor.h @@ -6,7 +6,6 @@ #include "bucketdbupdater.h" #include "pendingmessagetracker.h" #include "externaloperationhandler.h" -#include "maintenancebucket.h" #include "min_replica_provider.h" #include "distributorinterface.h" diff --git a/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.cpp b/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.cpp index 27f2c344e9f..ddf28b3d95d 100644 --- a/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.cpp +++ b/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.cpp @@ -16,12 +16,18 @@ namespace distributor { DistributorBucketSpaceRepo::DistributorBucketSpaceRepo() : _map() { - _map.emplace(BucketSpace::placeHolder(), std::make_unique<DistributorBucketSpace>()); + add(BucketSpace::placeHolder(), std::make_unique<DistributorBucketSpace>()); } DistributorBucketSpaceRepo::~DistributorBucketSpaceRepo() { } +void +DistributorBucketSpaceRepo::add(document::BucketSpace bucketSpace, std::unique_ptr<DistributorBucketSpace> distributorBucketSpace) +{ + _map.emplace(bucketSpace, std::move(distributorBucketSpace)); +} + void DistributorBucketSpaceRepo::setDefaultDistribution( std::shared_ptr<lib::Distribution> distr) { @@ -33,7 +39,6 @@ void DistributorBucketSpaceRepo::setDefaultDistribution( DistributorBucketSpace & DistributorBucketSpaceRepo::get(BucketSpace bucketSpace) { - assert(bucketSpace == BucketSpace::placeHolder()); auto itr = _map.find(bucketSpace); assert(itr != _map.end()); return *itr->second; @@ -42,7 +47,6 @@ DistributorBucketSpaceRepo::get(BucketSpace bucketSpace) const DistributorBucketSpace & DistributorBucketSpaceRepo::get(BucketSpace bucketSpace) const { - assert(bucketSpace == BucketSpace::placeHolder()); auto itr = _map.find(bucketSpace); assert(itr != _map.end()); return *itr->second; diff --git a/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.h b/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.h index a9892a1039f..b0f367e8be5 100644 --- a/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.h +++ b/storage/src/vespa/storage/distributor/distributor_bucket_space_repo.h @@ -37,6 +37,7 @@ public: void setDefaultDistribution(std::shared_ptr<lib::Distribution> distr); BucketSpaceMap::const_iterator begin() const { return _map.begin(); } BucketSpaceMap::const_iterator end() const { return _map.end(); } + void add(document::BucketSpace bucketSpace, std::unique_ptr<DistributorBucketSpace> distributorBucketSpace); }; } diff --git a/storage/src/vespa/storage/distributor/distributorinterface.h b/storage/src/vespa/storage/distributor/distributorinterface.h index cd51387964a..749e8a07651 100644 --- a/storage/src/vespa/storage/distributor/distributorinterface.h +++ b/storage/src/vespa/storage/distributor/distributorinterface.h @@ -5,7 +5,6 @@ #include <vespa/storage/common/messagesender.h> #include <vespa/storage/distributor/pendingmessagetracker.h> #include <vespa/storageapi/message/state.h> -#include <vespa/storage/distributor/maintenancebucket.h> #include <vespa/storage/bucketdb/bucketdatabase.h> #include <vespa/storage/distributor/bucketgctimecalculator.h> #include <vespa/storage/distributor/distributormetricsset.h> diff --git a/storage/src/vespa/storage/distributor/maintenance/maintenancescanner.h b/storage/src/vespa/storage/distributor/maintenance/maintenancescanner.h index 783e8e1e5ba..c1d76b57c7c 100644 --- a/storage/src/vespa/storage/distributor/maintenance/maintenancescanner.h +++ b/storage/src/vespa/storage/distributor/maintenance/maintenancescanner.h @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once +#include <vespa/document/bucket/bucketspace.h> #include <vespa/storage/bucketdb/bucketdatabase.h> namespace storage { @@ -13,20 +14,22 @@ public: class ScanResult { bool _done; + document::BucketSpace _bucketSpace; BucketDatabase::Entry _entry; public: bool isDone() const { return _done; } + document::BucketSpace getBucketSpace() const { return _bucketSpace; } const BucketDatabase::Entry& getEntry() const { return _entry; } static ScanResult createDone() { return ScanResult(true); } - static ScanResult createNotDone(BucketDatabase::Entry entry) { - return ScanResult(entry); + static ScanResult createNotDone(document::BucketSpace bucketSpace, BucketDatabase::Entry entry) { + return ScanResult(bucketSpace, entry); } private: - ScanResult(bool done) : _done(done), _entry() {} - ScanResult(const BucketDatabase::Entry& e) : _done(false), _entry(e) {} + ScanResult(bool done) : _done(done), _bucketSpace(document::BucketSpace::placeHolder()), _entry() {} + ScanResult(document::BucketSpace bucketSpace, const BucketDatabase::Entry& e) : _done(false), _bucketSpace(bucketSpace), _entry(e) {} }; virtual ScanResult scanNext() = 0; diff --git a/storage/src/vespa/storage/distributor/maintenance/simplemaintenancescanner.cpp b/storage/src/vespa/storage/distributor/maintenance/simplemaintenancescanner.cpp index 2bdef7ed320..870dcc25a4b 100644 --- a/storage/src/vespa/storage/distributor/maintenance/simplemaintenancescanner.cpp +++ b/storage/src/vespa/storage/distributor/maintenance/simplemaintenancescanner.cpp @@ -1,8 +1,20 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "simplemaintenancescanner.h" +#include <vespa/storage/distributor/distributor_bucket_space.h> namespace storage::distributor { +SimpleMaintenanceScanner::SimpleMaintenanceScanner(BucketPriorityDatabase& bucketPriorityDb, + const MaintenancePriorityGenerator& priorityGenerator, + const DistributorBucketSpaceRepo& bucketSpaceRepo) + : _bucketPriorityDb(bucketPriorityDb), + _priorityGenerator(priorityGenerator), + _bucketSpaceRepo(bucketSpaceRepo), + _bucketSpaceItr(_bucketSpaceRepo.begin()), + _bucketCursor() +{ +} + SimpleMaintenanceScanner::~SimpleMaintenanceScanner() {} SimpleMaintenanceScanner::PendingMaintenanceStats::PendingMaintenanceStats() {} @@ -14,19 +26,28 @@ SimpleMaintenanceScanner::PendingMaintenanceStats::operator = (const PendingMain MaintenanceScanner::ScanResult SimpleMaintenanceScanner::scanNext() { - BucketDatabase::Entry entry(_bucketDb.getNext(_bucketCursor)); - if (!entry.valid()) { - return ScanResult::createDone(); + for (;;) { + if (_bucketSpaceItr == _bucketSpaceRepo.end()) { + return ScanResult::createDone(); + } + const auto &bucketDb(_bucketSpaceItr->second->getBucketDatabase()); + BucketDatabase::Entry entry(bucketDb.getNext(_bucketCursor)); + if (!entry.valid()) { + ++_bucketSpaceItr; + _bucketCursor = document::BucketId(); + continue; + } + prioritizeBucket(document::Bucket(_bucketSpaceItr->first, entry.getBucketId())); + _bucketCursor = entry.getBucketId(); + return ScanResult::createNotDone(_bucketSpaceItr->first, entry); } - prioritizeBucket(document::Bucket(document::BucketSpace::placeHolder(), entry.getBucketId())); - _bucketCursor = entry.getBucketId(); - return ScanResult::createNotDone(entry); } void SimpleMaintenanceScanner::reset() { _bucketCursor = document::BucketId(); + _bucketSpaceItr = _bucketSpaceRepo.begin(); _pendingMaintenance = PendingMaintenanceStats(); } diff --git a/storage/src/vespa/storage/distributor/maintenance/simplemaintenancescanner.h b/storage/src/vespa/storage/distributor/maintenance/simplemaintenancescanner.h index 05de7674d6a..f4ad53957e9 100644 --- a/storage/src/vespa/storage/distributor/maintenance/simplemaintenancescanner.h +++ b/storage/src/vespa/storage/distributor/maintenance/simplemaintenancescanner.h @@ -5,7 +5,7 @@ #include "bucketprioritydatabase.h" #include "maintenanceprioritygenerator.h" #include "node_maintenance_stats_tracker.h" -#include <vespa/storage/bucketdb/bucketdatabase.h> +#include <vespa/storage/distributor/distributor_bucket_space_repo.h> namespace storage { namespace distributor { @@ -31,18 +31,14 @@ public: private: BucketPriorityDatabase& _bucketPriorityDb; const MaintenancePriorityGenerator& _priorityGenerator; - const BucketDatabase& _bucketDb; + const DistributorBucketSpaceRepo &_bucketSpaceRepo; + DistributorBucketSpaceRepo::BucketSpaceMap::const_iterator _bucketSpaceItr; document::BucketId _bucketCursor; PendingMaintenanceStats _pendingMaintenance; public: SimpleMaintenanceScanner(BucketPriorityDatabase& bucketPriorityDb, const MaintenancePriorityGenerator& priorityGenerator, - const BucketDatabase& bucketDb) - : _bucketPriorityDb(bucketPriorityDb), - _priorityGenerator(priorityGenerator), - _bucketDb(bucketDb), - _bucketCursor() - {} + const DistributorBucketSpaceRepo& bucketSpaceRepo); SimpleMaintenanceScanner(const SimpleMaintenanceScanner&) = delete; SimpleMaintenanceScanner& operator=(const SimpleMaintenanceScanner&) = delete; ~SimpleMaintenanceScanner(); diff --git a/storage/src/vespa/storage/distributor/maintenancebucket.h b/storage/src/vespa/storage/distributor/maintenancebucket.h deleted file mode 100644 index a44381830c5..00000000000 --- a/storage/src/vespa/storage/distributor/maintenancebucket.h +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#pragma once - -#include <vespa/document/bucket/bucketid.h> -#include <vespa/vespalib/util/stringfmt.h> -#include <vespa/storage/distributor/maintenance/maintenancepriority.h> - -namespace storage { - -namespace distributor { - -/** - * Simple container to communicate a bucket that needs to be - * checked for maintenanceoperations. - */ -class MaintenanceBucket { -public: - typedef MaintenancePriority::Priority Priority; - - MaintenanceBucket() - : node(0), - pri(MaintenancePriority::NO_MAINTENANCE_NEEDED) - {} - - MaintenanceBucket(const document::BucketId& bid_, - uint16_t node_, - Priority pri_) - : bid(bid_), - node(node_), - pri(pri_) - { - - } - - // The bucket to be checked. - document::BucketId bid; - - // The primary node of the bucket. - uint16_t node; - - // The priority to check the bucket. - Priority pri; - - bool requiresMaintenance() const { - return pri != MaintenancePriority::NO_MAINTENANCE_NEEDED; - } - - std::string toString() const { - return vespalib::make_string("MaintenanceBucket(%s: Node %d, Pri %s)", - bid.toString().c_str(), - (int)node, - MaintenancePriority::toString(pri).c_str()); - } -}; - -} - -} - diff --git a/storage/src/vespa/storage/distributor/statechecker.h b/storage/src/vespa/storage/distributor/statechecker.h index 9f9f57dd3bf..e204cf5325a 100644 --- a/storage/src/vespa/storage/distributor/statechecker.h +++ b/storage/src/vespa/storage/distributor/statechecker.h @@ -2,7 +2,7 @@ #pragma once #include "bucketgctimecalculator.h" -#include "maintenancebucket.h" +#include <vespa/storage/distributor/maintenance/maintenancepriority.h> #include <vespa/storage/distributor/operations/idealstate/idealstateoperation.h> #include <vespa/storage/common/storagecomponent.h> #include <vespa/storage/bucketdb/bucketdatabase.h> diff --git a/storage/src/vespa/storage/distributor/statecheckers.cpp b/storage/src/vespa/storage/distributor/statecheckers.cpp index 49e9ba9f1c4..1f0cb19ef93 100644 --- a/storage/src/vespa/storage/distributor/statecheckers.cpp +++ b/storage/src/vespa/storage/distributor/statecheckers.cpp @@ -849,7 +849,7 @@ SynchronizeAndMoveStateChecker::check(StateChecker::Context& c) op->setPriority(result.priority()); op->setDetailedReason(result.reason()); MaintenancePriority::Priority schedPri( - result.needsMoveOnly() ? MaintenancePriority::VERY_LOW + result.needsMoveOnly() ? MaintenancePriority::LOW : MaintenancePriority::MEDIUM); return Result::createStoredResult(std::move(op), schedPri); @@ -1142,7 +1142,7 @@ GarbageCollectionStateChecker::check(Context& c) op->setPriority(c.distributorConfig.getMaintenancePriorities() .garbageCollection); op->setDetailedReason(reason.c_str()); - return Result::createStoredResult(std::move(op), MaintenancePriority::LOW); + return Result::createStoredResult(std::move(op), MaintenancePriority::VERY_LOW); } else { return Result::noMaintenanceNeeded(); } diff --git a/storage/src/vespa/storage/persistence/filestorage/modifiedbucketchecker.cpp b/storage/src/vespa/storage/persistence/filestorage/modifiedbucketchecker.cpp index 093576622db..1834c93209d 100644 --- a/storage/src/vespa/storage/persistence/filestorage/modifiedbucketchecker.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/modifiedbucketchecker.cpp @@ -12,6 +12,32 @@ using document::BucketSpace; namespace storage { +ModifiedBucketChecker::CyclicBucketSpaceIterator:: +CyclicBucketSpaceIterator(const ContentBucketSpaceRepo::BucketSpaces &bucketSpaces) + : _bucketSpaces(bucketSpaces), + _idx(0) +{ + std::sort(_bucketSpaces.begin(), _bucketSpaces.end()); +} + +ModifiedBucketChecker::BucketIdListResult::BucketIdListResult() + : _bucketSpace(document::BucketSpace::placeHolder()), + _buckets() +{ +} + +void +ModifiedBucketChecker::BucketIdListResult::reset(document::BucketSpace bucketSpace, + document::bucket::BucketIdList &buckets) +{ + _bucketSpace = bucketSpace; + assert(_buckets.empty()); + _buckets.swap(buckets); + // We pick chunks from the end of the list, so reverse it to get + // the same send order as order received. + std::reverse(_buckets.begin(), _buckets.end()); +} + ModifiedBucketChecker::ModifiedBucketChecker( ServiceLayerComponentRegister& compReg, spi::PersistenceProvider& provider, @@ -23,6 +49,8 @@ ModifiedBucketChecker::ModifiedBucketChecker( _configFetcher(configUri.getContext()), _monitor(), _stateLock(), + _bucketSpaces(), + _rechecksNotStarted(), _pendingRequests(0), _maxPendingChunkSize(100), _singleThreadMode(false) @@ -33,6 +61,7 @@ ModifiedBucketChecker::ModifiedBucketChecker( std::ostringstream threadName; threadName << "Modified bucket checker " << static_cast<void*>(this); _component.reset(new ServiceLayerComponent(compReg, threadName.str())); + _bucketSpaces = std::make_unique<CyclicBucketSpaceIterator>(_component->getBucketSpaceRepo().getBucketSpaces()); } ModifiedBucketChecker::~ModifiedBucketChecker() @@ -120,9 +149,9 @@ ModifiedBucketChecker::onInternalReply( } bool -ModifiedBucketChecker::requestModifiedBucketsFromProvider() +ModifiedBucketChecker::requestModifiedBucketsFromProvider(document::BucketSpace bucketSpace) { - spi::BucketIdListResult result(_provider.getModifiedBuckets(document::BucketSpace::placeHolder())); + spi::BucketIdListResult result(_provider.getModifiedBuckets(bucketSpace)); if (result.hasError()) { LOG(debug, "getModifiedBuckets() failed: %s", result.toString().c_str()); @@ -130,11 +159,7 @@ ModifiedBucketChecker::requestModifiedBucketsFromProvider() } { vespalib::LockGuard guard(_stateLock); - assert(_rechecksNotStarted.empty()); - _rechecksNotStarted.swap(result.getList()); - // We pick chunks from the end of the list, so reverse it to get - // the same send order as order received. - std::reverse(_rechecksNotStarted.begin(), _rechecksNotStarted.end()); + _rechecksNotStarted.reset(bucketSpace, result.getList()); } return true; } @@ -148,7 +173,7 @@ ModifiedBucketChecker::nextRecheckChunk( size_t n = std::min(_maxPendingChunkSize, _rechecksNotStarted.size()); for (size_t i = 0; i < n; ++i) { - document::Bucket bucket(BucketSpace::placeHolder(), _rechecksNotStarted.back()); + document::Bucket bucket(_rechecksNotStarted.bucketSpace(), _rechecksNotStarted.back()); commandsToSend.emplace_back(new RecheckBucketInfoCommand(bucket)); _rechecksNotStarted.pop_back(); } @@ -184,7 +209,7 @@ ModifiedBucketChecker::tick() shouldRequestFromProvider = !moreChunksRemaining(); } if (shouldRequestFromProvider) { - if (!requestModifiedBucketsFromProvider()) { + if (!requestModifiedBucketsFromProvider(_bucketSpaces->next())) { return false; } } diff --git a/storage/src/vespa/storage/persistence/filestorage/modifiedbucketchecker.h b/storage/src/vespa/storage/persistence/filestorage/modifiedbucketchecker.h index 3e43481bf49..c6f13ce1a4c 100644 --- a/storage/src/vespa/storage/persistence/filestorage/modifiedbucketchecker.h +++ b/storage/src/vespa/storage/persistence/filestorage/modifiedbucketchecker.h @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once +#include <vespa/storage/common/content_bucket_space_repo.h> #include <vespa/storage/common/storagecomponent.h> #include <vespa/storage/common/servicelayercomponent.h> #include <vespa/storage/common/storagelink.h> @@ -49,17 +50,45 @@ private: bool moreChunksRemaining() const { return !_rechecksNotStarted.empty(); } - bool requestModifiedBucketsFromProvider(); + bool requestModifiedBucketsFromProvider(document::BucketSpace bucketSpace); void nextRecheckChunk(std::vector<RecheckBucketInfoCommand::SP>&); void dispatchAllToPersistenceQueues(const std::vector<RecheckBucketInfoCommand::SP>&); + class CyclicBucketSpaceIterator { + private: + ContentBucketSpaceRepo::BucketSpaces _bucketSpaces; + size_t _idx; + public: + using UP = std::unique_ptr<CyclicBucketSpaceIterator>; + CyclicBucketSpaceIterator(const ContentBucketSpaceRepo::BucketSpaces &bucketSpaces); + document::BucketSpace next() { + return _bucketSpaces[(_idx++)%_bucketSpaces.size()]; + } + }; + + class BucketIdListResult { + private: + document::BucketSpace _bucketSpace; + document::bucket::BucketIdList _buckets; + public: + BucketIdListResult(); + void reset(document::BucketSpace bucketSpace, + document::bucket::BucketIdList &buckets); + const document::BucketSpace &bucketSpace() const { return _bucketSpace; } + size_t size() const { return _buckets.size(); } + bool empty() const { return _buckets.empty(); } + const document::BucketId &back() const { return _buckets.back(); } + void pop_back() { _buckets.pop_back(); } + }; + spi::PersistenceProvider& _provider; ServiceLayerComponent::UP _component; framework::Thread::UP _thread; config::ConfigFetcher _configFetcher; vespalib::Monitor _monitor; vespalib::Lock _stateLock; - document::bucket::BucketIdList _rechecksNotStarted; + CyclicBucketSpaceIterator::UP _bucketSpaces; + BucketIdListResult _rechecksNotStarted; size_t _pendingRequests; size_t _maxPendingChunkSize; bool _singleThreadMode; // For unit testing only diff --git a/storage/src/vespa/storage/storageserver/mergethrottler.cpp b/storage/src/vespa/storage/storageserver/mergethrottler.cpp index be30c459bdf..60dedab5ce8 100644 --- a/storage/src/vespa/storage/storageserver/mergethrottler.cpp +++ b/storage/src/vespa/storage/storageserver/mergethrottler.cpp @@ -431,7 +431,7 @@ bool MergeThrottler::isMergeAlreadyKnown(const api::StorageMessage::SP& msg) const { auto& mergeCmd = static_cast<const api::MergeBucketCommand&>(*msg); - return _merges.find(mergeCmd.getBucketId()) != _merges.end(); + return _merges.find(mergeCmd.getBucket()) != _merges.end(); } bool @@ -830,10 +830,8 @@ MergeThrottler::processNewMergeCommand( // and that we can fit it into our window. // Register the merge now so that it will contribute to filling up our // merge throttling window. - assert(_merges.find(mergeCmd.getBucketId()) == _merges.end()); - auto state = _merges.insert( - std::make_pair(mergeCmd.getBucketId(), - ChainedMergeState(msg))).first; + assert(_merges.find(mergeCmd.getBucket()) == _merges.end()); + auto state = _merges.emplace(mergeCmd.getBucket(), ChainedMergeState(msg)).first; LOG(debug, "Added merge %s to internal state", mergeCmd.toString().c_str()); @@ -911,7 +909,7 @@ MergeThrottler::processCycledMergeCommand( MergeNodeSequence nodeSeq(mergeCmd, _component.getIndex()); - auto mergeIter = _merges.find(mergeCmd.getBucketId()); + auto mergeIter = _merges.find(mergeCmd.getBucket()); assert(mergeIter != _merges.end()); if (mergeIter->second.isAborted()) { @@ -964,7 +962,7 @@ MergeThrottler::processMergeReply( { auto& mergeReply = dynamic_cast<const api::MergeBucketReply&>(*msg); - auto mergeIter = _merges.find(mergeReply.getBucketId()); + auto mergeIter = _merges.find(mergeReply.getBucket()); if (mergeIter == _merges.end()) { LOG(warning, "Received %s, which has no command mapped " "for it. Cannot send chained reply!", @@ -1075,7 +1073,7 @@ MergeThrottler::onDown(const std::shared_ptr<api::StorageMessage>& msg) } else if (isDiffCommand(*msg)) { vespalib::LockGuard lock(_stateLock); auto& cmd = static_cast<api::StorageCommand&>(*msg); - if (bucketIsUnknownOrAborted(cmd.getBucketId())) { + if (bucketIsUnknownOrAborted(cmd.getBucket())) { sendUp(makeAbortReply(cmd, "no state recorded for bucket in merge " "throttler, source merge probably aborted earlier")); return true; @@ -1104,7 +1102,7 @@ MergeThrottler::isMergeReply(const api::StorageMessage& msg) const } bool -MergeThrottler::bucketIsUnknownOrAborted(const document::BucketId& bucket) const +MergeThrottler::bucketIsUnknownOrAborted(const document::Bucket& bucket) const { auto it = _merges.find(bucket); if (it == _merges.end()) { diff --git a/storage/src/vespa/storage/storageserver/mergethrottler.h b/storage/src/vespa/storage/storageserver/mergethrottler.h index 69fdfdc1b95..d62e9a042b2 100644 --- a/storage/src/vespa/storage/storageserver/mergethrottler.h +++ b/storage/src/vespa/storage/storageserver/mergethrottler.h @@ -13,7 +13,7 @@ #include <vespa/storage/distributor/messageguard.h> #include <vespa/storageframework/generic/status/htmlstatusreporter.h> #include <vespa/storageapi/message/bucket.h> -#include <vespa/document/bucket/bucketid.h> +#include <vespa/document/bucket/bucket.h> #include <vespa/vespalib/util/document_runnable.h> #include <vespa/messagebus/staticthrottlepolicy.h> #include <vespa/metrics/metrics.h> @@ -134,7 +134,7 @@ private: const std::string& getMergeCmdString() const { return _cmdString; } }; - typedef std::map<document::BucketId, ChainedMergeState> ActiveMergeMap; + typedef std::map<document::Bucket, ChainedMergeState> ActiveMergeMap; // Use a set rather than a priority_queue, since we want to be // able to iterate over the collection during status rendering @@ -371,7 +371,7 @@ private: bool isDiffCommand(const api::StorageMessage& msg) const; bool isMergeCommand(const api::StorageMessage& msg) const; bool isMergeReply(const api::StorageMessage& msg) const; - bool bucketIsUnknownOrAborted(const document::BucketId& bucket) const; + bool bucketIsUnknownOrAborted(const document::Bucket& bucket) const; std::shared_ptr<api::StorageMessage> makeAbortReply( api::StorageCommand& cmd, diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization.h b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization.h index e32942d0303..d1e5783e609 100644 --- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization.h +++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization.h @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once +#include <vespa/document/bucket/bucket.h> #include <vespa/document/repo/documenttyperepo.h> #include <vespa/messagebus/routable.h> #include <vespa/storageapi/mbusprot/storagemessage.h> @@ -176,6 +177,10 @@ protected: virtual SCmd::UP onDecodeBatchPutRemoveCommand(BBuf&) const = 0; virtual SRep::UP onDecodeBatchPutRemoveReply(const SCmd&, BBuf&) const = 0; + virtual document::Bucket getBucket(document::ByteBuffer& buf) const = 0; + virtual void putBucket(const document::Bucket& bucket, vespalib::GrowableByteBuffer& buf) const = 0; + virtual document::BucketSpace getBucketSpace(document::ByteBuffer& buf) const = 0; + virtual void putBucketSpace(document::BucketSpace bucketSpace, vespalib::GrowableByteBuffer& buf) const = 0; virtual api::BucketInfo getBucketInfo(document::ByteBuffer& buf) const = 0; virtual void putBucketInfo(const api::BucketInfo& info, vespalib::GrowableByteBuffer& buf) const = 0; diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization4_2.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization4_2.cpp index e4993a1ee7b..cd8e4992ba5 100644 --- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization4_2.cpp +++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization4_2.cpp @@ -41,7 +41,7 @@ void ProtocolSerialization4_2::onEncode( char* pos = buf.allocate(docBlockSize); vdslib::DocumentList copy(msg.getOperations(), pos, docBlockSize); buf.putBoolean(msg.keepTimeStamps()); - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); onEncodeBucketInfoCommand(buf, msg); } @@ -52,8 +52,7 @@ ProtocolSerialization4_2::onDecodeMultiOperationCommand(BBuf& buf) const std::vector<char> buffer(length); buf.getBytes(&buffer[0], length); bool keepTimestamps = SH::getBoolean(buf); - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::MultiOperationCommand::UP msg( new api::MultiOperationCommand(getTypeRepoSp(), bucket, buffer, keepTimestamps)); @@ -67,7 +66,7 @@ ProtocolSerialization4_2::onEncode( { // Serialization format - allow different types of serialization depending on source. buf.putByte(0); - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putInt(msg.getOperationCount()); for (uint32_t i = 0; i < msg.getOperationCount(); i++) { @@ -101,8 +100,7 @@ api::StorageCommand::UP ProtocolSerialization4_2::onDecodeBatchPutRemoveCommand(BBuf& buf) const { SH::getByte(buf); - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); std::unique_ptr<api::BatchPutRemoveCommand> cmd(new api::BatchPutRemoveCommand(bucket)); int length = SH::getInt(buf); @@ -164,7 +162,7 @@ void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::GetCommand& msg) const { buf.putString(msg.getDocumentId().toString()); - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putLong(msg.getBeforeTimestamp()); buf.putBoolean(msg.getFieldSet() == "[header]"); onEncodeCommand(buf, msg); @@ -174,8 +172,7 @@ api::StorageCommand::UP ProtocolSerialization4_2::onDecodeGetCommand(BBuf& buf) const { document::DocumentId did(SH::getString(buf)); - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::Timestamp beforeTimestamp(SH::getLong(buf)); bool headerOnly(SH::getBoolean(buf)); api::GetCommand::UP msg( @@ -188,7 +185,7 @@ void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::RemoveCommand& msg) const { buf.putString(msg.getDocumentId().toString()); - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putLong(msg.getTimestamp()); onEncodeBucketInfoCommand(buf, msg); } @@ -197,8 +194,7 @@ api::StorageCommand::UP ProtocolSerialization4_2::onDecodeRemoveCommand(BBuf& buf) const { document::DocumentId did(SH::getString(buf)); - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::Timestamp timestamp(SH::getLong(buf)); api::RemoveCommand::UP msg(new api::RemoveCommand(bucket, did, timestamp)); onDecodeBucketInfoCommand(buf, *msg); @@ -208,7 +204,7 @@ ProtocolSerialization4_2::onDecodeRemoveCommand(BBuf& buf) const void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::RevertCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putInt(msg.getRevertTokens().size()); for (uint32_t i=0, n=msg.getRevertTokens().size(); i<n; ++i) { buf.putLong(msg.getRevertTokens()[i]); @@ -219,8 +215,7 @@ void ProtocolSerialization4_2::onEncode( api::StorageCommand::UP ProtocolSerialization4_2::onDecodeRevertCommand(BBuf& buf) const { - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); std::vector<api::Timestamp> tokens(SH::getInt(buf)); for (uint32_t i=0, n=tokens.size(); i<n; ++i) { tokens[i] = SH::getLong(buf); @@ -233,15 +228,14 @@ ProtocolSerialization4_2::onDecodeRevertCommand(BBuf& buf) const void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::CreateBucketCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); onEncodeBucketInfoCommand(buf, msg); } api::StorageCommand::UP ProtocolSerialization4_2::onDecodeCreateBucketCommand(BBuf& buf) const { - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::CreateBucketCommand::UP msg(new api::CreateBucketCommand(bucket)); onDecodeBucketInfoCommand(buf, *msg); return api::StorageCommand::UP(msg.release()); @@ -250,7 +244,7 @@ ProtocolSerialization4_2::onDecodeCreateBucketCommand(BBuf& buf) const void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::MergeBucketCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); const std::vector<api::MergeBucketCommand::Node>& nodes(msg.getNodes()); buf.putShort(nodes.size()); for (uint32_t i=0; i<nodes.size(); ++i) { @@ -265,8 +259,7 @@ api::StorageCommand::UP ProtocolSerialization4_2::onDecodeMergeBucketCommand(BBuf& buf) const { typedef api::MergeBucketCommand::Node Node; - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); uint16_t nodeCount = SH::getShort(buf); std::vector<Node> nodes; nodes.reserve(nodeCount); @@ -285,7 +278,7 @@ ProtocolSerialization4_2::onDecodeMergeBucketCommand(BBuf& buf) const void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::GetBucketDiffCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); const std::vector<api::MergeBucketCommand::Node>& nodes(msg.getNodes()); buf.putShort(nodes.size()); for (uint32_t i=0; i<nodes.size(); ++i) { @@ -305,8 +298,7 @@ api::StorageCommand::UP ProtocolSerialization4_2::onDecodeGetBucketDiffCommand(BBuf& buf) const { typedef api::MergeBucketCommand::Node Node; - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); uint16_t nodeCount = SH::getShort(buf); std::vector<Node> nodes; nodes.reserve(nodeCount); @@ -335,7 +327,7 @@ ProtocolSerialization4_2::onDecodeGetBucketDiffCommand(BBuf& buf) const void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::ApplyBucketDiffCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); const std::vector<api::MergeBucketCommand::Node>& nodes(msg.getNodes()); buf.putShort(nodes.size()); for (uint32_t i=0; i<nodes.size(); ++i) { @@ -363,8 +355,7 @@ api::StorageCommand::UP ProtocolSerialization4_2::onDecodeApplyBucketDiffCommand(BBuf& buf) const { typedef api::MergeBucketCommand::Node Node; - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); uint16_t nodeCount = SH::getShort(buf); std::vector<Node> nodes; nodes.reserve(nodeCount); @@ -440,7 +431,7 @@ ProtocolSerialization4_2::onDecodeRequestBucketInfoReply(const SCmd& cmd, void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::NotifyBucketChangeCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); putBucketInfo(msg.getBucketInfo(), buf); onEncodeCommand(buf, msg); } @@ -448,8 +439,7 @@ void ProtocolSerialization4_2::onEncode( api::StorageCommand::UP ProtocolSerialization4_2::onDecodeNotifyBucketChangeCommand(BBuf& buf) const { - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::BucketInfo info(getBucketInfo(buf)); api::NotifyBucketChangeCommand::UP msg( new api::NotifyBucketChangeCommand(bucket, info)); @@ -476,7 +466,7 @@ ProtocolSerialization4_2::onDecodeNotifyBucketChangeReply(const SCmd& cmd, void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::SplitBucketCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putByte(msg.getMinSplitBits()); buf.putByte(msg.getMaxSplitBits()); buf.putInt(msg.getMinByteSize()); @@ -487,8 +477,7 @@ void ProtocolSerialization4_2::onEncode( api::StorageCommand::UP ProtocolSerialization4_2::onDecodeSplitBucketCommand(BBuf& buf) const { - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::SplitBucketCommand::UP msg(new api::SplitBucketCommand(bucket)); msg->setMinSplitBits(SH::getByte(buf)); msg->setMaxSplitBits(SH::getByte(buf)); @@ -531,6 +520,7 @@ void ProtocolSerialization4_2::onEncode( GBBuf& buf, const api::CreateVisitorCommand& msg) const { + putBucketSpace(msg.getBucketSpace(), buf); buf.putString(msg.getLibraryName()); buf.putString(msg.getInstanceId()); buf.putString(msg.getDocumentSelection()); @@ -562,7 +552,7 @@ ProtocolSerialization4_2::onEncode( api::StorageCommand::UP ProtocolSerialization4_2::onDecodeCreateVisitorCommand(BBuf& buf) const { - BucketSpace bucketSpace(BucketSpace::placeHolder()); + BucketSpace bucketSpace = getBucketSpace(buf); vespalib::stringref libraryName = SH::getString(buf); vespalib::stringref instanceId = SH::getString(buf); vespalib::stringref selection = SH::getString(buf); @@ -638,7 +628,7 @@ void ProtocolSerialization4_2::onEncode(GBBuf& buf, const api::RemoveLocationCommand& msg) const { buf.putString(msg.getDocumentSelection()); - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); onEncodeCommand(buf, msg); } @@ -646,8 +636,7 @@ api::StorageCommand::UP ProtocolSerialization4_2::onDecodeRemoveLocationCommand(BBuf& buf) const { vespalib::stringref documentSelection = SH::getString(buf); - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::RemoveLocationCommand::UP msg; msg.reset(new api::RemoveLocationCommand(documentSelection, bucket)); diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp index 22f58ebc58b..d6d47d877e5 100644 --- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp +++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp @@ -12,6 +12,30 @@ using document::BucketSpace; namespace storage { namespace mbusprot { +document::Bucket +ProtocolSerialization5_0::getBucket(document::ByteBuffer& buf) const +{ + document::BucketId bucketId(SH::getLong(buf)); + return document::Bucket(BucketSpace::placeHolder(), bucketId); +} + +void +ProtocolSerialization5_0::putBucket(const document::Bucket& bucket, vespalib::GrowableByteBuffer& buf) const +{ + buf.putLong(bucket.getBucketId().getRawId()); +} + +document::BucketSpace +ProtocolSerialization5_0::getBucketSpace(document::ByteBuffer&) const +{ + return BucketSpace::placeHolder(); +} + +void +ProtocolSerialization5_0::putBucketSpace(document::BucketSpace, vespalib::GrowableByteBuffer&) const +{ +} + api::BucketInfo ProtocolSerialization5_0::getBucketInfo(document::ByteBuffer& buf) const { @@ -93,7 +117,7 @@ void ProtocolSerialization5_0::onEncode( GBBuf& buf, const api::PutCommand& msg) const { SH::putDocument(msg.getDocument().get(), buf); - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putLong(msg.getTimestamp()); buf.putLong(msg.getUpdateTimestamp()); onEncodeBucketInfoCommand(buf, msg); @@ -103,8 +127,7 @@ api::StorageCommand::UP ProtocolSerialization5_0::onDecodePutCommand(BBuf& buf) const { document::Document::SP doc(SH::getDocument(buf, getTypeRepo())); - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::Timestamp ts(SH::getLong(buf)); api::PutCommand::UP msg(new api::PutCommand(bucket, doc, ts)); msg->setUpdateTimestamp(SH::getLong(buf)); @@ -203,7 +226,7 @@ void ProtocolSerialization5_0::onEncode( buf.putInt(0); } - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putLong(msg.getTimestamp()); buf.putLong(msg.getOldTimestamp()); onEncodeBucketInfoCommand(buf, msg); @@ -224,8 +247,7 @@ ProtocolSerialization5_0::onDecodeUpdateCommand(BBuf& buf) const SERIALIZE_HEAD)); } - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::Timestamp timestamp(SH::getLong(buf)); api::UpdateCommand::UP msg( new api::UpdateCommand(bucket, update, timestamp)); @@ -270,7 +292,7 @@ void ProtocolSerialization5_0::onEncode( GBBuf& buf, const api::DeleteBucketCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); onEncodeBucketInfoCommand(buf, msg); putBucketInfo(msg.getBucketInfo(), buf); } @@ -278,8 +300,7 @@ ProtocolSerialization5_0::onEncode( api::StorageCommand::UP ProtocolSerialization5_0::onDecodeDeleteBucketCommand(BBuf& buf) const { - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::DeleteBucketCommand::UP msg(new api::DeleteBucketCommand(bucket)); onDecodeBucketInfoCommand(buf, *msg); if (buf.getRemaining() >= SH::BUCKET_INFO_SERIALIZED_SIZE) { @@ -495,7 +516,7 @@ void ProtocolSerialization5_0::onEncode( GBBuf& buf, const api::JoinBucketsCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putInt(msg.getSourceBuckets().size()); for (uint32_t i=0, n=msg.getSourceBuckets().size(); i<n; ++i) { buf.putLong(msg.getSourceBuckets()[i].getRawId()); @@ -507,8 +528,7 @@ ProtocolSerialization5_0::onEncode( api::StorageCommand::UP ProtocolSerialization5_0::onDecodeJoinBucketsCommand(BBuf& buf) const { - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::JoinBucketsCommand::UP msg(new api::JoinBucketsCommand(bucket)); uint32_t size = SH::getInt(buf); if (size > buf.getRemaining()) { @@ -621,6 +641,7 @@ void ProtocolSerialization5_0::onEncode( for (uint32_t i=0; i<buckets.size(); ++i) { buf.putLong(buckets[i].getRawId()); } + putBucketSpace(msg.getBucketSpace(), buf); if (buckets.size() == 0) { buf.putShort(msg.getDistributor()); buf.putString(msg.getSystemState().toString()); @@ -638,7 +659,7 @@ ProtocolSerialization5_0::onDecodeRequestBucketInfoCommand(BBuf& buf) const buckets[i] = document::BucketId(SH::getLong(buf)); } api::RequestBucketInfoCommand::UP msg; - BucketSpace bucketSpace(BucketSpace::placeHolder()); + BucketSpace bucketSpace = getBucketSpace(buf); if (buckets.size() != 0) { msg.reset(new api::RequestBucketInfoCommand(bucketSpace, buckets)); } else { diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.h b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.h index ff9f08d38a9..c1285939a1c 100644 --- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.h +++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.h @@ -15,6 +15,10 @@ public: ProtocolSerialization5_0(const document::DocumentTypeRepo::SP&, const documentapi::LoadTypeSet& loadTypes); + document::Bucket getBucket(document::ByteBuffer& buf) const override; + void putBucket(const document::Bucket& bucket, vespalib::GrowableByteBuffer& buf) const override; + document::BucketSpace getBucketSpace(document::ByteBuffer& buf) const override; + void putBucketSpace(document::BucketSpace bucketSpace, vespalib::GrowableByteBuffer& buf) const override; api::BucketInfo getBucketInfo(document::ByteBuffer& buf) const override; void putBucketInfo(const api::BucketInfo& info, vespalib::GrowableByteBuffer& buf) const override; diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_1.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_1.cpp index dc97742b733..0afdfebd5b7 100644 --- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_1.cpp +++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_1.cpp @@ -64,7 +64,7 @@ ProtocolSerialization5_1::ProtocolSerialization5_1( void ProtocolSerialization5_1::onEncode( GBBuf& buf, const api::SetBucketStateCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putByte(static_cast<uint8_t>(msg.getState())); onEncodeCommand(buf, msg); } @@ -72,8 +72,7 @@ void ProtocolSerialization5_1::onEncode( api::StorageCommand::UP ProtocolSerialization5_1::onDecodeSetBucketStateCommand(BBuf& buf) const { - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::SetBucketStateCommand::BUCKET_STATE state( static_cast<api::SetBucketStateCommand::BUCKET_STATE>( SH::getByte(buf))); @@ -103,7 +102,7 @@ void ProtocolSerialization5_1::onEncode( GBBuf& buf, const api::GetCommand& msg) const { buf.putString(msg.getDocumentId().toString()); - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putLong(msg.getBeforeTimestamp()); buf.putString(msg.getFieldSet()); onEncodeCommand(buf, msg); @@ -113,8 +112,7 @@ api::StorageCommand::UP ProtocolSerialization5_1::onDecodeGetCommand(BBuf& buf) const { document::DocumentId did(SH::getString(buf)); - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); api::Timestamp beforeTimestamp(SH::getLong(buf)); std::string fieldSet(SH::getString(buf)); api::GetCommand::UP msg( @@ -127,6 +125,7 @@ void ProtocolSerialization5_1::onEncode( GBBuf& buf, const api::CreateVisitorCommand& msg) const { + putBucketSpace(msg.getBucketSpace(), buf); buf.putString(msg.getLibraryName()); buf.putString(msg.getInstanceId()); buf.putString(msg.getDocumentSelection()); @@ -161,7 +160,7 @@ ProtocolSerialization5_1::onEncode( api::StorageCommand::UP ProtocolSerialization5_1::onDecodeCreateVisitorCommand(BBuf& buf) const { - BucketSpace bucketSpace(BucketSpace::placeHolder()); + BucketSpace bucketSpace = getBucketSpace(buf); vespalib::stringref libraryName = SH::getString(buf); vespalib::stringref instanceId = SH::getString(buf); vespalib::stringref selection = SH::getString(buf); @@ -208,7 +207,7 @@ ProtocolSerialization5_1::onDecodeCreateVisitorCommand(BBuf& buf) const void ProtocolSerialization5_1::onEncode( GBBuf& buf, const api::CreateBucketCommand& msg) const { - buf.putLong(msg.getBucketId().getRawId()); + putBucket(msg.getBucket(), buf); buf.putBoolean(msg.getActive()); onEncodeBucketInfoCommand(buf, msg); } @@ -216,8 +215,7 @@ void ProtocolSerialization5_1::onEncode( api::StorageCommand::UP ProtocolSerialization5_1::onDecodeCreateBucketCommand(BBuf& buf) const { - document::BucketId bucketId(SH::getLong(buf)); - document::Bucket bucket(BucketSpace::placeHolder(), bucketId); + document::Bucket bucket = getBucket(buf); bool setActive = SH::getBoolean(buf); api::CreateBucketCommand::UP msg(new api::CreateBucketCommand(bucket)); msg->setActive(setActive); diff --git a/storageapi/src/vespa/storageapi/message/bucket.cpp b/storageapi/src/vespa/storageapi/message/bucket.cpp index 18ad95c2c02..0961a8f6edc 100644 --- a/storageapi/src/vespa/storageapi/message/bucket.cpp +++ b/storageapi/src/vespa/storageapi/message/bucket.cpp @@ -475,6 +475,12 @@ RequestBucketInfoCommand::RequestBucketInfoCommand( { } +document::Bucket +RequestBucketInfoCommand::getBucket() const +{ + return document::Bucket(_bucketSpace, document::BucketId()); +} + void RequestBucketInfoCommand::print(std::ostream& out, bool verbose, const std::string& indent) const diff --git a/storageapi/src/vespa/storageapi/message/bucket.h b/storageapi/src/vespa/storageapi/message/bucket.h index 05838600a24..5fba1a3bf65 100644 --- a/storageapi/src/vespa/storageapi/message/bucket.h +++ b/storageapi/src/vespa/storageapi/message/bucket.h @@ -363,6 +363,7 @@ public: const vespalib::string& getDistributionHash() const { return _distributionHash; } document::BucketSpace getBucketSpace() const { return _bucketSpace; } + document::Bucket getBucket() const override; void print(std::ostream& out, bool verbose, const std::string& indent) const override; diff --git a/storageapi/src/vespa/storageapi/message/visitor.cpp b/storageapi/src/vespa/storageapi/message/visitor.cpp index 7b5a614bd3e..3cb6f72d5d9 100644 --- a/storageapi/src/vespa/storageapi/message/visitor.cpp +++ b/storageapi/src/vespa/storageapi/message/visitor.cpp @@ -68,6 +68,12 @@ CreateVisitorCommand::CreateVisitorCommand(const CreateVisitorCommand& o) CreateVisitorCommand::~CreateVisitorCommand() {} +document::Bucket +CreateVisitorCommand::getBucket() const +{ + return document::Bucket(_bucketSpace, document::BucketId()); +} + void CreateVisitorCommand::print(std::ostream& out, bool verbose, const std::string& indent) const diff --git a/storageapi/src/vespa/storageapi/message/visitor.h b/storageapi/src/vespa/storageapi/message/visitor.h index f252ecd344f..e1850686222 100644 --- a/storageapi/src/vespa/storageapi/message/visitor.h +++ b/storageapi/src/vespa/storageapi/message/visitor.h @@ -86,6 +86,7 @@ public: VisitorId getVisitorId() const { return _visitorId; } uint32_t getVisitorCmdId() const { return _visitorCmdId; } document::BucketSpace getBucketSpace() const { return _bucketSpace; } + document::Bucket getBucket() const override; const vespalib::string & getLibraryName() const { return _libName; } const vespalib::string & getInstanceId() const { return _instanceId; } const vespalib::string & getControlDestination() const diff --git a/vespabase/src/common-env.sh b/vespabase/src/common-env.sh index 8cd296b61c2..76f5d69b3a4 100755 --- a/vespabase/src/common-env.sh +++ b/vespabase/src/common-env.sh @@ -111,6 +111,7 @@ populate_environment PATH=$VESPA_HOME/bin64:$VESPA_HOME/bin:/usr/local/bin:/usr/X11R6/bin:/sbin:/bin:/usr/sbin:/usr/bin export LD_LIBRARY_PATH=$VESPA_HOME/lib64 +export MALLOC_ARENA_MAX=1 # how to find the "java" program? # should be available in $VESPA_HOME/bin or JAVA_HOME diff --git a/vespajlib/src/main/java/com/yahoo/tensor/functions/Concat.java b/vespajlib/src/main/java/com/yahoo/tensor/functions/Concat.java index 401f9a10eda..1dbb94fdb20 100644 --- a/vespajlib/src/main/java/com/yahoo/tensor/functions/Concat.java +++ b/vespajlib/src/main/java/com/yahoo/tensor/functions/Concat.java @@ -134,9 +134,7 @@ public class Concat extends PrimitiveTensorFunction { if (currentDimension.equals(concatDimension)) concatSizes.set(i, aSize + bSize); else if (aSize != 0 && bSize != 0 && aSize!=bSize ) - throw new IllegalArgumentException("Dimension " + currentDimension + " must be of the same size when " + - "concatenating " + a.type() + " and " + b.type() + " along dimension " + - concatDimension + ", but was " + aSize + " and " + bSize); + concatSizes.set(i, Math.min(aSize, bSize)); else concatSizes.set(i, Math.max(aSize, bSize)); } diff --git a/vespajlib/src/test/java/com/yahoo/tensor/functions/ConcatTestCase.java b/vespajlib/src/test/java/com/yahoo/tensor/functions/ConcatTestCase.java index a653ef97734..7e1f292eb7b 100644 --- a/vespajlib/src/test/java/com/yahoo/tensor/functions/ConcatTestCase.java +++ b/vespajlib/src/test/java/com/yahoo/tensor/functions/ConcatTestCase.java @@ -43,12 +43,7 @@ public class ConcatTestCase { Tensor a = Tensor.from("tensor(x[]):{ {x:0}:1, {x:1}:2 }"); Tensor b = Tensor.from("tensor(x[]):{ {x:0}:4, {x:1}:5, {x:2}:6 }"); assertEquals(Tensor.from("tensor(x[5]):{ {x:0}:1, {x:1}:2, {x:2}:4, {x:3}:5, {x:4}:6 }"), a.concat(b, "x")); - try { - a.concat(b, "y"); - fail("Expected exception"); - } catch (IllegalArgumentException expected) { - // success - } + assertEquals(Tensor.from("tensor(x[2],y[2]):{ {x:0,y:0}:1, {x:1,y:0}:2, {x:0,y:1}:4, {x:1,y:1}:5 }"), a.concat(b, "y")); } @Test |