diff options
64 files changed, 1353 insertions, 1131 deletions
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 e3a937919fe..8ce13111536 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 @@ -10,10 +10,12 @@ import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.node.Allocation; +import java.net.InetAddress; import java.security.PrivateKey; import java.security.Signature; import java.time.Instant; import java.util.Base64; +import java.util.Objects; /** * @author mortent @@ -86,5 +88,28 @@ public class IdentityDocumentGenerator { private static String toZoneDnsSuffix(Zone zone, String dnsSuffix) { return zone.environment().value() + "-" + zone.region().value() + "." + dnsSuffix; } + + /* + * Basic access control until we have mutual auth where athenz x509certs are distributed on all docker nodes by node admin + * Checks: + * If remote hostname == requested hostname --> OK + * If remote hostname is parent of requested hostname in node repo --> OK + * Otherwise NOT OK + */ + boolean validateAccess(String hostname, String remoteAddr) { + try { + InetAddress addr = InetAddress.getByName(remoteAddr); + String remoteHostname = addr.getHostName(); + if (Objects.equals(hostname, remoteHostname)) { + return true; + } + Node node = nodeRepository.getNode(hostname).orElseThrow(() -> new RuntimeException("Unable to find node " + hostname)); + return node.parentHostname() + .map(parent -> Objects.equals(parent, remoteHostname)) + .orElse(false); + } catch (Exception e) { + throw new RuntimeException(e); + } + } } diff --git a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentResource.java b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentResource.java index b3e5aee97b3..a39a7cf1a05 100644 --- a/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentResource.java +++ b/athenz-identity-provider-service/src/main/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentResource.java @@ -5,12 +5,15 @@ import com.google.inject.Inject; import com.yahoo.container.jaxrs.annotation.Component; import com.yahoo.log.LogLevel; +import javax.servlet.http.HttpServletRequest; import javax.ws.rs.BadRequestException; +import javax.ws.rs.ForbiddenException; import javax.ws.rs.GET; import javax.ws.rs.InternalServerErrorException; import javax.ws.rs.Path; import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; +import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import java.util.logging.Logger; @@ -31,8 +34,14 @@ public class IdentityDocumentResource { @GET @Produces(MediaType.APPLICATION_JSON) - public SignedIdentityDocument getIdentityDocument(@QueryParam("hostname") String hostname) { + public SignedIdentityDocument getIdentityDocument(@QueryParam("hostname") String hostname, + @Context HttpServletRequest request) { // TODO Use TLS client authentication instead of blindly trusting hostname + // Until we have distributed Athenz x509 certificates we will validate that remote address + // is authorized to access the provided hostname. This means any container + if (!identityDocumentGenerator.validateAccess(hostname, request.getRemoteAddr())) { + throw new ForbiddenException(); + } if (hostname == null) { throw new BadRequestException("The 'hostname' query parameter is missing"); } @@ -44,5 +53,4 @@ public class IdentityDocumentResource { throw new InternalServerErrorException(message, e); } } - } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/SecretStoreValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/SecretStoreValidator.java new file mode 100644 index 00000000000..105bc57e770 --- /dev/null +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/SecretStoreValidator.java @@ -0,0 +1,38 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.model.application.validation; + +import com.yahoo.config.model.ConfigModelContext.ApplicationType; +import com.yahoo.config.model.deploy.DeployState; +import com.yahoo.vespa.model.VespaModel; +import com.yahoo.vespa.model.container.ContainerCluster; +import com.yahoo.vespa.model.container.IdentityProvider; +import com.yahoo.vespa.model.container.component.Component; + +/** + * Validates the requirements for setting up a secret store. + * + * @author gjoranv + */ +public class SecretStoreValidator extends Validator { + + @Override + public void validate(VespaModel model, DeployState deployState) { + if (! deployState.isHosted()) return; + if (model.getAdmin().getApplicationType() != ApplicationType.DEFAULT) return; + + for (ContainerCluster cluster : model.getContainerClusters().values()) { + if (cluster.getSecretStore().isPresent() && ! hasIdentityProvider(cluster)) { + throw new IllegalArgumentException(String.format( + "Container cluster '%s' uses a secret store, so an Athenz domain and an Athenz service" + + " must be declared in deployment.xml.", cluster.getName())); + } + } + } + + private boolean hasIdentityProvider(ContainerCluster cluster) { + for (Component<?, ?> component : cluster.getAllComponents()) { + if (component instanceof IdentityProvider) return true; + } + return false; + } +} diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java index f390d6368d1..6407f581a62 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/Validation.java @@ -55,6 +55,7 @@ public class Validation { new NoPrefixForIndexes().validate(model, deployState); new DeploymentFileValidator().validate(model, deployState); new RankingConstantsValidator().validate(model, deployState); + new SecretStoreValidator().validate(model, deployState); Optional<Model> currentActiveModel = deployState.getPreviousModel(); if (currentActiveModel.isPresent() && (currentActiveModel.get() instanceof VespaModel)) diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/first/AccessControlValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/first/AccessControlValidator.java index d7fb46a8fb2..26a1478d0a7 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/first/AccessControlValidator.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/first/AccessControlValidator.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.model.application.validation.first; import com.yahoo.config.model.ConfigModelContext.ApplicationType; import com.yahoo.config.model.deploy.DeployState; -import com.yahoo.config.provision.SystemName; import com.yahoo.vespa.model.VespaModel; import com.yahoo.vespa.model.application.validation.Validator; import com.yahoo.vespa.model.container.ContainerCluster; @@ -29,10 +28,6 @@ public class AccessControlValidator extends Validator { if (! deployState.zone().environment().isProduction()) return; if (model.getAdmin().getApplicationType() != ApplicationType.DEFAULT) return; - // Temporarily validate apps in CD zones only - // TODO: remove, and also remove the zone setting in the unit test - if (deployState.zone().system() != SystemName.cd) return; - List<String> offendingClusters = new ArrayList<>(); for (ContainerCluster cluster : model.getContainerClusters().values()) { if (cluster.getHttp() == null diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/SecretStoreValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/SecretStoreValidatorTest.java new file mode 100644 index 00000000000..cac3e65de89 --- /dev/null +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/SecretStoreValidatorTest.java @@ -0,0 +1,92 @@ +package com.yahoo.vespa.model.application.validation; + +import com.yahoo.config.application.api.ApplicationPackage; +import com.yahoo.config.model.NullConfigModelRegistry; +import com.yahoo.config.model.deploy.DeployProperties; +import com.yahoo.config.model.deploy.DeployState; +import com.yahoo.config.model.test.MockApplicationPackage; +import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.RegionName; +import com.yahoo.config.provision.Zone; +import com.yahoo.vespa.model.VespaModel; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +import static com.yahoo.config.model.test.TestUtil.joinLines; +import static org.junit.Assert.assertTrue; + +/** + * @author gjoranv + */ +public class SecretStoreValidatorTest { + @Rule + public final ExpectedException exceptionRule = ExpectedException.none(); + + private static String servicesXml() { + return joinLines("<services version='1.0'>", + " <container id='default' version='1.0'>", + " <secret-store>", + " <group name='group1' environment='prod'/>", + " </secret-store>", + " </container>", + "</services>"); + } + + private static String deploymentXml(boolean addAthenz) { + return joinLines("<deployment version='1.0' " + (addAthenz ? + "athenz-domain='domain' athenz-service='service'" : "") + ">", + " <prod />", + "</deployment>"); + } + + @Test + public void app_with_athenz_in_deployment_passes_validation() throws Exception { + DeployState deployState = deployState(servicesXml(), deploymentXml(true)); + VespaModel model = new VespaModel(new NullConfigModelRegistry(), deployState); + + new SecretStoreValidator().validate(model, deployState); + } + + @Test + public void app_without_athenz_in_deployment_fails_validation() throws Exception { + exceptionRule.expect(IllegalArgumentException.class); + exceptionRule.expectMessage( + "Container cluster 'default' uses a secret store, so an Athenz domain and" + + " an Athenz service must be declared in deployment.xml."); + + DeployState deployState = deployState(servicesXml(), deploymentXml(false)); + VespaModel model = new VespaModel(new NullConfigModelRegistry(), deployState); + + new SecretStoreValidator().validate(model, deployState); + + } + + @Test + public void app_without_secret_store_passes_validation_without_athenz_in_deployment() throws Exception { + String servicesXml = joinLines("<services version='1.0'>", + " <container id='default' version='1.0' />", + "</services>"); + DeployState deployState = deployState(servicesXml, deploymentXml(false)); + VespaModel model = new VespaModel(new NullConfigModelRegistry(), deployState); + + new SecretStoreValidator().validate(model, deployState); + } + + private static DeployState deployState(String servicesXml, String deploymentXml) { + ApplicationPackage app = new MockApplicationPackage.Builder() + .withServices(servicesXml) + .withDeploymentSpec(deploymentXml) + .build(); + DeployState.Builder builder = new DeployState.Builder() + .applicationPackage(app) + .zone(new Zone(Environment.prod, RegionName.from("foo"))) + .properties(new DeployProperties.Builder() + .hostedVespa(true) + .build()); + final DeployState deployState = builder.build(true); + + assertTrue("Test must emulate a hosted deployment.", deployState.isHosted()); + return deployState; + } +} diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/first/AccessControlValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/first/AccessControlValidatorTest.java index 82d521516b4..3f109b53bd9 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/first/AccessControlValidatorTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/first/AccessControlValidatorTest.java @@ -8,7 +8,6 @@ import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.test.MockApplicationPackage; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; -import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.Zone; import com.yahoo.vespa.model.VespaModel; import org.junit.Rule; @@ -138,9 +137,10 @@ public class AccessControlValidatorTest { ApplicationPackage app = new MockApplicationPackage.Builder() .withServices(servicesXml) .build(); + DeployState.Builder builder = new DeployState.Builder() .applicationPackage(app) - .zone(new Zone(SystemName.cd, Environment.prod, RegionName.from("foo")) )// TODO: remove cd setting + .zone(new Zone(Environment.prod, RegionName.from("foo")) ) .properties(new DeployProperties.Builder() .hostedVespa(true) .build()); diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/metric/MetricUpdater.java b/container-disc/src/main/java/com/yahoo/container/jdisc/metric/MetricUpdater.java index 05445d26e27..3d026172c86 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/metric/MetricUpdater.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/metric/MetricUpdater.java @@ -1,10 +1,9 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.container.jdisc.metric; import com.google.inject.Inject; import com.yahoo.component.AbstractComponent; import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.statistics.ActiveContainerMetrics; import java.nio.file.DirectoryStream; import java.nio.file.Files; @@ -35,13 +34,13 @@ public class MetricUpdater extends AbstractComponent { private final Scheduler scheduler; @Inject - public MetricUpdater(Metric metric, ActiveContainerMetrics activeContainerMetrics) { - this(new TimerScheduler(), metric, activeContainerMetrics); + public MetricUpdater(Metric metric) { + this(new TimerScheduler(), metric); } - MetricUpdater(Scheduler scheduler, Metric metric, ActiveContainerMetrics activeContainerMetrics) { + MetricUpdater(Scheduler scheduler, Metric metric) { this.scheduler = scheduler; - scheduler.schedule(new UpdaterTask(metric, activeContainerMetrics), Duration.ofSeconds(10)); + scheduler.schedule(new UpdaterTask(metric), Duration.ofSeconds(10)); } @Override @@ -88,11 +87,9 @@ public class MetricUpdater extends AbstractComponent { private final Runtime runtime = Runtime.getRuntime(); private final Metric metric; - private final ActiveContainerMetrics activeContainerMetrics; - public UpdaterTask(Metric metric, ActiveContainerMetrics activeContainerMetrics) { + public UpdaterTask(Metric metric) { this.metric = metric; - this.activeContainerMetrics = activeContainerMetrics; } @SuppressWarnings("deprecation") @@ -109,7 +106,6 @@ public class MetricUpdater extends AbstractComponent { metric.set(TOTAL_MEMORY_BYTES, totalMemory, null); metric.set(MEMORY_MAPPINGS_COUNT, count_mappings(), null); metric.set(OPEN_FILE_DESCRIPTORS, count_open_files(), null); - activeContainerMetrics.emitMetrics(metric); } } diff --git a/container-disc/src/test/java/com/yahoo/container/jdisc/metric/MetricUpdaterTest.java b/container-disc/src/test/java/com/yahoo/container/jdisc/metric/MetricUpdaterTest.java index 66d0686244d..d94cea033f5 100644 --- a/container-disc/src/test/java/com/yahoo/container/jdisc/metric/MetricUpdaterTest.java +++ b/container-disc/src/test/java/com/yahoo/container/jdisc/metric/MetricUpdaterTest.java @@ -1,8 +1,7 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.container.jdisc.metric; import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.statistics.ActiveContainerMetrics; import org.junit.Test; import java.time.Duration; @@ -21,9 +20,7 @@ public class MetricUpdaterTest { @Test public void metrics_are_updated_in_scheduler_cycle() throws InterruptedException { Metric metric = mock(Metric.class); - ActiveContainerMetrics activeContainerMetrics = mock(ActiveContainerMetrics.class); - new MetricUpdater(new MockScheduler(), metric, activeContainerMetrics); - verify(activeContainerMetrics, times(1)).emitMetrics(any()); + new MetricUpdater(new MockScheduler(), metric); verify(metric, times(8)).set(anyString(), any(), any()); } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/BuildService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/BuildService.java index 26ef20bbc51..d853443bf5d 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/BuildService.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/BuildService.java @@ -9,11 +9,16 @@ import java.util.Objects; public interface BuildService { /** - * Enqueue a job defined by buildJob in an external build system, and return the outcome of the enqueue request. - * This method should return false only when a retry is in order, and true otherwise, e.g., on success, or for - * invalid jobs. + * Enqueues a job defined by buildJob in an external build system. + * + * Implementations should throw an exception if the triggering fails. */ - boolean trigger(BuildJob buildJob); + void trigger(BuildJob buildJob); + + /** + * Returns whether the given job is currently running. + */ + boolean isRunning(BuildJob buildJob); // TODO jvenstad: Implement with DeploymentTrigger.Job diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/MockBuildService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/MockBuildService.java index 2f6307ae10d..b942a6baa98 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/MockBuildService.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/MockBuildService.java @@ -1,12 +1,43 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.integration.stubs; +import com.yahoo.component.AbstractComponent; import com.yahoo.vespa.hosted.controller.api.integration.BuildService; -public class MockBuildService implements BuildService { +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * @author jvenstad + */ +public class MockBuildService extends AbstractComponent implements BuildService { + + private final List<BuildJob> jobs = Collections.synchronizedList(new ArrayList<>()); + + @Override + public void trigger(BuildJob buildJob) { + jobs.add(buildJob); + } @Override - public boolean trigger(BuildJob buildJob) { - return true; + public boolean isRunning(BuildJob buildJob) { + return jobs.contains(buildJob); + } + + /** List all running jobs. */ + public List<BuildJob> jobs() { + return new ArrayList<>(jobs); + } + + /** Clears all running jobs. */ + public void clear() { + jobs.clear(); + } + + /** Removes the given job for the given project and returns whether it was found. */ + public boolean removeJob(long projectId, String jobType) { + return jobs.remove(new BuildJob(projectId, jobType)); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java index baf03485beb..800eb447bc7 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java @@ -43,7 +43,6 @@ import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.tenant.Tenant; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger; import com.yahoo.vespa.hosted.controller.maintenance.DeploymentExpirer; -import com.yahoo.vespa.hosted.controller.persistence.ControllerDb; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; import com.yahoo.vespa.hosted.controller.rotation.Rotation; import com.yahoo.vespa.hosted.controller.rotation.RotationLock; @@ -82,10 +81,7 @@ public class ApplicationController { /** The controller owning this */ private final Controller controller; - /** For permanent storage */ - private final ControllerDb db; - - /** For working memory storage and sharing between controllers */ + /** For persistence */ private final CuratorDb curator; private final ArtifactRepository artifactRepository; @@ -98,13 +94,12 @@ public class ApplicationController { private final DeploymentTrigger deploymentTrigger; - ApplicationController(Controller controller, ControllerDb db, CuratorDb curator, + ApplicationController(Controller controller, CuratorDb curator, AthenzClientFactory zmsClientFactory, RotationsConfig rotationsConfig, NameService nameService, ConfigServerClient configServer, ArtifactRepository artifactRepository, RoutingGenerator routingGenerator, BuildService buildService, Clock clock) { this.controller = controller; - this.db = db; this.curator = curator; this.zmsClientFactory = zmsClientFactory; this.nameService = nameService; @@ -116,14 +111,14 @@ public class ApplicationController { this.rotationRepository = new RotationRepository(rotationsConfig, this, curator); this.deploymentTrigger = new DeploymentTrigger(controller, curator, buildService, clock); - for (Application application : db.listApplications()) { + for (Application application : curator.readApplications()) { lockIfPresent(application.id(), this::store); } } /** Returns the application with the given id, or null if it is not present */ public Optional<Application> get(ApplicationId id) { - return db.getApplication(id); + return curator.readApplication(id); } /** @@ -137,12 +132,12 @@ public class ApplicationController { /** Returns a snapshot of all applications */ public List<Application> asList() { - return sort(db.listApplications()); + return sort(curator.readApplications()); } /** Returns all applications of a tenant */ public List<Application> asList(TenantName tenant) { - return sort(db.listApplications(tenant)); + return sort(curator.readApplications(tenant)); } /** @@ -363,7 +358,7 @@ public class ApplicationController { private LockedApplication withRotation(LockedApplication application, ZoneId zone) { if (zone.environment() == Environment.prod && application.deploymentSpec().globalServiceId().isPresent()) { try (RotationLock rotationLock = rotationRepository.lock()) { - Rotation rotation = rotationRepository.getRotation(application, rotationLock); + Rotation rotation = rotationRepository.getOrAssignRotation(application, rotationLock); application = application.with(rotation.id()); store(application); // store assigned rotation even if deployment fails @@ -499,7 +494,7 @@ public class ApplicationController { .deleteApplication(((AthenzTenant) tenant).domain(), new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(id.application().value())); } - db.deleteApplication(id); + curator.removeApplication(id); log.info("Deleted " + application); })); @@ -511,7 +506,7 @@ public class ApplicationController { * @param application a locked application to store */ public void store(LockedApplication application) { - db.store(application); + curator.writeApplication(application); } /** diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java index f63dc52fb1e..adc082ab5eb 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java @@ -26,9 +26,8 @@ import com.yahoo.vespa.hosted.controller.api.integration.routing.GlobalRoutingSe import com.yahoo.vespa.hosted.controller.api.integration.routing.RotationStatus; import com.yahoo.vespa.hosted.controller.api.integration.routing.RoutingGenerator; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry; -import com.yahoo.vespa.hosted.controller.persistence.ControllerDb; -import com.yahoo.vespa.hosted.controller.persistence.ControllerDbProxy; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; +import com.yahoo.vespa.hosted.controller.rotation.Rotation; import com.yahoo.vespa.hosted.controller.versions.VersionStatus; import com.yahoo.vespa.hosted.controller.versions.VespaVersion; import com.yahoo.vespa.hosted.rotation.config.RotationsConfig; @@ -44,15 +43,12 @@ import java.util.function.Predicate; import java.util.logging.Logger; /** - * API to the controller. This contains (currently: should contain) the object model of everything the - * controller cares about, mainly tenants and applications. - * - * As the controller runtime and Controller object are singletons, this instance can read from the object model - * in memory. However, all changes to the object model must be persisted in the controller db. + * API to the controller. This contains the object model of everything the controller cares about, mainly tenants and + * applications. The object model is persisted to curator. * * All the individual model objects reachable from the Controller are immutable. * - * Access to the controller is multithread safe, provided the locking methods are + * Access to the controller is multi-thread safe, provided the locking methods are * used when accessing, modifying and storing objects provided by the controller. * * @author bratseth @@ -79,24 +75,23 @@ public class Controller extends AbstractComponent { /** * Creates a controller * - * @param db the db storing persistent state - * @param curator the curator instance storing working state shared between controller instances + * @param curator the curator instance storing the persistent state of the controller. */ @Inject - public Controller(ControllerDb db, CuratorDb curator, RotationsConfig rotationsConfig, + public Controller(CuratorDb curator, RotationsConfig rotationsConfig, GitHub gitHub, EntityService entityService, Organization organization, GlobalRoutingService globalRoutingService, ZoneRegistry zoneRegistry, ConfigServerClient configServer, NodeRepositoryClientInterface nodeRepository, MetricsService metricsService, NameService nameService, RoutingGenerator routingGenerator, Chef chef, AthenzClientFactory athenzClientFactory, ArtifactRepository artifactRepository, BuildService buildService) { - this(db, curator, rotationsConfig, + this(curator, rotationsConfig, gitHub, entityService, organization, globalRoutingService, zoneRegistry, configServer, nodeRepository, metricsService, nameService, routingGenerator, chef, Clock.systemUTC(), athenzClientFactory, artifactRepository, buildService); } - public Controller(ControllerDb db, CuratorDb curator, RotationsConfig rotationsConfig, + public Controller(CuratorDb curator, RotationsConfig rotationsConfig, GitHub gitHub, EntityService entityService, Organization organization, GlobalRoutingService globalRoutingService, ZoneRegistry zoneRegistry, ConfigServerClient configServer, NodeRepositoryClientInterface nodeRepository, @@ -118,9 +113,7 @@ public class Controller extends AbstractComponent { this.clock = Objects.requireNonNull(clock, "Clock cannot be null"); this.athenzClientFactory = Objects.requireNonNull(athenzClientFactory, "AthenzClientFactory cannot be null"); - ControllerDbProxy dbProxy = new ControllerDbProxy(Objects.requireNonNull(db, "Controller db cannot be null"), - curator); - applicationController = new ApplicationController(this, dbProxy, curator, athenzClientFactory, + applicationController = new ApplicationController(this, curator, athenzClientFactory, Objects.requireNonNull(rotationsConfig, "RotationsConfig cannot be null"), Objects.requireNonNull(nameService, "NameService cannot be null"), configServer, @@ -128,7 +121,7 @@ public class Controller extends AbstractComponent { Objects.requireNonNull(routingGenerator, "RoutingGenerator cannot be null"), Objects.requireNonNull(buildService, "BuildService cannot be null"), clock); - tenantController = new TenantController(this, dbProxy, curator, athenzClientFactory); + tenantController = new TenantController(this, curator, athenzClientFactory); } /** Returns the instance controlling tenants */ @@ -154,8 +147,8 @@ public class Controller extends AbstractComponent { public ZoneRegistry zoneRegistry() { return zoneRegistry; } - public Map<String, RotationStatus> getHealthStatus(String hostname) { - return globalRoutingService.getHealthStatus(hostname); + public Map<String, RotationStatus> rotationStatus(Rotation rotation) { + return globalRoutingService.getHealthStatus(rotation.name()); } // TODO: Model the response properly diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java index b7e72987573..505775aef12 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java @@ -54,7 +54,7 @@ public class LockedApplication extends Application { builder.outstandingChange, builder.ownershipIssueId, builder.metrics, builder.rotation); } - public LockedApplication withProjectId(long projectId) { + public LockedApplication withProjectId(Optional<Long> projectId) { return new LockedApplication(new Builder(this).with(deploymentJobs().withProjectId(projectId))); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java index ae73f964b1c..2e8fe795fb5 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java @@ -9,12 +9,10 @@ import com.yahoo.vespa.curator.Lock; import com.yahoo.vespa.hosted.controller.api.identifiers.UserId; import com.yahoo.vespa.hosted.controller.api.integration.athenz.AthenzClientFactory; import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsClient; +import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; import com.yahoo.vespa.hosted.controller.tenant.Tenant; import com.yahoo.vespa.hosted.controller.tenant.UserTenant; -import com.yahoo.vespa.hosted.controller.persistence.ControllerDb; -import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; -import com.yahoo.vespa.hosted.controller.persistence.PersistenceException; import java.time.Duration; import java.util.Comparator; @@ -38,22 +36,17 @@ public class TenantController { /** The controller owning this */ private final Controller controller; - /** For permanent storage */ - private final ControllerDb db; - - /** For working memory storage and sharing between controllers */ + /** For persistence */ private final CuratorDb curator; private final AthenzClientFactory athenzClientFactory; - public TenantController(Controller controller, ControllerDb db, CuratorDb curator, - AthenzClientFactory athenzClientFactory) { + public TenantController(Controller controller, CuratorDb curator, AthenzClientFactory athenzClientFactory) { this.controller = controller; - this.db = db; this.curator = curator; this.athenzClientFactory = athenzClientFactory; // Write all tenants to ensure persisted data uses latest serialization format - for (Tenant tenant : db.listTenants()) { + for (Tenant tenant : curator.readTenants()) { try (Lock lock = lock(tenant.name())) { if (tenant instanceof AthenzTenant) { curator.writeTenant((AthenzTenant) tenant); @@ -68,9 +61,9 @@ public class TenantController { /** Returns a list of all known tenants sorted by name */ public List<Tenant> asList() { - return db.listTenants().stream() - .sorted(Comparator.comparing(Tenant::name)) - .collect(Collectors.toList()); + return curator.readTenants().stream() + .sorted(Comparator.comparing(Tenant::name)) + .collect(Collectors.toList()); } /** Returns a list of all tenants accessible by the given user */ @@ -88,7 +81,7 @@ public class TenantController { public void create(UserTenant tenant) { try (Lock lock = lock(tenant.name())) { requireNonExistent(tenant.name()); - db.createTenant(tenant); + curator.writeTenant(tenant); log.info("Created " + tenant); } } @@ -107,7 +100,7 @@ public class TenantController { "'"); } athenzClientFactory.createZmsClientWithAuthorizedServiceToken(token).createTenant(domain); - db.createTenant(tenant); + curator.writeTenant(tenant); log.info("Created " + tenant); } } @@ -121,11 +114,7 @@ public class TenantController { /** Find tenant by name */ public Optional<Tenant> tenant(TenantName name) { - try { - return db.getTenant(name); - } catch (PersistenceException e) { - throw new RuntimeException(e); - } + return curator.readTenant(name); } /** Find tenant by name */ @@ -135,11 +124,7 @@ public class TenantController { /** Find Athenz tenant by name */ public Optional<AthenzTenant> athenzTenant(TenantName name) { - try { - return db.getAthenzTenant(name); - } catch (PersistenceException e) { - throw new RuntimeException(e); - } + return curator.readAthenzTenant(name); } /** Update Athenz tenant */ @@ -147,10 +132,8 @@ public class TenantController { try (Lock lock = lock(updatedTenant.name())) { requireExists(updatedTenant.name()); updateAthenzDomain(updatedTenant, token); - db.updateTenant(updatedTenant); + curator.writeTenant(updatedTenant); log.info("Updated " + updatedTenant); - } catch (PersistenceException e) { - throw new RuntimeException(e); } } @@ -170,16 +153,12 @@ public class TenantController { } private void deleteTenant(TenantName name) { - try { - if ( ! controller.applications().asList(name).isEmpty()) { - throw new IllegalArgumentException("Could not delete tenant '" + name.value() - + "': This tenant has active applications"); - } - db.deleteTenant(name); - log.info("Deleted " + name); - } catch (PersistenceException e) { - throw new RuntimeException(e); + if (!controller.applications().asList(name).isEmpty()) { + throw new IllegalArgumentException("Could not delete tenant '" + name.value() + + "': This tenant has active applications"); } + curator.removeTenant(name); + log.info("Deleted " + name); } private void updateAthenzDomain(AthenzTenant updatedTenant, NToken token) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java index 04e51cdecb6..dca1736d2c5 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java @@ -74,8 +74,8 @@ public class DeploymentJobs { return new DeploymentJobs(projectId, status, issueId); } - public DeploymentJobs withProjectId(long projectId) { - return new DeploymentJobs(Optional.of(projectId), status, issueId); + public DeploymentJobs withProjectId(Optional<Long> projectId) { + return new DeploymentJobs(projectId, status, issueId); } public DeploymentJobs with(IssueId issueId) { @@ -96,18 +96,6 @@ public class DeploymentJobs { return ! JobList.from(status.values()).failing().isEmpty(); } - /** Returns whether any job is currently in progress */ - public boolean isRunning(Instant timeoutLimit) { - return ! JobList.from(status.values()).running(timeoutLimit).isEmpty(); - } - - /** Returns whether the given job type is currently running and was started after timeoutLimit */ - public boolean isRunning(JobType jobType, Instant timeoutLimit) { - JobStatus jobStatus = status.get(jobType); - if ( jobStatus == null) return false; - return jobStatus.isRunning(timeoutLimit); - } - /** Returns whether change can be deployed to the given environment */ public boolean isDeployableTo(Environment environment, Change change) { // TODO jvenstad: Rewrite to verify versions when deployment is already decided. @@ -127,13 +115,6 @@ public class DeploymentJobs { return Optional.ofNullable(jobStatus().get(jobType)); } - /** Returns the last successful application version for the given job */ - public Optional<ApplicationVersion> lastSuccessfulApplicationVersionFor(JobType jobType) { - return statusOf(jobType) - .flatMap(JobStatus::lastSuccess) - .map(JobStatus.JobRun::applicationVersion); - } - /** * Returns the id of the Screwdriver project running these deployment jobs * - or empty when this is not known or does not exist. diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java index c19c8a31ec1..bb90370f6c0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java @@ -73,11 +73,6 @@ public class JobList { && job.lastSuccess().get().version().isBefore(job.lastTriggered().get().version())); } - /** Returns the subset of jobs which are currently running, according to the given timeout */ - public JobList running(Instant timeoutLimit) { - return filter(job -> job.isRunning(timeoutLimit)); - } - /** Returns the subset of jobs which are currently failing */ public JobList failing() { return filter(job -> ! job.isSuccess()); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java index 325af87a21a..febaac8b18f 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java @@ -77,7 +77,7 @@ public class JobStatus { if (type == DeploymentJobs.JobType.component) { // not triggered by us version = controller.systemVersion(); // TODO jvenstad: Get rid of this, and perhaps all of component info? reason = "Application commit"; - } else if (! lastTriggered.isPresent()) { + } else if ( ! lastTriggered.isPresent()) { throw new IllegalStateException("Got notified about completion of " + this + ", but that has neither been triggered nor deployed"); @@ -109,14 +109,6 @@ public class JobStatus { return lastCompleted().isPresent() && ! jobError.isPresent(); } - /** Returns true if last triggered is newer than last completed and was started after timeoutLimit */ - public boolean isRunning(Instant timeoutLimit) { - if ( ! lastTriggered.isPresent()) return false; - if (lastTriggered.get().at().isBefore(timeoutLimit)) return false; - if ( ! lastCompleted.isPresent()) return true; - return ! lastTriggered.get().at().isBefore(lastCompleted.get().at()); - } - /** The error of the last completion, or empty if the last run succeeded */ public Optional<DeploymentJobs.JobError> jobError() { return jobError; } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java index 2d02df55fe1..c565ced15fb 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java @@ -5,7 +5,6 @@ import com.yahoo.component.Version; import com.yahoo.config.application.api.DeploymentSpec; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.Environment; -import com.yahoo.config.provision.SystemName; import com.yahoo.log.LogLevel; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.ApplicationController; @@ -19,7 +18,6 @@ import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobError; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobReport; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType; -import com.yahoo.vespa.hosted.controller.application.JobList; import com.yahoo.vespa.hosted.controller.application.JobStatus; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; @@ -30,11 +28,13 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.NoSuchElementException; import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; import java.util.logging.Logger; +import java.util.stream.Collectors; import java.util.stream.Stream; import static java.util.Comparator.comparing; @@ -56,11 +56,6 @@ import static java.util.stream.Collectors.toSet; */ public class DeploymentTrigger { - /** - * The max duration a job may run before we consider it dead/hanging - */ - private final Duration jobTimeout; - private final static Logger log = Logger.getLogger(DeploymentTrigger.class.getName()); private final Controller controller; @@ -76,26 +71,21 @@ public class DeploymentTrigger { this.clock = clock; this.order = new DeploymentOrder(controller::system); this.buildService = buildService; - this.jobTimeout = controller.system().equals(SystemName.main) ? Duration.ofHours(12) : Duration.ofHours(1); - } - - /** - * Returns the time in the past before which jobs are at this moment considered unresponsive - */ - public Instant jobTimeoutLimit() { - return clock.instant().minus(jobTimeout); } public DeploymentOrder deploymentOrder() { return order; } - //--- Start of methods which triggers deployment jobs ------------------------- - /** * Called each time a job completes (successfully or not) to record information used when deciding what to trigger. */ public void notifyOfCompletion(JobReport report) { + log.log(LogLevel.INFO, String.format("Got notified of %s for %s of %s (%d).", + report.jobError().map(JobError::toString).orElse("success"), + report.jobType(), + report.applicationId(), + report.projectId())); if ( ! applications().get(report.applicationId()).isPresent()) { log.log(LogLevel.WARNING, "Ignoring completion of job of project '" + report.projectId() + "': Unknown application '" + report.applicationId() + "'"); @@ -106,7 +96,7 @@ public class DeploymentTrigger { ApplicationVersion applicationVersion = report.sourceRevision().map(sr -> ApplicationVersion.from(sr, report.buildNumber())) .orElse(ApplicationVersion.unknown); application = application.withJobCompletion(report, applicationVersion, clock.instant(), controller); - application = application.withProjectId(report.projectId()); + application = application.withProjectId(Optional.of(report.projectId())); if (report.jobType() == JobType.component && report.success()) { if (acceptNewApplicationVersion(application)) @@ -123,7 +113,7 @@ public class DeploymentTrigger { /** * Finds and triggers jobs that can and should run but are currently not, and returns the number of triggered jobs. * - * Only one job is triggered each run for test jobs, since those environments have limited capacity. + * Only one job is triggered each run for test jobs, since their environments have limited capacity. */ public long triggerReadyJobs() { return computeReadyJobs().collect(partitioningBy(job -> job.jobType().isTest())) @@ -147,29 +137,33 @@ public class DeploymentTrigger { } /** - * Triggers the given job for the given application. + * Attempts to trigger the given job for the given application and returns the outcome. + * + * If the build service can not find the given job, or claims it is illegal to trigger it, + * the project id is removed from the application owning the job, to prevent further trigger attemps. */ public boolean trigger(Job job) { - log.info(String.format("Attempting to trigger %s for %s, deploying %s: %s", job.jobType, job.id, job.change, job.reason)); + log.log(LogLevel.INFO, String.format("Attempting to trigger %s for %s, deploying %s: %s (platform: %s, application: %s)", job.jobType, job.id, job.change, job.reason, job.platformVersion, job.applicationVersion.id())); - BuildService.BuildJob buildJob = new BuildService.BuildJob(job.projectId, job.jobType.jobName()); - if (buildService.trigger(buildJob)) { + try { + buildService.trigger(new BuildService.BuildJob(job.projectId, job.jobType.jobName())); applications().lockOrThrow(job.id, application -> applications().store(application.withJobTriggering( job.jobType, new JobStatus.JobRun(-1, job.platformVersion, job.applicationVersion, job.reason, clock.instant())))); return true; } - // TODO jvenstad: On 404: set empty projectId (and send ticket?). Throw and catch NoSuchElementException. - // TODO jvensatd: On 401, 403 with crumb issues: refresh auth. Handle in client. - // TODO jvenstad: On 403 with missing collaborator status: send deployment issue ticket? Throw and catch IllegalArumentException. - log.log(LogLevel.WARNING, "Failed to trigger " + buildJob + " for " + job.id); - return false; + catch (RuntimeException e) { + if (e instanceof NoSuchElementException || e instanceof IllegalArgumentException) + applications().lockOrThrow(job.id, application -> applications().store(application.withProjectId(Optional.empty()))); + log.log(LogLevel.WARNING, String.format("Exception triggering %s for %s (%s): %s", job.jobType, job.id, job.projectId, e)); + return false; + } } /** * Triggers a change of this application * * @param applicationId the application to trigger - * @throws IllegalArgumentException if this application already have an ongoing change + * @throws IllegalArgumentException if this application already has an ongoing change */ public void triggerChange(ApplicationId applicationId, Change change) { applications().lockOrThrow(applicationId, application -> { @@ -184,11 +178,7 @@ public class DeploymentTrigger { }); } - /** - * Cancels any ongoing upgrade of the given application - * - * @param applicationId the application to trigger - */ + /** Cancels a platform upgrade of the given application, and an application upgrade as well if {@code keepApplicationChange}. */ public void cancelChange(ApplicationId applicationId, boolean keepApplicationChange) { applications().lockOrThrow(applicationId, application -> { applications().store(application.withChange(application.change().application() @@ -198,10 +188,57 @@ public class DeploymentTrigger { }); } + /** Returns the set of all jobs which have changes to propagate from the upstream steps, sorted by job. */ + public Stream<Job> computeReadyJobs() { + return ApplicationList.from(applications().asList()) + .notPullRequest() + .withProjectId() + .deploying() + .idList().stream() + .map(this::computeReadyJobs) + .flatMap(List::stream); + } + + /** Returns whether the given job is currently running; false if completed since last triggered, asking the build service othewise. */ + public boolean isRunning(Application application, JobType jobType) { + return ! application.deploymentJobs().statusOf(jobType) + .flatMap(job -> job.lastCompleted().map(run -> run.at().isAfter(job.lastTriggered().get().at()))).orElse(false) + && buildService.isRunning(new BuildService.BuildJob(application.deploymentJobs().projectId().get(), jobType.jobName())); + } + + public Job forcedDeploymentJob(Application application, JobType jobType, String reason) { + return deploymentJob(application, jobType, reason, clock.instant(), Collections.emptySet()); + } + + private Job deploymentJob(Application application, JobType jobType, String reason, Instant availableSince, Collection<JobType> concurrentlyWith) { + boolean isRetry = application.deploymentJobs().statusOf(jobType).flatMap(JobStatus::jobError) + .filter(JobError.outOfCapacity::equals).isPresent(); + if (isRetry) reason += "; retrying on out of capacity"; + + Change change = application.change(); + // For both versions, use the newer of the change's and the currently deployed versions, or a fallback if none of these exist. + Version platform = jobType == JobType.component + ? Version.emptyVersion + : deploymentFor(application, jobType).map(Deployment::version) + .filter(version -> ! change.upgrades(version)) + .orElse(change.platform() + .orElse(application.oldestDeployedPlatform() + .orElse(controller.systemVersion()))); + ApplicationVersion applicationVersion = jobType == JobType.component + ? ApplicationVersion.unknown + : deploymentFor(application, jobType).map(Deployment::applicationVersion) + .filter(version -> ! change.upgrades(version)) + .orElse(change.application() + .orElseGet(() -> application.oldestDeployedApplication() + .orElseThrow(() -> new IllegalArgumentException("Cannot determine application version to use for " + jobType)))); + + return new Job(application, jobType, reason, availableSince, concurrentlyWith, isRetry, change, platform, applicationVersion); + } + /** - * Finds the next step to trigger for the given application, if any, and triggers it + * Finds the next step to trigger for the given application, if any, and returns these as a list. */ - public List<Job> computeReadyJobs(ApplicationId id) { + private List<Job> computeReadyJobs(ApplicationId id) { List<Job> jobs = new ArrayList<>(); applications().lockIfPresent(id, application -> { List<DeploymentSpec.Step> steps = application.deploymentSpec().steps().isEmpty() @@ -240,19 +277,6 @@ public class DeploymentTrigger { } /** - * Returns the set of all jobs which have changes to propagate from the upstream steps, sorted by job. - */ - public Stream<Job> computeReadyJobs() { - return ApplicationList.from(applications().asList()) - .notPullRequest() - .withProjectId() - .deploying() - .idList().stream() - .map(this::computeReadyJobs) - .flatMap(List::stream); - } - - /** * Returns the instant when the given change is complete for the given application for the given job. * * Any job is complete if the given change is already successful on that job. @@ -279,16 +303,13 @@ public class DeploymentTrigger { if ( ! application.deploymentJobs().isDeployableTo(job.jobType.environment(), application.change())) return false; - if (application.deploymentJobs().isRunning(job.jobType, jobTimeoutLimit())) + if (isRunning(application, job.jobType)) return false; if ( ! job.jobType.isProduction()) return true; - if ( ! job.concurrentlyWith.containsAll(JobList.from(application) - .production() - .running(jobTimeoutLimit()) - .mapToList(JobStatus::type))) + if ( ! job.concurrentlyWith.containsAll(runningProductionJobsFor(application))) return false; if ( ! application.changeAt(clock.instant()).isPresent()) @@ -297,6 +318,13 @@ public class DeploymentTrigger { return true; } + private List<JobType> runningProductionJobsFor(Application application) { + return application.deploymentJobs().jobStatus().keySet().parallelStream() + .filter(job -> job.isProduction()) + .filter(job -> isRunning(application, job)) + .collect(Collectors.toList()); + } + private ApplicationController applications() { return controller.applications(); } @@ -312,36 +340,6 @@ public class DeploymentTrigger { return Optional.ofNullable(application.deployments().get(jobType.zone(controller.system()).get())); } - public Job forcedDeploymentJob(Application application, JobType jobType, String reason) { - return deploymentJob(application, jobType, reason, clock.instant(), Collections.emptySet()); - } - - public Job deploymentJob(Application application, JobType jobType, String reason, Instant availableSince, Collection<JobType> concurrentlyWith) { - boolean isRetry = application.deploymentJobs().statusOf(jobType).flatMap(JobStatus::jobError) - .filter(JobError.outOfCapacity::equals).isPresent(); - if (isRetry) reason += "; retrying on out of capacity"; - - Change change = application.change(); - // For both versions, use the newer of the change's and the currently deployed versions, or a fallback if none of these exist. - Version platform = jobType == JobType.component - ? Version.emptyVersion - : deploymentFor(application, jobType).map(Deployment::version) - .filter(version -> ! change.upgrades(version)) - .orElse(change.platform() - .orElse(application.oldestDeployedPlatform() - .orElse(controller.systemVersion()))); - ApplicationVersion applicationVersion = jobType == JobType.component - ? ApplicationVersion.unknown - : deploymentFor(application, jobType).map(Deployment::applicationVersion) - .filter(version -> ! change.upgrades(version)) - .orElse(change.application() - .orElseGet(() -> application.oldestDeployedApplication() - .orElseThrow(() -> new IllegalArgumentException("Cannot determine application version to use for " + jobType)))); - - return new Job(application, jobType, reason, availableSince, concurrentlyWith, isRetry, change, platform, applicationVersion); - } - - public static class Job { private final ApplicationId id; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerDb.java deleted file mode 100644 index c2078aa48b6..00000000000 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerDb.java +++ /dev/null @@ -1,68 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.controller.persistence; - -import com.google.common.base.Joiner; -import com.yahoo.config.provision.ApplicationId; -import com.yahoo.config.provision.TenantName; -import com.yahoo.vespa.hosted.controller.Application; -import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; -import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; - -import java.util.List; -import java.util.Optional; - -/** - * Used to store the permanent data of the controller. - * - * @author Stian Kristoffersen - * @author bratseth - */ -public interface ControllerDb { - - // --------- Tenants - - void createTenant(UserTenant tenant); - - void createTenant(AthenzTenant tenant); - - // TODO: Remove exception from all signatures - void updateTenant(AthenzTenant tenant) throws PersistenceException; - - void deleteTenant(TenantName name) throws PersistenceException; - - Optional<Tenant> getTenant(TenantName name) throws PersistenceException; - - Optional<AthenzTenant> getAthenzTenant(TenantName name) throws PersistenceException; - - List<Tenant> listTenants(); - - // --------- Applications - - // ONLY call this from ApplicationController.store() - void store(Application application); - - void deleteApplication(ApplicationId applicationId); - - Optional<Application> getApplication(ApplicationId applicationId); - - /** Returns all applications */ - List<Application> listApplications(); - - /** Returns all applications of a tenant */ - List<Application> listApplications(TenantName name); - - /** Returns the given elements joined by dot "." */ - default String path(TenantName... elements) { - return Joiner.on(".").join(elements); - } - - default String path(String... elements) { - return Joiner.on(".").join(elements); - } - - default String path(ApplicationId applicationId) { - return applicationId.tenant().value() + "." + applicationId.application().value() + "." + applicationId.instance().value(); - } - -} diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerDbProxy.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerDbProxy.java deleted file mode 100644 index 80fca1ab72f..00000000000 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ControllerDbProxy.java +++ /dev/null @@ -1,95 +0,0 @@ -// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.controller.persistence; - -import com.yahoo.config.provision.ApplicationId; -import com.yahoo.config.provision.TenantName; -import com.yahoo.vespa.hosted.controller.Application; -import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; -import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; - -import java.util.List; -import java.util.Optional; - -/** - * Controller database implementation that writes to both a ControllerDb and a CuratorDb. - * - * @author mpolden - */ -// TODO: Remove this and ControllerDb and use only CuratorDb -public class ControllerDbProxy implements ControllerDb { - - private final ControllerDb db; - private final CuratorDb curator; - - public ControllerDbProxy(ControllerDb db, CuratorDb curator) { - this.db = db; - this.curator = curator; - } - - @Override - public void createTenant(UserTenant tenant) { - db.createTenant(tenant); - curator.writeTenant(tenant); - } - - @Override - public void createTenant(AthenzTenant tenant) { - db.createTenant(tenant); - curator.writeTenant(tenant); - } - - @Override - public void updateTenant(AthenzTenant tenant) throws PersistenceException { - db.updateTenant(tenant); - curator.writeTenant(tenant); - } - - @Override - public void deleteTenant(TenantName name) throws PersistenceException { - db.deleteTenant(name); - curator.removeTenant(name); - } - - @Override - public Optional<Tenant> getTenant(TenantName name) throws PersistenceException { - return db.getTenant(name); - } - - @Override - public Optional<AthenzTenant> getAthenzTenant(TenantName name) throws PersistenceException { - return db.getAthenzTenant(name); - } - - @Override - public List<Tenant> listTenants() { - return db.listTenants(); - } - - @Override - public void store(Application application) { - db.store(application); - curator.writeApplication(application); - } - - @Override - public void deleteApplication(ApplicationId applicationId) { - db.deleteApplication(applicationId); - curator.removeApplication(applicationId); - } - - @Override - public Optional<Application> getApplication(ApplicationId applicationId) { - return db.getApplication(applicationId); - } - - @Override - public List<Application> listApplications() { - return db.listApplications(); - } - - @Override - public List<Application> listApplications(TenantName name) { - return db.listApplications(name); - } -} diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java index ba713a9b6f8..5a5a8263b3f 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java @@ -35,8 +35,8 @@ import java.util.logging.Logger; import java.util.stream.Collectors; /** - * Curator backed database for storing working state shared between controller servers. - * This maps controller specific operations to general curator operations. + * Curator backed database for storing the persistence state of controllers. This maps controller specific operations + * to general curator operations. * * @author bratseth * @author mpolden diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MemoryControllerDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MemoryControllerDb.java deleted file mode 100644 index b7003c7eafd..00000000000 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MemoryControllerDb.java +++ /dev/null @@ -1,83 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.controller.persistence; - -import com.yahoo.config.provision.ApplicationId; -import com.yahoo.config.provision.TenantName; -import com.yahoo.vespa.hosted.controller.Application; -import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; -import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; - -import java.util.List; -import java.util.Optional; - -/** - * A controller db implementation backed by a mock curator. Useful for testing. - * - * @author Stian Kristoffersen - */ -public class MemoryControllerDb implements ControllerDb { - - private final MockCuratorDb curator = new MockCuratorDb(); - - @Override - public void createTenant(UserTenant tenant) { - curator.writeTenant(tenant); - } - - @Override - public void createTenant(AthenzTenant tenant) { - curator.writeTenant(tenant); - } - - @Override - public void updateTenant(AthenzTenant tenant) { - curator.writeTenant(tenant); - } - - @Override - public void deleteTenant(TenantName name) { - curator.removeTenant(name); - } - - @Override - public Optional<Tenant> getTenant(TenantName name) { - return curator.readTenant(name); - } - - @Override - public Optional<AthenzTenant> getAthenzTenant(TenantName name) { - return curator.readAthenzTenant(name); - } - - @Override - public List<Tenant> listTenants() { - return curator.readTenants(); - } - - @Override - public void store(Application application) { - curator.writeApplication(application); - } - - @Override - public void deleteApplication(ApplicationId applicationId) { - curator.removeApplication(applicationId); - } - - @Override - public Optional<Application> getApplication(ApplicationId applicationId) { - return curator.readApplication(applicationId); - } - - @Override - public List<Application> listApplications() { - return curator.readApplications(); - } - - @Override - public List<Application> listApplications(TenantName name) { - return curator.readApplications(name); - } - -} diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/PersistenceException.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/PersistenceException.java deleted file mode 100644 index b963ecbfab9..00000000000 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/PersistenceException.java +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.controller.persistence; - -/** - * Exception thrown by persistence layer. - * - * @author mpolden - */ -public class PersistenceException extends Exception { - - public PersistenceException(String message, Throwable cause) { - super(message, cause); - } - - public PersistenceException(Throwable cause) { - super(cause); - } - -} 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 aebd23f2386..ab369a1baa5 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 @@ -411,12 +411,10 @@ public class ApplicationApiHandler extends LoggingRequestHandler { deploymentObject.setString("environment", deployment.zone().environment().value()); deploymentObject.setString("region", deployment.zone().region().value()); deploymentObject.setString("instance", application.id().instance().value()); // pointless - if (application.rotation().isPresent()) { - Map<String, RotationStatus> rotationHealthStatus = application.rotation() - .map(rotation -> controller.getHealthStatus(rotation.dnsName())) - .orElse(Collections.emptyMap()); + controller.applications().rotationRepository().getRotation(application).ifPresent(rotation -> { + Map<String, RotationStatus> rotationHealthStatus = controller.rotationStatus(rotation); setRotationStatus(deployment, rotationHealthStatus, deploymentObject); - } + }); if (recurseOverDeployments(request)) // List full deployment information when recursive. toSlime(deploymentObject, new DeploymentId(application.id(), deployment.zone()), deployment, request); @@ -572,11 +570,14 @@ public class ApplicationApiHandler extends LoggingRequestHandler { Slime slime = new Slime(); Cursor response = slime.setObject(); - Map<String, RotationStatus> rotationHealthStatus = controller.getHealthStatus(application.rotation().get().dnsName()); - for (String rotationEndpoint : rotationHealthStatus.keySet()) { + Map<String, RotationStatus> rotationStatus = controller.applications().rotationRepository() + .getRotation(application) + .map(controller::rotationStatus) + .orElseGet(Collections::emptyMap); + for (String rotationEndpoint : rotationStatus.keySet()) { if (rotationEndpoint.contains(toDns(environment)) && rotationEndpoint.contains(toDns(region))) { Cursor bcpStatusObject = response.setObject("bcpStatus"); - bcpStatusObject.setString("rotationStatus", rotationHealthStatus.getOrDefault(rotationEndpoint, RotationStatus.UNKNOWN).name()); + bcpStatusObject.setString("rotationStatus", rotationStatus.getOrDefault(rotationEndpoint, RotationStatus.UNKNOWN).name()); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java index c0d3fd4758e..08aedcd12b0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java @@ -13,6 +13,7 @@ import java.util.Comparator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -45,6 +46,11 @@ public class RotationRepository { return new RotationLock(curator.lockRotations()); } + /** Get rotation for given application */ + public Optional<Rotation> getRotation(Application application) { + return application.rotation().map(r -> allRotations.get(r.id())); + } + /** * Returns a rotation for the given application * @@ -54,7 +60,7 @@ public class RotationRepository { * @param application The application requesting a rotation * @param lock Lock which must be acquired by the caller */ - public Rotation getRotation(Application application, RotationLock lock) { + public Rotation getOrAssignRotation(Application application, RotationLock lock) { if (application.rotation().isPresent()) { return allRotations.get(application.rotation().get().id()); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java index 3195bff1325..79dd6156220 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java @@ -31,7 +31,7 @@ import com.yahoo.vespa.hosted.controller.application.JobStatus; import com.yahoo.vespa.hosted.controller.application.SourceRevision; import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder; import com.yahoo.vespa.hosted.controller.deployment.BuildJob; -import com.yahoo.vespa.hosted.controller.deployment.MockBuildService; +import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockBuildService; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import com.yahoo.vespa.hosted.controller.persistence.ApplicationSerializer; import com.yahoo.vespa.hosted.controller.rotation.RotationId; @@ -436,7 +436,9 @@ public class ControllerTest { assertEquals(3, mockBuildService.jobs().size()); // Abort all running jobs, so we have three candidate jobs, of which only one should be triggered at a time. - tester.buildService().takeJobsToRun(); + synchronized (tester.buildService()) { + tester.buildService().clear(); + } tester.clock().advance(Duration.ofHours(13)); List<BuildService.BuildJob> jobs = new ArrayList<>(); @@ -456,8 +458,8 @@ public class ControllerTest { // Remove the jobs for app1 and app2, and then let app3 fail with outOfCapacity. // All three jobs are now eligible, but the one for app3 should trigger first as an outOfCapacity-retry. - tester.buildService().removeJob(app1.deploymentJobs().projectId().get(), stagingTest); - tester.buildService().removeJob(app2.deploymentJobs().projectId().get(), stagingTest); + tester.buildService().removeJob(app1.deploymentJobs().projectId().get(), stagingTest.jobName()); + tester.buildService().removeJob(app2.deploymentJobs().projectId().get(), stagingTest.jobName()); tester.clock().advance(Duration.ofHours(13)); jobs.remove(new BuildService.BuildJob(app1.deploymentJobs().projectId().get(), stagingTest.jobName())); jobs.remove(new BuildService.BuildJob(app2.deploymentJobs().projectId().get(), stagingTest.jobName())); @@ -489,8 +491,8 @@ public class ControllerTest { // Let the last system test job start, then remove the ones for apps 1 and 2, and let app3 fail with outOfCapacity again. tester.readyJobTrigger().maintain(); - tester.buildService().removeJob(app1.deploymentJobs().projectId().get(), systemTest); - tester.buildService().removeJob(app2.deploymentJobs().projectId().get(), systemTest); + tester.buildService().removeJob(app1.deploymentJobs().projectId().get(), systemTest.jobName()); + tester.buildService().removeJob(app2.deploymentJobs().projectId().get(), systemTest.jobName()); tester.clock().advance(Duration.ofHours(13)); jobs.clear(); jobs.add(new BuildService.BuildJob(app1.deploymentJobs().projectId().get(), stagingTest.jobName())); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java index 8793ce3a7e4..5abc6c4e14e 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java @@ -30,18 +30,16 @@ import com.yahoo.vespa.hosted.controller.api.integration.organization.MockOrgani import com.yahoo.vespa.hosted.controller.api.integration.routing.MemoryGlobalRoutingService; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; -import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; -import com.yahoo.vespa.hosted.controller.tenant.Tenant; import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzClientFactoryMock; import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzDbMock; -import com.yahoo.vespa.hosted.controller.deployment.MockBuildService; +import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockBuildService; import com.yahoo.vespa.hosted.controller.integration.MockMetricsService; import com.yahoo.vespa.hosted.controller.persistence.ApplicationSerializer; -import com.yahoo.vespa.hosted.controller.persistence.ControllerDb; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; -import com.yahoo.vespa.hosted.controller.persistence.MemoryControllerDb; import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb; import com.yahoo.vespa.hosted.controller.routing.MockRoutingGenerator; +import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; +import com.yahoo.vespa.hosted.controller.tenant.Tenant; import com.yahoo.vespa.hosted.controller.versions.VersionStatus; import com.yahoo.vespa.hosted.rotation.config.RotationsConfig; @@ -58,7 +56,6 @@ import static org.junit.Assert.assertNotNull; */ public final class ControllerTester { - private final ControllerDb db; private final AthenzDbMock athenzDb; private final ManualClock clock; private final ConfigServerClientMock configServer; @@ -74,29 +71,28 @@ public final class ControllerTester { private Controller controller; public ControllerTester() { - this(new MemoryControllerDb(), new AthenzDbMock(), new ManualClock(), new ConfigServerClientMock(), + this(new AthenzDbMock(), new ManualClock(), new ConfigServerClientMock(), new ZoneRegistryMock(), new GitHubMock(), new MockCuratorDb(), defaultRotationsConfig(), new MemoryNameService(), new ArtifactRepositoryMock(), new MemoryEntityService(), new MockBuildService()); } public ControllerTester(ManualClock clock) { - this(new MemoryControllerDb(), new AthenzDbMock(), clock, new ConfigServerClientMock(), + this(new AthenzDbMock(), clock, new ConfigServerClientMock(), new ZoneRegistryMock(), new GitHubMock(), new MockCuratorDb(), defaultRotationsConfig(), new MemoryNameService(), new ArtifactRepositoryMock(), new MemoryEntityService(), new MockBuildService()); } public ControllerTester(RotationsConfig rotationsConfig) { - this(new MemoryControllerDb(), new AthenzDbMock(), new ManualClock(), new ConfigServerClientMock(), + this(new AthenzDbMock(), new ManualClock(), new ConfigServerClientMock(), new ZoneRegistryMock(), new GitHubMock(), new MockCuratorDb(), rotationsConfig, new MemoryNameService(), new ArtifactRepositoryMock(), new MemoryEntityService(), new MockBuildService()); } - private ControllerTester(ControllerDb db, AthenzDbMock athenzDb, ManualClock clock, + private ControllerTester(AthenzDbMock athenzDb, ManualClock clock, ConfigServerClientMock configServer, ZoneRegistryMock zoneRegistry, GitHubMock gitHub, CuratorDb curator, RotationsConfig rotationsConfig, MemoryNameService nameService, ArtifactRepositoryMock artifactRepository, EntityService entityService, MockBuildService buildService) { - this.db = db; this.athenzDb = athenzDb; this.clock = clock; this.configServer = configServer; @@ -108,7 +104,7 @@ public final class ControllerTester { this.artifactRepository = artifactRepository; this.entityService = entityService; this.buildService = buildService; - this.controller = createController(db, curator, rotationsConfig, configServer, clock, gitHub, zoneRegistry, + this.controller = createController(curator, rotationsConfig, configServer, clock, gitHub, zoneRegistry, athenzDb, nameService, artifactRepository, entityService, buildService); // Set the log output from the root logger to use timestamps from the manual clock ;) @@ -143,7 +139,7 @@ public final class ControllerTester { /** Create a new controller instance. Useful to verify that controller state is rebuilt from persistence */ public final void createNewController() { - controller = createController(db, curator, rotationsConfig, configServer, clock, gitHub, zoneRegistry, athenzDb, + controller = createController(curator, rotationsConfig, configServer, clock, gitHub, zoneRegistry, athenzDb, nameService, artifactRepository, entityService, buildService); } @@ -221,7 +217,7 @@ public final class ControllerTester { ApplicationId applicationId = ApplicationId.from(tenant.value(), applicationName, instanceName); controller().applications().createApplication(applicationId, Optional.of(TestIdentities.userNToken)); controller().applications().lockOrThrow(applicationId, lockedApplication -> - controller().applications().store(lockedApplication.withProjectId(projectId))); + controller().applications().store(lockedApplication.withProjectId(Optional.of(projectId)))); return controller().applications().require(applicationId); } @@ -251,14 +247,13 @@ public final class ControllerTester { return new LockedApplication(application, new Lock("/test", new MockCurator())); } - private static Controller createController(ControllerDb db, CuratorDb curator, RotationsConfig rotationsConfig, + private static Controller createController(CuratorDb curator, RotationsConfig rotationsConfig, ConfigServerClientMock configServerClientMock, ManualClock clock, GitHubMock gitHubClientMock, ZoneRegistryMock zoneRegistryMock, AthenzDbMock athensDb, MemoryNameService nameService, ArtifactRepository artifactRepository, EntityService entityService, BuildService buildService) { - Controller controller = new Controller(db, - curator, + Controller controller = new Controller(curator, rotationsConfig, gitHubClientMock, entityService, diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index f6a4578f70b..4caf4645233 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -13,6 +13,7 @@ import com.yahoo.vespa.hosted.controller.ConfigServerClientMock; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.ControllerTester; import com.yahoo.vespa.hosted.controller.api.integration.BuildService; +import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockBuildService; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.application.Change; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs; @@ -258,10 +259,10 @@ public class DeploymentTester { } private void notifyJobCompletion(DeploymentJobs.JobReport report) { - if (report.jobType() != JobType.component && ! buildService().removeJob(report.projectId(), report.jobType())) + if (report.jobType() != JobType.component && ! buildService().removeJob(report.projectId(), report.jobType().jobName())) throw new IllegalArgumentException(report.jobType() + " is not running for " + report.applicationId()); assertFalse("Unexpected entry '" + report.jobType() + "@" + report.projectId() + " in: " + buildService().jobs(), - buildService().removeJob(report.projectId(), report.jobType())); + buildService().removeJob(report.projectId(), report.jobType().jobName())); clock().advance(Duration.ofMillis(1)); applications().deploymentTrigger().notifyOfCompletion(report); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java index 701d5ed69db..097066dc847 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java @@ -3,12 +3,12 @@ package com.yahoo.vespa.hosted.controller.deployment; import com.yahoo.component.Version; import com.yahoo.config.provision.Environment; -import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.TenantName; import com.yahoo.test.ManualClock; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.ControllerTester; import com.yahoo.vespa.hosted.controller.api.integration.BuildService; +import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockBuildService; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.application.ApplicationVersion; @@ -22,7 +22,6 @@ import org.junit.Test; import java.time.Duration; import java.time.Instant; -import java.util.Collections; import java.util.Optional; import java.util.function.Supplier; @@ -34,6 +33,7 @@ import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobTy import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionUsWest1; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.stagingTest; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.systemTest; +import static java.util.Collections.singletonList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; @@ -78,7 +78,9 @@ public class DeploymentTriggerTest { tester.deployAndNotify(app, applicationPackage, true, JobType.systemTest); // staging-test times out and is retried - tester.buildService().takeJobsToRun(); + synchronized (tester.buildService()) { + tester.buildService().clear(); + } tester.clock().advance(Duration.ofHours(12).plus(Duration.ofSeconds(1))); tester.readyJobTrigger().maintain(); assertEquals("Retried dead job", 1, tester.buildService().jobs().size()); @@ -324,9 +326,8 @@ public class DeploymentTriggerTest { tester.clock().advance(Duration.ofHours(2)); // ---------------- Exit block window: 20:30 tester.deploymentTrigger().triggerReadyJobs(); // Schedules the blocked production job(s) - assertEquals(1, tester.buildService().jobs().size()); - BuildService.BuildJob productionJob = tester.buildService().takeJobsToRun().get(0); - assertEquals("production-us-west-1", productionJob.jobName()); + assertEquals(singletonList(new BuildService.BuildJob(app.deploymentJobs().projectId().get(), "production-us-west-1")), + tester.buildService().jobs()); } @Test diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/MockBuildService.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/MockBuildService.java deleted file mode 100644 index 80b56867308..00000000000 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/MockBuildService.java +++ /dev/null @@ -1,40 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.controller.deployment; - -import com.yahoo.vespa.hosted.controller.api.integration.BuildService; -import com.yahoo.vespa.hosted.controller.application.DeploymentJobs; -import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType; - -import java.util.ArrayList; -import java.util.List; - -/** - * @author jvenstad - */ -public class MockBuildService implements BuildService { - - private final List<BuildJob> jobs = new ArrayList<>(); - - @Override - public boolean trigger(BuildJob buildJob) { - return jobs.add(buildJob); - } - - /** List all running jobs. */ - public List<BuildJob> jobs() { - return new ArrayList<>(jobs); - } - - /** List and remove all running jobs. */ - public List<BuildJob> takeJobsToRun() { - List<BuildJob> jobsToRun = jobs(); - jobs.clear(); - return jobsToRun; - } - - /** Remove all running jobs for the given project. */ - public boolean removeJob(long projectId, JobType jobType) { - return jobs.remove(new BuildJob(projectId, jobType.jobName())); - } - -} diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/FailureRedeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/FailureRedeployerTest.java index 59df3cd4553..2d76d395804 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/FailureRedeployerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/FailureRedeployerTest.java @@ -8,7 +8,6 @@ import com.yahoo.slime.Slime; import com.yahoo.vespa.config.SlimeUtils; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.api.integration.BuildService; -import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs; import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder; @@ -21,9 +20,7 @@ import java.time.Duration; import java.util.Collections; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.component; -import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionCdUsCentral1; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionUsEast3; -import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.stagingTest; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.systemTest; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -95,36 +92,6 @@ public class FailureRedeployerTest { assertTrue("All jobs consumed", tester.buildService().jobs().isEmpty()); assertFalse("No failures", tester.application(app.id()).deploymentJobs().hasFailures()); } - - @Test - public void testRetriesDeploymentWithStuckJobs() { - DeploymentTester tester = new DeploymentTester(); - ApplicationPackage applicationPackage = new ApplicationPackageBuilder() - .upgradePolicy("canary") - .environment(Environment.prod) - .region("us-east-3") - .build(); - - Application app = tester.createApplication("app1", "tenant1", 1, 11L); - tester.jobCompletion(component).application(app).uploadArtifact(applicationPackage).submit(); - tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.systemTest); - - // staging-test starts, but does not complete - assertEquals(DeploymentJobs.JobType.stagingTest.jobName(), tester.buildService().takeJobsToRun().get(0).jobName()); - tester.readyJobTrigger().maintain(); - assertTrue("No jobs retried", tester.buildService().jobs().isEmpty()); - - // Just over 12 hours pass, job is retried - tester.clock().advance(Duration.ofHours(12).plus(Duration.ofSeconds(1))); - tester.readyJobTrigger().maintain(); - tester.assertRunning(app.id(), stagingTest); - - // Deployment completes - tester.deployAndNotify(app, applicationPackage, true, stagingTest); - tester.deployAndNotify(app, applicationPackage, true, DeploymentJobs.JobType.productionUsEast3); - assertTrue("All jobs consumed", tester.buildService().jobs().isEmpty()); - } - @Test public void testRetriesJobsFailingForCurrentChange() { DeploymentTester tester = new DeploymentTester(); @@ -158,8 +125,10 @@ public class FailureRedeployerTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); - // Job is left "running", so needs to time out before it can be retried. - tester.clock().advance(Duration.ofHours(13)); + boolean result; + synchronized (tester.buildService()) { + result = tester.buildService().removeJob((long) 1, systemTest.jobName()); + } tester.upgrader().maintain(); tester.readyJobTrigger().maintain(); assertEquals("Application has pending upgrade to " + version, version, tester.application(app.id()).change().platform().get()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java index 02672f06ea6..7de53d47fe4 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java @@ -123,8 +123,14 @@ public class UpgraderTest { tester.updateVersionStatus(version); assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber()); tester.upgrader().maintain(); - tester.buildService().removeJob(canary0.deploymentJobs().projectId().get(), stagingTest); - tester.buildService().removeJob(canary1.deploymentJobs().projectId().get(), systemTest); + boolean result1; + synchronized (tester.buildService()) { + result1 = tester.buildService().removeJob(canary0.deploymentJobs().projectId().get(), stagingTest.jobName()); + } + boolean result; + synchronized (tester.buildService()) { + result = tester.buildService().removeJob(canary1.deploymentJobs().projectId().get(), systemTest.jobName()); + } tester.readyJobTrigger().maintain(); tester.readyJobTrigger().maintain(); @@ -254,7 +260,9 @@ public class UpgraderTest { tester.jobCompletion(DeploymentJobs.JobType.productionUsWest1).application(default3).unsuccessful().submit(); tester.upgrader().maintain(); - tester.buildService().takeJobsToRun(); + synchronized (tester.buildService()) { + tester.buildService().clear(); + } tester.readyJobTrigger().maintain(); assertEquals("Upgrade of defaults are scheduled on 5.4 instead, since 5.5 broken: " + "This is default3 since it failed upgrade on both 5.4 and 5.5", @@ -339,7 +347,9 @@ public class UpgraderTest { // > 40% and at least 4 failed - version is broken tester.updateVersionStatus(version); tester.upgrader().maintain(); - tester.buildService().takeJobsToRun(); + synchronized (tester.buildService()) { + tester.buildService().clear(); + } tester.readyJobTrigger().maintain(); assertEquals(VespaVersion.Confidence.broken, tester.controller().versionStatus().systemVersion().get().confidence()); assertEquals("Upgrades are cancelled", 0, tester.buildService().jobs().size()); @@ -393,12 +403,11 @@ public class UpgraderTest { tester.jobCompletion(stagingTest).application(app).unsuccessful().submit(); assertTrue("Application still has failures", tester.application(app.id()).deploymentJobs().hasFailures()); assertEquals(1, tester.buildService().jobs().size()); - tester.buildService().takeJobsToRun(); // Upgrader runs again, nothing happens as there's already a job in progress for this change tester.upgrader().maintain(); tester.readyJobTrigger().maintain(); - assertTrue("No more jobs triggered at this time", tester.buildService().jobs().isEmpty()); + assertEquals(1, tester.buildService().jobs().size()); } @Test @@ -518,7 +527,9 @@ public class UpgraderTest { tester.deploymentTrigger().cancelChange(default1.id(), false); tester.deploymentTrigger().cancelChange(default2.id(), false); tester.deploymentTrigger().cancelChange(default3.id(), false); - tester.buildService().takeJobsToRun(); + synchronized (tester.buildService()) { + tester.buildService().clear(); + } tester.clock().advance(Duration.ofHours(13)); // Currently we don't cancel running jobs, so this is necessary to allow a new triggering below // Applications with default policy start upgrading to V2 @@ -542,7 +553,9 @@ public class UpgraderTest { assertEquals(v2, tester.application("default0").deployments().get(ZoneId.from("prod.us-west-1")).version()); assertEquals(v0, tester.application("default0").deployments().get(ZoneId.from("prod.us-east-3")).version()); tester.upgrader().maintain(); - tester.buildService().takeJobsToRun(); + synchronized (tester.buildService()) { + tester.buildService().clear(); + } tester.clock().advance(Duration.ofHours(13)); // TODO jvenstad: Reduce all these when build service is polled for status. tester.readyJobTrigger().maintain(); tester.readyJobTrigger().maintain(); @@ -859,7 +872,7 @@ public class UpgraderTest { // 5th app never reports back and has a dead job, but no ongoing change Application deadLocked = tester.applications().require(default4.id()); - assertTrue("Jobs in progress", deadLocked.deploymentJobs().isRunning(tester.controller().applications().deploymentTrigger().jobTimeoutLimit())); + assertTrue("Jobs in progress", tester.deploymentTrigger().isRunning(deadLocked, systemTest)); assertFalse("No change present", deadLocked.change().isPresent()); // 4 out of 5 applications are repaired and confidence is restored diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java index fc5e3e72f57..5184eeacc33 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java @@ -22,6 +22,7 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; import com.yahoo.vespa.hosted.controller.api.identifiers.ScrewdriverId; import com.yahoo.vespa.hosted.controller.api.integration.athenz.ApplicationAction; import com.yahoo.vespa.hosted.controller.api.integration.athenz.HostedAthenzIdentities; +import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockBuildService; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; @@ -39,6 +40,8 @@ import java.io.IOException; import java.time.Duration; import java.util.Optional; +import static org.junit.Assert.assertFalse; + /** * Provides testing of controller functionality accessed through the container * @@ -119,13 +122,19 @@ public class ContainerControllerTester { .addRoleMember(action, HostedAthenzIdentities.from(screwdriverId)); } - private void notifyJobCompletion(DeploymentJobs.JobReport jobReport) { + private void notifyJobCompletion(DeploymentJobs.JobReport report) { + + MockBuildService buildService = (MockBuildService) containerTester.container().components().getComponent(MockBuildService.class.getName()); + if (report.jobType() != DeploymentJobs.JobType.component && ! buildService.removeJob(report.projectId(), report.jobType().jobName())) + throw new IllegalArgumentException(report.jobType() + " is not running for " + report.applicationId()); + assertFalse("Unexpected entry '" + report.jobType() + "@" + report.projectId() + " in: " + buildService.jobs(), + buildService.removeJob(report.projectId(), report.jobType().jobName())); try { Thread.sleep(1); } catch (InterruptedException e) { throw new RuntimeException(e); } - controller().applications().deploymentTrigger().notifyOfCompletion(jobReport); + controller().applications().deploymentTrigger().notifyOfCompletion(report); controller().applications().deploymentTrigger().triggerReadyJobs(); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java index 5ccdcdbf4e8..c51689c1a40 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java @@ -78,7 +78,6 @@ public class ControllerContainerTest { " <component id='com.yahoo.vespa.hosted.controller.integration.MockMetricsService'/>\n" + " <component id='com.yahoo.vespa.hosted.controller.maintenance.ControllerMaintenance'/>\n" + " <component id='com.yahoo.vespa.hosted.controller.maintenance.JobControl'/>\n" + - " <component id='com.yahoo.vespa.hosted.controller.persistence.MemoryControllerDb'/>\n" + " <component id='com.yahoo.vespa.hosted.controller.routing.MockRoutingGenerator'/>\n" + " <component id='com.yahoo.vespa.hosted.controller.ArtifactRepositoryMock'/>\n" + " <handler id='com.yahoo.vespa.hosted.controller.restapi.application.ApplicationApiHandler'>\n" + diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java index 743baf76759..f52cc9e9e11 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java @@ -42,7 +42,6 @@ public class DeploymentApiTest extends ControllerContainerTest { ContainerControllerTester tester = new ContainerControllerTester(container, responseFiles); Version version = Version.fromString("5.0"); tester.containerTester().updateSystemVersion(version); - long projectId = 11; ApplicationPackage applicationPackage = new ApplicationPackageBuilder() .environment(Environment.prod) .region("corp-us-east-1") @@ -55,11 +54,11 @@ public class DeploymentApiTest extends ControllerContainerTest { "application2"); Application applicationWithoutDeployment = tester.createApplication("domain3", "tenant3", "application3"); - deployCompletely(failingApplication, applicationPackage, projectId, true); - deployCompletely(productionApplication, applicationPackage, projectId, true); + deployCompletely(failingApplication, applicationPackage, 1L, true); + deployCompletely(productionApplication, applicationPackage, 2L, true); // Deploy once so that job information is stored, then remove the deployment - deployCompletely(applicationWithoutDeployment, applicationPackage, projectId, true); + deployCompletely(applicationWithoutDeployment, applicationPackage, 3L, true); tester.controller().applications().deactivate(applicationWithoutDeployment, ZoneId.from("prod", "corp-us-east-1")); @@ -70,8 +69,9 @@ public class DeploymentApiTest extends ControllerContainerTest { // Applications upgrade, 1/2 succeed tester.upgrader().maintain(); tester.controller().applications().deploymentTrigger().triggerReadyJobs(); - deployCompletely(failingApplication, applicationPackage, projectId, false); - deployCompletely(productionApplication, applicationPackage, projectId, true); + tester.controller().applications().deploymentTrigger().triggerReadyJobs(); + deployCompletely(failingApplication, applicationPackage, 1L, false); + deployCompletely(productionApplication, applicationPackage, 2L, true); tester.controller().updateVersionStatus(censorConfigServers(VersionStatus.compute(tester.controller()), tester.controller())); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java index 2911b9ab72f..42f291d5f96 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java @@ -10,6 +10,7 @@ import org.junit.Test; import java.io.File; import java.nio.charset.StandardCharsets; +import java.util.Optional; /** * @author bratseth @@ -38,7 +39,7 @@ public class ScrewdriverApiTest extends ControllerContainerTest { Application app = tester.createApplication(); tester.controller().applications().lockOrThrow(app.id(), application -> - tester.controller().applications().store(application.withProjectId(1))); + tester.controller().applications().store(application.withProjectId(Optional.of(1L)))); // Unknown application assertResponse(new Request("http://localhost:8080/screwdriver/v1/trigger/tenant/foo/application/bar", diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationTest.java index c73bf9ac34e..f13c65a06fe 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationTest.java @@ -68,7 +68,7 @@ public class RotationTest { assertEquals(URI.create("http://app1.tenant1.global.vespa.yahooapis.com:4080/"), application.rotation().get().url()); try (RotationLock lock = repository.lock()) { - Rotation rotation = repository.getRotation(tester.applications().require(application.id()), lock); + Rotation rotation = repository.getOrAssignRotation(tester.applications().require(application.id()), lock); assertEquals(expected, rotation); } @@ -93,7 +93,7 @@ public class RotationTest { application = tester.applications().require(application.id()); try (RotationLock lock = repository.lock()) { - Rotation rotation = repository.getRotation(application, lock); + Rotation rotation = repository.getOrAssignRotation(application, lock); Rotation assignedRotation = new Rotation(new RotationId("foo-1"), "foo-1.com"); assertEquals(assignedRotation, rotation); } diff --git a/eval/CMakeLists.txt b/eval/CMakeLists.txt index 3db05c09613..22479952270 100644 --- a/eval/CMakeLists.txt +++ b/eval/CMakeLists.txt @@ -42,6 +42,7 @@ vespa_define_module( src/tests/tensor/tensor_performance src/tests/tensor/tensor_serialization src/tests/tensor/tensor_slime_serialization + src/tests/tensor/typed_cells src/tests/tensor/vector_from_doubles_function LIBS diff --git a/eval/src/tests/tensor/typed_cells/CMakeLists.txt b/eval/src/tests/tensor/typed_cells/CMakeLists.txt new file mode 100644 index 00000000000..d57ff33eda6 --- /dev/null +++ b/eval/src/tests/tensor/typed_cells/CMakeLists.txt @@ -0,0 +1,8 @@ +# Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +vespa_add_executable(eval_typed_cells_test_app TEST + SOURCES + typed_cells_test.cpp + DEPENDS + vespaeval +) +vespa_add_test(NAME eval_typed_cells_test_app COMMAND eval_typed_cells_test_app) diff --git a/eval/src/tests/tensor/typed_cells/typed_cells_test.cpp b/eval/src/tests/tensor/typed_cells/typed_cells_test.cpp new file mode 100644 index 00000000000..ccb522fd496 --- /dev/null +++ b/eval/src/tests/tensor/typed_cells/typed_cells_test.cpp @@ -0,0 +1,622 @@ +// Copyright 2017 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/vespalib/util/arrayref.h> +#include <memory> + +using namespace vespalib; + +//----------------------------------------------------------------------------- +//----------------------------------------------------------------------------- +// Low-level typed cells reference +//----------------------------------------------------------------------------- +//----------------------------------------------------------------------------- + +enum class CellType : char { DOUBLE, FLOAT, INT }; +template <typename T> bool check_type(CellType type); +template <> bool check_type<double>(CellType type) { return (type == CellType::DOUBLE); } +template <> bool check_type<float>(CellType type) { return (type == CellType::FLOAT); } +template <> bool check_type<int>(CellType type) { return (type == CellType::INT); } + +struct TypedCells { + const void *data; + CellType type; + size_t size:56; + explicit TypedCells(ConstArrayRef<double> cells) : data(cells.begin()), type(CellType::DOUBLE), size(cells.size()) {} + explicit TypedCells(ConstArrayRef<float> cells) : data(cells.begin()), type(CellType::FLOAT), size(cells.size()) {} + explicit TypedCells(ConstArrayRef<int> cells) : data(cells.begin()), type(CellType::INT), size(cells.size()) {} + template <typename T> bool check_type() const { return ::check_type<T>(type); } + template <typename T> ConstArrayRef<T> typify() const { + assert(check_type<T>()); + return ConstArrayRef<T>((const T *)data, size); + } + template <typename T> ConstArrayRef<T> unsafe_typify() const { + return ConstArrayRef<T>((const T *)data, size); + } +}; + +TEST("require that structures are of expected size") { + EXPECT_EQUAL(sizeof(void*), 8u); + EXPECT_EQUAL(sizeof(size_t), 8u); + EXPECT_EQUAL(sizeof(CellType), 1u); + EXPECT_EQUAL(sizeof(TypedCells), 16u); +} + +//----------------------------------------------------------------------------- +//----------------------------------------------------------------------------- +// CASE STUDY: Direct dispatch, minimal runtime type resolving +//----------------------------------------------------------------------------- +//----------------------------------------------------------------------------- + +struct CellwiseAdd { + template <typename A, typename B, typename C> + static void call(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, const ConstArrayRef<C> &c, size_t cnt) __attribute__ ((noinline)); +}; + +template <typename A, typename B, typename C> +void CellwiseAdd::call(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, const ConstArrayRef<C> &c, size_t cnt) { + auto dst = unconstify(c); + for (size_t i = 0; i < cnt; ++i) { + dst[i] = a[i] + b[i]; + } +} + +//----------------------------------------------------------------------------- + +struct DotProduct { + template <typename A, typename B> + static double call(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, size_t cnt) __attribute__ ((noinline)); +}; + +template <typename A, typename B> +double DotProduct::call(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, size_t cnt) { + double result = 0.0; + for (size_t i = 0; i < cnt; ++i) { + result += (a[i] * b[i]); + } + return result; +} + +//----------------------------------------------------------------------------- + +struct Sum { + template <typename A> + static double call(const ConstArrayRef<A> &a) __attribute__ ((noinline)); +}; + +template <typename A> +double Sum::call(const ConstArrayRef<A> &a) { + double result = 0.0; + for (const auto &value: a) { + result += value; + } + return result; +} + +//----------------------------------------------------------------------------- + +template <typename T> +struct Typify { + template <typename... Args> + static auto typify_1(const TypedCells &a, Args &&...args) { + switch(a.type) { + case CellType::DOUBLE: return T::call(a.unsafe_typify<double>(), std::forward<Args>(args)...); + case CellType::FLOAT: return T::call(a.unsafe_typify<float>(), std::forward<Args>(args)...); + case CellType::INT: return T::call(a.unsafe_typify<int>(), std::forward<Args>(args)...); + } + abort(); + } + template <typename A, typename... Args> + static auto typify_2(A &&a, const TypedCells &b, Args &&...args) { + switch(b.type) { + case CellType::DOUBLE: return T::call(std::forward<A>(a), b.unsafe_typify<double>(), std::forward<Args>(args)...); + case CellType::FLOAT: return T::call(std::forward<A>(a), b.unsafe_typify<float>(), std::forward<Args>(args)...); + case CellType::INT: return T::call(std::forward<A>(a), b.unsafe_typify<int>(), std::forward<Args>(args)...); + } + abort(); + } + template <typename A, typename B, typename... Args> + static auto typify_3(A &&a, B &&b, const TypedCells &c, Args &&...args) { + switch(c.type) { + case CellType::DOUBLE: return T::call(std::forward<A>(a), std::forward<B>(b), c.unsafe_typify<double>(), std::forward<Args>(args)...); + case CellType::FLOAT: return T::call(std::forward<A>(a), std::forward<B>(b), c.unsafe_typify<float>(), std::forward<Args>(args)...); + case CellType::INT: return T::call(std::forward<A>(a), std::forward<B>(b), c.unsafe_typify<int>(), std::forward<Args>(args)...); + } + abort(); + } +}; + +template <typename Fun> +struct Dispatch3 { + using Self = Dispatch3<Fun>; + template <typename A, typename B, typename C, typename... Args> + static auto call(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, const ConstArrayRef<C> &c, Args &&...args) { + return Fun::call(a, b, c, std::forward<Args>(args)...); + } + template <typename A, typename B, typename... Args> + static auto call(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, const TypedCells &c, Args &&...args) { + return Typify<Self>::typify_3(a, b, c, std::forward<Args>(args)...); + } + template <typename A, typename... Args> + static auto call(const ConstArrayRef<A> &a, const TypedCells &b, const TypedCells &c, Args &&...args) { + return Typify<Self>::typify_2(a, b, c, std::forward<Args>(args)...); + } + template <typename A, typename C, typename... Args> + static auto call(const ConstArrayRef<A> &a, const TypedCells &b, const ConstArrayRef<C> &c, Args &&...args) { + return Typify<Self>::typify_2(a, b, c, std::forward<Args>(args)...); + } + template <typename... Args> + static auto call(const TypedCells &a, const TypedCells &b, const TypedCells &c, Args &&...args) { + return Typify<Self>::typify_1(a, b, c, std::forward<Args>(args)...); + } + template <typename B, typename... Args> + static auto call(const TypedCells &a, const ConstArrayRef<B> &b, const TypedCells &c, Args &&...args) { + return Typify<Self>::typify_1(a, b, c, std::forward<Args>(args)...); + } + template <typename C, typename... Args> + static auto call(const TypedCells &a, const TypedCells &b, const ConstArrayRef<C> &c, Args &&...args) { + return Typify<Self>::typify_1(a, b, c, std::forward<Args>(args)...); + } + template <typename B, typename C, typename... Args> + static auto call(const TypedCells &a, const ConstArrayRef<B> &b, const ConstArrayRef<C> &c, Args &&...args) { + return Typify<Self>::typify_1(a, b, c, std::forward<Args>(args)...); + } +}; + +template <typename Fun> +struct Dispatch2 { + using Self = Dispatch2<Fun>; + template <typename A, typename B, typename... Args> + static auto call(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, Args &&...args) { + return Fun::call(a, b, std::forward<Args>(args)...); + } + template <typename A, typename... Args> + static auto call(const ConstArrayRef<A> &a, const TypedCells &b, Args &&...args) { + return Typify<Self>::typify_2(a, b, std::forward<Args>(args)...); + } + template <typename... Args> + static auto call(const TypedCells &a, const TypedCells &b, Args &&...args) { + return Typify<Self>::typify_1(a, b, std::forward<Args>(args)...); + } + template <typename B, typename... Args> + static auto call(const TypedCells &a, const ConstArrayRef<B> &b, Args &&...args) { + return Typify<Self>::typify_1(a, b, std::forward<Args>(args)...); + } +}; + +template <typename Fun> +struct Dispatch1 { + using Self = Dispatch1<Fun>; + template <typename A, typename... Args> + static auto call(const ConstArrayRef<A> &a, Args &&...args) { + return Fun::call(a, std::forward<Args>(args)...); + } + template <typename... Args> + static auto call(const TypedCells &a, Args &&...args) { + return Typify<Self>::typify_1(a, std::forward<Args>(args)...); + } +}; + +//----------------------------------------------------------------------------- + +TEST("require that direct dispatch 'a op b -> c' works") { + std::vector<int> a({1,2,3}); + std::vector<float> b({1.5,2.5,3.5}); + std::vector<double> c(3, 0.0); + ConstArrayRef<int> a_ref(a); + ConstArrayRef<float> b_ref(b); + ConstArrayRef<double> c_ref(c); + TypedCells a_cells(a); + TypedCells b_cells(b); + TypedCells c_cells(c); + + Dispatch3<CellwiseAdd>::call(a_cells, b_cells, c_cells, 3); + Dispatch3<CellwiseAdd>::call(a_cells, b_ref, c_cells, 3); + Dispatch3<CellwiseAdd>::call(a_cells, b_cells, c_ref, 3); + Dispatch3<CellwiseAdd>::call(a_cells, b_ref, c_ref, 3); + Dispatch3<CellwiseAdd>::call(a_ref, b_cells, c_cells, 3); + Dispatch3<CellwiseAdd>::call(a_ref, b_cells, c_ref, 3); + Dispatch3<CellwiseAdd>::call(a_ref, b_ref, c_cells, 3); + Dispatch3<CellwiseAdd>::call(a_ref, b_ref, c_ref, 3); + + EXPECT_EQUAL(c[0], 2.5); + EXPECT_EQUAL(c[1], 4.5); + EXPECT_EQUAL(c[2], 6.5); +} + +TEST("require that direct dispatch 'dot product' with return value works") { + std::vector<int> a({1,2,3}); + std::vector<float> b({1.5,2.5,3.5}); + ConstArrayRef<int> a_ref(a); + ConstArrayRef<float> b_ref(b); + TypedCells a_cells(a); + TypedCells b_cells(b); + double expect = 1.5 + (2 * 2.5) + (3 * 3.5); + + EXPECT_EQUAL(expect, Dispatch2<DotProduct>::call(a_cells, b_cells, 3)); + EXPECT_EQUAL(expect, Dispatch2<DotProduct>::call(a_cells, b_ref, 3)); + EXPECT_EQUAL(expect, Dispatch2<DotProduct>::call(a_ref, b_cells, 3)); + EXPECT_EQUAL(expect, Dispatch2<DotProduct>::call(a_ref, b_ref, 3)); +} + +TEST("require that direct dispatch 'sum' with return value works") { + std::vector<int> a({1,2,3}); + ConstArrayRef<int> a_ref(a); + TypedCells a_cells(a); + double expect = (1 + 2 + 3); + + EXPECT_EQUAL(expect, Dispatch1<Sum>::call(a_cells)); + EXPECT_EQUAL(expect, Dispatch1<Sum>::call(a_ref)); +} + +//----------------------------------------------------------------------------- +//----------------------------------------------------------------------------- +// CASE STUDY: Pre-resolved templated subclass +//----------------------------------------------------------------------------- +//----------------------------------------------------------------------------- + +struct CellwiseAdd2 { + virtual void call(const TypedCells &a, const TypedCells &b, const TypedCells &c, size_t cnt) const = 0; + template <typename A, typename B, typename C> + static std::unique_ptr<CellwiseAdd2> create(); + virtual ~CellwiseAdd2() {} +}; + +template <typename A, typename B, typename C> +struct CellwiseAdd2Impl : CellwiseAdd2 { + void call_impl(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, const ConstArrayRef<C> &c, size_t cnt) const { + auto dst = unconstify(c); + for (size_t i = 0; i < cnt; ++i) { + dst[i] = a[i] + b[i]; + } + } + void call(const TypedCells &a, const TypedCells &b, const TypedCells &c, size_t cnt) const override { + call_impl(a.unsafe_typify<A>(), b.unsafe_typify<B>(), c.unsafe_typify<C>(), cnt); + } +}; + +template <typename A, typename B, typename C> +std::unique_ptr<CellwiseAdd2> CellwiseAdd2::create() { + return std::make_unique<CellwiseAdd2Impl<A, B, C> >(); +} + +//----------------------------------------------------------------------------- + +struct DotProduct2 { + virtual double call(const TypedCells &a, const TypedCells &b, size_t cnt) const = 0; + template <typename A, typename B> + static std::unique_ptr<DotProduct2> create(); + virtual ~DotProduct2() {} +}; + +template <typename A, typename B> +struct DotProduct2Impl : DotProduct2 { + double call_impl(const ConstArrayRef<A> &a, const ConstArrayRef<B> &b, size_t cnt) const { + double result = 0.0; + for (size_t i = 0; i < cnt; ++i) { + result += (a[i] * b[i]); + } + return result; + } + double call(const TypedCells &a, const TypedCells &b, size_t cnt) const override { + return call_impl(a.unsafe_typify<A>(), b.unsafe_typify<B>(), cnt); + } +}; + +template <typename A, typename B> +std::unique_ptr<DotProduct2> DotProduct2::create() { + return std::make_unique<DotProduct2Impl<A, B> >(); +} + +//----------------------------------------------------------------------------- + +struct Sum2 { + virtual double call(const TypedCells &a) const = 0; + template <typename A> + static std::unique_ptr<Sum2> create(); + virtual ~Sum2() {} +}; + +template <typename A> +struct Sum2Impl : Sum2 { + double call_impl(const ConstArrayRef<A> &a) const { + double result = 0.0; + for (const auto &value: a) { + result += value; + } + return result; + } + double call(const TypedCells &a) const override { + return call_impl(a.unsafe_typify<A>()); + } +}; + +template <typename A> +std::unique_ptr<Sum2> Sum2::create() { + return std::make_unique<Sum2Impl<A> >(); +} + +//----------------------------------------------------------------------------- + +template <typename T, typename... Args> +std::unique_ptr<T> create(CellType a_type) { + switch(a_type) { + case CellType::DOUBLE: return T::template create<double, Args...>(); + case CellType::FLOAT: return T::template create<float, Args...>(); + case CellType::INT: return T::template create<int, Args...>(); + } + abort(); +} + +template <typename T, typename... Args> +std::unique_ptr<T> create(CellType a_type, CellType b_type) { + switch(b_type) { + case CellType::DOUBLE: return create<T, double, Args...>(a_type); + case CellType::FLOAT: return create<T, float, Args...>(a_type); + case CellType::INT: return create<T, int, Args...>(a_type); + } + abort(); +} + +template <typename T> +std::unique_ptr<T> create(CellType a_type, CellType b_type, CellType c_type) { + switch(c_type) { + case CellType::DOUBLE: return create<T, double>(a_type, b_type); + case CellType::FLOAT: return create<T, float>(a_type, b_type); + case CellType::INT: return create<T, int>(a_type, b_type); + } + abort(); +} + +//----------------------------------------------------------------------------- + +TEST("require that pre-resolved subclass 'a op b -> c' works") { + std::vector<int> a({1,2,3}); + std::vector<float> b({1.5,2.5,3.5}); + std::vector<double> c(3, 0.0); + TypedCells a_cells(a); + TypedCells b_cells(b); + TypedCells c_cells(c); + + auto op = create<CellwiseAdd2>(a_cells.type, b_cells.type, c_cells.type); + op->call(a_cells, b_cells, c_cells, 3); + + EXPECT_EQUAL(c[0], 2.5); + EXPECT_EQUAL(c[1], 4.5); + EXPECT_EQUAL(c[2], 6.5); +} + +TEST("require that pre-resolved subclass 'dot product' with return value works") { + std::vector<int> a({1,2,3}); + std::vector<float> b({1.5,2.5,3.5}); + TypedCells a_cells(a); + TypedCells b_cells(b); + double expect = 1.5 + (2 * 2.5) + (3 * 3.5); + + auto op = create<DotProduct2>(a_cells.type, b_cells.type); + + EXPECT_EQUAL(expect, op->call(a_cells, b_cells, 3)); +} + +TEST("require that pre-resolved subclass 'sum' with return value works") { + std::vector<int> a({1,2,3}); + TypedCells a_cells(a); + double expect = (1 + 2 + 3); + + auto op = create<Sum2>(a_cells.type); + + EXPECT_EQUAL(expect, op->call(a_cells)); +} + +//----------------------------------------------------------------------------- +//----------------------------------------------------------------------------- +// CASE STUDY: self-updating cached function pointer +//----------------------------------------------------------------------------- +//----------------------------------------------------------------------------- + +template <typename T, typename... Args> +auto get_fun(CellType a_type) { + switch(a_type) { + case CellType::DOUBLE: return T::template get_fun<double, Args...>(); + case CellType::FLOAT: return T::template get_fun<float, Args...>(); + case CellType::INT: return T::template get_fun<int, Args...>(); + } + abort(); +} + +template <typename T, typename... Args> +auto get_fun(CellType a_type, CellType b_type) { + switch(b_type) { + case CellType::DOUBLE: return get_fun<T, double, Args...>(a_type); + case CellType::FLOAT: return get_fun<T, float, Args...>(a_type); + case CellType::INT: return get_fun<T, int, Args...>(a_type); + } + abort(); +} + +template <typename T> +auto get_fun(CellType a_type, CellType b_type, CellType c_type) { + switch(c_type) { + case CellType::DOUBLE: return get_fun<T, double>(a_type, b_type); + case CellType::FLOAT: return get_fun<T, float>(a_type, b_type); + case CellType::INT: return get_fun<T, int>(a_type, b_type); + } + abort(); +} + +//----------------------------------------------------------------------------- + +struct CellwiseAdd3 { + struct Self; + using fun_type = void (*)(const TypedCells &x, const TypedCells &y, const TypedCells &z, size_t cnt, Self &self); + template <typename A, typename B, typename C> + static fun_type get_fun(); + struct Self { + fun_type my_fun; + Self(); + }; + Self self; + void call(const TypedCells &x, const TypedCells &y, const TypedCells &z, size_t cnt) { + self.my_fun(x, y, z, cnt, self); + } +}; + +template <typename A, typename B, typename C> +void cellwise_add(const TypedCells &x, const TypedCells &y, const TypedCells &z, size_t cnt, CellwiseAdd3::Self &self) { + if (!x.check_type<A>() || !y.check_type<B>() || !z.check_type<C>()) { + auto new_fun = get_fun<CellwiseAdd3>(x.type, y.type, z.type); + self.my_fun = new_fun; + return new_fun(x, y, z, cnt, self); + } + auto a = x.unsafe_typify<A>(); + auto b = y.unsafe_typify<B>(); + auto c = z.unsafe_typify<C>(); + auto dst = unconstify(c); + for (size_t i = 0; i < cnt; ++i) { + dst[i] = a[i] + b[i]; + } +}; + +template <typename A, typename B, typename C> +CellwiseAdd3::fun_type CellwiseAdd3::get_fun() { + return cellwise_add<A, B, C>; +} + +CellwiseAdd3::Self::Self() + : my_fun(cellwise_add<double, double, double>) +{ +} + +//----------------------------------------------------------------------------- + +struct DotProduct3 { + struct Self; + using fun_type = double (*)(const TypedCells &x, const TypedCells &y, size_t cnt, Self &self); + template <typename A, typename B> + static fun_type get_fun(); + struct Self { + fun_type my_fun; + Self(); + }; + Self self; + double call(const TypedCells &x, const TypedCells &y, size_t cnt) { + return self.my_fun(x, y, cnt, self); + } +}; + +template <typename A, typename B> +double dot_product(const TypedCells &x, const TypedCells &y, size_t cnt, DotProduct3::Self &self) { + if (!x.check_type<A>() || !y.check_type<B>()) { + auto new_fun = get_fun<DotProduct3>(x.type, y.type); + self.my_fun = new_fun; + return new_fun(x, y, cnt, self); + } + auto a = x.unsafe_typify<A>(); + auto b = y.unsafe_typify<B>(); + double result = 0.0; + for (size_t i = 0; i < cnt; ++i) { + result += (a[i] * b[i]); + } + return result; +} + +template <typename A, typename B> +DotProduct3::fun_type DotProduct3::get_fun() { + return dot_product<A, B>; +} + +DotProduct3::Self::Self() + : my_fun(dot_product<double, double>) +{ +} + +//----------------------------------------------------------------------------- + +struct Sum3 { + struct Self; + using fun_type = double (*)(const TypedCells &x, Self &self); + template <typename A> + static fun_type get_fun(); + struct Self { + fun_type my_fun; + Self(); + }; + Self self; + double call(const TypedCells &x) { + return self.my_fun(x, self); + } +}; + +template <typename A> +double sum(const TypedCells &x, Sum3::Self &self) { + if (!x.check_type<A>()) { + auto new_fun = get_fun<Sum3>(x.type); + self.my_fun = new_fun; + return new_fun(x, self); + } + auto a = x.unsafe_typify<A>(); + double result = 0.0; + for (const auto &value: a) { + result += value; + } + return result; +} + +template <typename A> +Sum3::fun_type Sum3::get_fun() { + return sum<A>; +} + +Sum3::Self::Self() + : my_fun(sum<double>) +{ +} + +//----------------------------------------------------------------------------- + +TEST("require that self-updating cached function pointer 'a op b -> c' works") { + std::vector<int> a({1,2,3}); + std::vector<float> b({1.5,2.5,3.5}); + std::vector<double> c(3, 0.0); + TypedCells a_cells(a); + TypedCells b_cells(b); + TypedCells c_cells(c); + + CellwiseAdd3 op; + EXPECT_EQUAL(op.self.my_fun, (&cellwise_add<double,double,double>)); + op.call(a_cells, b_cells, c_cells, 3); + EXPECT_EQUAL(op.self.my_fun, (&cellwise_add<int,float,double>)); + EXPECT_NOT_EQUAL(op.self.my_fun, (&cellwise_add<double,double,double>)); + + EXPECT_EQUAL(c[0], 2.5); + EXPECT_EQUAL(c[1], 4.5); + EXPECT_EQUAL(c[2], 6.5); +} + +TEST("require that self-updating cached function pointer 'dot product' with return value works") { + std::vector<int> a({1,2,3}); + std::vector<float> b({1.5,2.5,3.5}); + TypedCells a_cells(a); + TypedCells b_cells(b); + double expect = 1.5 + (2 * 2.5) + (3 * 3.5); + + DotProduct3 op; + EXPECT_EQUAL(op.self.my_fun, (&dot_product<double,double>)); + EXPECT_EQUAL(expect, op.call(a_cells, b_cells, 3)); + EXPECT_EQUAL(op.self.my_fun, (&dot_product<int,float>)); + EXPECT_NOT_EQUAL(op.self.my_fun, (&dot_product<double,double>)); +} + +TEST("require that self-updating cached function pointer 'sum' with return value works") { + std::vector<int> a({1,2,3}); + TypedCells a_cells(a); + double expect = (1 + 2 + 3); + + Sum3 op; + EXPECT_EQUAL(op.self.my_fun, (&sum<double>)); + EXPECT_EQUAL(expect, op.call(a_cells)); + EXPECT_EQUAL(op.self.my_fun, (&sum<int>)); + EXPECT_NOT_EQUAL(op.self.my_fun, (&sum<double>)); +} + +TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java b/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java index 7a76c588fb4..91bb69243c3 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/Request.java @@ -120,6 +120,7 @@ public class Request extends AbstractResource { * * @return The container instance. */ + // TODO: Vespa 7 remove. public Container container() { return parent != null ? parent.container() : container; } diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java index 907d2a050fb..38fa91b4831 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainer.java @@ -18,12 +18,16 @@ import com.yahoo.jdisc.service.ServerProvider; import java.net.URI; import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.logging.Logger; /** * @author Simon Thoresen + * @author bjorncs */ public class ActiveContainer extends AbstractResource implements CurrentContainer { + private static final Logger log = Logger.getLogger(ActiveContainer.class.getName()); + private final ContainerTermination termination; private final Injector guiceInjector; private final Iterable<ServerProvider> serverProviders; @@ -32,7 +36,7 @@ public class ActiveContainer extends AbstractResource implements CurrentContaine private final Map<String, BindingSet<RequestHandler>> clientBindings; private final BindingSetSelector bindingSetSelector; private final TimeoutManagerImpl timeoutMgr; - final Destructor destructor; + private final Destructor destructor; public ActiveContainer(ContainerBuilder builder) { serverProviders = builder.serverProviders().activate(); @@ -122,8 +126,19 @@ public class ActiveContainer extends AbstractResource implements CurrentContaine return new ContainerSnapshot(this, serverBindings, clientBindings); } + // TODO Rewrite to use cleaners after Java 9 migration + @Override + protected void finalize() throws Throwable { + boolean alreadyDestructed = destructor.destruct(); + if (!alreadyDestructed) { + log.severe(toString() + " was not correctly cleaned up " + + "because of a resource leak or invalid use of reference counting."); + } + super.finalize(); + } + // NOTE: An instance of this class must never contain a reference to the outer class (ActiveContainer). - static class Destructor { + private static class Destructor { private final ResourcePool resourceReferences; private final TimeoutManagerImpl timeoutMgr; private final ContainerTermination termination; diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdog.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdog.java deleted file mode 100644 index e41c149c0af..00000000000 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdog.java +++ /dev/null @@ -1,232 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.jdisc.core; - -import com.google.inject.Inject; -import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.statistics.ActiveContainerMetrics; - -import java.lang.ref.PhantomReference; -import java.lang.ref.ReferenceQueue; -import java.time.Clock; -import java.time.Duration; -import java.time.Instant; -import java.util.HashSet; -import java.util.List; -import java.util.Set; -import java.util.WeakHashMap; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ScheduledThreadPoolExecutor; -import java.util.concurrent.TimeUnit; -import java.util.logging.Level; -import java.util.logging.Logger; - -import static java.util.Comparator.comparing; -import static java.util.stream.Collectors.toList; - -/** - * A watchdog that monitors all deactivated {@link ActiveContainer} instances with the purpose of detecting containers - * that are unable to be garbage collected by the JVM. - * - * @author bjorncs - */ -// TODO Rewrite to use cleaners instead of phantom references after Java 9 migration -class ActiveContainerDeactivationWatchdog implements ActiveContainerMetrics, AutoCloseable { - // Frequency of writing warnings on stale container in the log - static final Duration REPORTING_FREQUENCY = Duration.ofMinutes(20); - // Only stale containers past the report grace period will be included in the metrics and log reporting - static final Duration REPORTING_GRACE_PERIOD = Duration.ofHours(4); - - // The frequency specifies how often gc can be triggered. - // This value should be a fraction of REPORTING_GRACE_PERIOD to eliminate chance of reporting deactivated containers - // that are unreferenced, but have not been gc'ed yet. - static final Duration GC_TRIGGER_FREQUENCY = Duration.ofHours(1); // Must be a fraction of REPORTING_GRACE_PERIOD - // Gc will only be triggered if there are any stale containers past the grace period. - // This value should be a fraction of REPORTING_GRACE_PERIOD to eliminate chance of reporting deactivated containers - // that are unreferenced, but have not been gc'ed yet. - static final Duration GC_GRACE_PERIOD = Duration.ofHours(2); - - // How often destruction of stale containers should be performed - static final Duration ENFORCE_DESTRUCTION_GCED_CONTAINERS_FREQUENCY = Duration.ofMinutes(5); - - private static final Logger log = Logger.getLogger(ActiveContainerDeactivationWatchdog.class.getName()); - - private final Object monitor = new Object(); - private final WeakHashMap<ActiveContainer, LifecycleStats> deactivatedContainers = new WeakHashMap<>(); - private final ReferenceQueue<ActiveContainer> garbageCollectedContainers = new ReferenceQueue<>(); - @SuppressWarnings("MismatchedQueryAndUpdateOfCollection") - // Instances of the phantom references must be kept alive until they are polled from the reference queue - private final Set<ActiveContainerPhantomReference> destructorReferences = new HashSet<>(); - private final ScheduledExecutorService scheduler; - private final Clock clock; - - private ActiveContainer currentContainer; - private Instant currentContainerActivationTime; - - @Inject - ActiveContainerDeactivationWatchdog() { - this( - Clock.systemUTC(), - new ScheduledThreadPoolExecutor(1, runnable -> { - Thread thread = new Thread(runnable, "active-container-deactivation-watchdog"); - thread.setDaemon(true); - return thread; - })); - } - - ActiveContainerDeactivationWatchdog(Clock clock, - ScheduledExecutorService scheduler) { - this.clock = clock; - this.scheduler = scheduler; - // NOTE: Make sure to update the unit test if the order commands are registered is changed. - this.scheduler.scheduleAtFixedRate(this::warnOnStaleContainers, - REPORTING_FREQUENCY.getSeconds(), - REPORTING_FREQUENCY.getSeconds(), - TimeUnit.SECONDS); - this.scheduler.scheduleAtFixedRate(this::triggerGc, - GC_TRIGGER_FREQUENCY.getSeconds(), - GC_TRIGGER_FREQUENCY.getSeconds(), - TimeUnit.SECONDS); - this.scheduler.scheduleAtFixedRate(this::enforceDestructionOfGarbageCollectedContainers, - ENFORCE_DESTRUCTION_GCED_CONTAINERS_FREQUENCY.getSeconds(), - ENFORCE_DESTRUCTION_GCED_CONTAINERS_FREQUENCY.getSeconds(), - TimeUnit.SECONDS); - } - - void onContainerActivation(ActiveContainer nextContainer) { - synchronized (monitor) { - Instant now = clock.instant(); - if (currentContainer != null) { - deactivatedContainers.put(currentContainer, new LifecycleStats(currentContainerActivationTime, now)); - destructorReferences.add(new ActiveContainerPhantomReference(currentContainer, garbageCollectedContainers)); - } - currentContainer = nextContainer; - currentContainerActivationTime = now; - } - } - - @Override - public void emitMetrics(Metric metric) { - List<DeactivatedContainer> snapshot = getDeactivatedContainersSnapshot(REPORTING_GRACE_PERIOD); - long containersWithRetainedRefsCount = snapshot.stream() - .filter(c -> c.activeContainer.retainCount() > 0) - .count(); - metric.set(TOTAL_DEACTIVATED_CONTAINERS, snapshot.size(), null); - metric.set(DEACTIVATED_CONTAINERS_WITH_RETAINED_REFERENCES, containersWithRetainedRefsCount, null); - } - - @Override - public void close() { - synchronized (monitor) { - scheduler.shutdown(); - deactivatedContainers.clear(); - destructorReferences.clear(); - currentContainer = null; - currentContainerActivationTime = null; - } - } - - private void warnOnStaleContainers() { - log.log(Level.FINE, "Checking for stale containers"); - try { - List<DeactivatedContainer> snapshot = getDeactivatedContainersSnapshot(REPORTING_GRACE_PERIOD); - if (snapshot.isEmpty()) return; - logWarning(snapshot); - } catch (Throwable t) { - log.log(Level.WARNING, "Watchdog task died!", t); - } - } - - private void triggerGc() { - int deactivatedContainers = getDeactivatedContainersSnapshot(GC_GRACE_PERIOD).size(); - boolean shouldGc = deactivatedContainers > 0; - if (!shouldGc) return; - log.log(Level.INFO, String.format("Triggering GC and finalization (currently %d deactivated containers still alive)", deactivatedContainers)); - System.gc(); - System.runFinalization(); - } - - private void enforceDestructionOfGarbageCollectedContainers() { - log.log(Level.FINE, "Enforcing destruction of GCed containers"); - ActiveContainerPhantomReference reference; - while ((reference = (ActiveContainerPhantomReference) garbageCollectedContainers.poll()) != null) { - try { - reference.enforceDestruction(); - } catch (Throwable t) { - log.log(Level.SEVERE, "Failed to do post-GC destruction of " + reference.containerName, t); - } finally { - destructorReferences.remove(reference); - reference.clear(); - } - } - } - - private List<DeactivatedContainer> getDeactivatedContainersSnapshot(Duration gracePeriod) { - Instant now = clock.instant(); - synchronized (monitor) { - return deactivatedContainers.entrySet().stream() - .filter(e -> e.getValue().isPastGracePeriod(now, gracePeriod)) - .map(e -> new DeactivatedContainer(e.getKey(), e.getValue())) - .sorted(comparing(e -> e.lifecycleStats.timeActivated)) - .collect(toList()); - } - } - - private static void logWarning(List<DeactivatedContainer> snapshot) { - log.warning(String.format("%s instances of deactivated containers are still alive.", snapshot.size())); - for (DeactivatedContainer deactivatedContainer : snapshot) { - log.warning(" - " + deactivatedContainer.toSummaryString()); - } - } - - private static class LifecycleStats { - public final Instant timeActivated; - public final Instant timeDeactivated; - - public LifecycleStats(Instant timeActivated, Instant timeDeactivated) { - this.timeActivated = timeActivated; - this.timeDeactivated = timeDeactivated; - } - - public boolean isPastGracePeriod(Instant instant, Duration gracePeriod) { - return timeDeactivated.plus(gracePeriod).isBefore(instant); - } - } - - private static class DeactivatedContainer { - public final ActiveContainer activeContainer; - public final LifecycleStats lifecycleStats; - - public DeactivatedContainer(ActiveContainer activeContainer, LifecycleStats lifecycleStats) { - this.activeContainer = activeContainer; - this.lifecycleStats = lifecycleStats; - } - - public String toSummaryString() { - return String.format("%s: time activated = %s, time deactivated = %s, reference count = %d", - activeContainer.toString(), - lifecycleStats.timeActivated.toString(), - lifecycleStats.timeDeactivated.toString(), - activeContainer.retainCount()); - } - } - - private static class ActiveContainerPhantomReference extends PhantomReference<ActiveContainer> { - public final String containerName; - private final ActiveContainer.Destructor destructor; - - public ActiveContainerPhantomReference(ActiveContainer activeContainer, - ReferenceQueue<? super ActiveContainer> q) { - super(activeContainer, q); - this.containerName = activeContainer.toString(); - this.destructor = activeContainer.destructor; - } - - public void enforceDestruction() { - boolean alreadyCompleted = destructor.destruct(); - if (!alreadyCompleted) { - log.severe(containerName + " was not correctly cleaned up " + - "because of a resource leak or invalid use of reference counting."); - } - } - } -} diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationEnvironmentModule.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationEnvironmentModule.java index 7d2f300ab7d..b7cab923454 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationEnvironmentModule.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationEnvironmentModule.java @@ -1,4 +1,4 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.jdisc.core; import com.google.inject.AbstractModule; @@ -8,12 +8,11 @@ import com.yahoo.jdisc.application.ContainerBuilder; import com.yahoo.jdisc.application.ContainerThread; import com.yahoo.jdisc.application.OsgiFramework; import com.yahoo.jdisc.service.CurrentContainer; -import com.yahoo.jdisc.statistics.ActiveContainerMetrics; import java.util.concurrent.ThreadFactory; /** - * @author <a href="mailto:simon@yahoo-inc.com">Simon Thoresen</a> + * @author Simon Thoresen */ class ApplicationEnvironmentModule extends AbstractModule { @@ -29,7 +28,6 @@ class ApplicationEnvironmentModule extends AbstractModule { bind(CurrentContainer.class).toInstance(loader); bind(OsgiFramework.class).toInstance(loader.osgiFramework()); bind(ThreadFactory.class).to(ContainerThread.Factory.class); - bind(ActiveContainerMetrics.class).toInstance(loader.getActiveContainerMetrics()); } @Provides diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationLoader.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationLoader.java index 10b4c9f6508..81eb5815a01 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationLoader.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/ApplicationLoader.java @@ -1,4 +1,4 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.jdisc.core; import com.google.inject.AbstractModule; @@ -15,7 +15,6 @@ import com.yahoo.jdisc.application.OsgiFramework; import com.yahoo.jdisc.application.OsgiHeader; import com.yahoo.jdisc.service.ContainerNotReadyException; import com.yahoo.jdisc.service.CurrentContainer; -import com.yahoo.jdisc.statistics.ActiveContainerMetrics; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.BundleException; @@ -41,7 +40,6 @@ public class ApplicationLoader implements BootstrapLoader, ContainerActivator, C private final AtomicReference<ActiveContainer> containerRef = new AtomicReference<>(); private final Object appLock = new Object(); private final List<Bundle> appBundles = new ArrayList<>(); - private final ActiveContainerDeactivationWatchdog watchdog = new ActiveContainerDeactivationWatchdog(); private Application application; private ApplicationInUseTracker applicationInUseTracker; @@ -70,7 +68,6 @@ public class ApplicationLoader implements BootstrapLoader, ContainerActivator, C next.retainReference(applicationInUseTracker); } - watchdog.onContainerActivation(next); prev = containerRef.getAndSet(next); if (prev == null) { return null; @@ -195,7 +192,6 @@ public class ApplicationLoader implements BootstrapLoader, ContainerActivator, C @Override public void destroy() { log.finer("Destroying application loader."); - watchdog.close(); try { osgiFramework.stop(); } catch (BundleException e) { @@ -209,10 +205,6 @@ public class ApplicationLoader implements BootstrapLoader, ContainerActivator, C } } - public ActiveContainerMetrics getActiveContainerMetrics() { - return watchdog; - } - public OsgiFramework osgiFramework() { return osgiFramework; } @@ -227,18 +219,4 @@ public class ApplicationLoader implements BootstrapLoader, ContainerActivator, C } } - private static class Interruption { - interface Runnable_throws<E extends Throwable> { - void run() throws E; - } - - public static void mapExceptionToThreadState(Runnable_throws<InterruptedException> runnable) { - try { - runnable.run(); - } catch (InterruptedException ignored) { - Thread.currentThread().interrupt(); - } - } - } - } diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/package-info.java b/jdisc_core/src/main/java/com/yahoo/jdisc/package-info.java index d1d842fa3f6..ff8dd242587 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/package-info.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/package-info.java @@ -50,5 +50,6 @@ * @see com.yahoo.jdisc.service * @see com.yahoo.jdisc.test */ +// TODO: Vespa 7 remove internal classes (at least Container) out of this PublicApi package. @com.yahoo.api.annotations.PublicApi package com.yahoo.jdisc; diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/statistics/ActiveContainerMetrics.java b/jdisc_core/src/main/java/com/yahoo/jdisc/statistics/ActiveContainerMetrics.java deleted file mode 100644 index 440c9af6bf5..00000000000 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/statistics/ActiveContainerMetrics.java +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.jdisc.statistics; - -import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.core.ActiveContainer; - -/** - * Tracks statistics on stale {@link ActiveContainer} instances. - * - * @author bjorncs - */ -public interface ActiveContainerMetrics { - - String TOTAL_DEACTIVATED_CONTAINERS = "jdisc.deactivated_containers.total"; - String DEACTIVATED_CONTAINERS_WITH_RETAINED_REFERENCES = "jdisc.deactivated_containers.with_retained_refs"; - - void emitMetrics(Metric metric); - -} diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/statistics/package-info.java b/jdisc_core/src/main/java/com/yahoo/jdisc/statistics/package-info.java deleted file mode 100644 index 2558e693e39..00000000000 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/statistics/package-info.java +++ /dev/null @@ -1,4 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -@com.yahoo.osgi.annotation.ExportPackage -package com.yahoo.jdisc.statistics; diff --git a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdogTest.java b/jdisc_core/src/test/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdogTest.java deleted file mode 100644 index e2de08e760f..00000000000 --- a/jdisc_core/src/test/java/com/yahoo/jdisc/core/ActiveContainerDeactivationWatchdogTest.java +++ /dev/null @@ -1,159 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.jdisc.core; - -import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.ResourceReference; -import com.yahoo.jdisc.statistics.ActiveContainerMetrics; -import com.yahoo.jdisc.test.TestDriver; -import com.yahoo.test.ManualClock; -import org.junit.Ignore; -import org.junit.Test; - -import java.lang.ref.WeakReference; -import java.time.Duration; -import java.time.Instant; -import java.util.Map; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledFuture; -import java.util.concurrent.ScheduledThreadPoolExecutor; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; - -/** - * @author bjorncs - */ -public class ActiveContainerDeactivationWatchdogTest { - - @Test - public void watchdog_counts_deactivated_containers() { - TestDriver driver = TestDriver.newSimpleApplicationInstanceWithoutOsgi(); - ManualClock clock = new ManualClock(Instant.now()); - ActiveContainerDeactivationWatchdog watchdog = - new ActiveContainerDeactivationWatchdog(clock, Executors.newScheduledThreadPool(1)); - MockMetric metric = new MockMetric(); - - ActiveContainer containerWithoutRetainedResources = new ActiveContainer(driver.newContainerBuilder()); - - watchdog.onContainerActivation(containerWithoutRetainedResources); - watchdog.emitMetrics(metric); - assertEquals(0, metric.totalCount); - assertEquals(0, metric.withRetainedReferencesCount); - - watchdog.onContainerActivation(null); - containerWithoutRetainedResources.release(); - clock.advance(ActiveContainerDeactivationWatchdog.REPORTING_GRACE_PERIOD); - watchdog.emitMetrics(metric); - assertEquals(0, metric.totalCount); - assertEquals(0, metric.withRetainedReferencesCount); - - clock.advance(Duration.ofSeconds(1)); - watchdog.emitMetrics(metric); - assertEquals(1, metric.totalCount); - assertEquals(0, metric.withRetainedReferencesCount); - - ActiveContainer containerWithRetainedResources = new ActiveContainer(driver.newContainerBuilder()); - try (ResourceReference ignoredRef = containerWithRetainedResources.refer()) { - watchdog.onContainerActivation(containerWithRetainedResources); - containerWithRetainedResources.release(); - watchdog.onContainerActivation(null); - clock.advance(ActiveContainerDeactivationWatchdog.REPORTING_GRACE_PERIOD.plusSeconds(1)); - watchdog.emitMetrics(metric); - assertEquals(2, metric.totalCount); - assertEquals(1, metric.withRetainedReferencesCount); - } - - } - - @Test - @Ignore("Ignored as it assumes phantom references are enqueued right after first GC have cleared the weak reference. " + - "This is the case on most JVMs.") - public void deactivated_container_destructed_if_its_reference_counter_is_nonzero() { - ExecutorMock executor = new ExecutorMock(); - ManualClock clock = new ManualClock(Instant.now()); - ActiveContainerDeactivationWatchdog watchdog = new ActiveContainerDeactivationWatchdog(clock, executor); - ActiveContainer container = - new ActiveContainer(TestDriver.newSimpleApplicationInstanceWithoutOsgi().newContainerBuilder()); - AtomicBoolean destructed = new AtomicBoolean(false); - container.shutdown().notifyTermination(() -> destructed.set(true)); - - container.refer(); // increase reference counter to simulate a leaking resource - watchdog.onContainerActivation(container); - container.release(); // release resource - watchdog.onContainerActivation(null); // deactivate container - - WeakReference<ActiveContainer> containerWeakReference = new WeakReference<>(container); - container = null; // make container instance collectable by GC - clock.advance(ActiveContainerDeactivationWatchdog.GC_GRACE_PERIOD.plusSeconds(1)); - - executor.triggerGcCommand.run(); - - assertNull("Container is not GCed - probably because the watchdog has a concrete reference to it", - containerWeakReference.get()); - - executor.enforceDestructionOfGarbageCollectedContainersCommand.run(); - - assertTrue("Destructor is not called on deactivated container", destructed.get()); - } - - private static class MockMetric implements Metric { - public int totalCount; - public int withRetainedReferencesCount; - - @Override - public void set(String key, Number val, Context ctx) { - switch (key) { - case ActiveContainerMetrics.TOTAL_DEACTIVATED_CONTAINERS: - totalCount = val.intValue(); - break; - case ActiveContainerMetrics.DEACTIVATED_CONTAINERS_WITH_RETAINED_REFERENCES: - withRetainedReferencesCount = val.intValue(); - break; - default: - throw new UnsupportedOperationException(); - } - } - - @Override - public void add(String key, Number val, Context ctx) { - throw new UnsupportedOperationException(); - } - - @Override - public Context createContext(Map<String, ?> properties) { - throw new UnsupportedOperationException(); - } - } - - private static class ExecutorMock extends ScheduledThreadPoolExecutor { - - public Runnable warnOnStaleContainersCommand; - public Runnable triggerGcCommand; - public Runnable enforceDestructionOfGarbageCollectedContainersCommand; - private int registrationCounter = 0; - - public ExecutorMock() { - super(1); - } - - @Override - public ScheduledFuture<?> scheduleAtFixedRate(Runnable command, long initialDelay, long period, TimeUnit unit) { - if (registrationCounter == 0) { - warnOnStaleContainersCommand = command; - } else if (registrationCounter == 1) { - triggerGcCommand = command; - } else if (registrationCounter == 2) { - enforceDestructionOfGarbageCollectedContainersCommand = command; - } else { - throw new IllegalStateException("Unexpected registration"); - } - ++registrationCounter; - return null; - } - - } - -} diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java index 824511af79f..7c79897ac27 100644 --- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java +++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpServerTest.java @@ -132,10 +132,11 @@ public class HttpServerTest { binder -> binder.bind(AccessLog.class).toInstance(accessLogMock)); driver.client().get("/status.html") .expectStatusCode(is(REQUEST_URI_TOO_LONG)); + assertThat(driver.close(), is(true)); + assertThat(accessLogMock.logEntries.size(), equalTo(1)); AccessLogEntry accessLogEntry = accessLogMock.logEntries.get(0); assertThat(accessLogEntry.getStatusCode(), equalTo(414)); - assertThat(driver.close(), is(true)); } private static class AccessLogMock extends AccessLog { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index 4bf7e70d06b..ade2c94a25c 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -40,6 +40,7 @@ import java.util.Set; import java.util.TreeSet; import java.util.function.UnaryOperator; import java.util.stream.Collectors; +import java.util.stream.Stream; /** * The hosted Vespa production node repository, which stores its state in Zookeeper. @@ -394,12 +395,31 @@ public class NodeRepository extends AbstractComponent { return db.writeTo(Node.State.dirty, node, agent, Optional.of(reason)); } - public Node setDirty(String hostname, Agent agent, String reason) { - Node node = getNode(hostname, Node.State.provisioned, Node.State.failed, Node.State.parked).orElseThrow(() -> - new IllegalArgumentException("Could not deallocate " + hostname + - ": No such node in the provisioned, failed or parked state")); + public List<Node> dirtyRecursively(String hostname, Agent agent, String reason) { + Node nodeToDirty = getNode(hostname).orElseThrow(() -> + new IllegalArgumentException("Could not deallocate " + hostname + ": Node not found")); - return setDirty(node, agent, reason); + List<Node> nodesToDirty = + (nodeToDirty.type().isDockerHost() ? + Stream.concat(getChildNodes(hostname).stream(), Stream.of(nodeToDirty)) : + Stream.of(nodeToDirty)) + .filter(node -> node.state() != Node.State.dirty) + .collect(Collectors.toList()); + + List<String> hostnamesNotAllowedToDirty = nodesToDirty.stream() + .filter(node -> node.state() != Node.State.provisioned) + .filter(node -> node.state() != Node.State.failed) + .filter(node -> node.state() != Node.State.parked) + .map(Node::hostname) + .collect(Collectors.toList()); + if (!hostnamesNotAllowedToDirty.isEmpty()) { + throw new IllegalArgumentException("Could not deallocate " + hostname + ": " + + String.join(", ", hostnamesNotAllowedToDirty) + " must be in either provisioned, failed or parked state"); + } + + return nodesToDirty.stream() + .map(node -> setDirty(node, agent, reason)) + .collect(Collectors.toList()); } /** diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java index af0dc859d89..0d5525370ee 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java @@ -42,7 +42,7 @@ public class InactiveExpirer extends Expirer { if (node.status().wantToRetire()) { nodeRepository.park(node.hostname(), Agent.system, "Expired by InactiveExpirer"); } else { - nodeRepository.setDirty(node, Agent.system, "Expired by InactiveExprier"); + nodeRepository.setDirty(node, Agent.system, "Expired by InactiveExpirer"); } }); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java index effa8f17d52..7fc305d397f 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java @@ -4,7 +4,6 @@ package com.yahoo.vespa.hosted.provision.persistence; import com.google.common.util.concurrent.UncheckedTimeoutException; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationLockException; -import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.NodeFlavors; import com.yahoo.config.provision.Zone; import com.yahoo.log.LogLevel; @@ -217,9 +216,9 @@ public class CuratorDatabaseClient { return node.status(); } - /** In automated test environments, nodes need to be reused quickly to achieve fast test turnaronud time */ + /** In automated test environments, nodes need to be reused quickly to achieve fast test turnaround time */ private boolean needsFastNodeReuse(Zone zone) { - return zone.environment() == Environment.staging || zone.environment() == Environment.test; + return zone.environment().isTest(); } /** diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java index 2f7d3120211..03777078251 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java @@ -34,7 +34,6 @@ import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; -import java.util.concurrent.Executor; import java.util.logging.Level; import java.util.stream.Collectors; import javax.inject.Inject; @@ -122,8 +121,9 @@ public class NodesApiHandler extends LoggingRequestHandler { return new MessageResponse("Moved " + parkedHostnames + " to parked"); } else if (path.startsWith("/nodes/v2/state/dirty/")) { - nodeRepository.setDirty(lastElement(path), Agent.operator, "Dirtied through the nodes/v2 API"); - return new MessageResponse("Moved " + lastElement(path) + " to dirty"); + List<Node> dirtiedNodes = nodeRepository.dirtyRecursively(lastElement(path), Agent.operator, "Dirtied through the nodes/v2 API"); + String dirtiedHostnames = dirtiedNodes.stream().map(Node::hostname).sorted().collect(Collectors.joining(", ")); + return new MessageResponse("Moved " + dirtiedHostnames + " to dirty"); } else if (path.startsWith("/nodes/v2/state/active/")) { nodeRepository.reactivate(lastElement(path), Agent.operator, "Reactivated through nodes/v2 API"); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java index a21d03ff603..931ddc5e051 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java @@ -108,7 +108,7 @@ public class MockNodeRepository extends NodeRepository { setReady(nodes, Agent.system, getClass().getSimpleName()); fail("host5.yahoo.com", Agent.system, getClass().getSimpleName()); - setDirty("host55.yahoo.com", Agent.system, getClass().getSimpleName()); + dirtyRecursively("host55.yahoo.com", Agent.system, getClass().getSimpleName()); ApplicationId zoneApp = ApplicationId.from(TenantName.from("zoneapp"), ApplicationName.from("zoneapp"), InstanceName.from("zoneapp")); ClusterSpec zoneCluster = ClusterSpec.request(ClusterSpec.Type.container, diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java index 6e97f3d1269..5d217ce86e5 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java @@ -11,7 +11,12 @@ import com.yahoo.vespa.hosted.provision.node.Agent; import org.junit.Test; import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.HashSet; import java.util.Optional; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; import static junit.framework.TestCase.fail; import static org.junit.Assert.assertEquals; @@ -72,7 +77,7 @@ public class NodeRepositoryTest { // Expected } - tester.nodeRepository().setDirty("host1", Agent.system, getClass().getSimpleName()); + tester.nodeRepository().dirtyRecursively("host1", Agent.system, getClass().getSimpleName()); tester.nodeRepository().setReady("host1", Agent.system, getClass().getSimpleName()); tester.nodeRepository().removeRecursively("host1"); } @@ -89,7 +94,7 @@ public class NodeRepositoryTest { tester.addNode("node20", "node20", "host2", "docker", NodeType.tenant); assertEquals(6, tester.nodeRepository().getNodes().size()); - tester.nodeRepository().setDirty("node11", Agent.system, getClass().getSimpleName()); + tester.setNodeState("node11", Node.State.dirty); try { tester.nodeRepository().removeRecursively("host1"); @@ -111,4 +116,46 @@ public class NodeRepositoryTest { tester.nodeRepository().removeRecursively("host1"); assertEquals(0, tester.nodeRepository().getNodes().size()); } + + @Test + public void dirty_host_only_if_we_can_dirty_children() { + NodeRepositoryTester tester = new NodeRepositoryTester(); + + tester.addNode("id1", "host1", "default", NodeType.host); + tester.addNode("id2", "host2", "default", NodeType.host); + tester.addNode("node10", "node10", "host1", "docker", NodeType.tenant); + tester.addNode("node11", "node11", "host1", "docker", NodeType.tenant); + tester.addNode("node12", "node12", "host1", "docker", NodeType.tenant); + tester.addNode("node20", "node20", "host2", "docker", NodeType.tenant); + + tester.setNodeState("node11", Node.State.ready); + tester.setNodeState("node12", Node.State.active); + tester.setNodeState("node20", Node.State.failed); + + assertEquals(6, tester.nodeRepository().getNodes().size()); + + // Should be OK to dirty host2 as it is in provisioned and its only child is in failed + tester.nodeRepository().dirtyRecursively("host2", Agent.system, NodeRepositoryTest.class.getSimpleName()); + assertEquals(asSet("host2", "node20"), filterNodes(tester, node -> node.state() == Node.State.dirty)); + + // Cant dirty host1, node11 is ready and node12 is active + try { + tester.nodeRepository().dirtyRecursively("host1", Agent.system, NodeRepositoryTest.class.getSimpleName()); + fail("Should not be able to dirty host1"); + } catch (IllegalArgumentException ignored) { } // Expected; + + assertEquals(asSet("host2", "node20"), filterNodes(tester, node -> node.state() == Node.State.dirty)); + } + + private static Set<String> asSet(String... elements) { + return new HashSet<>(Arrays.asList(elements)); + } + + private static Set<String> filterNodes(NodeRepositoryTester tester, Predicate<Node> filter) { + return tester.nodeRepository() + .getNodes().stream() + .filter(filter) + .map(Node::hostname) + .collect(Collectors.toSet()); + } } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java index 1a42a71863d..03ada1e7951 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java @@ -9,6 +9,7 @@ import com.yahoo.config.provision.Zone; import com.yahoo.config.provisioning.FlavorsConfig; import com.yahoo.test.ManualClock; import com.yahoo.vespa.curator.mock.MockCurator; +import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.hosted.provision.provisioning.FlavorConfigBuilder; import com.yahoo.vespa.hosted.provision.testutils.MockNameResolver; @@ -58,6 +59,16 @@ public class NodeRepositoryTester { return nodeRepository.addNodes(Collections.singletonList(node)).get(0); } + /** + * Moves a node directly to the given state without doing any validation, useful + * to create wanted test scenario without having to move every node through series + * of valid state transitions + */ + public void setNodeState(String hostname, Node.State state) { + Node node = nodeRepository.getNode(hostname).orElseThrow(RuntimeException::new); + nodeRepository.database().writeTo(state, node, Agent.system, Optional.empty()); + } + private FlavorsConfig createConfig() { FlavorConfigBuilder b = new FlavorConfigBuilder(); b.addFlavor("default", 2., 4., 100, Flavor.Type.BARE_METAL).cost(3); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java index 622732460c0..5e7c6787973 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java @@ -125,7 +125,7 @@ public class MetricsReporterTest { Node dockerHost = Node.create("openStackId1", Collections.singleton("::1"), additionalIps, "dockerHost", Optional.empty(), nodeFlavors.getFlavorOrThrow("host"), NodeType.host); nodeRepository.addNodes(Collections.singletonList(dockerHost)); - nodeRepository.setDirty("dockerHost", Agent.system, getClass().getSimpleName()); + nodeRepository.dirtyRecursively("dockerHost", Agent.system, getClass().getSimpleName()); nodeRepository.setReady("dockerHost", Agent.system, getClass().getSimpleName()); Node container1 = Node.createDockerNode("openStackId1:1", Collections.singleton("::2"), Collections.emptySet(), "container1", Optional.of("dockerHost"), nodeFlavors.getFlavorOrThrow("docker"), NodeType.tenant); diff --git a/parent/pom.xml b/parent/pom.xml index 465771de9cf..7f22c5ccda4 100644 --- a/parent/pom.xml +++ b/parent/pom.xml @@ -116,7 +116,10 @@ <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-deploy-plugin</artifactId> - <version>2.5</version> + <version>2.8.1</version> + <configuration> + <retryFailedDeploymentCount>10</retryFailedDeploymentCount> + </configuration> </plugin> <plugin> <groupId>org.apache.maven.plugins</groupId> diff --git a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp index 3bdc622aa66..838df87662f 100644 --- a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp +++ b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp @@ -48,7 +48,11 @@ try{ \ namespace storage { namespace { - spi::LoadType defaultLoadType(0, "default"); + +spi::LoadType defaultLoadType(0, "default"); + +struct TestFileStorComponents; + } struct FileStorManagerTest : public CppUnit::TestFixture { @@ -97,6 +101,10 @@ struct FileStorManagerTest : public CppUnit::TestFixture { void testStateChange(); void testRepairNotifiesDistributorOnChange(); void testDiskMove(); + void put_command_size_is_added_to_metric(); + void update_command_size_is_added_to_metric(); + void remove_command_size_is_added_to_metric(); + void get_command_size_is_added_to_metric(); CPPUNIT_TEST_SUITE(FileStorManagerTest); CPPUNIT_TEST(testPut); @@ -131,6 +139,10 @@ struct FileStorManagerTest : public CppUnit::TestFixture { CPPUNIT_TEST(testStateChange); CPPUNIT_TEST(testRepairNotifiesDistributorOnChange); CPPUNIT_TEST(testDiskMove); + CPPUNIT_TEST(put_command_size_is_added_to_metric); + CPPUNIT_TEST(update_command_size_is_added_to_metric); + CPPUNIT_TEST(remove_command_size_is_added_to_metric); + CPPUNIT_TEST(get_command_size_is_added_to_metric); CPPUNIT_TEST_SUITE_END(); void createBucket(document::BucketId bid, uint16_t disk) @@ -215,6 +227,15 @@ struct FileStorManagerTest : public CppUnit::TestFixture { FileStorHandler& filestorHandler, const document::BucketId& bucket, uint32_t docNum); + + template <typename Metric> + void assert_request_size_set(TestFileStorComponents& c, + std::shared_ptr<api::StorageMessage> cmd, + const Metric& metric); + + auto& thread_metrics_of(FileStorManager& manager) { + return manager._metrics->disks[0]->threads[0]; + } }; CPPUNIT_TEST_SUITE_REGISTRATION(FileStorManagerTest); @@ -2679,4 +2700,57 @@ FileStorManagerTest::testCreateBucketSetsActiveFlagInDatabaseAndReply() } } +template <typename Metric> +void FileStorManagerTest::assert_request_size_set(TestFileStorComponents& c, std::shared_ptr<api::StorageMessage> cmd, const Metric& metric) { + api::StorageMessageAddress address("storage", lib::NodeType::STORAGE, 3); + cmd->setApproxByteSize(54321); + cmd->setAddress(address); + c.top.sendDown(cmd); + c.top.waitForMessages(1, _waitTime); + CPPUNIT_ASSERT_EQUAL(static_cast<int64_t>(cmd->getApproxByteSize()), metric.request_size.getLast()); +} + +void FileStorManagerTest::put_command_size_is_added_to_metric() { + TestFileStorComponents c(*this, "put_command_size_is_added_to_metric"); + document::BucketId bucket(16, 4000); + createBucket(bucket, 0); + auto cmd = std::make_shared<api::PutCommand>( + makeDocumentBucket(bucket), _node->getTestDocMan().createRandomDocument(), api::Timestamp(12345)); + + assert_request_size_set(c, std::move(cmd), thread_metrics_of(*c.manager)->put[defaultLoadType]); +} + +void FileStorManagerTest::update_command_size_is_added_to_metric() { + TestFileStorComponents c(*this, "update_command_size_is_added_to_metric"); + document::BucketId bucket(16, 4000); + createBucket(bucket, 0); + auto update = std::make_shared<document::DocumentUpdate>( + _node->getTestDocMan().createRandomDocument()->getType(), + document::DocumentId("id:foo:testdoctype1::bar")); + auto cmd = std::make_shared<api::UpdateCommand>( + makeDocumentBucket(bucket), std::move(update), api::Timestamp(123456)); + + assert_request_size_set(c, std::move(cmd), thread_metrics_of(*c.manager)->update[defaultLoadType]); +} + +void FileStorManagerTest::remove_command_size_is_added_to_metric() { + TestFileStorComponents c(*this, "remove_command_size_is_added_to_metric"); + document::BucketId bucket(16, 4000); + createBucket(bucket, 0); + auto cmd = std::make_shared<api::RemoveCommand>( + makeDocumentBucket(bucket), document::DocumentId("id:foo:testdoctype1::bar"), api::Timestamp(123456)); + + assert_request_size_set(c, std::move(cmd), thread_metrics_of(*c.manager)->remove[defaultLoadType]); +} + +void FileStorManagerTest::get_command_size_is_added_to_metric() { + TestFileStorComponents c(*this, "get_command_size_is_added_to_metric"); + document::BucketId bucket(16, 4000); + createBucket(bucket, 0); + auto cmd = std::make_shared<api::GetCommand>( + makeDocumentBucket(bucket), document::DocumentId("id:foo:testdoctype1::bar"), "[all]"); + + assert_request_size_set(c, std::move(cmd), thread_metrics_of(*c.manager)->get[defaultLoadType]); +} + } // storage diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp index 2efae7720e4..1293b5d1808 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp @@ -9,7 +9,7 @@ namespace storage { using metrics::MetricSet; using metrics::LoadTypeSet; -FileStorThreadMetrics::Op::Op(const std::string& id, const std::string name, MetricSet* owner) +FileStorThreadMetrics::Op::Op(const std::string& id, const std::string& name, MetricSet* owner) : MetricSet(id, id, name + " load in filestor thread", owner, "operationtype"), _name(name), count("count", "yamasdefault", "Number of requests processed.", this), @@ -17,7 +17,7 @@ FileStorThreadMetrics::Op::Op(const std::string& id, const std::string name, Met failed("failed", "yamasdefault", "Number of failed requests.", this) { } -FileStorThreadMetrics::Op::~Op() { } +FileStorThreadMetrics::Op::~Op() = default; MetricSet * FileStorThreadMetrics::Op::clone(std::vector<Metric::UP>& ownerList, @@ -31,13 +31,39 @@ FileStorThreadMetrics::Op::clone(std::vector<Metric::UP>& ownerList, return (Op*) (new Op(getName(), _name, owner))->assignValues(*this); } -FileStorThreadMetrics::OpWithNotFound::OpWithNotFound(const std::string& id, const std::string name, MetricSet* owner) +template <typename BaseOp> +FileStorThreadMetrics::OpWithRequestSize<BaseOp>::OpWithRequestSize(const std::string& id, const std::string& name, MetricSet* owner) + : BaseOp(id, name, owner), + request_size("request_size", "", "Size of requests, in bytes", this) +{ +} + +template <typename BaseOp> +FileStorThreadMetrics::OpWithRequestSize<BaseOp>::~OpWithRequestSize() = default; + +// FIXME this has very non-intuitive semantics, ending up with copy&paste patterns +template <typename BaseOp> +MetricSet* +FileStorThreadMetrics::OpWithRequestSize<BaseOp>::clone( + std::vector<Metric::UP>& ownerList, + CopyType copyType, + MetricSet* owner, + bool includeUnused) const +{ + if (copyType == INACTIVE) { + return MetricSet::clone(ownerList, INACTIVE, owner, includeUnused); + } + return static_cast<OpWithRequestSize<BaseOp>*>((new OpWithRequestSize<BaseOp>(this->getName(), this->_name, owner)) + ->assignValues(*this)); +} + +FileStorThreadMetrics::OpWithNotFound::OpWithNotFound(const std::string& id, const std::string& name, MetricSet* owner) : Op(id, name, owner), notFound("not_found", "", "Number of requests that could not be " "completed due to source document not found.", this) { } -FileStorThreadMetrics::OpWithNotFound::~OpWithNotFound() { } +FileStorThreadMetrics::OpWithNotFound::~OpWithNotFound() = default; MetricSet * FileStorThreadMetrics::OpWithNotFound::clone(std::vector<Metric::UP>& ownerList, @@ -54,11 +80,11 @@ FileStorThreadMetrics::OpWithNotFound::clone(std::vector<Metric::UP>& ownerList, } FileStorThreadMetrics::Update::Update(MetricSet* owner) - : OpWithNotFound("update", "Update", owner), + : OpWithRequestSize("update", "Update", owner), latencyRead("latency_read", "", "Latency of the source read in the request.", this) { } -FileStorThreadMetrics::Update::~Update() { } +FileStorThreadMetrics::Update::~Update() = default; MetricSet * FileStorThreadMetrics::Update::clone(std::vector<Metric::UP>& ownerList, @@ -77,7 +103,7 @@ FileStorThreadMetrics::Visitor::Visitor(MetricSet* owner) documentsPerIterate("docs", "", "Number of entries read per iterate call", this) { } -FileStorThreadMetrics::Visitor::~Visitor() { } +FileStorThreadMetrics::Visitor::~Visitor() = default; MetricSet * FileStorThreadMetrics::Visitor::clone(std::vector<Metric::UP>& ownerList, @@ -95,16 +121,16 @@ FileStorThreadMetrics::FileStorThreadMetrics(const std::string& name, const std: : MetricSet(name, "filestor partofsum thread", desc, nullptr, "thread"), operations("operations", "", "Number of operations processed.", this), failedOperations("failedoperations", "", "Number of operations throwing exceptions.", this), - put(lt, *&Op("put", "Put"), this), - get(lt, *&OpWithNotFound("get", "Get"), this), - remove(lt, *&OpWithNotFound("remove", "Remove"), this), - removeLocation(lt, *&Op("remove_location", "Remove location"), this), - statBucket(lt, *&Op("stat_bucket", "Stat bucket"), this), - update(lt, *&Update(), this), - revert(lt, *&OpWithNotFound("revert", "Revert"), this), + put(lt, OpWithRequestSize<Op>("put", "Put"), this), + get(lt, OpWithRequestSize<OpWithNotFound>("get", "Get"), this), + remove(lt, OpWithRequestSize<OpWithNotFound>("remove", "Remove"), this), + removeLocation(lt, Op("remove_location", "Remove location"), this), + statBucket(lt, Op("stat_bucket", "Stat bucket"), this), + update(lt, Update(), this), + revert(lt, OpWithNotFound("revert", "Revert"), this), createIterator("createiterator", "", this), - visit(lt, *&Visitor(), this), - multiOp(lt, *&Op("multioperations", "The number of multioperations that have been created"), this), + visit(lt, Visitor(), this), + multiOp(lt, Op("multioperations", "The number of multioperations that have been created"), this), createBuckets("createbuckets", "Number of buckets that has been created.", this), deleteBuckets("deletebuckets", "Number of buckets that has been deleted.", this), repairs("bucketverified", "Number of times buckets have been checked.", this), @@ -245,6 +271,8 @@ template class metrics::LoadMetric<storage::FileStorThreadMetrics::Op>; template class metrics::LoadMetric<storage::FileStorThreadMetrics::OpWithNotFound>; template class metrics::LoadMetric<storage::FileStorThreadMetrics::Update>; template class metrics::LoadMetric<storage::FileStorThreadMetrics::Visitor>; +template class metrics::LoadMetric<storage::FileStorThreadMetrics::OpWithRequestSize<storage::FileStorThreadMetrics::Op>>; +template class metrics::LoadMetric<storage::FileStorThreadMetrics::OpWithRequestSize<storage::FileStorThreadMetrics::OpWithNotFound>>; template class metrics::SumMetric<storage::FileStorThreadMetrics::Op>; template class metrics::SumMetric<storage::FileStorThreadMetrics::OpWithNotFound>; template class metrics::SumMetric<storage::FileStorThreadMetrics::Update>; diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h index c2fb579183b..3bc7fa33660 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h +++ b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h @@ -19,56 +19,64 @@ struct FileStorThreadMetrics : public metrics::MetricSet { typedef std::shared_ptr<FileStorThreadMetrics> SP; - struct Op : public metrics::MetricSet { + struct Op : metrics::MetricSet { std::string _name; metrics::LongCountMetric count; metrics::DoubleAverageMetric latency; metrics::LongCountMetric failed; - Op(const std::string& id, const std::string name, MetricSet* owner = 0); - ~Op(); + Op(const std::string& id, const std::string& name, MetricSet* owner = nullptr); + ~Op() override; MetricSet * clone(std::vector<Metric::UP>& ownerList, CopyType copyType, MetricSet* owner, bool includeUnused) const override; - Op* operator&() { return this; } }; - struct OpWithNotFound : public Op { + + template <typename BaseOp> + struct OpWithRequestSize : BaseOp { + metrics::LongAverageMetric request_size; + + OpWithRequestSize(const std::string& id, const std::string& name, MetricSet* owner = nullptr); + ~OpWithRequestSize() override; + + MetricSet * clone(std::vector<Metric::UP>& ownerList, CopyType copyType, + MetricSet* owner, bool includeUnused) const override; + }; + + struct OpWithNotFound : Op { metrics::LongCountMetric notFound; - OpWithNotFound(const std::string& id, const std::string name, metrics::MetricSet* owner = 0); - ~OpWithNotFound(); + OpWithNotFound(const std::string& id, const std::string& name, metrics::MetricSet* owner = nullptr); + ~OpWithNotFound() override; MetricSet* clone(std::vector<Metric::UP>& ownerList, CopyType copyType, MetricSet* owner, bool includeUnused) const override; - OpWithNotFound* operator&() { return this; } }; - struct Update : public OpWithNotFound { + struct Update : OpWithRequestSize<OpWithNotFound> { metrics::LongAverageMetric latencyRead; - Update(MetricSet* owner = 0); - ~Update(); + explicit Update(MetricSet* owner = nullptr); + ~Update() override; MetricSet* clone(std::vector<Metric::UP>& ownerList, CopyType copyType, MetricSet* owner, bool includeUnused) const override; - Update* operator&() { return this; } }; - struct Visitor : public Op { + struct Visitor : Op { metrics::LongAverageMetric documentsPerIterate; - Visitor(MetricSet* owner = 0); - ~Visitor(); + explicit Visitor(MetricSet* owner = nullptr); + ~Visitor() override; MetricSet * clone(std::vector<Metric::UP>& ownerList, CopyType copyType, MetricSet* owner, bool includeUnused) const override; - Visitor* operator&() { return this; } }; metrics::LongCountMetric operations; metrics::LongCountMetric failedOperations; - metrics::LoadMetric<Op> put; - metrics::LoadMetric<OpWithNotFound> get; - metrics::LoadMetric<OpWithNotFound> remove; + metrics::LoadMetric<OpWithRequestSize<Op>> put; + metrics::LoadMetric<OpWithRequestSize<OpWithNotFound>> get; + metrics::LoadMetric<OpWithRequestSize<OpWithNotFound>> remove; metrics::LoadMetric<Op> removeLocation; metrics::LoadMetric<Op> statBucket; metrics::LoadMetric<Update> update; @@ -103,7 +111,7 @@ struct FileStorThreadMetrics : public metrics::MetricSet metrics::LongAverageMetric batchingSize; FileStorThreadMetrics(const std::string& name, const std::string& desc, const metrics::LoadTypeSet& lt); - ~FileStorThreadMetrics(); + ~FileStorThreadMetrics() override; }; class FileStorStripeMetrics : public metrics::MetricSet @@ -113,7 +121,7 @@ public: metrics::LoadMetric<metrics::DoubleAverageMetric> averageQueueWaitingTime; FileStorStripeMetrics(const std::string& name, const std::string& description, const metrics::LoadTypeSet& loadTypes); - ~FileStorStripeMetrics(); + ~FileStorStripeMetrics() override; }; class FileStorDiskMetrics : public metrics::MetricSet @@ -133,7 +141,7 @@ public: FileStorDiskMetrics(const std::string& name, const std::string& description, const metrics::LoadTypeSet& loadTypes, MetricSet* owner); - ~FileStorDiskMetrics(); + ~FileStorDiskMetrics() override; void initDiskMetrics(const metrics::LoadTypeSet& loadTypes, uint32_t numStripes, uint32_t threadsPerDisk); }; @@ -146,8 +154,8 @@ struct FileStorMetrics : public metrics::MetricSet metrics::LongCountMetric partitionEvents; metrics::LongCountMetric diskEvents; - FileStorMetrics(const metrics::LoadTypeSet&); - ~FileStorMetrics(); + explicit FileStorMetrics(const metrics::LoadTypeSet&); + ~FileStorMetrics() override; void initDiskMetrics(uint16_t numDisks, const metrics::LoadTypeSet& loadTypes, uint32_t numStripes, uint32_t threadsPerDisk); }; diff --git a/storage/src/vespa/storage/persistence/persistencethread.cpp b/storage/src/vespa/storage/persistence/persistencethread.cpp index fe85b55fe2b..768e104e02b 100644 --- a/storage/src/vespa/storage/persistence/persistencethread.cpp +++ b/storage/src/vespa/storage/persistence/persistencethread.cpp @@ -105,7 +105,9 @@ bool PersistenceThread::tasConditionMatches(const api::TestAndSetCommand & cmd, MessageTracker::UP PersistenceThread::handlePut(api::PutCommand& cmd) { - auto tracker = std::make_unique<MessageTracker>(_env._metrics.put[cmd.getLoadType()],_env._component.getClock()); + auto& metrics = _env._metrics.put[cmd.getLoadType()]; + auto tracker = std::make_unique<MessageTracker>(metrics, _env._component.getClock()); + metrics.request_size.addValue(cmd.getApproxByteSize()); if (tasConditionExists(cmd) && !tasConditionMatches(cmd, *tracker)) { return tracker; @@ -120,7 +122,9 @@ PersistenceThread::handlePut(api::PutCommand& cmd) MessageTracker::UP PersistenceThread::handleRemove(api::RemoveCommand& cmd) { - auto tracker = std::make_unique<MessageTracker>(_env._metrics.remove[cmd.getLoadType()],_env._component.getClock()); + auto& metrics = _env._metrics.remove[cmd.getLoadType()]; + auto tracker = std::make_unique<MessageTracker>(metrics,_env._component.getClock()); + metrics.request_size.addValue(cmd.getApproxByteSize()); if (tasConditionExists(cmd) && !tasConditionMatches(cmd, *tracker)) { return tracker; @@ -140,7 +144,9 @@ PersistenceThread::handleRemove(api::RemoveCommand& cmd) MessageTracker::UP PersistenceThread::handleUpdate(api::UpdateCommand& cmd) { - auto tracker = std::make_unique<MessageTracker>(_env._metrics.update[cmd.getLoadType()],_env._component.getClock()); + auto& metrics = _env._metrics.update[cmd.getLoadType()]; + auto tracker = std::make_unique<MessageTracker>(metrics, _env._component.getClock()); + metrics.request_size.addValue(cmd.getApproxByteSize()); if (tasConditionExists(cmd) && !tasConditionMatches(cmd, *tracker)) { return tracker; @@ -159,7 +165,9 @@ PersistenceThread::handleUpdate(api::UpdateCommand& cmd) MessageTracker::UP PersistenceThread::handleGet(api::GetCommand& cmd) { - auto tracker = std::make_unique<MessageTracker>(_env._metrics.get[cmd.getLoadType()],_env._component.getClock()); + auto& metrics = _env._metrics.get[cmd.getLoadType()]; + auto tracker = std::make_unique<MessageTracker>(metrics,_env._component.getClock()); + metrics.request_size.addValue(cmd.getApproxByteSize()); document::FieldSetRepo repo; document::FieldSet::UP fieldSet = repo.parse(*_env._component.getTypeRepo(), cmd.getFieldSet()); |