diff options
71 files changed, 1548 insertions, 629 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 index beff50b52c6..3774eb015ed 100644 --- 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 @@ -2,28 +2,39 @@ package com.yahoo.vespa.hosted.athenz.instanceproviderservice; import com.google.inject.Inject; +import com.yahoo.cloud.config.ConfigserverConfig; import com.yahoo.component.AbstractComponent; import com.yahoo.config.provision.Zone; +import com.yahoo.container.jdisc.athenz.AthenzIdentityProvider; import com.yahoo.jdisc.http.ssl.SslKeyStoreConfigurator; import com.yahoo.jdisc.http.ssl.SslKeyStoreContext; import com.yahoo.log.LogLevel; +import com.yahoo.vespa.defaults.Defaults; import com.yahoo.vespa.hosted.athenz.instanceproviderservice.config.AthenzProviderServiceConfig; import com.yahoo.vespa.hosted.athenz.instanceproviderservice.impl.AthenzCertificateClient; +import java.io.BufferedInputStream; +import java.io.BufferedOutputStream; +import java.io.File; +import java.io.FileInputStream; +import java.io.FileOutputStream; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.GeneralSecurityException; import java.security.KeyStore; import java.security.KeyStoreException; -import java.security.NoSuchAlgorithmException; import java.security.PrivateKey; import java.security.cert.Certificate; -import java.security.cert.CertificateException; import java.security.cert.X509Certificate; import java.time.Duration; import java.time.Instant; +import java.util.Optional; 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; @@ -31,44 +42,78 @@ import static com.yahoo.vespa.hosted.athenz.instanceproviderservice.impl.Utils.g /** * @author bjorncs */ -// TODO Cache certificate on disk @SuppressWarnings("unused") // Component injected into Jetty connector factory 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 static final String CERTIFICATE_ALIAS = "athenz"; private static final String CERTIFICATE_PASSWORD = "athenz"; + private static final Duration EXPIRATION_MARGIN = Duration.ofHours(6); private final ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor(); private final AthenzCertificateClient certificateClient; private final KeyProvider keyProvider; private final AthenzProviderServiceConfig.Zones zoneConfig; - private final AtomicBoolean alreadyConfigured = new AtomicBoolean(); + private final Duration updatePeriod; + private final Path keystoreCachePath; private volatile KeyStore currentKeyStore; @Inject - public AthenzSslKeyStoreConfigurator(KeyProvider keyProvider, + public AthenzSslKeyStoreConfigurator(AthenzIdentityProvider bootstrapIdentity, + KeyProvider keyProvider, AthenzProviderServiceConfig config, - Zone zone) { + Zone zone, + ConfigserverConfig configserverConfig) { AthenzProviderServiceConfig.Zones zoneConfig = getZoneConfig(config, zone); - this.certificateClient = new AthenzCertificateClient(config, zoneConfig); + Path keystoreCachePath = createKeystoreCachePath(configserverConfig); + AthenzCertificateClient certificateClient = new AthenzCertificateClient(bootstrapIdentity, config, zoneConfig); + Duration updatePeriod = Duration.ofDays(config.updatePeriodDays()); + this.certificateClient = certificateClient; this.keyProvider = keyProvider; this.zoneConfig = zoneConfig; - this.currentKeyStore = downloadCertificate(keyProvider, certificateClient, zoneConfig); + this.currentKeyStore = initializeKeystore(keyProvider, certificateClient, zoneConfig, keystoreCachePath, updatePeriod); + this.updatePeriod = updatePeriod; + this.keystoreCachePath = keystoreCachePath; + } + + private static KeyStore initializeKeystore(KeyProvider keyProvider, + AthenzCertificateClient certificateClient, + AthenzProviderServiceConfig.Zones zoneConfig, + Path keystoreCachePath, + Duration updatePeriod) { + return tryReadKeystoreFile(keystoreCachePath.toFile(), updatePeriod) + .orElseGet(() -> downloadCertificate(keyProvider, certificateClient, zoneConfig, keystoreCachePath)); + } + + private static Optional<KeyStore> tryReadKeystoreFile(File certificateFile, Duration updatePeriod) { + try { + if (!certificateFile.exists()) return Optional.empty(); + KeyStore keyStore = KeyStore.getInstance("JKS"); + try (InputStream in = new BufferedInputStream(new FileInputStream(certificateFile))) { + keyStore.load(in, new char[0]); + } + Instant minimumExpiration = Instant.now().plus(updatePeriod).plus(EXPIRATION_MARGIN); + boolean isExpired = getCertificateExpiry(keyStore).isBefore(minimumExpiration); + if (isExpired) return Optional.empty(); + return Optional.of(keyStore); + } catch (IOException | GeneralSecurityException e) { + log.log(LogLevel.ERROR, "Failed to read keystore from disk: " + e.getMessage(), e); + return Optional.empty(); + } + } + + private static Path createKeystoreCachePath(ConfigserverConfig configserverConfig) { + return Paths.get( + Defaults.getDefaults().underVespaHome(configserverConfig.configServerDBDir()), + "server-x509-athenz-cert.jks"); } @Override public void configure(SslKeyStoreContext sslKeyStoreContext) { - if (alreadyConfigured.getAndSet(true)) { // For debugging purpose of SslKeyStoreConfigurator interface - throw new IllegalStateException("Already configured. configure() can only be called once."); - } sslKeyStoreContext.updateKeyStore(currentKeyStore, CERTIFICATE_PASSWORD); scheduler.scheduleAtFixedRate(new AthenzCertificateUpdater(sslKeyStoreContext), - CERTIFICATE_UPDATE_PERIOD.toMinutes()/*initial delay*/, - CERTIFICATE_UPDATE_PERIOD.toMinutes(), - TimeUnit.MINUTES); + updatePeriod.toDays()/*initial delay*/, + updatePeriod.toDays(), + TimeUnit.DAYS); } @Override @@ -81,36 +126,42 @@ public class AthenzSslKeyStoreConfigurator extends AbstractComponent implements } } - Instant getKeyStoreExpiry() throws KeyStoreException { - X509Certificate certificate = (X509Certificate) currentKeyStore.getCertificate(CERTIFICATE_ALIAS); - return certificate.getNotAfter().toInstant(); + Instant getCertificateExpiry() throws KeyStoreException { + return getCertificateExpiry(currentKeyStore); } + private static Instant getCertificateExpiry(KeyStore keyStore) throws KeyStoreException { + X509Certificate certificate = (X509Certificate) keyStore.getCertificate(CERTIFICATE_ALIAS); + return certificate.getNotAfter().toInstant(); + } private static KeyStore downloadCertificate(KeyProvider keyProvider, AthenzCertificateClient certificateClient, - AthenzProviderServiceConfig.Zones zoneConfig) { + AthenzProviderServiceConfig.Zones zoneConfig, + Path keystoreCachePath) { try { PrivateKey privateKey = keyProvider.getPrivateKey(zoneConfig.secretVersion()); - X509Certificate certificate = certificateClient.updateCertificate(privateKey, CERTIFICATE_EXPIRY_TIME); - verifyActualExpiry(certificate); + X509Certificate certificate = certificateClient.updateCertificate(privateKey); + Instant expirationTime = certificate.getNotAfter().toInstant(); + Duration expiry = Duration.between(certificate.getNotBefore().toInstant(), expirationTime); + log.log(LogLevel.INFO, String.format("Got Athenz x509 certificate with expiry %s (expires %s)", expiry, expirationTime)); KeyStore keyStore = KeyStore.getInstance("JKS"); keyStore.load(null); keyStore.setKeyEntry( CERTIFICATE_ALIAS, privateKey, CERTIFICATE_PASSWORD.toCharArray(), new Certificate[]{certificate}); + tryWriteKeystore(keyStore, keystoreCachePath); return keyStore; - } catch (IOException | NoSuchAlgorithmException | CertificateException | KeyStoreException e) { + } catch (IOException | GeneralSecurityException e) { throw new RuntimeException(e); } } - private static void verifyActualExpiry(X509Certificate certificate) { - Duration actualExpiry = - Duration.between(certificate.getNotBefore().toInstant(), certificate.getNotAfter().toInstant()); - if (CERTIFICATE_EXPIRY_TIME.compareTo(actualExpiry) > 0) { - log.log(LogLevel.WARNING, - String.format("Expected expiry %s, got %s", CERTIFICATE_EXPIRY_TIME, actualExpiry)); + private static void tryWriteKeystore(KeyStore keyStore, Path keystoreCachePath) { + try (OutputStream out = new BufferedOutputStream(new FileOutputStream(keystoreCachePath.toFile()))) { + keyStore.store(out, new char[0]); + } catch (IOException | GeneralSecurityException e) { + log.log(LogLevel.ERROR, "Failed to write keystore to disk: " + e.getMessage(), e); } } @@ -126,7 +177,7 @@ public class AthenzSslKeyStoreConfigurator extends AbstractComponent implements public void run() { try { log.log(LogLevel.INFO, "Updating Athenz certificate from ZTS"); - currentKeyStore = downloadCertificate(keyProvider, certificateClient, zoneConfig); + currentKeyStore = downloadCertificate(keyProvider, certificateClient, zoneConfig, keystoreCachePath); sslKeyStoreContext.updateKeyStore(currentKeyStore, CERTIFICATE_PASSWORD); log.log(LogLevel.INFO, "Athenz certificate reload successfully completed"); } catch (Throwable e) { diff --git a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/CertificateExpiryMetricUpdater.java b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/CertificateExpiryMetricUpdater.java index cf734facf34..2d80b15c7ec 100644 --- a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/CertificateExpiryMetricUpdater.java +++ b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/CertificateExpiryMetricUpdater.java @@ -59,7 +59,7 @@ public class CertificateExpiryMetricUpdater extends AbstractComponent { Instant now = Instant.now(); try { - Duration keyStoreExpiry = Duration.between(now, keyStoreConfigurator.getKeyStoreExpiry()); + Duration keyStoreExpiry = Duration.between(now, keyStoreConfigurator.getCertificateExpiry()); metric.set(ATHENZ_CONFIGSERVER_CERT_METRIC_NAME, keyStoreExpiry.getSeconds(), null); } catch (KeyStoreException e) { logger.log(Level.WARNING, "Failed to update key store expiry metric", e); diff --git a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentGenerator.java b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentGenerator.java index 4dd6881c07e..eb1c6b09f0f 100644 --- a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentGenerator.java +++ b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentGenerator.java @@ -38,7 +38,7 @@ public class IdentityDocumentGenerator { this.nodeRepository = nodeRepository; this.zone = zone; this.keyProvider = keyProvider; - this.dnsSuffix = config.certDnsSuffix(); + this.dnsSuffix = zoneConfig.certDnsSuffix(); this.providerService = zoneConfig.serviceName(); this.ztsUrl = config.ztsUrl(); this.providerDomain = zoneConfig.domain(); diff --git a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/impl/AthenzCertificateClient.java b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/impl/AthenzCertificateClient.java index c6aee673f9c..62c7038a265 100644 --- a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/impl/AthenzCertificateClient.java +++ b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/impl/AthenzCertificateClient.java @@ -1,18 +1,15 @@ // 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.impl; -import com.yahoo.athenz.auth.impl.PrincipalAuthority; -import com.yahoo.athenz.auth.impl.SimpleServiceIdentityProvider; import com.yahoo.athenz.auth.util.Crypto; import com.yahoo.athenz.zts.InstanceRefreshRequest; import com.yahoo.athenz.zts.ZTSClient; +import com.yahoo.container.jdisc.athenz.AthenzIdentityProvider; import com.yahoo.vespa.hosted.athenz.instanceproviderservice.config.AthenzProviderServiceConfig; +import javax.net.ssl.SSLContext; import java.security.PrivateKey; import java.security.cert.X509Certificate; -import java.time.temporal.ChronoUnit; -import java.time.temporal.TemporalAmount; -import java.util.concurrent.TimeUnit; /** * @author bjorncs @@ -20,41 +17,27 @@ import java.util.concurrent.TimeUnit; public class AthenzCertificateClient { private final AthenzProviderServiceConfig config; - private final AthenzPrincipalAuthority authority; private final AthenzProviderServiceConfig.Zones zoneConfig; + private final AthenzIdentityProvider bootstrapIdentity; - public AthenzCertificateClient(AthenzProviderServiceConfig config, AthenzProviderServiceConfig.Zones zoneConfig) { + public AthenzCertificateClient(AthenzIdentityProvider bootstrapIdentity, + AthenzProviderServiceConfig config, + AthenzProviderServiceConfig.Zones zoneConfig) { + this.bootstrapIdentity = bootstrapIdentity; this.config = config; - this.authority = new AthenzPrincipalAuthority(config.athenzPrincipalHeaderName()); this.zoneConfig = zoneConfig; } - public X509Certificate updateCertificate(PrivateKey privateKey, TemporalAmount expiryTime) { - SimpleServiceIdentityProvider identityProvider = new SimpleServiceIdentityProvider( - authority, zoneConfig.domain(), zoneConfig.serviceName(), - privateKey, Integer.toString(zoneConfig.secretVersion()), TimeUnit.MINUTES.toSeconds(10)); - ZTSClient ztsClient = new ZTSClient( - config.ztsUrl(), zoneConfig.domain(), zoneConfig.serviceName(), identityProvider); + public X509Certificate updateCertificate(PrivateKey privateKey) { + SSLContext bootstrapSslContext = bootstrapIdentity.getIdentitySslContext(); + ZTSClient ztsClient = new ZTSClient(config.ztsUrl(), bootstrapSslContext); InstanceRefreshRequest req = ZTSClient.generateInstanceRefreshRequest( - zoneConfig.domain(), zoneConfig.serviceName(), privateKey, - config.certDnsSuffix(), (int)expiryTime.get(ChronoUnit.SECONDS)); + zoneConfig.domain(), zoneConfig.serviceName(), privateKey, zoneConfig.certDnsSuffix(), /*expiryTime*/0); + req.setKeyId(Integer.toString(zoneConfig.secretVersion())); String pemEncoded = ztsClient.postInstanceRefreshRequest(zoneConfig.domain(), zoneConfig.serviceName(), req) .getCertificate(); return Crypto.loadX509Certificate(pemEncoded); } - private static class AthenzPrincipalAuthority extends PrincipalAuthority { - private final String headerName; - - public AthenzPrincipalAuthority(String headerName) { - this.headerName = headerName; - } - - @Override - public String getHeader() { - return headerName; - } - } - } diff --git a/athenz-identity-provider-service/src/main/resources/configdefinitions/athenz-provider-service.def b/athenz-identity-provider-service/src/main/resources/configdefinitions/athenz-provider-service.def index 21f2aea6ab0..d92e0b685cc 100644 --- a/athenz-identity-provider-service/src/main/resources/configdefinitions/athenz-provider-service.def +++ b/athenz-identity-provider-service/src/main/resources/configdefinitions/athenz-provider-service.def @@ -13,14 +13,14 @@ zones{}.secretName string # Secret version zones{}.secretVersion int -# Athenz principal authority header name -athenzPrincipalHeaderName string default="Athenz-Principal-Auth" +# Certificate DNS suffix +zones{}.certDnsSuffix string # Athenz ZTS server url ztsUrl string -# Certificate DNS suffix -certDnsSuffix string - # Path to Athenz CA JKS trust store athenzCaTrustStore string + +# Period between certificate updates +updatePeriodDays int default=1 diff --git a/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/TestUtils.java b/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/TestUtils.java index da2bf929e82..5ae4b9f9bc5 100644 --- a/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/TestUtils.java +++ b/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/TestUtils.java @@ -19,13 +19,12 @@ public class TestUtils { .serviceName(service) .secretVersion(0) .domain(domain) + .certDnsSuffix(dnsSuffix) .secretName("s3cr3t"); return new AthenzProviderServiceConfig( new AthenzProviderServiceConfig.Builder() .zones(ImmutableMap.of(zone.environment().value() + "." + zone.region().value(), zoneConfig)) - .certDnsSuffix(dnsSuffix) .ztsUrl("localhost/zts") - .athenzPrincipalHeaderName("Athenz-Principal-Auth") .athenzCaTrustStore("/dummy/path/to/athenz-ca.jks")); } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterStatsAggregator.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterStatsAggregator.java index 3b8cb14e977..ed2ca32dab0 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterStatsAggregator.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ClusterStatsAggregator.java @@ -27,6 +27,7 @@ import java.util.*; public class ClusterStatsAggregator { private final Set<Integer> distributors; + private final Set<Integer> storageNodes; private final Set<Integer> nonUpdatedDistributors; // Maps the distributor node index to a map of content node index to the @@ -40,14 +41,26 @@ public class ClusterStatsAggregator { ClusterStatsAggregator(Set<Integer> distributors, Set<Integer> storageNodes) { this.distributors = distributors; + this.storageNodes = storageNodes; nonUpdatedDistributors = new HashSet<>(distributors); aggregatedStats = new ContentClusterStats(storageNodes); } - ContentClusterStats getAggregatedStats() { + public ContentClusterStats getAggregatedStats() { return aggregatedStats; } + public ContentNodeStats getAggregatedStatsForDistributor(int distributorIndex) { + ContentNodeStats result = new ContentNodeStats(distributorIndex); + ContentClusterStats distributorStats = distributorToStats.get(distributorIndex); + if (distributorStats != null) { + for (Iterator<ContentNodeStats> itr = distributorStats.iterator(); itr.hasNext(); ) { + result.add(itr.next()); + } + } + return result; + } + boolean hasUpdatesFromAllDistributors() { return nonUpdatedDistributors.isEmpty(); } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java index 74ddd941afb..27612bb8b7d 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentCluster.java @@ -46,6 +46,7 @@ public class ContentCluster { final StringBuilder sb, final Timer timer, final ClusterState state, + final ClusterStatsAggregator statsAggregator, final Distribution distribution, final FleetControllerOptions options, final EventLog eventLog) { @@ -71,6 +72,7 @@ public class ContentCluster { distributorNodeInfoByIndex, timer, state, + statsAggregator, options.maxPrematureCrashes, eventLog, clusterName, diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentClusterStats.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentClusterStats.java index 2698b079073..bc476923b23 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentClusterStats.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/ContentClusterStats.java @@ -29,11 +29,11 @@ public class ContentClusterStats implements Iterable<ContentNodeStats> { return mapToNodeStats.values().iterator(); } - ContentNodeStats getContentNode(Integer index) { + public ContentNodeStats getContentNode(Integer index) { return mapToNodeStats.get(index); } - int size() { + public int size() { return mapToNodeStats.size(); } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateVersionTracker.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateVersionTracker.java index 2063bdb24c9..1a57975c4bc 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateVersionTracker.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/StateVersionTracker.java @@ -78,6 +78,10 @@ public class StateVersionTracker { return currentClusterState.getBaselineClusterState(); } + public ClusterStatsAggregator getAggregatedClusterStats() { + return clusterStateView.getStatsAggregator(); + } + public ClusterStateBundle getVersionedClusterStateBundle() { return currentClusterState; } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/LegacyIndexPageRequestHandler.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/LegacyIndexPageRequestHandler.java index 2ced8648ced..c7f83743708 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/LegacyIndexPageRequestHandler.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/LegacyIndexPageRequestHandler.java @@ -65,6 +65,7 @@ public class LegacyIndexPageRequestHandler implements StatusPageServer.RequestHa content, timer, stateVersionTracker.getVersionedClusterState(), + stateVersionTracker.getAggregatedClusterStats(), data.getOptions().storageDistribution, data.getOptions(), eventLog diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/statuspage/VdsClusterHtmlRendrer.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/statuspage/VdsClusterHtmlRendrer.java index 2ed98855121..eb377718614 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/statuspage/VdsClusterHtmlRendrer.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/status/statuspage/VdsClusterHtmlRendrer.java @@ -1,14 +1,12 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.core.status.statuspage; +import com.yahoo.document.FixedBucketSpaces; import com.yahoo.vdslib.state.ClusterState; import com.yahoo.vdslib.state.NodeState; import com.yahoo.vdslib.state.NodeType; import com.yahoo.vdslib.state.State; -import com.yahoo.vespa.clustercontroller.core.EventLog; -import com.yahoo.vespa.clustercontroller.core.NodeInfo; -import com.yahoo.vespa.clustercontroller.core.RealTimer; -import com.yahoo.vespa.clustercontroller.core.Timer; +import com.yahoo.vespa.clustercontroller.core.*; import java.util.ArrayList; import java.util.HashMap; @@ -30,6 +28,9 @@ public class VdsClusterHtmlRendrer { private final HtmlTable.CellProperties headerProperties; private final StringBuilder contentBuilder = new StringBuilder(); private final static String TAG_NOT_SET = "not set"; + private final static HtmlTable.CellProperties WARNING_PROPERTY = new HtmlTable.CellProperties().setBackgroundColor(0xffffc0); + private final static HtmlTable.CellProperties ERROR_PROPERTY = new HtmlTable.CellProperties().setBackgroundColor(0xffc0c0); + private final static HtmlTable.CellProperties CENTERED_PROPERTY = new HtmlTable.CellProperties().align(HtmlTable.Orientation.CENTER); Table(final String clusterName, final int slobrokGenerationCount) { table.getTableProperties().align(HtmlTable.Orientation.RIGHT).setBackgroundColor(0xc0ffc0); @@ -38,8 +39,8 @@ public class VdsClusterHtmlRendrer { table.getColProperties(2).align(HtmlTable.Orientation.LEFT); table.getColProperties(3).align(HtmlTable.Orientation.LEFT); table.getColProperties(7).align(HtmlTable.Orientation.LEFT); - table.getColProperties(12).align(HtmlTable.Orientation.LEFT); - for (int i = 4; i < 13; ++i) table.getColProperties(i).allowLineBreaks(false); + table.getColProperties(14).align(HtmlTable.Orientation.LEFT); + for (int i = 4; i < 15; ++i) table.getColProperties(i).allowLineBreaks(false); headerProperties = new HtmlTable.CellProperties() .setBackgroundColor(0xffffff) .align(HtmlTable.Orientation.CENTER); @@ -64,6 +65,7 @@ public class VdsClusterHtmlRendrer { final TreeMap<Integer, NodeInfo> distributorNodeInfos, final Timer timer, final ClusterState state, + final ClusterStatsAggregator statsAggregator, final int maxPrematureCrashes, final EventLog eventLog, final String pathPrefix, @@ -75,6 +77,7 @@ public class VdsClusterHtmlRendrer { NodeType.STORAGE, timer, state, + statsAggregator, maxPrematureCrashes, eventLog, pathPrefix, @@ -84,6 +87,7 @@ public class VdsClusterHtmlRendrer { NodeType.DISTRIBUTOR, timer, state, + statsAggregator, maxPrematureCrashes, eventLog, pathPrefix, @@ -134,12 +138,16 @@ public class VdsClusterHtmlRendrer { .addCell(new HtmlTable.Cell("SSV<sup>4)</sup>")) .addCell(new HtmlTable.Cell("PC<sup>5)</sup>")) .addCell(new HtmlTable.Cell("ELW<sup>6)</sup>")) + .addCell(new HtmlTable.Cell("Buckets pending") + .addProperties(new HtmlTable.CellProperties().setColSpan(2).setRowSpan(1))) .addCell(new HtmlTable.Cell("Start Time")) .addCell(new HtmlTable.Cell("RPC Address"))); table.addRow(new HtmlTable.Row().setHeaderRow().addProperties(headerProperties) .addCell(new HtmlTable.Cell("Reported")) .addCell(new HtmlTable.Cell("Wanted")) - .addCell(new HtmlTable.Cell("System"))); + .addCell(new HtmlTable.Cell("System")) + .addCell(new HtmlTable.Cell(FixedBucketSpaces.defaultSpace())) + .addCell(new HtmlTable.Cell(FixedBucketSpaces.globalSpace()))); } private void renderNodesOneType( @@ -147,6 +155,7 @@ public class VdsClusterHtmlRendrer { final NodeType nodeType, final Timer timer, final ClusterState state, + final ClusterStatsAggregator statsAggregator, final int maxPrematureCrashes, final EventLog eventLog, final String pathPrefix, @@ -156,9 +165,6 @@ public class VdsClusterHtmlRendrer { addTableHeader(name, nodeType); for (final NodeInfo nodeInfo : nodeInfos.values()) { HtmlTable.Row row = new HtmlTable.Row(); - HtmlTable.CellProperties warning = new HtmlTable.CellProperties().setBackgroundColor(0xffffc0); - HtmlTable.CellProperties error = new HtmlTable.CellProperties().setBackgroundColor(0xffc0c0); - HtmlTable.CellProperties centered = new HtmlTable.CellProperties().align(HtmlTable.Orientation.CENTER); // Add node index row.addCell(new HtmlTable.Cell("<a href=\"" + pathPrefix + "/node=" + nodeInfo.getNode() @@ -168,18 +174,18 @@ public class VdsClusterHtmlRendrer { NodeState reportedState = nodeInfo.getReportedState().clone().setStartTimestamp(0); row.addCell(new HtmlTable.Cell(HtmlTable.escape(reportedState.toString(true)))); if (!nodeInfo.getReportedState().getState().equals(State.UP)) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } // Add wanted state if (nodeInfo.getWantedState() == null || nodeInfo.getWantedState().getState().equals(State.UP)) { - row.addCell(new HtmlTable.Cell("-").addProperties(centered)); + row.addCell(new HtmlTable.Cell("-").addProperties(CENTERED_PROPERTY)); } else { row.addCell(new HtmlTable.Cell(HtmlTable.escape(nodeInfo.getWantedState().toString(true)))); if (nodeInfo.getWantedState().toString(true).indexOf("Disabled by fleet controller") != -1) { - row.getLastCell().addProperties(error); + row.getLastCell().addProperties(ERROR_PROPERTY); } else { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } } @@ -188,13 +194,13 @@ public class VdsClusterHtmlRendrer { if (state.getClusterState().oneOf("uir")) { row.addCell(new HtmlTable.Cell(HtmlTable.escape(ns.toString(true)))); if (ns.getState().equals(State.DOWN)) { - row.getLastCell().addProperties(error); + row.getLastCell().addProperties(ERROR_PROPERTY); } else if (ns.getState().oneOf("mi")) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } } else { row.addCell(new HtmlTable.Cell("Cluster " + - state.getClusterState().name().toLowerCase()).addProperties(error)); + state.getClusterState().name().toLowerCase()).addProperties(ERROR_PROPERTY)); } // Add build tag version. @@ -204,7 +210,7 @@ public class VdsClusterHtmlRendrer { : TAG_NOT_SET; row.addCell(new HtmlTable.Cell(buildTagText)); if (! dominantVtag.equals(nodeInfo.getVtag())) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } // Add failed connection attempt count @@ -212,22 +218,22 @@ public class VdsClusterHtmlRendrer { long timeSinceContact = nodeInfo.getTimeOfFirstFailingConnectionAttempt() == 0 ? 0 : currentTime - nodeInfo.getTimeOfFirstFailingConnectionAttempt(); if (timeSinceContact > 60 * 1000) { - row.getLastCell().addProperties(error); + row.getLastCell().addProperties(ERROR_PROPERTY); } else if (nodeInfo.getConnectionAttemptCount() > 0) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } // Add time since first failing row.addCell(new HtmlTable.Cell((timeSinceContact / 1000) + " s")); if (timeSinceContact > 60 * 1000) { - row.getLastCell().addProperties(error); + row.getLastCell().addProperties(ERROR_PROPERTY); } else if (nodeInfo.getConnectionAttemptCount() > 0) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } // State pending time if (nodeInfo.getLatestNodeStateRequestTime() == null) { - row.addCell(new HtmlTable.Cell("-").addProperties(centered)); + row.addCell(new HtmlTable.Cell("-").addProperties(CENTERED_PROPERTY)); } else { row.addCell(new HtmlTable.Cell(HtmlTable.escape(RealTimer.printDuration( currentTime - nodeInfo.getLatestNodeStateRequestTime())))); @@ -236,17 +242,17 @@ public class VdsClusterHtmlRendrer { // System state version row.addCell(new HtmlTable.Cell("" + nodeInfo.getSystemStateVersionAcknowledged())); if (nodeInfo.getSystemStateVersionAcknowledged() < state.getVersion() - 2) { - row.getLastCell().addProperties(error); + row.getLastCell().addProperties(ERROR_PROPERTY); } else if (nodeInfo.getSystemStateVersionAcknowledged() < state.getVersion()) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } // Premature crashes row.addCell(new HtmlTable.Cell("" + nodeInfo.getPrematureCrashCount())); if (nodeInfo.getPrematureCrashCount() >= maxPrematureCrashes) { - row.getLastCell().addProperties(error); + row.getLastCell().addProperties(ERROR_PROPERTY); } else if (nodeInfo.getPrematureCrashCount() > 0) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } // Events last week @@ -254,14 +260,23 @@ public class VdsClusterHtmlRendrer { currentTime - eventLog.getRecentTimePeriod()); row.addCell(new HtmlTable.Cell("" + nodeEvents)); if (nodeEvents > 20) { - row.getLastCell().addProperties(error); + row.getLastCell().addProperties(ERROR_PROPERTY); } else if (nodeEvents > 3) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); + } + + // Buckets pending ('default' and 'global' spaces) + if (nodeType.equals(NodeType.STORAGE)) { + addBucketsPending(row, getStatsForContentNode(statsAggregator, nodeInfo, FixedBucketSpaces.defaultSpace())); + addBucketsPending(row, getStatsForContentNode(statsAggregator, nodeInfo, FixedBucketSpaces.globalSpace())); + } else { + addBucketsPending(row, getStatsForDistributorNode(statsAggregator, nodeInfo, FixedBucketSpaces.defaultSpace())); + addBucketsPending(row, getStatsForDistributorNode(statsAggregator, nodeInfo, FixedBucketSpaces.globalSpace())); } // Start time if (nodeInfo.getStartTimestamp() == 0) { - row.addCell(new HtmlTable.Cell("-").addProperties(error).addProperties(centered)); + row.addCell(new HtmlTable.Cell("-").addProperties(ERROR_PROPERTY).addProperties(CENTERED_PROPERTY)); } else { String startTime = RealTimer.printDateNoMilliSeconds( 1000 * nodeInfo.getStartTimestamp(), utcTimeZone); @@ -270,16 +285,50 @@ public class VdsClusterHtmlRendrer { // RPC address if (nodeInfo.getRpcAddress() == null) { - row.addCell(new HtmlTable.Cell("-").addProperties(error)); + row.addCell(new HtmlTable.Cell("-").addProperties(ERROR_PROPERTY)); } else { row.addCell(new HtmlTable.Cell(HtmlTable.escape(nodeInfo.getRpcAddress()))); if (nodeInfo.isRpcAddressOutdated()) { - row.getLastCell().addProperties(warning); + row.getLastCell().addProperties(WARNING_PROPERTY); } } table.addRow(row); } } + + private static ContentNodeStats.BucketSpaceStats getStatsForContentNode(ClusterStatsAggregator statsAggregator, + NodeInfo nodeInfo, + String bucketSpace) { + ContentNodeStats nodeStats = statsAggregator.getAggregatedStats().getContentNode(nodeInfo.getNodeIndex()); + if (nodeStats != null) { + return nodeStats.getBucketSpace(bucketSpace); + } + return null; + } + + private static ContentNodeStats.BucketSpaceStats getStatsForDistributorNode(ClusterStatsAggregator statsAggregator, + NodeInfo nodeInfo, + String bucketSpace) { + ContentNodeStats nodeStats = statsAggregator.getAggregatedStatsForDistributor(nodeInfo.getNodeIndex()); + return nodeStats.getBucketSpace(bucketSpace); + } + + private static void addBucketsPending(HtmlTable.Row row, ContentNodeStats.BucketSpaceStats bucketSpaceStats) { + if (bucketSpaceStats != null) { + long bucketsPending = bucketSpaceStats.getBucketsPending(); + String cellValue = String.valueOf(bucketsPending); + if (!bucketSpaceStats.valid()) { + cellValue += "?"; + } + row.addCell(new HtmlTable.Cell(cellValue)); + if (bucketsPending > 0 || !bucketSpaceStats.valid()) { + row.getLastCell().addProperties(WARNING_PROPERTY); + } + } else { + row.addCell(new HtmlTable.Cell("-").addProperties(CENTERED_PROPERTY)); + } + } + private void addFooter(final StringBuilder contentBuilder, final long stableStateTimePeriode) { contentBuilder.append("<font size=\"-1\">\n") .append("1) FC - Failed connections - We have tried to connect to the nodes this many times " + diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterStatsAggregatorTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterStatsAggregatorTest.java index ac6417d0077..90c9700e001 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterStatsAggregatorTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterStatsAggregatorTest.java @@ -18,24 +18,50 @@ public class ClusterStatsAggregatorTest { private static class Fixture { private ClusterStatsAggregator aggregator; + public Fixture(Set<Integer> distributorNodes, Set<Integer> contentNodes) { aggregator = new ClusterStatsAggregator(distributorNodes, contentNodes); } + public void update(int distributorIndex, ContentClusterStatsBuilder clusterStats) { aggregator.updateForDistributor(distributorIndex, clusterStats.build()); } + public void verify(ContentClusterStatsBuilder expectedStats) { assertEquals(expectedStats.build(), aggregator.getAggregatedStats()); } + + public void verify(int distributorIndex, ContentNodeStatsBuilder expectedStats) { + assertEquals(expectedStats.build(), aggregator.getAggregatedStatsForDistributor(distributorIndex)); + } + public boolean hasUpdatesFromAllDistributors() { return aggregator.hasUpdatesFromAllDistributors(); } + public boolean mayHaveBucketsPendingInGlobalSpace() { return aggregator.mayHaveBucketsPendingInGlobalSpace(); } } + private static class FourNodesFixture extends Fixture { + public FourNodesFixture() { + super(distributorNodes(1, 2), contentNodes(3, 4)); + + update(1, new ContentClusterStatsBuilder() + .add(3, "default", 10, 1) + .add(3, "global", 11, 2) + .add(4, "default", 12, 3) + .add(4, "global", 13, 4)); + update(2, new ContentClusterStatsBuilder() + .add(3, "default", 14, 5) + .add(3, "global", 15, 6) + .add(4, "default", 16, 7) + .add(4, "global", 17, 8)); + } + } + private static Set<Integer> distributorNodes(Integer... indices) { return Sets.newHashSet(indices); } @@ -56,18 +82,8 @@ public class ClusterStatsAggregatorTest { @Test public void aggregator_handles_updates_to_multiple_distributors_and_content_nodes() { - Fixture f = new Fixture(distributorNodes(1, 2), contentNodes(3, 4)); + Fixture f = new FourNodesFixture(); - f.update(1, new ContentClusterStatsBuilder() - .add(3, "default", 10, 1) - .add(3, "global", 11, 2) - .add(4, "default", 12, 3) - .add(4, "global", 13, 4)); - f.update(2, new ContentClusterStatsBuilder() - .add(3, "default", 14, 5) - .add(3, "global", 15, 6) - .add(4, "default", 16, 7) - .add(4, "global", 17, 8)); f.verify(new ContentClusterStatsBuilder() .add(3, "default", 10 + 14, 1 + 5) .add(3, "global", 11 + 15, 2 + 6) @@ -154,4 +170,17 @@ public class ClusterStatsAggregatorTest { assertFalse(f.mayHaveBucketsPendingInGlobalSpace()); } + @Test + public void aggregator_can_provide_aggregated_stats_per_distributor() { + Fixture f = new FourNodesFixture(); + + f.verify(1, ContentNodeStatsBuilder.forNode(1) + .add("default", 10 + 12, 1 + 3) + .add("global", 11 + 13, 2 + 4)); + + f.verify(2, ContentNodeStatsBuilder.forNode(2) + .add("default", 14 + 16, 5 + 7) + .add("global", 15 + 17, 6 + 8)); + } + } diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentClusterHtmlRendrerTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentClusterHtmlRendrerTest.java index 24d387f65de..6dc260d6bf3 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentClusterHtmlRendrerTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentClusterHtmlRendrerTest.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.core; +import com.google.common.collect.Sets; import com.yahoo.vdslib.state.ClusterState; import com.yahoo.vespa.clustercontroller.core.hostinfo.HostInfo; import com.yahoo.vespa.clustercontroller.core.status.statuspage.VdsClusterHtmlRendrer; @@ -48,12 +49,14 @@ public class ContentClusterHtmlRendrerTest { distributorNodeInfoByIndex.put(x, nodeInfo); } storageNodeInfoByIndex.put(2, new StorageNodeInfo(contentCluster, 2, false, "storage" + 2, null)); + ClusterStatsAggregator statsAggregator = new ClusterStatsAggregator(Sets.newHashSet(2), Sets.newHashSet(2)); table.renderNodes( storageNodeInfoByIndex, distributorNodeInfoByIndex, new FakeTimer(), state, + statsAggregator, 10, eventLog, "pathPrefix", diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentClusterStatsBuilder.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentClusterStatsBuilder.java index 16767cafa8f..dd0205658cb 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentClusterStatsBuilder.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentClusterStatsBuilder.java @@ -9,7 +9,7 @@ import java.util.Map; */ public class ContentClusterStatsBuilder { - private final Map<Integer, Map<String, ContentNodeStats.BucketSpaceStats>> stats = new HashMap<>(); + private final Map<Integer, ContentNodeStatsBuilder> stats = new HashMap<>(); public ContentClusterStatsBuilder add(int nodeIndex, String bucketSpace, long bucketsTotal, long bucketsPending) { return add(nodeIndex, bucketSpace, ContentNodeStats.BucketSpaceStats.of(bucketsTotal, bucketsPending)); @@ -24,24 +24,24 @@ public class ContentClusterStatsBuilder { } public ContentClusterStatsBuilder add(int nodeIndex, String bucketSpace, ContentNodeStats.BucketSpaceStats bucketSpaceStats) { - Map<String, ContentNodeStats.BucketSpaceStats> contentNodeStats = stats.get(nodeIndex); - if (contentNodeStats == null) { - contentNodeStats = new HashMap<>(); - stats.put(nodeIndex, contentNodeStats); + ContentNodeStatsBuilder nodeStatsBuilder = stats.get(nodeIndex); + if (nodeStatsBuilder == null) { + nodeStatsBuilder = ContentNodeStatsBuilder.forNode(nodeIndex); + stats.put(nodeIndex, nodeStatsBuilder); } - contentNodeStats.put(bucketSpace, bucketSpaceStats); + nodeStatsBuilder.add(bucketSpace, bucketSpaceStats); return this; } public ContentClusterStatsBuilder add(int nodeIndex) { - stats.put(nodeIndex, new HashMap<>()); + stats.put(nodeIndex, ContentNodeStatsBuilder.forNode(nodeIndex)); return this; } public ContentClusterStats build() { Map<Integer, ContentNodeStats> nodeToStatsMap = new HashMap<>(); - stats.forEach((nodeIndex, bucketSpaces) -> - nodeToStatsMap.put(nodeIndex, new ContentNodeStats(nodeIndex, bucketSpaces))); + stats.forEach((nodeIndex, nodeStatsBuilder) -> + nodeToStatsMap.put(nodeIndex, nodeStatsBuilder.build())); return new ContentClusterStats(nodeToStatsMap); } } diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentNodeStatsBuilder.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentNodeStatsBuilder.java new file mode 100644 index 00000000000..9fb020260e9 --- /dev/null +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ContentNodeStatsBuilder.java @@ -0,0 +1,31 @@ +package com.yahoo.vespa.clustercontroller.core; + +import java.util.HashMap; +import java.util.Map; + +public class ContentNodeStatsBuilder { + + private final int nodeIndex; + private final Map<String, ContentNodeStats.BucketSpaceStats> stats = new HashMap<>(); + + private ContentNodeStatsBuilder(int nodeIndex) { + this.nodeIndex = nodeIndex; + } + + public static ContentNodeStatsBuilder forNode(int nodeIndex) { + return new ContentNodeStatsBuilder(nodeIndex); + } + + public ContentNodeStatsBuilder add(String bucketSpace, long bucketsTotal, long bucketsPending) { + return add(bucketSpace, ContentNodeStats.BucketSpaceStats.of(bucketsTotal, bucketsPending)); + } + + public ContentNodeStatsBuilder add(String bucketSpace, ContentNodeStats.BucketSpaceStats bucketSpaceStats) { + stats.put(bucketSpace, bucketSpaceStats); + return this; + } + + public ContentNodeStats build() { + return new ContentNodeStats(nodeIndex, stats); + } +} diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/async/AsyncTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/async/AsyncTest.java index 9d8bb6d10e4..2817c83a041 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/async/AsyncTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/async/AsyncTest.java @@ -1,14 +1,15 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.utils.communication.async; -import junit.framework.TestCase; -import org.junit.Ignore; import org.junit.Test; import java.util.LinkedList; -public class AsyncTest extends TestCase { +import static org.junit.Assert.*; +public class AsyncTest { + + @Test public void testListeners() { AsyncOperationImpl<String> op = new AsyncOperationImpl<>("test"); class Listener implements AsyncCallback<String> { @@ -28,16 +29,17 @@ public class AsyncTest extends TestCase { op.unregister(l1); op.setResult("foo"); op.register(l4); - // Listener that is unregistered is not called + // Listener that is unregistered is not called assertEquals(false, l1.called); - // Listener that is registered is called + // Listener that is registered is called assertEquals(true, l2.called); - // Multiple listeners supported + // Multiple listeners supported assertEquals(true, l3.called); - // Listener called directly when registered after result is set + // Listener called directly when registered after result is set assertEquals(true, l4.called); } + @Test public void testMultipleResultSetters() { { AsyncOperationImpl<String> op = new AsyncOperationImpl<>("test"); @@ -62,6 +64,7 @@ public class AsyncTest extends TestCase { } } + @Test public void testPartialResultOnFailure() { AsyncOperationImpl<String> op = new AsyncOperationImpl<>("test"); op.setFailure(new Exception("bar"), "foo"); @@ -70,6 +73,7 @@ public class AsyncTest extends TestCase { assertEquals("bar", op.getCause().getMessage()); } + @Test public void testListenImpl() { class ListenImpl extends AsyncOperationListenImpl<String> { public ListenImpl(AsyncOperation<String> op) { @@ -93,6 +97,7 @@ public class AsyncTest extends TestCase { assertEquals(1, l1.calls); } + @Test public void testRedirectedOperation() { { final AsyncOperationImpl<String> op = new AsyncOperationImpl<>("test", "desc"); @@ -141,6 +146,7 @@ public class AsyncTest extends TestCase { } } + @Test public void testRedirectOnSuccessOperation() { { final AsyncOperationImpl<Integer> target = new AsyncOperationImpl<>("foo"); @@ -236,6 +242,7 @@ public class AsyncTest extends TestCase { } } + @Test public void testStressCompletionAndRegisterToDetectRace() throws Exception { int iterations = 1000; Object monitor = new Object(); @@ -247,7 +254,7 @@ public class AsyncTest extends TestCase { t1.start(); t2.start(); for (int i=0; i<iterations; ++i) { - final AsyncOperationImpl<String> op = new AsyncOperationImpl<>("test"); + AsyncOperationImpl<String> op = new AsyncOperationImpl<>("test"); synchronized (monitor) { completer.op = op; listener.op = op; @@ -263,23 +270,8 @@ public class AsyncTest extends TestCase { t1.join(); t2.join(); } - /* - System.out.println("Done with " + iterations + " iterations. " - + "Registered prior " + listener.priorReg + " times. " - + "Unset " + listener.unset + " times. "); - // */ assertEquals(0, listener.unset); assertEquals(iterations, listener.counter); } - public void ignoreTestExceptionOnCallback() throws Exception { - AsyncOperationImpl<String> impl = new AsyncOperationImpl<>("foo"); - impl.register(new AsyncCallback<String>() { - @Override - public void done(AsyncOperation<String> op) { - throw new RuntimeException("Foo"); - } - }); - impl.setResult(null); - } } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/AsyncHttpClientWithBaseTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/AsyncHttpClientWithBaseTest.java index e04eb1c043d..685ad492052 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/AsyncHttpClientWithBaseTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/AsyncHttpClientWithBaseTest.java @@ -3,10 +3,15 @@ package com.yahoo.vespa.clustercontroller.utils.communication.http; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperation; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperationImpl; -import junit.framework.TestCase; +import org.junit.Test; -public class AsyncHttpClientWithBaseTest extends TestCase { +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertTrue; +public class AsyncHttpClientWithBaseTest { + + @Test public void testOverride() { class HttpClient implements AsyncHttpClient<HttpResult> { HttpRequest lastRequest; @@ -39,11 +44,13 @@ public class AsyncHttpClientWithBaseTest extends TestCase { base.close(); } + @Test public void testClientMustBeSet() { - try{ - new AsyncHttpClientWithBase<HttpResult>(null); + try { + new AsyncHttpClientWithBase<>(null); assertTrue(false); } catch (IllegalArgumentException e) { } } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/HttpRequestTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/HttpRequestTest.java index 9d51954e567..7baff0453b0 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/HttpRequestTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/HttpRequestTest.java @@ -1,9 +1,13 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.utils.communication.http; -import junit.framework.TestCase; +import org.junit.Test; -public class HttpRequestTest extends TestCase { +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertTrue; + +public class HttpRequestTest { private HttpRequest createRequest() { return new HttpRequest() @@ -16,6 +20,7 @@ public class HttpRequestTest extends TestCase { .setTimeout(25); } + @Test public void testEquality() { assertEquals(createRequest(), createRequest()); assertNotSame(createRequest(), createRequest().setHost("localhost")); @@ -25,6 +30,7 @@ public class HttpRequestTest extends TestCase { assertNotSame(createRequest(), createRequest().setHttpOperation(HttpRequest.HttpOp.DELETE)); } + @Test public void testVerifyComplete() { // To be a complete request, an HTTP request must include: // - A path @@ -42,6 +48,7 @@ public class HttpRequestTest extends TestCase { new HttpRequest().setHttpOperation(HttpRequest.HttpOp.GET).setPath("/bar").verifyComplete(); } + @Test public void testMerge() { { HttpRequest base = new HttpRequest() @@ -84,16 +91,19 @@ public class HttpRequestTest extends TestCase { } } + @Test public void testNonExistingHeader() { assertEquals("foo", new HttpRequest().getHeader("asd", "foo")); assertEquals("foo", new HttpRequest().addHttpHeader("bar", "foo").getHeader("asd", "foo")); } + @Test public void testOption() { assertEquals("bar", new HttpRequest().addUrlOption("foo", "bar").getOption("foo", "foo")); assertEquals("foo", new HttpRequest().getOption("asd", "foo")); } + @Test public void testToString() { assertEquals("GET? http://localhost:8080/", new HttpRequest() @@ -113,9 +123,11 @@ public class HttpRequestTest extends TestCase { .toString(true)); } + @Test public void testNothingButGetCoverage() { assertEquals(false, new HttpRequest().equals(new Object())); new HttpRequest().getHeaders(); new HttpRequest().setUrlOptions(new HttpRequest().getUrlOptions()); } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/HttpResultTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/HttpResultTest.java index 6f165e8d35c..f137e9b9580 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/HttpResultTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/HttpResultTest.java @@ -1,10 +1,13 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.utils.communication.http; -import junit.framework.TestCase; +import org.junit.Test; -public class HttpResultTest extends TestCase { +import static org.junit.Assert.assertEquals; +public class HttpResultTest { + + @Test public void testSuccess() { assertEquals(false, new HttpResult().setHttpCode(199, "foo").isSuccess()); assertEquals(true, new HttpResult().setHttpCode(200, "foo").isSuccess()); @@ -12,6 +15,7 @@ public class HttpResultTest extends TestCase { assertEquals(false, new HttpResult().setHttpCode(300, "foo").isSuccess()); } + @Test public void testToString() { assertEquals("HTTP 200/OK", new HttpResult().setContent("Foo").toString()); assertEquals("HTTP 200/OK\n\nFoo", new HttpResult().setContent("Foo").toString(true)); @@ -19,7 +23,9 @@ public class HttpResultTest extends TestCase { assertEquals("HTTP 200/OK", new HttpResult().setContent("").toString(true)); } + @Test public void testNothingButGetCoverage() { new HttpResult().getHeaders(); } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/JsonAsyncHttpClientTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/JsonAsyncHttpClientTest.java index 5fa8ac87da6..3d3cd517020 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/JsonAsyncHttpClientTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/JsonAsyncHttpClientTest.java @@ -3,11 +3,16 @@ package com.yahoo.vespa.clustercontroller.utils.communication.http; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperation; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperationImpl; -import junit.framework.TestCase; import org.codehaus.jettison.json.JSONObject; +import org.junit.Test; -public class JsonAsyncHttpClientTest extends TestCase { +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +public class JsonAsyncHttpClientTest { + + @Test public void testJSONInJSONOut() throws Exception { DummyAsyncHttpClient dummy = new DummyAsyncHttpClient( new HttpResult().setContent(new JSONObject().put("bar", 42))); @@ -27,6 +32,7 @@ public class JsonAsyncHttpClientTest extends TestCase { client.close(); } + @Test public void testStringInJSONOut() throws Exception { DummyAsyncHttpClient dummy = new DummyAsyncHttpClient( new HttpResult().setContent(new JSONObject().put("bar", 42).toString())); @@ -40,6 +46,7 @@ public class JsonAsyncHttpClientTest extends TestCase { assertEquals(new JSONObject().put("bar", 42).toString(), result.getResult().getJson().toString()); } + @Test public void testIllegalJsonIn() throws Exception { DummyAsyncHttpClient dummy = new DummyAsyncHttpClient( new HttpResult().setContent(new JSONObject().put("bar", 42))); @@ -56,6 +63,7 @@ public class JsonAsyncHttpClientTest extends TestCase { } } + @Test public void testIllegalJSONOut() throws Exception { DummyAsyncHttpClient dummy = new DummyAsyncHttpClient( new HttpResult().setContent("my illegal json")); @@ -69,7 +77,8 @@ public class JsonAsyncHttpClientTest extends TestCase { assertEquals("{\"error\":\"Invalid JSON in output: A JSONObject text must begin with '{' at character 1 of my illegal json\",\"output\":\"my illegal json\"}", result.getResult().getJson().toString()); } - public void testEmptyReply() throws Exception { + @Test + public void testEmptyReply() { class Client implements AsyncHttpClient<HttpResult> { AsyncOperationImpl<HttpResult> lastOp; @Override @@ -87,6 +96,7 @@ public class JsonAsyncHttpClientTest extends TestCase { assertNull(op.getResult()); } + @Test public void testNotVerifyingJson() throws Exception { DummyAsyncHttpClient dummy = new DummyAsyncHttpClient( new HttpResult().setContent(new JSONObject().put("bar", 42))); @@ -105,4 +115,5 @@ public class JsonAsyncHttpClientTest extends TestCase { result.toString(); client.close(); } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/JsonHttpResultTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/JsonHttpResultTest.java index 07b11214301..8265247b124 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/JsonHttpResultTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/JsonHttpResultTest.java @@ -1,16 +1,20 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.utils.communication.http; -import junit.framework.TestCase; import org.codehaus.jettison.json.JSONException; import org.codehaus.jettison.json.JSONObject; +import org.junit.Test; -public class JsonHttpResultTest extends TestCase { +import static org.junit.Assert.assertEquals; +public class JsonHttpResultTest { + + @Test public void testCopyConstructor() { assertEquals("{}", new JsonHttpResult(new HttpResult()).getJson().toString()); } + @Test public void testOutput() { assertEquals("HTTP 200/OK\n" + "\n" @@ -22,6 +26,7 @@ public class JsonHttpResultTest extends TestCase { new JsonHttpResult(new HttpResult().setContent("{ \"foo\" : }")).toString(true)); } + @Test public void testNonJsonOutput() { JsonHttpResult result = new JsonHttpResult(); result.setContent("Foo"); @@ -30,6 +35,7 @@ public class JsonHttpResultTest extends TestCase { assertEquals("Foo", sb.toString()); } + @Test public void testInvalidJsonOutput() { JsonHttpResult result = new JsonHttpResult(); result.setJson(new JSONObject() { @@ -42,4 +48,5 @@ public class JsonHttpResultTest extends TestCase { result.printContent(sb); assertEquals("JSON: {}", sb.toString()); } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/LoggingAsyncHttpClientTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/LoggingAsyncHttpClientTest.java index 0191c29dd66..37841f7ca29 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/LoggingAsyncHttpClientTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/LoggingAsyncHttpClientTest.java @@ -3,12 +3,15 @@ package com.yahoo.vespa.clustercontroller.utils.communication.http; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperation; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperationImpl; -import junit.framework.TestCase; +import org.junit.Test; import java.util.logging.Level; import java.util.logging.Logger; -public class LoggingAsyncHttpClientTest extends TestCase { +import static org.junit.Assert.assertEquals; + +public class LoggingAsyncHttpClientTest { + class HttpClient implements AsyncHttpClient<HttpResult> { AsyncOperationImpl<HttpResult> lastOp; @Override @@ -20,11 +23,13 @@ public class LoggingAsyncHttpClientTest extends TestCase { } } - public void testWithoutDebugLog() throws Exception { + @Test + public void testWithoutDebugLog() { doRequests(); } - public void testWithDebugLog() throws Exception { + @Test + public void testWithDebugLog() { Logger log = Logger.getLogger(LoggingAsyncHttpClient.class.getName()); log.setLevel(Level.FINE); doRequests(); @@ -45,6 +50,6 @@ public class LoggingAsyncHttpClientTest extends TestCase { client.lastOp.setFailure(new Exception("foo")); assertEquals("foo", op.getCause().getMessage()); } - } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/ProxyAsyncHttpClientTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/ProxyAsyncHttpClientTest.java index d231dadc9b1..062fd4aaa32 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/ProxyAsyncHttpClientTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/ProxyAsyncHttpClientTest.java @@ -1,11 +1,15 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.utils.communication.http; -import junit.framework.TestCase; import org.codehaus.jettison.json.JSONObject; +import org.junit.Test; -public class ProxyAsyncHttpClientTest extends TestCase { +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +public class ProxyAsyncHttpClientTest { + + @Test public void testSimple() throws Exception { // Can't really test much here, but verifies that the code runs. DummyAsyncHttpClient dummy = new DummyAsyncHttpClient( @@ -28,6 +32,7 @@ public class ProxyAsyncHttpClientTest extends TestCase { dummy.lastRequest); } + @Test public void testNoAndEmptyPath() throws Exception { DummyAsyncHttpClient dummy = new DummyAsyncHttpClient( new HttpResult().setContent(new JSONObject().put("bar", 42))); @@ -40,4 +45,5 @@ public class ProxyAsyncHttpClientTest extends TestCase { } client.execute(new HttpRequest().setHost("local").setPath("")); } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/RequestQueueTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/RequestQueueTest.java index e3b6f7f7528..230920df53f 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/RequestQueueTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/RequestQueueTest.java @@ -4,11 +4,14 @@ package com.yahoo.vespa.clustercontroller.utils.communication.http; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncCallback; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperation; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperationImpl; -import junit.framework.TestCase; +import org.junit.Test; import java.util.LinkedList; -public class RequestQueueTest extends TestCase { +import static org.junit.Assert.assertEquals; + +public class RequestQueueTest { + public static class Request { public final HttpRequest request; public final AsyncOperationImpl<HttpResult> result; @@ -33,6 +36,7 @@ public class RequestQueueTest extends TestCase { public void close() {} }; + @Test public void testNormalUsage() { TestClient client = new TestClient(); RequestQueue<HttpResult> queue = new RequestQueue<>(client, 4); @@ -82,6 +86,7 @@ public class RequestQueueTest extends TestCase { } } + @Test public void testWaitUntilEmpty() throws Exception { TestClient client = new TestClient(); RequestQueue<HttpResult> queue = new RequestQueue<>(client, 4); @@ -105,4 +110,5 @@ public class RequestQueueTest extends TestCase { } assertEquals(1, result.size()); } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/TimeoutHandlerTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/TimeoutHandlerTest.java index 8c567a25b3a..72a2a4eab8a 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/TimeoutHandlerTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/TimeoutHandlerTest.java @@ -5,14 +5,19 @@ import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperatio import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncOperationImpl; import com.yahoo.vespa.clustercontroller.utils.communication.async.AsyncUtils; import com.yahoo.vespa.clustercontroller.utils.test.FakeClock; -import junit.framework.TestCase; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.Executor; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; -public class TimeoutHandlerTest extends TestCase { +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class TimeoutHandlerTest { public class TestClient implements AsyncHttpClient<HttpResult> { AsyncOperationImpl<HttpResult> lastOp; @@ -29,6 +34,7 @@ public class TimeoutHandlerTest extends TestCase { private FakeClock clock; private TimeoutHandler<HttpResult> handler; + @Before public void setUp() { executor = new ThreadPoolExecutor(10, 100, 100, TimeUnit.SECONDS, new ArrayBlockingQueue<Runnable>(1000)); clock = new FakeClock(); @@ -36,11 +42,13 @@ public class TimeoutHandlerTest extends TestCase { handler = new TimeoutHandler<>(executor, clock, client); } + @After public void tearDown() { handler.close(); executor.shutdown(); } + @Test public void testTimeout() { AsyncOperation<HttpResult> op = handler.execute(new HttpRequest().setTimeout(1000)); assertFalse(op.isDone()); @@ -59,6 +67,7 @@ public class TimeoutHandlerTest extends TestCase { assertTrue(op.getCause().getMessage(), op.getCause().getMessage().contains("Operation timeout")); } + @Test public void testNoTimeout() { AsyncOperation<HttpResult> op = handler.execute(new HttpRequest().setTimeout(1000)); clock.adjust(999); @@ -70,6 +79,7 @@ public class TimeoutHandlerTest extends TestCase { assertEquals("foo", op.getResult().getContent()); } + @Test public void testNoTimeoutFailing() { AsyncOperation<HttpResult> op = handler.execute(new HttpRequest().setTimeout(1000)); clock.adjust(999); @@ -81,6 +91,7 @@ public class TimeoutHandlerTest extends TestCase { assertEquals("foo", op.getCause().getMessage()); } + @Test public void testProvokeCompletedOpPurgeInTimeoutList() { AsyncOperation<HttpResult> op1 = handler.execute(new HttpRequest().setTimeout(1000)); AsyncOperationImpl<HttpResult> op1Internal = client.lastOp; @@ -97,6 +108,7 @@ public class TimeoutHandlerTest extends TestCase { assertEquals(false, op2.isSuccess()); } + @Test public void testNothingButGetCoverage() { AsyncOperation<HttpResult> op = handler.execute(new HttpRequest().setTimeout(1000)); op.getProgress(); @@ -111,4 +123,5 @@ public class TimeoutHandlerTest extends TestCase { client.lastOp.setResult(new HttpResult().setContent("foo")); AsyncUtils.waitFor(op); } + } diff --git a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/writer/HttpWriterTest.java b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/writer/HttpWriterTest.java index b358c3cbbb5..5ef6239894c 100644 --- a/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/writer/HttpWriterTest.java +++ b/clustercontroller-utils/src/test/java/com/yahoo/vespa/clustercontroller/utils/communication/http/writer/HttpWriterTest.java @@ -1,9 +1,13 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.clustercontroller.utils.communication.http.writer; -import junit.framework.TestCase; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class HttpWriterTest { -public class HttpWriterTest extends TestCase { private static String defaultTitle = "My Title"; private static String defaultHeader = "<html>\n" + " <head>\n" @@ -14,16 +18,20 @@ public class HttpWriterTest extends TestCase { private static String defaultFooter = " </body>\n" + "</html>\n"; - + @Test public void testStructure() { HttpWriter writer = new HttpWriter(); String header = defaultHeader.replace(defaultTitle, "Untitled page"); assertEquals(header + defaultFooter, writer.toString()); } + + @Test public void testTitle() { HttpWriter writer = new HttpWriter().addTitle(defaultTitle); assertEquals(defaultHeader + defaultFooter, writer.toString()); } + + @Test public void testParagraph() { String paragraph = "This is a paragraph"; String paragraph2 = "More text"; @@ -36,6 +44,8 @@ public class HttpWriterTest extends TestCase { + " </p>\n"; assertEquals(defaultHeader + content + defaultFooter, writer.toString()); } + + @Test public void testLink() { String name = "My link"; String link = "/foo/bar?hmm"; @@ -43,6 +53,8 @@ public class HttpWriterTest extends TestCase { String content = " <a href=\"" + link + "\">" + name + "</a>\n"; assertEquals(defaultHeader + content + defaultFooter, writer.toString()); } + + @Test public void testErrors() { try{ HttpWriter writer = new HttpWriter().addTitle(defaultTitle); @@ -57,4 +69,5 @@ public class HttpWriterTest extends TestCase { } catch (IllegalStateException e) { } } + } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/expressiontransforms/TensorFlowFeatureConverter.java b/config-model/src/main/java/com/yahoo/searchdefinition/expressiontransforms/TensorFlowFeatureConverter.java index f16697b5ba6..864cd823728 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/expressiontransforms/TensorFlowFeatureConverter.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/expressiontransforms/TensorFlowFeatureConverter.java @@ -23,10 +23,18 @@ import com.yahoo.searchlib.rankingexpression.rule.Arguments; import com.yahoo.searchlib.rankingexpression.rule.CompositeNode; import com.yahoo.searchlib.rankingexpression.rule.ConstantNode; import com.yahoo.searchlib.rankingexpression.rule.ExpressionNode; +import com.yahoo.searchlib.rankingexpression.rule.GeneratorLambdaFunctionNode; import com.yahoo.searchlib.rankingexpression.rule.ReferenceNode; +import com.yahoo.searchlib.rankingexpression.rule.TensorFunctionNode; import com.yahoo.searchlib.rankingexpression.transform.ExpressionTransformer; import com.yahoo.tensor.Tensor; import com.yahoo.tensor.TensorType; +import com.yahoo.tensor.functions.Generate; +import com.yahoo.tensor.functions.Join; +import com.yahoo.tensor.functions.Reduce; +import com.yahoo.tensor.functions.Rename; +import com.yahoo.tensor.functions.ScalarFunctions; +import com.yahoo.tensor.functions.TensorFunction; import com.yahoo.tensor.serialization.TypedBinaryFormat; import java.io.BufferedReader; @@ -37,9 +45,11 @@ import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.logging.Logger; /** @@ -197,7 +207,7 @@ public class TensorFlowFeatureConverter extends ExpressionTransformer<RankProfil */ private void verifyRequiredMacros(RankingExpression expression, Map<String, TensorType> requiredMacros, RankProfile profile, QueryProfileRegistry queryProfiles) { - List<String> macroNames = new ArrayList<>(); + Set<String> macroNames = new HashSet<>(); addMacroNamesIn(expression.getRoot(), macroNames); for (String macroName : macroNames) { TensorType requiredType = requiredMacros.get(macroName); @@ -223,10 +233,84 @@ public class TensorFlowFeatureConverter extends ExpressionTransformer<RankProfil "' of type " + requiredType + " which must be produced by a macro in the rank profile, but " + "this macro produces type " + actualType); + + // Check if batch dimensions can be reduced out. + reduceBatchDimensions(expression, macro, actualType); + } + } + + /** + * If the macro specifies that a single exemplar should be + * evaluated, we can reduce the batch dimension out. + */ + private void reduceBatchDimensions(RankingExpression expression, RankProfile.Macro macro, TensorType type) { + if (type.dimensions().size() > 1) { + List<String> reduceDimensions = new ArrayList<>(); + for (TensorType.Dimension dimension : type.dimensions()) { + if (dimension.size().orElse(-1L) == 1) { + reduceDimensions.add(dimension.name()); + } + } + if (reduceDimensions.size() > 0) { + ExpressionNode root = expression.getRoot(); + root = reduceBatchDimensionsAtInput(root, macro, reduceDimensions); + root = expandBatchDimensionsAtOutput(root, reduceDimensions); // todo: determine when we can skip this + expression.setRoot(root); + } + } + } + + private ExpressionNode reduceBatchDimensionsAtInput(ExpressionNode node, + RankProfile.Macro macro, + List<String> reduceDimensions) { + if (node instanceof TensorFunctionNode) { + TensorFunction tensorFunction = ((TensorFunctionNode) node).function(); + if (tensorFunction instanceof Rename) { + List<ExpressionNode> children = ((TensorFunctionNode)node).children(); + if (children.size() == 1 && children.get(0) instanceof ReferenceNode) { + ReferenceNode referenceNode = (ReferenceNode) children.get(0); + if (referenceNode.getName().equals(macro.getName())) { + return reduceBatchDimensionExpression(tensorFunction, reduceDimensions); + } + } + } + } + if (node instanceof ReferenceNode) { + ReferenceNode referenceNode = (ReferenceNode) node; + if (referenceNode.getName().equals(macro.getName())) { + return reduceBatchDimensionExpression(TensorFunctionNode.wrapArgument(node), reduceDimensions); + } + } + if (node instanceof CompositeNode) { + List<ExpressionNode> children = ((CompositeNode)node).children(); + List<ExpressionNode> transformedChildren = new ArrayList<>(children.size()); + for (ExpressionNode child : children) { + transformedChildren.add(reduceBatchDimensionsAtInput(child, macro, reduceDimensions)); + } + return ((CompositeNode)node).setChildren(transformedChildren); + } + return node; + } + + private ExpressionNode reduceBatchDimensionExpression(TensorFunction function, List<String> reduceDimensions) { + return new TensorFunctionNode(new Reduce(function, Reduce.Aggregator.sum, reduceDimensions)); + } + + private ExpressionNode expandBatchDimensionsAtOutput(ExpressionNode node, + List<String> reduceDimensions) { + TensorType.Builder typeBuilder = new TensorType.Builder(); + for (String name : reduceDimensions) { + typeBuilder.indexed(name, 1); } + TensorType generatedType = typeBuilder.build(); + ExpressionNode generatedExpression = new ConstantNode(new DoubleValue(1)); + Generate generatedFunction = new Generate(generatedType, + new GeneratorLambdaFunctionNode(generatedType, generatedExpression).asLongListToDoubleOperator()); + Join expand = new Join(TensorFunctionNode.wrapArgument(node), generatedFunction, ScalarFunctions.multiply()); + return new TensorFunctionNode(expand); } - private void addMacroNamesIn(ExpressionNode node, List<String> names) { + private void addMacroNamesIn(ExpressionNode node, Set<String> names) { if (node instanceof ReferenceNode) { ReferenceNode referenceNode = (ReferenceNode)node; if (referenceNode.getOutput() == null) // macro references cannot specify outputs diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/processing/RankingExpressionWithTensorFlowTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/processing/RankingExpressionWithTensorFlowTestCase.java index 19d37c5fb44..beba8ade1d8 100644 --- a/config-model/src/test/java/com/yahoo/searchdefinition/processing/RankingExpressionWithTensorFlowTestCase.java +++ b/config-model/src/test/java/com/yahoo/searchdefinition/processing/RankingExpressionWithTensorFlowTestCase.java @@ -252,10 +252,20 @@ public class RankingExpressionWithTensorFlowTestCase { } @Test + public void testTensorFlowReduceBatchDimension() { + final String expression = "join(join(reduce(join(reduce(rename(Placeholder, (d0, d1), (d0, d2)), sum, d0), constant(\"layer_Variable_read\"), f(a,b)(a * b)), sum, d2), constant(\"layer_Variable_1_read\"), f(a,b)(a + b)), tensor(d0[1])(1.0), f(a,b)(a * b))"; + RankProfileSearchFixture search = fixtureWith("tensor(d0[1],d1[784])(0.0)", + "tensorflow('mnist_softmax/saved')"); + search.assertFirstPhaseExpression(expression, "my_profile"); + assertLargeConstant("layer_Variable_1_read", search, Optional.of(10L)); + assertLargeConstant("layer_Variable_read", search, Optional.of(7840L)); + } + + @Test public void testImportingFromStoredExpressionsWithSmallConstants() throws IOException { - final String expression = "join(reduce(join(join(join(constant(\"dnn_hidden2_Const\"), join(reduce(join(join(join(0.009999999776482582, join(reduce(join(rename(input, (d0, d1), (d0, d4)), constant(\"dnn_hidden1_weights_read\"), f(a,b)(a * b)), sum, d4), constant(\"dnn_hidden1_bias_read\"), f(a,b)(a + b)), f(a,b)(a * b)), join(reduce(join(rename(input, (d0, d1), (d0, d4)), constant(\"dnn_hidden1_weights_read\"), f(a,b)(a * b)), sum, d4), constant(\"dnn_hidden1_bias_read\"), f(a,b)(a + b)), f(a,b)(max(a,b))), constant(\"dnn_hidden2_weights_read\"), f(a,b)(a * b)), sum, d3), constant(\"dnn_hidden2_bias_read\"), f(a,b)(a + b)), f(a,b)(a * b)), join(reduce(join(join(join(0.009999999776482582, join(reduce(join(rename(input, (d0, d1), (d0, d4)), constant(\"dnn_hidden1_weights_read\"), f(a,b)(a * b)), sum, d4), constant(\"dnn_hidden1_bias_read\"), f(a,b)(a + b)), f(a,b)(a * b)), join(reduce(join(rename(input, (d0, d1), (d0, d4)), constant(\"dnn_hidden1_weights_read\"), f(a,b)(a * b)), sum, d4), constant(\"dnn_hidden1_bias_read\"), f(a,b)(a + b)), f(a,b)(max(a,b))), constant(\"dnn_hidden2_weights_read\"), f(a,b)(a * b)), sum, d3), constant(\"dnn_hidden2_bias_read\"), f(a,b)(a + b)), f(a,b)(max(a,b))), constant(\"dnn_outputs_weights_read\"), f(a,b)(a * b)), sum, d2), constant(\"dnn_outputs_bias_read\"), f(a,b)(a + b))"; + final String expression = "join(join(reduce(join(join(join(constant(\"dnn_hidden2_Const\"), join(reduce(join(join(join(0.009999999776482582, join(reduce(join(reduce(rename(input, (d0, d1), (d0, d4)), sum, d0), constant(\"dnn_hidden1_weights_read\"), f(a,b)(a * b)), sum, d4), constant(\"dnn_hidden1_bias_read\"), f(a,b)(a + b)), f(a,b)(a * b)), join(reduce(join(reduce(rename(input, (d0, d1), (d0, d4)), sum, d0), constant(\"dnn_hidden1_weights_read\"), f(a,b)(a * b)), sum, d4), constant(\"dnn_hidden1_bias_read\"), f(a,b)(a + b)), f(a,b)(max(a,b))), constant(\"dnn_hidden2_weights_read\"), f(a,b)(a * b)), sum, d3), constant(\"dnn_hidden2_bias_read\"), f(a,b)(a + b)), f(a,b)(a * b)), join(reduce(join(join(join(0.009999999776482582, join(reduce(join(reduce(rename(input, (d0, d1), (d0, d4)), sum, d0), constant(\"dnn_hidden1_weights_read\"), f(a,b)(a * b)), sum, d4), constant(\"dnn_hidden1_bias_read\"), f(a,b)(a + b)), f(a,b)(a * b)), join(reduce(join(reduce(rename(input, (d0, d1), (d0, d4)), sum, d0), constant(\"dnn_hidden1_weights_read\"), f(a,b)(a * b)), sum, d4), constant(\"dnn_hidden1_bias_read\"), f(a,b)(a + b)), f(a,b)(max(a,b))), constant(\"dnn_hidden2_weights_read\"), f(a,b)(a * b)), sum, d3), constant(\"dnn_hidden2_bias_read\"), f(a,b)(a + b)), f(a,b)(max(a,b))), constant(\"dnn_outputs_weights_read\"), f(a,b)(a * b)), sum, d2), constant(\"dnn_outputs_bias_read\"), f(a,b)(a + b)), tensor(d0[1])(1.0), f(a,b)(a * b))"; StoringApplicationPackage application = new StoringApplicationPackage(applicationDir); - RankProfileSearchFixture search = fixtureWith("tensor(d0[2],d1[784])(0.0)", + RankProfileSearchFixture search = fixtureWith("tensor(d0[1],d1[784])(0.0)", "tensorflow('mnist/saved')", null, null, diff --git a/configgen/src/main/scala/com/yahoo/config/codegen/ConfigGenerator.scala b/configgen/src/main/scala/com/yahoo/config/codegen/ConfigGenerator.scala index 90e23f24cf3..38306a03575 100644 --- a/configgen/src/main/scala/com/yahoo/config/codegen/ConfigGenerator.scala +++ b/configgen/src/main/scala/com/yahoo/config/codegen/ConfigGenerator.scala @@ -179,36 +179,8 @@ object ConfigGenerator { } } - // TODO: The default ctor can be removed if the config library uses builders to set values from payload, but ... - // a default ctor is also needed for all innerArrays, because of InnerNodeVector.createNew() - def defaultConstructor = { - // TODO @link gives javadoc warnings, although the syntax seems to be valid - //def link = "{@link " + {nodeClass(inner)} + "#" + {nodeClass(inner)} + "(Builder)}" - def link = {nodeClass(inner)} + "(Builder)" - - def ctor = - <code> - |/** - | * @deprecated Not for public use. - | * Does not check for uninitialized fields. - | * Replaced by {link} - | */ - |@Deprecated - |private {nodeClass(inner)}() {{ - | this(new Builder(), false); - |}} - </code>.text.stripMargin.trim - - inner match { - case array: InnerCNode if inner.isArray => ctor - case _ => "" - } - } - // TODO: merge these two constructors into one when the config library uses builders to set values from payload. <code> - |{defaultConstructor} - | |public {nodeClass(inner)}(Builder builder) {{ | this(builder, true); |}} @@ -319,7 +291,7 @@ object ConfigGenerator { | for (Builder b : builders) { | elems.add(new %s(b)); | } - | return new InnerNodeVector<%s>(elems, new %s()); + | return new InnerNodeVector<%s>(elems); |} """.stripMargin.format(List.fill(5)(nodeClass(inner)): _*).trim } diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/component/Deconstructor.java b/container-disc/src/main/java/com/yahoo/container/jdisc/component/Deconstructor.java index a986fbc794b..f13c078c41d 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/component/Deconstructor.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/component/Deconstructor.java @@ -6,7 +6,9 @@ import com.yahoo.concurrent.ThreadFactoryFactory; import com.yahoo.container.di.ComponentDeconstructor; import com.yahoo.container.di.componentgraph.Provider; import com.yahoo.jdisc.SharedResource; +import com.yahoo.log.LogLevel; +import java.security.SecureRandom; import java.util.Random; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; @@ -20,6 +22,7 @@ import static java.util.logging.Level.WARNING; * @author gv */ public class Deconstructor implements ComponentDeconstructor { + private static final Logger log = Logger.getLogger(Deconstructor.class.getName()); private final ScheduledExecutorService executor = @@ -52,27 +55,44 @@ public class Deconstructor implements ComponentDeconstructor { } private static class DestructComponentTask implements Runnable { + private final AbstractComponent component; DestructComponentTask(AbstractComponent component) { this.component = component; } + /** Returns a random value which will be different across identical containers invoking this at the same time */ + private long random() { + return new SecureRandom().nextLong(); + } + public void run() { log.info("Starting deconstruction of " + component); try { component.deconstruct(); log.info("Finished deconstructing " + component); - } catch (Error e) { - try { - Thread.sleep((long) (new Random(System.nanoTime()).nextDouble() * 180 * 1000)); - } catch (InterruptedException exception) { } - com.yahoo.protect.Process.logAndDie("Error when deconstructing " + component, e); - } catch (Exception e) { + } + catch (Exception | NoClassDefFoundError e) { // May get class not found due to it being already unloaded log.log(WARNING, "Exception thrown when deconstructing " + component, e); - } catch (Throwable t) { - log.log(WARNING, "Unexpected Throwable thrown when deconstructing " + component, t); + } + catch (Error e) { + try { + // Randomize restart over 10 minutes to avoid simultaneous cluster restarts + long randomSleepSeconds = random() * 60 * 10; + log.log(LogLevel.FATAL, "Error when deconstructing " + component + ". Will sleep for " + + randomSleepSeconds + " seconds then restart", e); + Thread.sleep(randomSleepSeconds * 1000); + } + catch (InterruptedException exception) { + log.log(WARNING, "Randomized wait before dying disrupted. Dying now."); + } + com.yahoo.protect.Process.logAndDie("Shutting down due to error when deconstructing " + component); + } + catch (Throwable e) { + log.log(WARNING, "Non-error not exception throwable thrown when deconstructing " + component, e); } } } + } diff --git a/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterMonitor.java b/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterMonitor.java index 22890f781fe..4e708e32a2d 100644 --- a/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterMonitor.java +++ b/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterMonitor.java @@ -37,6 +37,7 @@ public class ClusterMonitor implements Runnable, Freezable { /** A map from Node to corresponding MonitoredNode */ private final Map<VespaBackEndSearcher, NodeMonitor> nodeMonitors = new java.util.IdentityHashMap<>(); + private ScheduledFuture<?> future; private boolean isFrozen = false; @@ -96,21 +97,31 @@ public class ClusterMonitor implements Runnable, Freezable { private void updateVipStatus() { if ( ! vipStatus.isPresent()) return; + if ( ! hasInformationAboutAllNodes()) return; - boolean hasWorkingNodesWithDocumentsOnline = false; - for (NodeMonitor node : nodeMonitors.values()) { - if (node.isWorking() && node.searchNodesOnline()) { - hasWorkingNodesWithDocumentsOnline = true; - break; - } - } - if (hasWorkingNodesWithDocumentsOnline) { + if (hasWorkingNodesWithDocumentsOnline()) { vipStatus.get().addToRotation(this); } else { vipStatus.get().removeFromRotation(this); } } + private boolean hasInformationAboutAllNodes() { + for (NodeMonitor monitor : nodeMonitors.values()) { + if ( ! monitor.statusIsKnown()) + return false; + } + return true; + } + + private boolean hasWorkingNodesWithDocumentsOnline() { + for (NodeMonitor node : nodeMonitors.values()) { + if (node.isWorking() && node.searchNodesOnline()) + return true; + } + return false; + } + /** * Ping all nodes which needs pinging to discover state changes */ @@ -130,7 +141,7 @@ public class ClusterMonitor implements Runnable, Freezable { } } - public void shutdown() throws InterruptedException { + public void shutdown() { if (future != null) { future.cancel(true); } diff --git a/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java b/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java index ca1afbd4171..89f58bc944b 100644 --- a/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java +++ b/container-search/src/main/java/com/yahoo/prelude/cluster/ClusterSearcher.java @@ -591,11 +591,7 @@ public class ClusterSearcher extends Searcher { @Override public void deconstruct() { - try { - monitor.shutdown(); - } catch (final InterruptedException e) { - Thread.currentThread().interrupt(); - } + monitor.shutdown(); } ExecutorService getExecutor() { diff --git a/container-search/src/main/java/com/yahoo/prelude/cluster/NodeMonitor.java b/container-search/src/main/java/com/yahoo/prelude/cluster/NodeMonitor.java index b60aecc2e51..5ccaff0f198 100644 --- a/container-search/src/main/java/com/yahoo/prelude/cluster/NodeMonitor.java +++ b/container-search/src/main/java/com/yahoo/prelude/cluster/NodeMonitor.java @@ -52,7 +52,7 @@ public class NodeMonitor { } // Whether or not dispatch has ever responded successfully - private boolean atStartUp = true; + private boolean statusIsKnown = false; public VespaBackEndSearcher getNode() { return node; @@ -88,23 +88,26 @@ public class NodeMonitor { this.searchNodesOnline = searchNodesOnline; if (! isWorking) setWorking(true, "Responds correctly"); - atStartUp = false; + statusIsKnown = true; } /** Changes the state of this node if required */ private void setWorking(boolean working, String explanation) { if (isWorking == working) return; // Old news - if (working && ! atStartUp) - log.info("Putting " + node + " in service: " + explanation); - else if (! atStartUp) - log.info("Taking " + node + " out of service: " + explanation); + if (statusIsKnown) { + if (working) + log.info("Putting " + node + " in service: " + explanation); + else + log.info("Taking " + node + " out of service: " + explanation); + } isWorking = working; } - boolean searchNodesOnline() { - return searchNodesOnline; - } + boolean searchNodesOnline() { return searchNodesOnline; } + + /** Returns true if we have had enough time to determine the status of this node since creating the monitor */ + boolean statusIsKnown() { return statusIsKnown; } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java index 603f16a9ef6..59cf5adb6e2 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java @@ -7,6 +7,7 @@ import com.yahoo.config.application.api.DeploymentSpec; import java.time.Instant; import java.util.Objects; import java.util.Optional; +import java.util.StringJoiner; /** * The changes to an application we currently wish to complete deploying. @@ -79,15 +80,11 @@ public final class Change { @Override public String toString() { - String platformString = platform.map(v -> "upgrade to " + v).orElse(null); - String applicationString = application.map(v -> "application change to " + v).orElse(null); - if (platformString != null && applicationString != null) - return platformString + " and " + applicationString; - if (platformString != null) - return platformString; - if (applicationString != null) - return applicationString; - return "no change"; + StringJoiner changes = new StringJoiner(" and "); + platform.ifPresent(version -> changes.add("upgrade to " + version.toString())); + application.ifPresent(version -> changes.add("application change to " + version.id())); + changes.setEmptyValue("no change"); + return changes.toString(); } public static Change of(ApplicationVersion applicationVersion) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentJobExecutor.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentJobExecutor.java index fde5b311cd9..b781b231e48 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentJobExecutor.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentJobExecutor.java @@ -42,6 +42,7 @@ public class DeploymentJobExecutor extends Maintainer { protected void maintain() { controller().applications().deploymentTrigger().deploymentQueue().takeJobsToRun() .forEach(buildJob -> executor.execute(() -> { + log.log(Level.INFO, "Attempting to trigger " + buildJob + " in Screwdriver."); for (int i = 0; i < triggeringRetries; i++) if (buildService.trigger(buildJob)) return; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index d3edd1c0ca5..368fd323fb0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -89,6 +89,7 @@ import java.io.InputStream; import java.net.URI; import java.net.URISyntaxException; import java.security.Principal; +import java.time.DayOfWeek; import java.time.Duration; import java.util.Collections; import java.util.List; @@ -375,6 +376,19 @@ public class ApplicationApiHandler extends LoggingRequestHandler { job.lastSuccess().ifPresent(jobRun -> toSlime(jobRun, jobObject.setObject("lastSuccess"))); } + // Change blockers + Cursor changeBlockers = object.setArray("changeBlockers"); + application.deploymentSpec().changeBlocker().forEach(changeBlocker -> { + Cursor changeBlockerObject = changeBlockers.addObject(); + changeBlockerObject.setBool("versions", changeBlocker.blocksVersions()); + changeBlockerObject.setBool("revisions", changeBlocker.blocksRevisions()); + changeBlockerObject.setString("timeZone", changeBlocker.window().zone().getId()); + Cursor days = changeBlockerObject.setArray("days"); + changeBlocker.window().days().stream().map(DayOfWeek::getValue).forEach(days::addLong); + Cursor hours = changeBlockerObject.setArray("hours"); + changeBlocker.window().hours().forEach(hours::addLong); + }); + // Compile version. The version that should be used when building an application object.setString("compileVersion", application.oldestDeployedVersion().orElse(controller.systemVersion()).toFullString()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java index be987e84cd8..c3dc80c65df 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java @@ -14,7 +14,6 @@ import com.yahoo.jdisc.http.filter.SecurityRequestFilterChain; import com.yahoo.slime.ArrayTraverser; import com.yahoo.slime.Inspector; import com.yahoo.slime.Slime; -import com.yahoo.slime.Type; import com.yahoo.vespa.config.SlimeUtils; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.versions.VersionStatus; @@ -152,12 +151,19 @@ public class ContainerTester { if ( ! fieldsToCensor.contains(fieldName)) return; String fromString; - if ( fieldValue.type().equals(Type.STRING)) - fromString = "\"" + fieldName + "\":\"" + fieldValue.asString() + "\""; - else if ( fieldValue.type().equals(Type.LONG)) - fromString = "\"" + fieldName + "\":" + fieldValue.asLong(); - else - throw new IllegalArgumentException("Can only censor strings and longs"); + switch (fieldValue.type()) { + case STRING: + fromString = "\"" + fieldName + "\":\"" + fieldValue.asString() + "\""; + break; + case LONG: + fromString = "\"" + fieldName + "\":" + fieldValue.asLong(); + break; + case BOOL: + fromString = "\"" + fieldName + "\":" + fieldValue.asBool(); + break; + default: + throw new IllegalArgumentException("Can only censor strings, longs and booleans"); + } String toString = "\"" + fieldName + "\":\"(ignore)\""; replaceStrings.add(new Pair<>(fromString, toString)); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index 4a263bf1fbd..4b59e57fd5a 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java @@ -87,6 +87,7 @@ public class ApplicationApiTest extends ControllerContainerTest { .region("corp-us-east-1") .region("us-east-3") .region("us-west-1") + .blockChange(false, true, "mon-fri", "0-8", "UTC") .build(); private static final AthenzDomain ATHENZ_TENANT_DOMAIN = new AthenzDomain("domain1"); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-without-change-multiple-deployments.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-without-change-multiple-deployments.json index d3b765551f0..9cd9329e36f 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-without-change-multiple-deployments.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-without-change-multiple-deployments.json @@ -219,6 +219,7 @@ } } ], + "changeBlockers": [], "compileVersion": "(ignore)", "globalRotations": [ "http://application1.tenant1.global.vespa.yahooapis.com:4080/", diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application.json index 155166dea4c..29d23034053 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application.json @@ -183,6 +183,31 @@ } } ], + "changeBlockers": [ + { + "versions": true, + "revisions": false, + "timeZone": "UTC", + "days": [ + 1, + 2, + 3, + 4, + 5 + ], + "hours": [ + 0, + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8 + ] + } + ], "compileVersion": "(ignore)", "globalRotations": [ "http://application1.tenant1.global.vespa.yahooapis.com:4080/", diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application1-recursive.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application1-recursive.json index 35124adbf68..a1414fa3511 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application1-recursive.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application1-recursive.json @@ -183,6 +183,31 @@ } } ], + "changeBlockers": [ + { + "versions": true, + "revisions": false, + "timeZone": "UTC", + "days": [ + 1, + 2, + 3, + 4, + 5 + ], + "hours": [ + 0, + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8 + ] + } + ], "compileVersion": "(ignore)", "globalRotations": [ "http://application1.tenant1.global.vespa.yahooapis.com:4080/", diff --git a/eval/CMakeLists.txt b/eval/CMakeLists.txt index 38abfec3c8c..a733b73d9d1 100644 --- a/eval/CMakeLists.txt +++ b/eval/CMakeLists.txt @@ -26,10 +26,11 @@ vespa_define_module( src/tests/gp/ponder_nov2017 src/tests/tensor/dense_dot_product_function src/tests/tensor/dense_fast_rename_function + src/tests/tensor/dense_inplace_join_function + src/tests/tensor/dense_inplace_map_function src/tests/tensor/dense_tensor_address_combiner src/tests/tensor/dense_tensor_builder src/tests/tensor/dense_xw_product_function - src/tests/tensor/vector_from_doubles_function src/tests/tensor/sparse_tensor_builder src/tests/tensor/tensor_address src/tests/tensor/tensor_conformance @@ -37,6 +38,7 @@ vespa_define_module( src/tests/tensor/tensor_performance src/tests/tensor/tensor_serialization src/tests/tensor/tensor_slime_serialization + src/tests/tensor/vector_from_doubles_function LIBS src/vespa/eval diff --git a/eval/src/tests/tensor/dense_inplace_join_function/CMakeLists.txt b/eval/src/tests/tensor/dense_inplace_join_function/CMakeLists.txt new file mode 100644 index 00000000000..2808675bc78 --- /dev/null +++ b/eval/src/tests/tensor/dense_inplace_join_function/CMakeLists.txt @@ -0,0 +1,8 @@ +# Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +vespa_add_executable(eval_dense_inplace_join_function_test_app TEST + SOURCES + dense_inplace_join_function_test.cpp + DEPENDS + vespaeval +) +vespa_add_test(NAME eval_dense_inplace_join_function_test_app COMMAND eval_dense_inplace_join_function_test_app) diff --git a/eval/src/tests/tensor/dense_inplace_join_function/dense_inplace_join_function_test.cpp b/eval/src/tests/tensor/dense_inplace_join_function/dense_inplace_join_function_test.cpp new file mode 100644 index 00000000000..c794b81f573 --- /dev/null +++ b/eval/src/tests/tensor/dense_inplace_join_function/dense_inplace_join_function_test.cpp @@ -0,0 +1,147 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include <vespa/vespalib/testkit/test_kit.h> +#include <vespa/eval/eval/tensor_function.h> +#include <vespa/eval/eval/simple_tensor.h> +#include <vespa/eval/eval/simple_tensor_engine.h> +#include <vespa/eval/tensor/default_tensor_engine.h> +#include <vespa/eval/tensor/dense/dense_inplace_join_function.h> +#include <vespa/eval/tensor/dense/dense_tensor.h> +#include <vespa/eval/eval/test/tensor_model.hpp> +#include <vespa/eval/eval/test/eval_fixture.h> + +#include <vespa/vespalib/util/stringfmt.h> +#include <vespa/vespalib/util/stash.h> + +using namespace vespalib; +using namespace vespalib::eval; +using namespace vespalib::eval::test; +using namespace vespalib::tensor; +using namespace vespalib::eval::tensor_function; + +const TensorEngine &prod_engine = DefaultTensorEngine::ref(); + +double seq_value = 0.0; + +struct GlobalSequence : public Sequence { + GlobalSequence() {} + double operator[](size_t) const override { + seq_value += 1.0; + return seq_value; + } + ~GlobalSequence() {} +}; +GlobalSequence seq; + +EvalFixture::ParamRepo make_params() { + return EvalFixture::ParamRepo() + .add("con_x5_A", spec({x(5)}, seq)) + .add("con_x5_B", spec({x(5)}, seq)) + .add("con_x5_C", spec({x(5)}, seq)) + .add("con_x5y3_A", spec({x(5),y(3)}, seq)) + .add("con_x5y3_B", spec({x(5),y(3)}, seq)) + .add_mutable("mut_dbl_A", spec(1.5)) + .add_mutable("mut_dbl_B", spec(2.5)) + .add_mutable("mut_x5_A", spec({x(5)}, seq)) + .add_mutable("mut_x5_B", spec({x(5)}, seq)) + .add_mutable("mut_x5_C", spec({x(5)}, seq)) + .add_mutable("mut_x4", spec({x(4)}, seq)) + .add_mutable("mut_x5y3_A", spec({x(5),y(3)}, seq)) + .add_mutable("mut_x5y3_B", spec({x(5),y(3)}, seq)) + .add_mutable("mut_x5_unbound", spec({x(5)}, seq), "tensor(x[])") + .add_mutable("mut_x_sparse", spec({x({"a", "b", "c"})}, seq)); +} +EvalFixture::ParamRepo param_repo = make_params(); + +void verify_optimized(const vespalib::string &expr, size_t cnt, size_t param_idx) { + EvalFixture fixture(prod_engine, expr, param_repo, true, true); + EXPECT_EQUAL(fixture.result(), EvalFixture::ref(expr, param_repo)); + for (size_t i = 0; i < fixture.num_params(); ++i) { + TEST_STATE(vespalib::make_string("param %zu", i).c_str()); + if (i == param_idx) { + EXPECT_EQUAL(fixture.get_param(i), fixture.result()); + } else { + EXPECT_NOT_EQUAL(fixture.get_param(i), fixture.result()); + } + } + auto info = fixture.find_all<DenseInplaceJoinFunction>(); + ASSERT_EQUAL(info.size(), cnt); + for (size_t i = 0; i < cnt; ++i) { + EXPECT_TRUE(info[i]->result_is_mutable()); + } +} + +void verify_p0_optimized(const vespalib::string &expr, size_t cnt) { + verify_optimized(expr, cnt, 0); +} + +void verify_p1_optimized(const vespalib::string &expr, size_t cnt) { + verify_optimized(expr, cnt, 1); +} + +void verify_p2_optimized(const vespalib::string &expr, size_t cnt) { + verify_optimized(expr, cnt, 2); +} + +void verify_not_optimized(const vespalib::string &expr) { + EvalFixture fixture(prod_engine, expr, param_repo, true, true); + EXPECT_EQUAL(fixture.result(), EvalFixture::ref(expr, param_repo)); + for (size_t i = 0; i < fixture.num_params(); ++i) { + EXPECT_NOT_EQUAL(fixture.get_param(i), fixture.result()); + } + auto info = fixture.find_all<DenseInplaceJoinFunction>(); + EXPECT_TRUE(info.empty()); +} + +TEST("require that mutable dense concrete tensors are optimized") { + TEST_DO(verify_p0_optimized("mut_x5_A-mut_x5_B", 1)); + TEST_DO(verify_p0_optimized("mut_x5_A-con_x5_B", 1)); + TEST_DO(verify_p1_optimized("con_x5_A-mut_x5_B", 1)); + TEST_DO(verify_p0_optimized("mut_x5y3_A-mut_x5y3_B", 1)); + TEST_DO(verify_p0_optimized("mut_x5y3_A-con_x5y3_B", 1)); + TEST_DO(verify_p1_optimized("con_x5y3_A-mut_x5y3_B", 1)); +} + +TEST("require that self-join operations can be optimized") { + TEST_DO(verify_p0_optimized("mut_x5_A+mut_x5_A", 1)); +} + +TEST("require that join(tensor,scalar) operations are not optimized") { + TEST_DO(verify_not_optimized("mut_x5_A-mut_dbl_B")); + TEST_DO(verify_not_optimized("mut_dbl_A-mut_x5_B")); +} + +TEST("require that join with different tensor shapes are not optimized") { + TEST_DO(verify_not_optimized("mut_x5_A-mut_x4")); + TEST_DO(verify_not_optimized("mut_x4-mut_x5_A")); + TEST_DO(verify_not_optimized("mut_x5_A*mut_x5y3_B")); +} + +TEST("require that inplace join operations can be chained") { + TEST_DO(verify_p0_optimized("mut_x5_A-(mut_x5_B-mut_x5_C)", 2)); + TEST_DO(verify_p0_optimized("(mut_x5_A-con_x5_B)-con_x5_C", 2)); + TEST_DO(verify_p1_optimized("con_x5_A-(mut_x5_B-con_x5_C)", 2)); + TEST_DO(verify_p2_optimized("con_x5_A-(con_x5_B-mut_x5_C)", 2)); +} + +TEST("require that abstract tensors are not optimized") { + TEST_DO(verify_not_optimized("mut_x5_unbound+mut_x5_A")); + TEST_DO(verify_not_optimized("mut_x5_A+mut_x5_unbound")); + TEST_DO(verify_not_optimized("mut_x5_unbound+mut_x5_unbound")); +} + +TEST("require that non-mutable tensors are not optimized") { + TEST_DO(verify_not_optimized("con_x5_A+con_x5_B")); +} + +TEST("require that scalar values are not optimized") { + TEST_DO(verify_not_optimized("mut_dbl_A+mut_dbl_B")); + TEST_DO(verify_not_optimized("mut_dbl_A+5")); + TEST_DO(verify_not_optimized("5+mut_dbl_B")); +} + +TEST("require that mapped tensors are not optimized") { + TEST_DO(verify_not_optimized("mut_x_sparse+mut_x_sparse")); +} + +TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/eval/src/tests/tensor/dense_inplace_map_function/CMakeLists.txt b/eval/src/tests/tensor/dense_inplace_map_function/CMakeLists.txt new file mode 100644 index 00000000000..3bd1dbaf271 --- /dev/null +++ b/eval/src/tests/tensor/dense_inplace_map_function/CMakeLists.txt @@ -0,0 +1,8 @@ +# Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +vespa_add_executable(eval_dense_inplace_map_function_test_app TEST + SOURCES + dense_inplace_map_function_test.cpp + DEPENDS + vespaeval +) +vespa_add_test(NAME eval_dense_inplace_map_function_test_app COMMAND eval_dense_inplace_map_function_test_app) diff --git a/eval/src/tests/tensor/dense_inplace_map_function/dense_inplace_map_function_test.cpp b/eval/src/tests/tensor/dense_inplace_map_function/dense_inplace_map_function_test.cpp new file mode 100644 index 00000000000..77af747a066 --- /dev/null +++ b/eval/src/tests/tensor/dense_inplace_map_function/dense_inplace_map_function_test.cpp @@ -0,0 +1,79 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include <vespa/vespalib/testkit/test_kit.h> +#include <vespa/eval/eval/tensor_function.h> +#include <vespa/eval/eval/simple_tensor.h> +#include <vespa/eval/eval/simple_tensor_engine.h> +#include <vespa/eval/tensor/default_tensor_engine.h> +#include <vespa/eval/tensor/dense/dense_inplace_map_function.h> +#include <vespa/eval/tensor/dense/dense_tensor.h> +#include <vespa/eval/eval/test/tensor_model.hpp> +#include <vespa/eval/eval/test/eval_fixture.h> + +#include <vespa/vespalib/util/stringfmt.h> +#include <vespa/vespalib/util/stash.h> + +using namespace vespalib; +using namespace vespalib::eval; +using namespace vespalib::eval::test; +using namespace vespalib::tensor; +using namespace vespalib::eval::tensor_function; + +const TensorEngine &prod_engine = DefaultTensorEngine::ref(); + +EvalFixture::ParamRepo make_params() { + return EvalFixture::ParamRepo() + .add("x5", spec({x(5)}, N())) + .add_mutable("_d", spec(5.0)) + .add_mutable("_x5", spec({x(5)}, N())) + .add_mutable("_x5y3", spec({x(5),y(3)}, N())) + .add_mutable("_x5_u", spec({x(5)}, N()), "tensor(x[])") + .add_mutable("_x_m", spec({x({"a", "b", "c"})}, N())); +} +EvalFixture::ParamRepo param_repo = make_params(); + +void verify_optimized(const vespalib::string &expr, size_t cnt) { + EvalFixture fixture(prod_engine, expr, param_repo, true, true); + EXPECT_EQUAL(fixture.result(), EvalFixture::ref(expr, param_repo)); + EXPECT_EQUAL(fixture.get_param(0), fixture.result()); + auto info = fixture.find_all<DenseInplaceMapFunction>(); + ASSERT_EQUAL(info.size(), cnt); + for (size_t i = 0; i < cnt; ++i) { + EXPECT_TRUE(info[i]->result_is_mutable()); + } +} + +void verify_not_optimized(const vespalib::string &expr) { + EvalFixture fixture(prod_engine, expr, param_repo, true, true); + EXPECT_EQUAL(fixture.result(), EvalFixture::ref(expr, param_repo)); + EXPECT_NOT_EQUAL(fixture.get_param(0), fixture.result()); + auto info = fixture.find_all<DenseInplaceMapFunction>(); + EXPECT_TRUE(info.empty()); +} + +TEST("require that mutable dense concrete tensors are optimized") { + TEST_DO(verify_optimized("map(_x5,f(x)(x+10))", 1)); + TEST_DO(verify_optimized("map(_x5y3,f(x)(x+10))", 1)); +} + +TEST("require that inplace map operations can be chained") { + TEST_DO(verify_optimized("map(map(_x5,f(x)(x+10)),f(x)(x-5))", 2)); +} + +TEST("require that abstract tensors are not optimized") { + TEST_DO(verify_not_optimized("map(_x5_u,f(x)(x+10))")); +} + +TEST("require that non-mutable tensors are not optimized") { + TEST_DO(verify_not_optimized("map(x5,f(x)(x+10))")); +} + +TEST("require that scalar values are not optimized") { + TEST_DO(verify_not_optimized("map(_d,f(x)(x+10))")); +} + +TEST("require that mapped tensors are not optimized") { + TEST_DO(verify_not_optimized("map(_x_m,f(x)(x+10))")); +} + +TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/eval/src/vespa/eval/eval/tensor_function.h b/eval/src/vespa/eval/eval/tensor_function.h index 442e082d9d9..2c3e9d21e0b 100644 --- a/eval/src/vespa/eval/eval/tensor_function.h +++ b/eval/src/vespa/eval/eval/tensor_function.h @@ -26,8 +26,8 @@ class Tensor; /** * Interface used to describe a tensor function as a tree of nodes - * with information about operation sequencing and intermediate result - * types. Each node in the tree describes a single tensor + * with information about operation sequencing and intermediate + * results. Each node in the tree describes a single tensor * operation. This is the intermediate representation of a tensor * function. * @@ -38,10 +38,14 @@ class Tensor; * * The generic tree will then be optimized (in-place, bottom-up) where * sub-expressions may be replaced with optimized - * implementation-specific alternatives. + * implementation-specific alternatives. Note that multiple nodes in + * the original representation can be replaced with a single + * specialized node in the optimized tree. * * This leaves us with a mixed-mode tree with some generic and some - * specialized nodes, that may be evaluated recursively. + * specialized nodes. This tree will then be compiled into a sequence + * of instructions (each node will map to a single instruction) and + * evaluated in the context of an interpreted function. **/ struct TensorFunction { @@ -65,6 +69,7 @@ struct TensorFunction void set(const TensorFunction &child) const { ptr = &child; } }; virtual const ValueType &result_type() const = 0; + virtual bool result_is_mutable() const { return false; } /** * Push references to all children (NB: implementation must use diff --git a/eval/src/vespa/eval/eval/test/eval_fixture.cpp b/eval/src/vespa/eval/eval/test/eval_fixture.cpp index b7e3764f6ce..bd4dcc37e3c 100644 --- a/eval/src/vespa/eval/eval/test/eval_fixture.cpp +++ b/eval/src/vespa/eval/eval/test/eval_fixture.cpp @@ -20,11 +20,44 @@ NodeTypes get_types(const Function &function, const ParamRepo ¶m_repo) { return NodeTypes(function, param_types); } -const TensorFunction &make_tfun(bool optimized, const TensorEngine &engine, const Function &function, - const NodeTypes &node_types, Stash &stash) -{ - const TensorFunction &plain_fun = make_tensor_function(engine, function.root(), node_types, stash); - return optimized ? engine.optimize(plain_fun, stash) : plain_fun; +std::set<size_t> get_mutable(const Function &function, const ParamRepo ¶m_repo) { + std::set<size_t> mutable_set; + for (size_t i = 0; i < function.num_params(); ++i) { + auto pos = param_repo.map.find(function.param_name(i)); + ASSERT_TRUE(pos != param_repo.map.end()); + if (pos->second.is_mutable) { + mutable_set.insert(i); + } + } + return mutable_set; +} + +struct MyMutableInject : public tensor_function::Inject { + MyMutableInject(const ValueType &result_type_in, size_t param_idx_in) + : Inject(result_type_in, param_idx_in) {} + bool result_is_mutable() const override { return true; } +}; + +const TensorFunction &maybe_patch(bool allow_mutable, const TensorFunction &plain_fun, const std::set<size_t> &mutable_set, Stash &stash) { + using Child = TensorFunction::Child; + if (!allow_mutable) { + return plain_fun; + } + Child root(plain_fun); + std::vector<Child::CREF> nodes({root}); + for (size_t i = 0; i < nodes.size(); ++i) { + nodes[i].get().get().push_children(nodes); + } + while (!nodes.empty()) { + const Child &child = nodes.back(); + if (auto inject = as<tensor_function::Inject>(child.get())) { + if (mutable_set.count(inject->param_idx()) > 0) { + child.set(stash.create<MyMutableInject>(inject->result_type(), inject->param_idx())); + } + } + nodes.pop_back(); + } + return root.get(); } std::vector<Value::UP> make_params(const TensorEngine &engine, const Function &function, @@ -53,12 +86,16 @@ std::vector<Value::CREF> get_refs(const std::vector<Value::UP> &values) { EvalFixture::EvalFixture(const TensorEngine &engine, const vespalib::string &expr, const ParamRepo ¶m_repo, - bool optimized) + bool optimized, + bool allow_mutable) : _engine(engine), _stash(), _function(Function::parse(expr)), _node_types(get_types(_function, param_repo)), - _tensor_function(make_tfun(optimized, _engine, _function, _node_types, _stash)), + _mutable_set(get_mutable(_function, param_repo)), + _plain_tensor_function(make_tensor_function(_engine, _function.root(), _node_types, _stash)), + _patched_tensor_function(maybe_patch(allow_mutable, _plain_tensor_function, _mutable_set, _stash)), + _tensor_function(optimized ? _engine.optimize(_patched_tensor_function, _stash) : _patched_tensor_function), _ifun(_engine, _tensor_function), _ictx(_ifun), _param_values(make_params(_engine, _function, param_repo)), @@ -69,4 +106,17 @@ EvalFixture::EvalFixture(const TensorEngine &engine, ASSERT_TRUE(!result_type.is_error()); } +const TensorSpec +EvalFixture::get_param(size_t idx) const +{ + ASSERT_LESS(idx, _param_values.size()); + return _engine.to_spec(*(_param_values[idx])); +} + +size_t +EvalFixture::num_params() const +{ + return _param_values.size(); +} + } // namespace vespalib::eval::test diff --git a/eval/src/vespa/eval/eval/test/eval_fixture.h b/eval/src/vespa/eval/eval/test/eval_fixture.h index 1f864e980cc..e20d435f608 100644 --- a/eval/src/vespa/eval/eval/test/eval_fixture.h +++ b/eval/src/vespa/eval/eval/test/eval_fixture.h @@ -9,6 +9,7 @@ #include <vespa/eval/eval/simple_tensor_engine.h> #include <vespa/eval/tensor/default_tensor_engine.h> #include <vespa/vespalib/util/stash.h> +#include <set> namespace vespalib::eval::test { @@ -18,23 +19,30 @@ public: struct Param { TensorSpec value; // actual parameter value vespalib::string type; // pre-defined type (could be abstract) - Param(TensorSpec value_in) - : value(std::move(value_in)), type(value.type()) {} - Param(TensorSpec value_in, const vespalib::string &type_in) - : value(std::move(value_in)), type(type_in) {} + bool is_mutable; // input will be mutable (if allow_mutable is true) + Param(TensorSpec value_in, const vespalib::string &type_in, bool is_mutable_in) + : value(std::move(value_in)), type(type_in), is_mutable(is_mutable_in) {} ~Param() {} }; struct ParamRepo { std::map<vespalib::string,Param> map; ParamRepo() : map() {} - ParamRepo &add(const vespalib::string &name, TensorSpec value_in) { - map.insert_or_assign(name, Param(std::move(value_in))); + ParamRepo &add(const vespalib::string &name, TensorSpec value_in, const vespalib::string &type_in, bool is_mutable_in) { + map.insert_or_assign(name, Param(std::move(value_in), type_in, is_mutable_in)); return *this; } - ParamRepo &add(const vespalib::string &name, TensorSpec value_in, const vespalib::string &type_in) { - map.insert_or_assign(name, Param(std::move(value_in), type_in)); - return *this; + ParamRepo &add(const vespalib::string &name, TensorSpec value, const vespalib::string &type) { + return add(name, value, type, false); + } + ParamRepo &add_mutable(const vespalib::string &name, TensorSpec value, const vespalib::string &type) { + return add(name, value, type, true); + } + ParamRepo &add(const vespalib::string &name, const TensorSpec &value) { + return add(name, value, value.type(), false); + } + ParamRepo &add_mutable(const vespalib::string &name, const TensorSpec &value) { + return add(name, value, value.type(), true); } ~ParamRepo() {} }; @@ -44,6 +52,9 @@ private: Stash _stash; Function _function; NodeTypes _node_types; + std::set<size_t> _mutable_set; + const TensorFunction &_plain_tensor_function; + const TensorFunction &_patched_tensor_function; const TensorFunction &_tensor_function; InterpretedFunction _ifun; InterpretedFunction::Context _ictx; @@ -64,7 +75,8 @@ private: } public: - EvalFixture(const TensorEngine &engine, const vespalib::string &expr, const ParamRepo ¶m_repo, bool optimized); + EvalFixture(const TensorEngine &engine, const vespalib::string &expr, const ParamRepo ¶m_repo, + bool optimized = true, bool allow_mutable = false); ~EvalFixture() {} template <typename T> std::vector<const T *> find_all() { @@ -73,11 +85,13 @@ public: return list; } const TensorSpec &result() const { return _result; } + const TensorSpec get_param(size_t idx) const; + size_t num_params() const; static TensorSpec ref(const vespalib::string &expr, const ParamRepo ¶m_repo) { - return EvalFixture(SimpleTensorEngine::ref(), expr, param_repo, false).result(); + return EvalFixture(SimpleTensorEngine::ref(), expr, param_repo, false, false).result(); } static TensorSpec prod(const vespalib::string &expr, const ParamRepo ¶m_repo) { - return EvalFixture(tensor::DefaultTensorEngine::ref(), expr, param_repo, true).result(); + return EvalFixture(tensor::DefaultTensorEngine::ref(), expr, param_repo, true, false).result(); } }; diff --git a/eval/src/vespa/eval/tensor/default_tensor_engine.cpp b/eval/src/vespa/eval/tensor/default_tensor_engine.cpp index 7b4a502dd4d..457e9310b80 100644 --- a/eval/src/vespa/eval/tensor/default_tensor_engine.cpp +++ b/eval/src/vespa/eval/tensor/default_tensor_engine.cpp @@ -10,6 +10,8 @@ #include "dense/dense_dot_product_function.h" #include "dense/dense_xw_product_function.h" #include "dense/dense_fast_rename_function.h" +#include "dense/dense_inplace_join_function.h" +#include "dense/dense_inplace_map_function.h" #include "dense/vector_from_doubles_function.h" #include <vespa/eval/eval/value.h> #include <vespa/eval/eval/tensor_spec.h> @@ -223,6 +225,8 @@ DefaultTensorEngine::optimize(const TensorFunction &expr, Stash &stash) const child.set(DenseDotProductFunction::optimize(child.get(), stash)); child.set(DenseXWProductFunction::optimize(child.get(), stash)); child.set(DenseFastRenameFunction::optimize(child.get(), stash)); + child.set(DenseInplaceMapFunction::optimize(child.get(), stash)); + child.set(DenseInplaceJoinFunction::optimize(child.get(), stash)); nodes.pop_back(); } return root.get(); diff --git a/eval/src/vespa/eval/tensor/dense/CMakeLists.txt b/eval/src/vespa/eval/tensor/dense/CMakeLists.txt index 73315b7a120..f78e49dc2f3 100644 --- a/eval/src/vespa/eval/tensor/dense/CMakeLists.txt +++ b/eval/src/vespa/eval/tensor/dense/CMakeLists.txt @@ -3,6 +3,8 @@ vespa_add_library(eval_tensor_dense OBJECT SOURCES dense_dot_product_function.cpp dense_fast_rename_function.cpp + dense_inplace_join_function.cpp + dense_inplace_map_function.cpp dense_tensor.cpp dense_tensor_address_combiner.cpp dense_tensor_builder.cpp diff --git a/eval/src/vespa/eval/tensor/dense/dense_inplace_join_function.cpp b/eval/src/vespa/eval/tensor/dense/dense_inplace_join_function.cpp new file mode 100644 index 00000000000..53a5fe9bb27 --- /dev/null +++ b/eval/src/vespa/eval/tensor/dense/dense_inplace_join_function.cpp @@ -0,0 +1,87 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include "dense_inplace_join_function.h" +#include "dense_tensor.h" +#include "dense_tensor_view.h" +#include <vespa/eval/eval/value.h> +#include <vespa/eval/tensor/tensor.h> + +namespace vespalib::tensor { + +using CellsRef = DenseTensorView::CellsRef; +using eval::Value; +using eval::ValueType; +using eval::TensorFunction; +using eval::as; +using namespace eval::tensor_function; + +namespace { + +CellsRef getCellsRef(const eval::Value &value) { + const DenseTensorView &denseTensor = static_cast<const DenseTensorView &>(value); + return denseTensor.cellsRef(); +} + +template <bool write_left> +void my_inplace_join_op(eval::InterpretedFunction::State &state, uint64_t param) { + join_fun_t function = (join_fun_t)param; + CellsRef lhs_cells = getCellsRef(state.peek(1)); + CellsRef rhs_cells = getCellsRef(state.peek(0)); + auto dst_cells = unconstify(write_left ? lhs_cells : rhs_cells); + for (size_t i = 0; i < dst_cells.size(); ++i) { + dst_cells[i] = function(lhs_cells[i], rhs_cells[i]); + } + if (write_left) { + state.stack.pop_back(); + } else { + const Value &result = state.stack.back(); + state.pop_pop_push(result); + } +} + +bool sameShapeConcreteDenseTensors(const ValueType &a, const ValueType &b) { + return (a.is_dense() && !a.is_abstract() && (a == b)); +} + +} // namespace vespalib::tensor::<unnamed> + + +DenseInplaceJoinFunction::DenseInplaceJoinFunction(const ValueType &result_type, + const TensorFunction &lhs, + const TensorFunction &rhs, + join_fun_t function_in, + bool write_left_in) + : eval::tensor_function::Op2(result_type, lhs, rhs), + _function(function_in), + _write_left(write_left_in) +{ +} + +DenseInplaceJoinFunction::~DenseInplaceJoinFunction() +{ +} + +eval::InterpretedFunction::Instruction +DenseInplaceJoinFunction::compile_self(Stash &) const +{ + auto op = _write_left ? my_inplace_join_op<true> : my_inplace_join_op<false>; + return eval::InterpretedFunction::Instruction(op, (uint64_t)_function); +} + +const TensorFunction & +DenseInplaceJoinFunction::optimize(const eval::TensorFunction &expr, Stash &stash) +{ + if (auto join = as<Join>(expr)) { + const TensorFunction &lhs = join->lhs(); + const TensorFunction &rhs = join->rhs(); + if ((lhs.result_is_mutable() || rhs.result_is_mutable()) && + sameShapeConcreteDenseTensors(lhs.result_type(), rhs.result_type())) + { + return stash.create<DenseInplaceJoinFunction>(join->result_type(), lhs, rhs, + join->function(), lhs.result_is_mutable()); + } + } + return expr; +} + +} // namespace vespalib::tensor diff --git a/eval/src/vespa/eval/tensor/dense/dense_inplace_join_function.h b/eval/src/vespa/eval/tensor/dense/dense_inplace_join_function.h new file mode 100644 index 00000000000..de2cdae3778 --- /dev/null +++ b/eval/src/vespa/eval/tensor/dense/dense_inplace_join_function.h @@ -0,0 +1,33 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#pragma once + +#include <vespa/eval/eval/tensor_function.h> + +namespace vespalib::tensor { + +/** + * Tensor function for inplace join operation on mutable dense tensors. + **/ +class DenseInplaceJoinFunction : public eval::tensor_function::Op2 +{ +public: + using join_fun_t = ::vespalib::eval::tensor_function::join_fun_t; +private: + join_fun_t _function; + bool _write_left; +public: + DenseInplaceJoinFunction(const eval::ValueType &result_type, + const TensorFunction &lhs, + const TensorFunction &rhs, + join_fun_t function_in, + bool write_left_in); + ~DenseInplaceJoinFunction(); + join_fun_t function() const { return _function; } + bool write_left() const { return _write_left; } + bool result_is_mutable() const override { return true; } + eval::InterpretedFunction::Instruction compile_self(Stash &stash) const override; + static const eval::TensorFunction &optimize(const eval::TensorFunction &expr, Stash &stash); +}; + +} // namespace vespalib::tensor diff --git a/eval/src/vespa/eval/tensor/dense/dense_inplace_map_function.cpp b/eval/src/vespa/eval/tensor/dense/dense_inplace_map_function.cpp new file mode 100644 index 00000000000..162bdb2ebfe --- /dev/null +++ b/eval/src/vespa/eval/tensor/dense/dense_inplace_map_function.cpp @@ -0,0 +1,67 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include "dense_inplace_map_function.h" +#include "dense_tensor.h" +#include "dense_tensor_view.h" +#include <vespa/eval/eval/value.h> +#include <vespa/eval/tensor/tensor.h> + +namespace vespalib::tensor { + +using CellsRef = DenseTensorView::CellsRef; +using eval::Value; +using eval::ValueType; +using eval::TensorFunction; +using eval::as; +using namespace eval::tensor_function; + +namespace { + +ArrayRef<double> getMutableCells(const eval::Value &value) { + const DenseTensorView &denseTensor = static_cast<const DenseTensorView &>(value); + return unconstify(denseTensor.cellsRef()); +} + +void my_inplace_map_op(eval::InterpretedFunction::State &state, uint64_t param) { + map_fun_t function = (map_fun_t)param; + for (double &cell: getMutableCells(state.peek(0))) { + cell = function(cell); + } +} + +bool isConcreteDenseTensor(const ValueType &type) { + return (type.is_dense() && !type.is_abstract()); +} + +} // namespace vespalib::tensor::<unnamed> + +DenseInplaceMapFunction::DenseInplaceMapFunction(const eval::ValueType &result_type, + const eval::TensorFunction &child, + map_fun_t function_in) + : eval::tensor_function::Op1(result_type, child), + _function(function_in) +{ +} + +DenseInplaceMapFunction::~DenseInplaceMapFunction() +{ +} + +eval::InterpretedFunction::Instruction +DenseInplaceMapFunction::compile_self(Stash &) const +{ + return eval::InterpretedFunction::Instruction(my_inplace_map_op, (uint64_t)_function); +} + +const TensorFunction & +DenseInplaceMapFunction::optimize(const eval::TensorFunction &expr, Stash &stash) +{ + if (auto map = as<Map>(expr)) { + if (map->child().result_is_mutable() && isConcreteDenseTensor(map->result_type())) { + return stash.create<DenseInplaceMapFunction>(map->result_type(), map->child(), map->function()); + } + } + return expr; +} + +} // namespace vespalib::tensor diff --git a/eval/src/vespa/eval/tensor/dense/dense_inplace_map_function.h b/eval/src/vespa/eval/tensor/dense/dense_inplace_map_function.h new file mode 100644 index 00000000000..f02f83edae1 --- /dev/null +++ b/eval/src/vespa/eval/tensor/dense/dense_inplace_map_function.h @@ -0,0 +1,29 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#pragma once + +#include <vespa/eval/eval/tensor_function.h> + +namespace vespalib::tensor { + +/** + * Tensor function for inplace map operation on mutable dense tensors. + **/ +class DenseInplaceMapFunction : public eval::tensor_function::Op1 +{ +public: + using map_fun_t = ::vespalib::eval::tensor_function::map_fun_t; +private: + map_fun_t _function; +public: + DenseInplaceMapFunction(const eval::ValueType &result_type, + const eval::TensorFunction &child, + map_fun_t function_in); + ~DenseInplaceMapFunction(); + map_fun_t function() const { return _function; } + bool result_is_mutable() const override { return true; } + eval::InterpretedFunction::Instruction compile_self(Stash &stash) const override; + static const eval::TensorFunction &optimize(const eval::TensorFunction &expr, Stash &stash); +}; + +} // namespace vespalib::tensor diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/SecurityRequestFilterChain.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/SecurityRequestFilterChain.java index 993c87f63b5..2d97bbdc494 100644 --- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/SecurityRequestFilterChain.java +++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/SecurityRequestFilterChain.java @@ -69,7 +69,7 @@ public final class SecurityRequestFilterChain extends AbstractResource implement } } - /** Returns an unmodifiable viuew of the filters in this */ + /** Returns an unmodifiable view of the filters in this */ public List<SecurityRequestFilter> getFilters() { return Collections.unmodifiableList(filters); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/containerdata/ConfigServerContainerData.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/containerdata/ConfigServerContainerData.java index 2ddfebea36f..3a41bc3e2da 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/containerdata/ConfigServerContainerData.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/containerdata/ConfigServerContainerData.java @@ -35,8 +35,14 @@ public class ConfigServerContainerData { " <environment>" + environment.getEnvironment() + "</environment>\n" + " <region>" + environment.getRegion() + "</region>\n" + " <hostedVespa>true</hostedVespa>\n" + - " <defaultFlavor>t2.xlarge</defaultFlavor>\n" + // TODO: Avoid hardcoding + " <multitenant>true</multitenant>\n" + + " <useVespaVersionInRequest>true</useVespaVersionInRequest>\n" + + // TODO: Avoid hardcoding of default flavor + " <defaultFlavor>t2.xlarge</defaultFlavor>\n" + createZookeeperServers() + + " <zookeeper>\n" + + " <barrierTimeout>1200</barrierTimeout>\n" + + " </zookeeper>\n" + " <serverId>" + configServerNodeHostName + "</serverId>\n" + " <nodeAdminInContainer>false</nodeAdminInContainer>\n" + "</config>\n"; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/Authorizer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/Authorizer.java index 8ea26d053e3..55a1a1e620d 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/Authorizer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/Authorizer.java @@ -9,15 +9,20 @@ import org.apache.http.client.utils.URLEncodedUtils; import java.net.URI; import java.nio.charset.StandardCharsets; -import java.nio.file.Paths; import java.security.Principal; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.function.BiPredicate; import java.util.stream.Collectors; /** - * An authorizer for the node-repository REST API. This contains the authorization rules for all API paths in this - * module. + * Authorizer for the node-repository and orchestrator REST APIs. This contains the authorization rules for all API + * paths. + * + * Ideally, the authorization rules for orchestrator APIs should live in the orchestrator module. However, the node + * repository is required to make decisions in some cases, which is not accessible in the orchestrator module. * * @author mpolden */ @@ -40,12 +45,7 @@ public class Authorizer implements BiPredicate<Principal, URI> { } // Nodes can only access its own resources - if (isNodeResource(uri) && canAccess(hostnameFrom(uri), principal)) { - return true; - } - - // For resources that support filtering, nodes can only apply filter to themselves and their children - if (supportsFiltering(uri) && canAccess(hostnamesFrom(uri), principal)) { + if (canAccess(hostnamesFrom(uri), principal)) { return true; } @@ -54,10 +54,17 @@ public class Authorizer implements BiPredicate<Principal, URI> { /** Returns whether principal can access node identified by hostname */ private boolean canAccess(String hostname, Principal principal) { + // Ignore potential path traversal. Node repository happily passes arguments unsanitized all the way down to + // curator... + if (hostname.chars().allMatch(c -> c == '.')) { + return false; + } + // Node can always access itself if (principal.getName().equals(hostname)) { return true; } + // Parent node can access its children return nodeRepository.getNode(hostname) .flatMap(Node::parentHostname) @@ -78,13 +85,8 @@ public class Authorizer implements BiPredicate<Principal, URI> { return "vespa.vespa.hosting"; } - /** Returns the last element (basename) of given path */ - private static String hostnameFrom(URI uri) { - return Paths.get(uri.getPath()).getFileName().toString(); - } - /** Returns hostnames contained in query parameters of given URI */ - private static List<String> hostnamesFrom(URI uri) { + private static List<String> hostnamesFromQuery(URI uri) { return URLEncodedUtils.parse(uri, StandardCharsets.UTF_8.name()) .stream() .filter(pair -> "hostname".equals(pair.getName()) || @@ -94,17 +96,29 @@ public class Authorizer implements BiPredicate<Principal, URI> { .collect(Collectors.toList()); } - /** Returns whether given URI is a node-specific resource, e.g. /nodes/v2/node/node1.fqdn */ - private static boolean isNodeResource(URI uri) { - return isChildOf("/nodes/v2/acl/", uri.getPath()) || - isChildOf("/nodes/v2/node/", uri.getPath()) || - isChildOf("/nodes/v2/state/", uri.getPath()); - } - - /** Returns whether given path supports filtering through query parameters */ - private static boolean supportsFiltering(URI uri) { - return isChildOf("/nodes/v2/command/", uri.getPath()) || - "/nodes/v2/node/".equals(uri.getPath()); + /** Returns hostnames from a URI if any, e.g. /nodes/v2/node/node1.fqdn */ + private static List<String> hostnamesFrom(URI uri) { + if (isChildOf("/nodes/v2/acl/", uri.getPath()) || + isChildOf("/nodes/v2/node/", uri.getPath()) || + isChildOf("/nodes/v2/state/", uri.getPath())) { + return Collections.singletonList(lastChildOf(uri.getPath())); + } + if (isChildOf("/orchestrator/v1/hosts/", uri.getPath())) { + return firstChildOf("/orchestrator/v1/hosts/", uri.getPath()) + .map(Collections::singletonList) + .orElseGet(Collections::emptyList); + } + if (isChildOf("/orchestrator/v1/suspensions/hosts/", uri.getPath())) { + List<String> hostnames = new ArrayList<>(); + hostnames.add(lastChildOf(uri.getPath())); + hostnames.addAll(hostnamesFromQuery(uri)); + return hostnames; + } + if (isChildOf("/nodes/v2/command/", uri.getPath()) || + "/nodes/v2/node/".equals(uri.getPath())) { + return hostnamesFromQuery(uri); + } + return Collections.emptyList(); } /** Returns whether child is a sub-path of parent */ @@ -112,4 +126,29 @@ public class Authorizer implements BiPredicate<Principal, URI> { return child.startsWith(parent) && child.length() > parent.length(); } + /** Returns the first component of path relative to root */ + private static Optional<String> firstChildOf(String root, String path) { + if (!isChildOf(root, path)) { + return Optional.empty(); + } + path = path.substring(root.length(), path.length()); + int firstSeparator = path.indexOf('/'); + if (firstSeparator == -1) { + return Optional.of(path); + } + return Optional.of(path.substring(0, firstSeparator)); + } + + /** Returns the last component of the given path */ + private static String lastChildOf(String path) { + if (path.endsWith("/")) { + path = path.substring(0, path.length() - 1); + } + int lastSeparator = path.lastIndexOf("/"); + if (lastSeparator == - 1) { + return path; + } + return path.substring(lastSeparator + 1, path.length()); + } + } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/AuthorizerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/AuthorizerTest.java index 7f7a26cbcb2..097c8d92165 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/AuthorizerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/AuthorizerTest.java @@ -1,14 +1,24 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.provision.restapi.v2; +import com.yahoo.config.provision.Flavor; +import com.yahoo.config.provision.NodeFlavors; +import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.curator.mock.MockCurator; +import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.testutils.MockNodeFlavors; import com.yahoo.vespa.hosted.provision.testutils.MockNodeRepository; import org.junit.Before; import org.junit.Test; import java.net.URI; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -23,12 +33,30 @@ public class AuthorizerTest { @Before public void before() { - nodeRepository = new MockNodeRepository(new MockCurator(), new MockNodeFlavors()); + NodeFlavors flavors = new MockNodeFlavors(); + nodeRepository = new MockNodeRepository(new MockCurator(), flavors); authorizer = new Authorizer(SystemName.main, nodeRepository); + { // Populate with nodes used in this test. Note that only nodes requiring node repository lookup are added here + Set<String> ipAddresses = new HashSet<>(Arrays.asList("127.0.0.1", "::1")); + Flavor flavor = flavors.getFlavorOrThrow("default"); + List<Node> nodes = new ArrayList<>(); + nodes.add(nodeRepository.createNode("host1", "host1", ipAddresses, + Optional.empty(), flavor, NodeType.host)); + nodes.add(nodeRepository.createNode("child1-1", "child1-1", ipAddresses, + Optional.of("host1"), flavor, NodeType.tenant)); + nodes.add(nodeRepository.createNode("child1-2", "child1-2", ipAddresses, + Optional.of("host1"), flavor, NodeType.tenant)); + + nodes.add(nodeRepository.createNode("host2", "host2", ipAddresses, + Optional.empty(), flavor, NodeType.host)); + nodes.add(nodeRepository.createNode("child2-1", "child2-1", ipAddresses, + Optional.of("host1.tld"), flavor, NodeType.tenant)); + nodeRepository.addNodes(nodes); + } } @Test - public void authorization() { + public void nodes_authorization() { // Empty principal assertFalse(authorized("", "")); assertFalse(authorized("", "/")); @@ -41,6 +69,9 @@ public class AuthorizerTest { assertFalse(authorized("node1", "/nodes/v2/node/node2")); assertFalse(authorized("node1", "/nodes/v2/state/dirty/")); assertFalse(authorized("node1", "/nodes/v2/state/dirty/node2")); + // Path traversal fails gracefully + assertFalse(authorized("node1", "/nodes/v2/node/.")); + assertFalse(authorized("node1", "/nodes/v2/node/..")); assertFalse(authorized("node1", "/nodes/v2/acl/node2")); assertFalse(authorized("node1", "/nodes/v2/node/?parentHost=node2")); // Node resource always takes precedence over filter @@ -55,11 +86,11 @@ public class AuthorizerTest { assertTrue(authorized("node1", "/nodes/v2/node/?parentHost=node1")); // Host node can access itself and its children - assertFalse(authorized("dockerhost1.yahoo.com", "/nodes/v2/node/host5.yahoo.com")); - assertFalse(authorized("dockerhost1.yahoo.com", "/nodes/v2/command/reboot?hostname=host5.yahoo.com")); - assertTrue(authorized("dockerhost1.yahoo.com", "/nodes/v2/node/dockerhost1.yahoo.com")); - assertTrue(authorized("dockerhost1.yahoo.com", "/nodes/v2/node/host4.yahoo.com")); - assertTrue(authorized("dockerhost1.yahoo.com", "/nodes/v2/command/reboot?hostname=host4.yahoo.com")); + assertFalse(authorized("host1", "/nodes/v2/node/child2-1")); + assertFalse(authorized("host1", "/nodes/v2/command/reboot?hostname=child2-1")); + assertTrue(authorized("host1", "/nodes/v2/node/host1")); + assertTrue(authorized("host1", "/nodes/v2/node/child1-1")); + assertTrue(authorized("host1", "/nodes/v2/command/reboot?hostname=child1-1")); // Trusted services can access everything in their own system assertFalse(authorized("vespa.vespa.cd.hosting", "/")); // Wrong system @@ -69,6 +100,30 @@ public class AuthorizerTest { assertTrue(authorized("vespa.vespa.hosting", "/nodes/v2/node/node1")); } + @Test + public void orchestrator_authorization() { + // Node can only access its own resources + assertFalse(authorized("node1", "/orchestrator/v1/hosts")); + assertFalse(authorized("node1", "/orchestrator/v1/hosts/")); + assertFalse(authorized("node1", "/orchestrator/v1/hosts/node2")); + assertFalse(authorized("node1", "/orchestrator/v1/hosts/node2/suspended")); + + // Node can suspend itself + assertTrue(authorized("node1", "/orchestrator/v1/hosts/node1")); + assertTrue(authorized("node1", "/orchestrator/v1/hosts/node1/suspended")); + + // Host node can suspend itself and its children + assertFalse(authorized("host1", "/orchestrator/v1/hosts/child2-1/suspended")); + assertFalse(authorized("host1", "/orchestrator/v1/suspensions/hosts/host1?hostname=child2-1")); + // All given hostnames must be children + assertFalse(authorized("host1", "/orchestrator/v1/suspensions/hosts/host1?hostname=child1-1&hostname=child2-1")); + assertTrue(authorized("host1", "/orchestrator/v1/hosts/host1/suspended")); + assertTrue(authorized("host1", "/orchestrator/v1/hosts/child1-1/suspended")); + assertTrue(authorized("host1", "/orchestrator/v1/suspensions/hosts/host1?hostname=child1-1")); + // Multiple children + assertTrue(authorized("host1", "/orchestrator/v1/suspensions/hosts/host1?hostname=child1-1&hostname=child1-2")); + } + private boolean authorized(String principal, String path) { return authorizer.test(() -> principal, uri(path)); } diff --git a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostApi.java b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostApi.java index ad0f3e094eb..1c4d138acef 100644 --- a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostApi.java +++ b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostApi.java @@ -14,9 +14,7 @@ import javax.ws.rs.PUT; import javax.ws.rs.Path; import javax.ws.rs.PathParam; import javax.ws.rs.Produces; -import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; -import javax.ws.rs.core.UriInfo; /** * Definition of Orchestrator's REST API for hosts. diff --git a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java index a5ca15d2d15..a9846134eff 100644 --- a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java +++ b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java @@ -6,9 +6,16 @@ import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult; import javax.ws.rs.Consumes; import javax.ws.rs.PUT; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; import javax.ws.rs.Produces; +import javax.ws.rs.QueryParam; import javax.ws.rs.core.MediaType; +import java.util.List; +/** + * @author hakonhall + */ public interface HostSuspensionApi { /** * Path prefix for this api. Resources implementing this API should use this with a @Path annotation. @@ -27,5 +34,13 @@ public interface HostSuspensionApi { @PUT @Produces(MediaType.APPLICATION_JSON) @Consumes(MediaType.APPLICATION_JSON) + @Deprecated // TODO: Remove after 2018-04-01 BatchOperationResult suspendAll(BatchHostSuspendRequest request); + + @PUT + @Path("/{hostname}") + @Produces(MediaType.APPLICATION_JSON) + @Consumes(MediaType.APPLICATION_JSON) + BatchOperationResult suspendAll(@PathParam("hostname") String parentHostname, + @QueryParam("hostname") List<String> hostnames); } diff --git a/orchestrator/pom.xml b/orchestrator/pom.xml index ae22b6718f9..3559e4282c3 100644 --- a/orchestrator/pom.xml +++ b/orchestrator/pom.xml @@ -131,6 +131,16 @@ <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> + <configuration> + <compilerArgs> + <arg>-Xlint:all</arg> + <arg>-Xlint:-deprecation</arg> + <arg>-Xlint:-serial</arg> + <arg>-Xlint:-varargs</arg> + <arg>-Xlint:-try</arg> + <arg>-Werror</arg> + </compilerArgs> + </configuration> </plugin> </plugins> </build> diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionResource.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionResource.java index 85166ecc741..c1e312ddee6 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionResource.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/HostSuspensionResource.java @@ -21,6 +21,9 @@ import java.util.List; import java.util.logging.Logger; import java.util.stream.Collectors; +/** + * @author hakonhall + */ @Path(HostSuspensionApi.PATH_PREFIX) public class HostSuspensionResource implements HostSuspensionApi { @@ -47,26 +50,28 @@ public class HostSuspensionResource implements HostSuspensionApi { log.log(LogLevel.DEBUG, message); throw createWebApplicationException(message, Response.Status.BAD_REQUEST); } + return suspendAll(parentHostnameString, hostnamesAsStrings); + } + @Override + public BatchOperationResult suspendAll(String parentHostnameString, List<String> hostnamesAsStrings) { HostName parentHostname = new HostName(parentHostnameString); - List<HostName> hostNames = hostnamesAsStrings.stream().map(HostName::new).collect(Collectors.toList()); - + List<HostName> hostnames = hostnamesAsStrings.stream().map(HostName::new).collect(Collectors.toList()); try { - orchestrator.suspendAll(parentHostname, hostNames); + orchestrator.suspendAll(parentHostname, hostnames); } catch (BatchHostStateChangeDeniedException e) { - log.log(LogLevel.DEBUG, "Failed to suspend nodes " + hostNames + " with parent host " + parentHostname, e); + log.log(LogLevel.DEBUG, "Failed to suspend nodes " + hostnames + " with parent host " + parentHostname, e); throw createWebApplicationException(e.getMessage(), Response.Status.CONFLICT); } catch (BatchHostNameNotFoundException e) { - log.log(LogLevel.DEBUG, "Failed to suspend nodes " + hostNames + " with parent host " + parentHostname, e); + log.log(LogLevel.DEBUG, "Failed to suspend nodes " + hostnames + " with parent host " + parentHostname, e); // Note that we're returning BAD_REQUEST instead of NOT_FOUND because the resource identified // by the URL path was found. It's one of the hostnames in the request it failed to find. throw createWebApplicationException(e.getMessage(), Response.Status.BAD_REQUEST); } catch (BatchInternalErrorException e) { - log.log(LogLevel.DEBUG, "Failed to suspend nodes " + hostNames + " with parent host " + parentHostname, e); + log.log(LogLevel.DEBUG, "Failed to suspend nodes " + hostnames + " with parent host " + parentHostname, e); throw createWebApplicationException(e.getMessage(), Response.Status.INTERNAL_SERVER_ERROR); } - - log.log(LogLevel.DEBUG, "Suspended " + hostNames + " with parent " + parentHostname); + log.log(LogLevel.DEBUG, "Suspended " + hostnames + " with parent " + parentHostname); return BatchOperationResult.successResult(); } diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java index 65309440aee..2c7db25ae30 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java @@ -21,7 +21,6 @@ import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactoryMoc import com.yahoo.vespa.orchestrator.model.ApplicationApi; import com.yahoo.vespa.orchestrator.policy.HostStateChangeDeniedException; import com.yahoo.vespa.orchestrator.policy.Policy; -import com.yahoo.vespa.orchestrator.restapi.wire.BatchHostSuspendRequest; import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult; import com.yahoo.vespa.orchestrator.restapi.wire.GetHostResponse; import com.yahoo.vespa.orchestrator.restapi.wire.PatchHostRequest; @@ -56,7 +55,11 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +/** + * @author hakonhall + */ public class HostResourceTest { + private static final int SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS = 0; private static final TenantId TENANT_ID = new TenantId("tenantId"); private static final ApplicationInstanceId APPLICATION_INSTANCE_ID = new ApplicationInstanceId("applicationId"); @@ -110,20 +113,20 @@ public class HostResourceTest { public void grantSuspensionRequest( ApplicationInstance applicationInstance, HostName hostName, - MutableStatusRegistry hostStatusService) throws HostStateChangeDeniedException { + MutableStatusRegistry hostStatusService) { } @Override - public void grantSuspensionRequest(ApplicationApi applicationApi) throws HostStateChangeDeniedException { + public void grantSuspensionRequest(ApplicationApi applicationApi) { } @Override - public void releaseSuspensionGrant(ApplicationApi application) throws HostStateChangeDeniedException { + public void releaseSuspensionGrant(ApplicationApi application) { } @Override - public void acquirePermissionToRemove(ApplicationApi applicationApi) throws HostStateChangeDeniedException { + public void acquirePermissionToRemove(ApplicationApi applicationApi) { } @Override @@ -151,7 +154,7 @@ public class HostResourceTest { private final UriInfo uriInfo = mock(UriInfo.class); @Test - public void returns_200_on_success() throws Exception { + public void returns_200_on_success() { HostResource hostResource = new HostResource(alwaysAllowOrchestrator, uriInfo); @@ -163,25 +166,23 @@ public class HostResourceTest { } @Test - public void returns_200_on_success_batch() throws Exception { + public void returns_200_on_success_batch() { HostSuspensionResource hostSuspensionResource = new HostSuspensionResource(alwaysAllowOrchestrator); - BatchHostSuspendRequest request = - new BatchHostSuspendRequest("parentHostname", Arrays.asList("hostname1", "hostname2")); - BatchOperationResult response = hostSuspensionResource.suspendAll(request); + BatchOperationResult response = hostSuspensionResource.suspendAll("parentHostname", + Arrays.asList("hostname1", "hostname2")); assertThat(response.success()); } @Test - public void returns_200_empty_batch() throws Exception { + public void returns_200_empty_batch() { HostSuspensionResource hostSuspensionResource = new HostSuspensionResource(alwaysAllowOrchestrator); - BatchHostSuspendRequest request = - new BatchHostSuspendRequest("parentHostname", Collections.emptyList()); - BatchOperationResult response = hostSuspensionResource.suspendAll(request); + BatchOperationResult response = hostSuspensionResource.suspendAll("parentHostname", + Collections.emptyList());; assertThat(response.success()); } @Test - public void throws_404_when_host_unknown() throws Exception { + public void throws_404_when_host_unknown() { try { HostResource hostResource = new HostResource(hostNotFoundOrchestrator, uriInfo); @@ -196,12 +197,10 @@ public class HostResourceTest { // This is so because the hostname is part of the URL path for single-host, while the // hostnames are part of the request body for multi-host. @Test - public void throws_400_when_host_unknown_for_batch() throws Exception { + public void throws_400_when_host_unknown_for_batch() { try { HostSuspensionResource hostSuspensionResource = new HostSuspensionResource(hostNotFoundOrchestrator); - BatchHostSuspendRequest request = - new BatchHostSuspendRequest("parentHostname", Arrays.asList("hostname1", "hostname2")); - hostSuspensionResource.suspendAll(request); + hostSuspensionResource.suspendAll("parentHostname", Arrays.asList("hostname1", "hostname2")); fail(); } catch (WebApplicationException w) { assertThat(w.getResponse().getStatus()).isEqualTo(400); @@ -249,7 +248,7 @@ public class HostResourceTest { } @Test - public void throws_409_when_request_rejected_by_policies() throws Exception { + public void throws_409_when_request_rejected_by_policies() { final OrchestratorImpl alwaysRejectResolver = new OrchestratorImpl( new AlwaysFailPolicy(), new ClusterControllerClientFactoryMock(), @@ -266,7 +265,7 @@ public class HostResourceTest { } @Test - public void throws_409_when_request_rejected_by_policies_for_batch() throws Exception { + public void throws_409_when_request_rejected_by_policies_for_batch() { final OrchestratorImpl alwaysRejectResolver = new OrchestratorImpl( new AlwaysFailPolicy(), new ClusterControllerClientFactoryMock(), @@ -276,9 +275,7 @@ public class HostResourceTest { try { HostSuspensionResource hostSuspensionResource = new HostSuspensionResource(alwaysRejectResolver); - BatchHostSuspendRequest request = - new BatchHostSuspendRequest("parentHostname", Arrays.asList("hostname1", "hostname2")); - hostSuspensionResource.suspendAll(request); + hostSuspensionResource.suspendAll("parentHostname", Arrays.asList("hostname1", "hostname2")); fail(); } catch (WebApplicationException w) { assertThat(w.getResponse().getStatus()).isEqualTo(409); diff --git a/searchcore/src/vespa/searchcore/config/proton.def b/searchcore/src/vespa/searchcore/config/proton.def index c7a3fc7cb62..5076b923482 100644 --- a/searchcore/src/vespa/searchcore/config/proton.def +++ b/searchcore/src/vespa/searchcore/config/proton.def @@ -90,7 +90,7 @@ flush.memory.conservative.lowwatermarkfactor double default=0.9 ## The prepare for restart flush strategy will choose a set of components to flush ## such that the cost of flushing these + the cost of replaying the transaction log ## is as low as possible. -flush.preparerestart.replaycost double default=2.0 +flush.preparerestart.replaycost double default=8.0 ## The cost of replaying an operation when replaying the transaction log. ## @@ -99,12 +99,12 @@ flush.preparerestart.replaycost double default=2.0 ## ## The default value is chosen based on the following example: ## Assume we can replay 9 MB/s and this corresponds to 24000 ops/s. -## replayoperationcost = (bytes to replay) * replaycost / (operations to replay) = 9 MB * 2.0 / 24000 = 750 +## replayoperationcost = (bytes to replay) * replaycost / (operations to replay) = 9 MB * 8.0 / 24000 = 3000 ## ## The prepare for restart flush strategy will choose a set of components to flush ## such that the cost of flushing these + the cost of replaying the transaction log ## is as low as possible. -flush.preparerestart.replayoperationcost double default=750.0 +flush.preparerestart.replayoperationcost double default=3000.0 ## The cost of writing a byte when flushing components to disk. ## diff --git a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzCredentials.java b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzCredentials.java index c5dce1c5b1d..8127ac9feb3 100644 --- a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzCredentials.java +++ b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzCredentials.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.athenz.identityprovider; import java.security.KeyPair; import java.security.cert.X509Certificate; -import java.time.Instant; /** * @author bjorncs @@ -14,18 +13,15 @@ class AthenzCredentials { private final X509Certificate certificate; private final KeyPair keyPair; private final SignedIdentityDocument identityDocument; - private final Instant createdAt; AthenzCredentials(String nToken, X509Certificate certificate, KeyPair keyPair, - SignedIdentityDocument identityDocument, - Instant createdAt) { + SignedIdentityDocument identityDocument) { this.nToken = nToken; this.certificate = certificate; this.keyPair = keyPair; this.identityDocument = identityDocument; - this.createdAt = createdAt; } String getNToken() { @@ -44,8 +40,5 @@ class AthenzCredentials { return identityDocument; } - Instant getCreatedAt() { - return createdAt; - } } diff --git a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzCredentialsService.java b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzCredentialsService.java index dd816929bfb..b9fb7e94782 100644 --- a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzCredentialsService.java +++ b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzCredentialsService.java @@ -79,7 +79,7 @@ class AthenzCredentialsService { SignedIdentityDocument identityDocument) { X509Certificate certificate = instanceIdentity.getX509Certificate(); String serviceToken = instanceIdentity.getServiceToken(); - return new AthenzCredentials(serviceToken, certificate, keyPair, identityDocument, clock.instant()); + return new AthenzCredentials(serviceToken, certificate, keyPair, identityDocument); } private static SignedIdentityDocument parseSignedIdentityDocument(String rawDocument) { diff --git a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzIdentityProviderImpl.java b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzIdentityProviderImpl.java index 95113e1b0b1..78ad95f84f3 100644 --- a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzIdentityProviderImpl.java +++ b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/identityprovider/AthenzIdentityProviderImpl.java @@ -19,6 +19,7 @@ import java.time.Duration; import java.time.Instant; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; @@ -33,35 +34,19 @@ public final class AthenzIdentityProviderImpl extends AbstractComponent implemen // TODO Make some of these values configurable through config. Match requested expiration of register/update requests. // TODO These should match the requested expiration - static final Duration EXPIRES_AFTER = Duration.ofDays(1); - static final Duration EXPIRATION_MARGIN = Duration.ofMinutes(30); - static final Duration INITIAL_WAIT_NTOKEN = Duration.ofMinutes(5); - static final Duration UPDATE_PERIOD = EXPIRES_AFTER.dividedBy(3); - static final Duration REDUCED_UPDATE_PERIOD = Duration.ofMinutes(30); - static final Duration INITIAL_BACKOFF_DELAY = Duration.ofMinutes(4); - static final Duration MAX_REGISTER_BACKOFF_DELAY = Duration.ofHours(1); - static final int BACKOFF_DELAY_MULTIPLIER = 2; + static final Duration UPDATE_PERIOD = Duration.ofDays(1); static final Duration AWAIT_TERMINTATION_TIMEOUT = Duration.ofSeconds(90); - private static final Duration CERTIFICATE_EXPIRY_METRIC_UPDATE_PERIOD = Duration.ofMinutes(5); - private static final String CERTIFICATE_EXPIRY_METRIC_NAME = "athenz-tenant-cert.expiry.seconds"; - - static final String REGISTER_INSTANCE_TAG = "register-instance"; - static final String UPDATE_CREDENTIALS_TAG = "update-credentials"; - static final String TIMEOUT_INITIAL_WAIT_TAG = "timeout-initial-wait"; - static final String METRICS_UPDATER_TAG = "metrics-updater"; - + public static final String CERTIFICATE_EXPIRY_METRIC_NAME = "athenz-tenant-cert.expiry.seconds"; private volatile AthenzCredentials credentials; - private final AtomicReference<Throwable> lastThrowable = new AtomicReference<>(); + private final Metric metric; private final AthenzCredentialsService athenzCredentialsService; - private final Scheduler scheduler; + private final ScheduledExecutorService scheduler; private final Clock clock; private final String domain; private final String service; - private final CertificateExpiryMetricUpdater metricUpdater; - @Inject public AthenzIdentityProviderImpl(IdentityConfig config, Metric metric) { this(config, @@ -70,7 +55,7 @@ public final class AthenzIdentityProviderImpl extends AbstractComponent implemen new IdentityDocumentService(config.loadBalancerAddress()), new AthenzService(), Clock.systemUTC()), - new ThreadPoolScheduler(), + new ScheduledThreadPoolExecutor(1), Clock.systemUTC()); } @@ -78,22 +63,22 @@ public final class AthenzIdentityProviderImpl extends AbstractComponent implemen AthenzIdentityProviderImpl(IdentityConfig config, Metric metric, AthenzCredentialsService athenzCredentialsService, - Scheduler scheduler, + ScheduledExecutorService scheduler, Clock clock) { + this.metric = metric; this.athenzCredentialsService = athenzCredentialsService; this.scheduler = scheduler; this.clock = clock; this.domain = config.domain(); this.service = config.service(); - metricUpdater = new CertificateExpiryMetricUpdater(metric); registerInstance(); } private void registerInstance() { try { credentials = athenzCredentialsService.registerInstance(); - scheduler.schedule(new UpdateCredentialsTask(), UPDATE_PERIOD); - scheduler.submit(metricUpdater); + scheduler.scheduleAtFixedRate(this::refreshCertificate, 0, 5, TimeUnit.MINUTES); + scheduler.scheduleAtFixedRate(this::reportMetrics, UPDATE_PERIOD.toMinutes(), UPDATE_PERIOD.toMinutes(), TimeUnit.MINUTES); } catch (Throwable t) { throw new AthenzIdentityProviderException("Could not retrieve Athenz credentials", t); } @@ -121,7 +106,12 @@ public final class AthenzIdentityProviderImpl extends AbstractComponent implemen @Override public void deconstruct() { - scheduler.shutdown(AWAIT_TERMINTATION_TIMEOUT); + try { + scheduler.shutdownNow(); + scheduler.awaitTermination(AWAIT_TERMINTATION_TIMEOUT.getSeconds(), TimeUnit.SECONDS); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } } private boolean isExpired(AthenzCredentials credentials) { @@ -129,96 +119,28 @@ public final class AthenzIdentityProviderImpl extends AbstractComponent implemen } private static Instant getExpirationTime(AthenzCredentials credentials) { - return credentials.getCreatedAt().plus(EXPIRES_AFTER).minus(EXPIRATION_MARGIN); + return credentials.getCertificate().getNotAfter().toInstant(); } - private class UpdateCredentialsTask implements RunnableWithTag { - @Override - public void run() { - try { - AthenzCredentials newCredentials = isExpired(credentials) - ? athenzCredentialsService.registerInstance() - : athenzCredentialsService.updateCredentials(credentials); - credentials = newCredentials; - scheduler.schedule(new UpdateCredentialsTask(), UPDATE_PERIOD); - } catch (Throwable t) { - log.log(LogLevel.WARNING, "Failed to update credentials: " + t.getMessage(), t); - lastThrowable.set(t); - Duration timeToExpiration = Duration.between(clock.instant(), getExpirationTime(credentials)); - // NOTE: Update period might be after timeToExpiration, still we do not want to DDoS Athenz. - Duration updatePeriod = - timeToExpiration.compareTo(UPDATE_PERIOD) > 0 ? UPDATE_PERIOD : REDUCED_UPDATE_PERIOD; - scheduler.schedule(new UpdateCredentialsTask(), updatePeriod); - } - } - - @Override - public String tag() { - return UPDATE_CREDENTIALS_TAG; + void refreshCertificate() { + try { + AthenzCredentials newCredentials = isExpired(credentials) + ? athenzCredentialsService.registerInstance() + : athenzCredentialsService.updateCredentials(credentials); + credentials = newCredentials; + } catch (Throwable t) { + log.log(LogLevel.WARNING, "Failed to update credentials: " + t.getMessage(), t); } } - private class CertificateExpiryMetricUpdater implements RunnableWithTag { - private final Metric metric; - - private CertificateExpiryMetricUpdater(Metric metric) { - this.metric = metric; - } - - @Override - public void run() { + void reportMetrics() { + try { Instant expirationTime = getExpirationTime(credentials); Duration remainingLifetime = Duration.between(clock.instant(), expirationTime); metric.set(CERTIFICATE_EXPIRY_METRIC_NAME, remainingLifetime.getSeconds(), null); - scheduler.schedule(this, CERTIFICATE_EXPIRY_METRIC_UPDATE_PERIOD); - } - - @Override - public String tag() { - return METRICS_UPDATER_TAG; - } - } - - private static class ThreadPoolScheduler implements Scheduler { - - private static final Logger log = Logger.getLogger(ThreadPoolScheduler.class.getName()); - - private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(0); - - @Override - public void schedule(RunnableWithTag runnable, Duration delay) { - log.log(LogLevel.FINE, String.format("Scheduling task '%s' in '%s'", runnable.tag(), delay)); - executor.schedule(runnable, delay.getSeconds(), TimeUnit.SECONDS); - } - - @Override - public void submit(RunnableWithTag runnable) { - log.log(LogLevel.FINE, String.format("Scheduling task '%s' now", runnable.tag())); - executor.submit(runnable); - } - - @Override - public void shutdown(Duration timeout) { - try { - executor.shutdownNow(); - executor.awaitTermination(AWAIT_TERMINTATION_TIMEOUT.getSeconds(), TimeUnit.SECONDS); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } + } catch (Throwable t) { + log.log(LogLevel.WARNING, "Failed to update metrics: " + t.getMessage(), t); } - - } - - public interface Scheduler { - void schedule(RunnableWithTag runnable, Duration delay); - default void submit(RunnableWithTag runnable) { schedule(runnable, Duration.ZERO); } - default void shutdown(Duration timeout) {} } - - public interface RunnableWithTag extends Runnable { - - String tag(); - } - } diff --git a/vespa-athenz/src/test/java/com/yahoo/vespa/athenz/identityprovider/AthenzIdentityProviderImplTest.java b/vespa-athenz/src/test/java/com/yahoo/vespa/athenz/identityprovider/AthenzIdentityProviderImplTest.java index d9dbd73a94e..3bb074cc1dc 100644 --- a/vespa-athenz/src/test/java/com/yahoo/vespa/athenz/identityprovider/AthenzIdentityProviderImplTest.java +++ b/vespa-athenz/src/test/java/com/yahoo/vespa/athenz/identityprovider/AthenzIdentityProviderImplTest.java @@ -2,34 +2,25 @@ package com.yahoo.vespa.athenz.identityprovider; import com.yahoo.container.core.identity.IdentityConfig; -import com.yahoo.container.jdisc.athenz.AthenzIdentityProvider; import com.yahoo.container.jdisc.athenz.AthenzIdentityProviderException; import com.yahoo.jdisc.Metric; import com.yahoo.test.ManualClock; -import com.yahoo.vespa.athenz.identityprovider.AthenzIdentityProviderImpl.RunnableWithTag; -import com.yahoo.vespa.athenz.identityprovider.AthenzIdentityProviderImpl.Scheduler; import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import java.security.cert.X509Certificate; import java.time.Duration; import java.time.Instant; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.PriorityQueue; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Predicate; - -import static com.yahoo.vespa.athenz.identityprovider.AthenzIdentityProviderImpl.METRICS_UPDATER_TAG; -import static com.yahoo.vespa.athenz.identityprovider.AthenzIdentityProviderImpl.REDUCED_UPDATE_PERIOD; -import static com.yahoo.vespa.athenz.identityprovider.AthenzIdentityProviderImpl.UPDATE_CREDENTIALS_TAG; -import static com.yahoo.vespa.athenz.identityprovider.AthenzIdentityProviderImpl.UPDATE_PERIOD; -import static org.junit.Assert.assertEquals; +import java.util.Date; +import java.util.concurrent.ScheduledExecutorService; +import java.util.function.Supplier; + import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -38,72 +29,79 @@ import static org.mockito.Mockito.when; */ public class AthenzIdentityProviderImplTest { - private static final Metric DUMMY_METRIC = new Metric() { - @Override - public void set(String s, Number number, Context context) { - } - - @Override - public void add(String s, Number number, Context context) { - } - - @Override - public Context createContext(Map<String, ?> stringMap) { - return null; - } - }; + public static final Duration certificateValidity = Duration.ofDays(30); private static final IdentityConfig IDENTITY_CONFIG = new IdentityConfig(new IdentityConfig.Builder() .service("tenantService").domain("tenantDomain").loadBalancerAddress("cfg")); - @Test (expected = AthenzIdentityProviderException.class) + @Test(expected = AthenzIdentityProviderException.class) public void component_creation_fails_when_credentials_not_found() { AthenzCredentialsService credentialService = mock(AthenzCredentialsService.class); when(credentialService.registerInstance()) .thenThrow(new RuntimeException("athenz unavailable")); - ManualClock clock = new ManualClock(Instant.EPOCH); - MockScheduler scheduler = new MockScheduler(clock); - AthenzIdentityProvider identityProvider = - new AthenzIdentityProviderImpl(IDENTITY_CONFIG, DUMMY_METRIC, credentialService, scheduler, clock); + new AthenzIdentityProviderImpl(IDENTITY_CONFIG, mock(Metric.class), credentialService, mock(ScheduledExecutorService.class), new ManualClock(Instant.EPOCH)); } @Test - public void failed_credentials_updates_will_schedule_retries() { + public void metrics_updated_on_refresh() { IdentityDocumentService identityDocumentService = mock(IdentityDocumentService.class); AthenzService athenzService = mock(AthenzService.class); ManualClock clock = new ManualClock(Instant.EPOCH); - MockScheduler scheduler = new MockScheduler(clock); - X509Certificate x509Certificate = mock(X509Certificate.class); + Metric metric = mock(Metric.class); when(identityDocumentService.getSignedIdentityDocument()).thenReturn(getIdentityDocument()); - when(athenzService.sendInstanceRegisterRequest(any(), any())).thenReturn( - new InstanceIdentity(null, "TOKEN")); + when(athenzService.sendInstanceRegisterRequest(any(), any())).then(new Answer<InstanceIdentity>() { + @Override + public InstanceIdentity answer(InvocationOnMock invocationOnMock) throws Throwable { + return new InstanceIdentity(getCertificate(getExpirationSupplier(clock)), "TOKEN"); + } + }); + when(athenzService.sendInstanceRefreshRequest(anyString(), anyString(), anyString(), anyString(), any(), any(), any(), any())) .thenThrow(new RuntimeException("#1")) .thenThrow(new RuntimeException("#2")) - .thenThrow(new RuntimeException("#3")) - .thenReturn(new InstanceIdentity(null, "TOKEN")); + .thenReturn(new InstanceIdentity(getCertificate(getExpirationSupplier(clock)), "TOKEN")); + AthenzCredentialsService credentialService = new AthenzCredentialsService(IDENTITY_CONFIG, identityDocumentService, athenzService, clock); - AthenzIdentityProvider identityProvider = - new AthenzIdentityProviderImpl(IDENTITY_CONFIG, DUMMY_METRIC, credentialService, scheduler, clock); - - List<MockScheduler.CompletedTask> expectedTasks = - Arrays.asList( - new MockScheduler.CompletedTask(UPDATE_CREDENTIALS_TAG, UPDATE_PERIOD), - new MockScheduler.CompletedTask(UPDATE_CREDENTIALS_TAG, UPDATE_PERIOD), - new MockScheduler.CompletedTask(UPDATE_CREDENTIALS_TAG, REDUCED_UPDATE_PERIOD), - new MockScheduler.CompletedTask(UPDATE_CREDENTIALS_TAG, REDUCED_UPDATE_PERIOD), - new MockScheduler.CompletedTask(UPDATE_CREDENTIALS_TAG, UPDATE_PERIOD)); - AtomicInteger counter = new AtomicInteger(0); - List<MockScheduler.CompletedTask> completedTasks = - scheduler.runAllTasks(task -> !task.tag().equals(METRICS_UPDATER_TAG) && - counter.getAndIncrement() < expectedTasks.size()); - assertEquals(expectedTasks, completedTasks); + AthenzIdentityProviderImpl identityProvider = + new AthenzIdentityProviderImpl(IDENTITY_CONFIG, metric, credentialService, mock(ScheduledExecutorService.class), clock); + + identityProvider.reportMetrics(); + verify(metric).set(eq(AthenzIdentityProviderImpl.CERTIFICATE_EXPIRY_METRIC_NAME), eq(certificateValidity.getSeconds()), any()); + + // Advance 1 day, refresh fails, cert is 1 day old + clock.advance(Duration.ofDays(1)); + identityProvider.refreshCertificate(); + identityProvider.reportMetrics(); + verify(metric).set(eq(AthenzIdentityProviderImpl.CERTIFICATE_EXPIRY_METRIC_NAME), eq(certificateValidity.minus(Duration.ofDays(1)).getSeconds()), any()); + + // Advance 1 more day, refresh fails, cert is 2 days old + clock.advance(Duration.ofDays(1)); + identityProvider.refreshCertificate(); + identityProvider.reportMetrics(); + verify(metric).set(eq(AthenzIdentityProviderImpl.CERTIFICATE_EXPIRY_METRIC_NAME), eq(certificateValidity.minus(Duration.ofDays(2)).getSeconds()), any()); + + // Advance 1 more day, refresh succeds, cert is new + clock.advance(Duration.ofDays(1)); + identityProvider.refreshCertificate(); + identityProvider.reportMetrics(); + verify(metric).set(eq(AthenzIdentityProviderImpl.CERTIFICATE_EXPIRY_METRIC_NAME), eq(certificateValidity.getSeconds()), any()); + + } + + private Supplier<Date> getExpirationSupplier(ManualClock clock) { + return () -> new Date(clock.instant().plus(certificateValidity).toEpochMilli()); + } + + private X509Certificate getCertificate(Supplier<Date> expiry) { + X509Certificate x509Certificate = mock(X509Certificate.class); + when(x509Certificate.getNotAfter()).thenReturn(expiry.get()); + return x509Certificate; } private static String getIdentityDocument() { @@ -119,82 +117,4 @@ public class AthenzIdentityProviderImplTest { "}"; } - - private static class MockScheduler implements Scheduler { - - private final PriorityQueue<DelayedTask> tasks = new PriorityQueue<>(); - private final ManualClock clock; - - MockScheduler(ManualClock clock) { - this.clock = clock; - } - - @Override - public void schedule(RunnableWithTag task, Duration delay) { - tasks.offer(new DelayedTask(task, delay, clock.instant().plus(delay))); - } - - List<CompletedTask> runAllTasks(Predicate<RunnableWithTag> filter) { - List<CompletedTask> completedTasks = new ArrayList<>(); - while (!tasks.isEmpty()) { - DelayedTask task = tasks.poll(); - RunnableWithTag runnable = task.runnableWithTag; - if (filter.test(runnable)) { - clock.setInstant(task.startTime); - runnable.run(); - completedTasks.add(new CompletedTask(runnable.tag(), task.delay)); - } - } - return completedTasks; - } - - private static class DelayedTask implements Comparable<DelayedTask> { - final RunnableWithTag runnableWithTag; - final Duration delay; - final Instant startTime; - - DelayedTask(RunnableWithTag runnableWithTag, Duration delay, Instant startTime) { - this.runnableWithTag = runnableWithTag; - this.delay = delay; - this.startTime = startTime; - } - - @Override - public int compareTo(DelayedTask other) { - return this.startTime.compareTo(other.startTime); - } - } - - private static class CompletedTask { - final String tag; - final Duration delay; - - CompletedTask(String tag, Duration delay) { - this.tag = tag; - this.delay = delay; - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - CompletedTask that = (CompletedTask) o; - return Objects.equals(tag, that.tag) && - Objects.equals(delay, that.delay); - } - - @Override - public int hashCode() { - return Objects.hash(tag, delay); - } - - @Override - public String toString() { - return "CompletedTask{" + - "tag='" + tag + '\'' + - ", delay=" + delay + - '}'; - } - } - } } diff --git a/vespajlib/src/main/java/com/yahoo/protect/Process.java b/vespajlib/src/main/java/com/yahoo/protect/Process.java index 67da6a1cdd8..a4396f13fd3 100644 --- a/vespajlib/src/main/java/com/yahoo/protect/Process.java +++ b/vespajlib/src/main/java/com/yahoo/protect/Process.java @@ -3,6 +3,7 @@ package com.yahoo.protect; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Level; import java.util.logging.Logger; @@ -14,6 +15,9 @@ import java.util.logging.Logger; */ public final class Process { + private static final AtomicBoolean alreadyShuttingDown = new AtomicBoolean(false); + private static final AtomicBoolean busyDumpingThreads = new AtomicBoolean(false); + private static final Logger log = Logger.getLogger(Process.class.getName()); /** Die with a message, without dumping thread state */ @@ -41,44 +45,55 @@ public final class Process { * log with level info before shutting down */ public static void logAndDie(String message, Throwable thrown, boolean dumpThreads) { + boolean shutDownInProgress = alreadyShuttingDown.getAndSet(true); try { + if (thrown != null) { + log.log(Level.SEVERE, message, thrown); + } else { + log.log(Level.SEVERE, message); + } + log.log(Level.INFO, "About to shut down."); if (dumpThreads) { - log.log(Level.INFO, "About to shut down."); dumpThreads(); } - if (thrown != null) - log.log(Level.SEVERE, message, thrown); - else - log.log(Level.SEVERE, message); } finally { - try { - Runtime.getRuntime().halt(1); - } - catch (Throwable t) { - log.log(Level.SEVERE, "Runtime.halt rejected. Throwing an error."); - throw new ShutdownError("Shutdown requested, but failed to shut down"); + if ( ! shutDownInProgress ) { + try { + Runtime.getRuntime().halt(1); + } catch (Throwable t) { + log.log(Level.SEVERE, "Runtime.halt rejected. Throwing an error."); + throw new ShutdownError("Shutdown requested, but failed to shut down"); + } + } else { + log.log(Level.WARNING, "Shutdown already in progress. Will just let death come upon us normally."); } } } public static void dumpThreads() { - try { - log.log(Level.INFO, "Commencing full thread dump for diagnosis."); - Map<Thread, StackTraceElement[]> allStackTraces = Thread.getAllStackTraces(); - for (Map.Entry<Thread, StackTraceElement[]> e : allStackTraces.entrySet()) { - Thread t = e.getKey(); - StackTraceElement[] stack = e.getValue(); - StringBuilder forOneThread = new StringBuilder(); - forOneThread.append("Stack for thread: ").append(t.getName()).append(": "); - for (StackTraceElement s : stack) { - forOneThread.append('\n').append(s.toString()); + boolean alreadyDumpingThreads = busyDumpingThreads.getAndSet(true); + if ( ! alreadyDumpingThreads ) { + try { + log.log(Level.INFO, "Commencing full thread dump for diagnosis."); + Map<Thread, StackTraceElement[]> allStackTraces = Thread.getAllStackTraces(); + for (Map.Entry<Thread, StackTraceElement[]> e : allStackTraces.entrySet()) { + Thread t = e.getKey(); + StackTraceElement[] stack = e.getValue(); + StringBuilder forOneThread = new StringBuilder(); + forOneThread.append("Stack for thread: ").append(t.getName()).append(": "); + for (StackTraceElement s : stack) { + forOneThread.append('\n').append(s.toString()); + } + log.log(Level.INFO, forOneThread.toString()); } - log.log(Level.INFO, forOneThread.toString()); + log.log(Level.INFO, "End of diagnostic thread dump."); + } catch (Exception e) { + // just give up... } - log.log(Level.INFO, "End of diagnostic thread dump."); - } catch (Exception e) { - // just give up... + busyDumpingThreads.set(false); + } else { + log.log(Level.WARNING, "Thread dump already in progress. Skipping it."); } } |