diff options
19 files changed, 119 insertions, 176 deletions
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ConfigServerMaintainer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ConfigServerMaintainer.java index 007ca8dcf53..5854b1d85da 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ConfigServerMaintainer.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/ConfigServerMaintainer.java @@ -14,7 +14,6 @@ import com.yahoo.vespa.flags.FlagSource; import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.flags.ListFlag; -import java.time.Clock; import java.time.Duration; import java.util.Map; import java.util.Set; @@ -31,14 +30,13 @@ public abstract class ConfigServerMaintainer extends Maintainer { ConfigServerMaintainer(ApplicationRepository applicationRepository, Curator curator, FlagSource flagSource, Duration initialDelay, Duration interval) { super(null, interval, initialDelay, new JobControl(new JobControlFlags(curator, flagSource)), - jobMetrics(applicationRepository.clock(), applicationRepository.metric())); + jobMetrics(applicationRepository.metric())); this.applicationRepository = applicationRepository; } - private static JobMetrics jobMetrics(Clock clock, Metric metric) { - return new JobMetrics(clock, (job, instant) -> { - Duration sinceSuccess = Duration.between(instant, clock.instant()); - metric.set("maintenance.secondsSinceSuccess", sinceSuccess.getSeconds(), metric.createContext(Map.of("job", job))); + private static JobMetrics jobMetrics(Metric metric) { + return new JobMetrics((job, consecutiveFailures) -> { + metric.set("maintenance.consecutiveFailures", consecutiveFailures, metric.createContext(Map.of("job", job))); }); } diff --git a/container-core/src/main/java/com/yahoo/container/core/config/ApplicationBundleLoader.java b/container-core/src/main/java/com/yahoo/container/core/config/ApplicationBundleLoader.java index 3e8ef65ade6..f87dd3f42d2 100644 --- a/container-core/src/main/java/com/yahoo/container/core/config/ApplicationBundleLoader.java +++ b/container-core/src/main/java/com/yahoo/container/core/config/ApplicationBundleLoader.java @@ -2,11 +2,8 @@ package com.yahoo.container.core.config; import com.yahoo.config.FileReference; -import com.yahoo.container.Container; -import com.yahoo.filedistribution.fileacquirer.FileAcquirer; import com.yahoo.osgi.Osgi; import org.osgi.framework.Bundle; -import org.osgi.framework.wiring.BundleRevision; import java.util.ArrayList; import java.util.HashSet; @@ -24,6 +21,7 @@ import java.util.stream.Collectors; * @author Tony Vaagenes */ public class ApplicationBundleLoader { + private static final Logger log = Logger.getLogger(ApplicationBundleLoader.class.getName()); /* Map of file refs of active bundles (not scheduled for uninstall) to the installed bundle. * @@ -34,14 +32,12 @@ public class ApplicationBundleLoader { */ private final Map<FileReference, Bundle> reference2Bundle = new LinkedHashMap<>(); - private final Logger log = Logger.getLogger(ApplicationBundleLoader.class.getName()); private final Osgi osgi; + private final FileAcquirerBundleInstaller bundleInstaller; - // A custom bundle installer for non-disk bundles, to be used for testing - private BundleInstaller customBundleInstaller = null; - - public ApplicationBundleLoader(Osgi osgi) { + public ApplicationBundleLoader(Osgi osgi, FileAcquirerBundleInstaller bundleInstaller) { this.osgi = osgi; + this.bundleInstaller = bundleInstaller; } /** @@ -70,7 +66,6 @@ public class ApplicationBundleLoader { return obsoleteReferences; } - /** * Returns the bundles that will not be retained by the new application generation. */ @@ -89,19 +84,16 @@ public class ApplicationBundleLoader { bundlesToInstall.removeAll(reference2Bundle.keySet()); if (!bundlesToInstall.isEmpty()) { - FileAcquirer fileAcquirer = Container.get().getFileAcquirer(); - boolean hasFileDistribution = (fileAcquirer != null); - if (hasFileDistribution) { - installWithFileDistribution(bundlesToInstall, new FileAcquirerBundleInstaller(fileAcquirer)); - } else if (customBundleInstaller != null) { - installWithFileDistribution(bundlesToInstall, customBundleInstaller); + if (bundleInstaller.hasFileDistribution()) { + installWithFileDistribution(bundlesToInstall, bundleInstaller); } else { log.warning("Can't retrieve bundles since file distribution is disabled."); } } } - private void installWithFileDistribution(Set<FileReference> bundlesToInstall, BundleInstaller bundleInstaller) { + private void installWithFileDistribution(Set<FileReference> bundlesToInstall, + FileAcquirerBundleInstaller bundleInstaller) { for (FileReference reference : bundlesToInstall) { try { log.info("Installing bundle with reference '" + reference.value() + "'"); @@ -133,11 +125,6 @@ public class ApplicationBundleLoader { } // Only for testing - void useCustomBundleInstaller(BundleInstaller bundleInstaller) { - customBundleInstaller = bundleInstaller; - } - - // Only for testing List<FileReference> getActiveFileReferences() { return new ArrayList<>(reference2Bundle.keySet()); } diff --git a/container-core/src/main/java/com/yahoo/container/core/config/BundleInstaller.java b/container-core/src/main/java/com/yahoo/container/core/config/BundleInstaller.java deleted file mode 100644 index fc919571b6c..00000000000 --- a/container-core/src/main/java/com/yahoo/container/core/config/BundleInstaller.java +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright 2020 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.container.core.config; - -import com.yahoo.config.FileReference; -import com.yahoo.osgi.Osgi; -import org.osgi.framework.Bundle; - -import java.util.List; - -/** - * @author gjoranv - */ -public interface BundleInstaller { - - /** - * Installs the bundle with the given file reference, plus all bundles in its X-JDisc-Preinstall-Bundle directive. - * Returns all bundles installed to the given OSGi framework as a result of this call. - */ - List<Bundle> installBundles(FileReference reference, Osgi osgi) throws InterruptedException; - -} diff --git a/container-core/src/main/java/com/yahoo/container/core/config/FileAcquirerBundleInstaller.java b/container-core/src/main/java/com/yahoo/container/core/config/FileAcquirerBundleInstaller.java index 72951e67b4e..51d77462652 100644 --- a/container-core/src/main/java/com/yahoo/container/core/config/FileAcquirerBundleInstaller.java +++ b/container-core/src/main/java/com/yahoo/container/core/config/FileAcquirerBundleInstaller.java @@ -13,9 +13,11 @@ import java.util.concurrent.TimeUnit; import java.util.logging.Logger; /** + * Retrieves bundles with file distribution, and installs them to the OSGi framework. + * * @author gjoranv */ -public class FileAcquirerBundleInstaller implements BundleInstaller { +public class FileAcquirerBundleInstaller { private static Logger log = Logger.getLogger(FileAcquirerBundleInstaller.class.getName()); private final FileAcquirer fileAcquirer; @@ -24,7 +26,6 @@ public class FileAcquirerBundleInstaller implements BundleInstaller { this.fileAcquirer = fileAcquirer; } - @Override public List<Bundle> installBundles(FileReference reference, Osgi osgi) throws InterruptedException { File file = fileAcquirer.waitFor(reference, 7, TimeUnit.DAYS); @@ -46,6 +47,10 @@ public class FileAcquirerBundleInstaller implements BundleInstaller { return osgi.install(file.getAbsolutePath()); } + public boolean hasFileDistribution() { + return fileAcquirer != null; + } + private static boolean notReadable(File file) { return ! Files.isReadable(file.toPath()); } diff --git a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java index a58dff13f09..13d50b9b30f 100644 --- a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java +++ b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java @@ -16,6 +16,7 @@ import com.yahoo.container.di.config.SubscriberFactory; import com.yahoo.container.di.osgi.BundleClasses; import com.yahoo.container.di.osgi.OsgiUtil; import com.yahoo.container.logging.AccessLog; +import com.yahoo.filedistribution.fileacquirer.FileAcquirer; import com.yahoo.jdisc.application.OsgiFramework; import com.yahoo.jdisc.handler.RequestHandler; import com.yahoo.jdisc.service.ClientProvider; @@ -68,7 +69,6 @@ public class HandlersConfigurerDi { } private final com.yahoo.container.Container vespaContainer; - private final OsgiWrapper osgiWrapper; private final Container container; private volatile ComponentGraph currentGraph = new ComponentGraph(0); @@ -81,7 +81,7 @@ public class HandlersConfigurerDi { OsgiFramework osgiFramework) { this(subscriberFactory, vespaContainer, configId, deconstructor, discInjector, - new ContainerAndDiOsgi(osgiFramework)); + new ContainerAndDiOsgi(osgiFramework, vespaContainer.getFileAcquirer())); } // Only public for testing @@ -93,7 +93,6 @@ public class HandlersConfigurerDi { OsgiWrapper osgiWrapper) { this.vespaContainer = vespaContainer; - this.osgiWrapper = osgiWrapper; container = new Container(subscriberFactory, configId, deconstructor, osgiWrapper); getNewComponentGraph(discInjector, false); } @@ -104,12 +103,12 @@ public class HandlersConfigurerDi { private final ApplicationBundleLoader applicationBundleLoader; private final PlatformBundleLoader platformBundleLoader; - public ContainerAndDiOsgi(OsgiFramework osgiFramework) { + public ContainerAndDiOsgi(OsgiFramework osgiFramework, FileAcquirer fileAcquirer) { super(osgiFramework); this.osgiFramework = osgiFramework; OsgiImpl osgi = new OsgiImpl(osgiFramework); - applicationBundleLoader = new ApplicationBundleLoader(osgi); + applicationBundleLoader = new ApplicationBundleLoader(osgi, new FileAcquirerBundleInstaller(fileAcquirer)); platformBundleLoader = new PlatformBundleLoader(osgi); } diff --git a/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java b/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java index ee71e8a8f4c..b56e5d99123 100644 --- a/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java +++ b/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java @@ -2,6 +2,9 @@ package com.yahoo.container.core.config; import com.yahoo.config.FileReference; +import com.yahoo.filedistribution.fileacquirer.FileAcquirer; +import com.yahoo.filedistribution.fileacquirer.MockFileAcquirer; +import com.yahoo.osgi.Osgi; import org.junit.Before; import org.junit.Test; import org.osgi.framework.Bundle; @@ -29,9 +32,9 @@ public class ApplicationBundleLoaderTest { @Before public void setup() { osgi = new TestOsgi(testBundles()); - var bundleInstaller = new TestBundleInstaller(); - bundleLoader = new ApplicationBundleLoader(osgi); - bundleLoader.useCustomBundleInstaller(bundleInstaller); + var bundleInstaller = new TestBundleInstaller(MockFileAcquirer.returnFile(null)); + + bundleLoader = new ApplicationBundleLoader(osgi, bundleInstaller); } @Test @@ -103,4 +106,17 @@ public class ApplicationBundleLoaderTest { BUNDLE_2_REF.value(), BUNDLE_2); } + static class TestBundleInstaller extends FileAcquirerBundleInstaller { + + TestBundleInstaller(FileAcquirer fileAcquirer) { + super(fileAcquirer); + } + + @Override + public List<Bundle> installBundles(FileReference reference, Osgi osgi) { + return osgi.install(reference.value()); + } + + } + } diff --git a/container-core/src/test/java/com/yahoo/container/core/config/TestBundleInstaller.java b/container-core/src/test/java/com/yahoo/container/core/config/TestBundleInstaller.java deleted file mode 100644 index 43a5268eabf..00000000000 --- a/container-core/src/test/java/com/yahoo/container/core/config/TestBundleInstaller.java +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2020 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.container.core.config; - -import com.yahoo.config.FileReference; -import com.yahoo.osgi.Osgi; -import org.osgi.framework.Bundle; - -import java.util.List; - -/** - * @author gjoranv - */ -class TestBundleInstaller implements BundleInstaller { - - @Override - public List<Bundle> installBundles(FileReference reference, Osgi osgi) { - return osgi.install(reference.value()); - } - -} diff --git a/container-di/src/main/java/com/yahoo/container/di/ConfigRetriever.java b/container-di/src/main/java/com/yahoo/container/di/ConfigRetriever.java index 6ed9c2a2994..d8350ec2f6b 100644 --- a/container-di/src/main/java/com/yahoo/container/di/ConfigRetriever.java +++ b/container-di/src/main/java/com/yahoo/container/di/ConfigRetriever.java @@ -74,9 +74,7 @@ public final class ConfigRetriever { return getConfigs(componentConfigKeys, leastGeneration, false); } - /** - * Try to get config just once - */ + // TODO: duplicate code, let getConfigs call this. Optional<ConfigSnapshot> getConfigsOnce(Set<ConfigKey<? extends ConfigInstance>> componentConfigKeys, long leastGeneration, boolean restartOnRedeploy) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java index 3a16cdb6143..19d55d35f2f 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java @@ -10,9 +10,6 @@ import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.zone.RoutingMethod; import com.yahoo.config.provision.zone.ZoneId; -import com.yahoo.vespa.flags.BooleanFlag; -import com.yahoo.vespa.flags.FetchVector; -import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.hosted.controller.api.application.v4.model.EndpointStatus; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ContainerEndpoint; @@ -65,14 +62,12 @@ public class RoutingController { private final Controller controller; private final RoutingPolicies routingPolicies; private final RotationRepository rotationRepository; - private final BooleanFlag allowDirectRouting; public RoutingController(Controller controller, RotationsConfig rotationsConfig) { this.controller = Objects.requireNonNull(controller, "controller must be non-null"); this.routingPolicies = new RoutingPolicies(controller); this.rotationRepository = new RotationRepository(rotationsConfig, controller.applications(), controller.curator()); - this.allowDirectRouting = Flags.ALLOW_DIRECT_ROUTING.bindTo(controller.flagSource()); } public RoutingPolicies policies() { @@ -267,7 +262,7 @@ public class RoutingController { private boolean canRouteDirectlyTo(DeploymentId deploymentId, Application application) { if (controller.system().isPublic()) return true; // Public always supports direct routing if (controller.system().isCd()) return true; // CD deploys directly so we cannot enforce all requirements below - if(deploymentId.zoneId().environment().isManuallyDeployed()) return true; // Manually deployed zones does not include any use cases where direct routing is not supported + if (deploymentId.zoneId().environment().isManuallyDeployed()) return true; // Manually deployed zones always support direct routing // Check Athenz service presence. The test framework uses this identity when sending requests to the // deployment's container(s). @@ -287,12 +282,7 @@ public class RoutingController { .or(() -> application.latestVersion().flatMap(ApplicationVersion::compileVersion)); if (compileVersion.isEmpty()) return false; if (compileVersion.get().isBefore(DIRECT_ROUTING_MIN_VERSION)) return false; - - // Check feature flag - // TODO(mpolden): Remove once we make this default - return this.allowDirectRouting.with(FetchVector.Dimension.APPLICATION_ID, - deploymentId.applicationId().serializedForm()) - .value(); + return true; } /** Compute global endpoints for given routing ID, application and deployments */ diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintainer.java index 76003a873fe..9bf6352813a 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintainer.java @@ -7,7 +7,6 @@ import com.yahoo.config.provision.SystemName; import com.yahoo.jdisc.Metric; import com.yahoo.vespa.hosted.controller.Controller; -import java.time.Clock; import java.time.Duration; import java.util.EnumSet; import java.util.Map; @@ -35,7 +34,7 @@ public abstract class ControllerMaintainer extends Maintainer { public ControllerMaintainer(Controller controller, Duration interval, String name, Set<SystemName> activeSystems) { super(name, interval, controller.clock().instant(), controller.jobControl(), - jobMetrics(controller.clock(), controller.metric()), controller.curator().cluster()); + jobMetrics(controller.metric()), controller.curator().cluster()); this.controller = controller; this.activeSystems = Set.copyOf(Objects.requireNonNull(activeSystems)); } @@ -48,10 +47,9 @@ public abstract class ControllerMaintainer extends Maintainer { super.run(); } - private static JobMetrics jobMetrics(Clock clock, Metric metric) { - return new JobMetrics(clock, (job, instant) -> { - Duration sinceSuccess = Duration.between(instant, clock.instant()); - metric.set("maintenance.secondsSinceSuccess", sinceSuccess.getSeconds(), metric.createContext(Map.of("job", job))); + private static JobMetrics jobMetrics(Metric metric) { + return new JobMetrics((job, consecutiveFailures) -> { + metric.set("maintenance.consecutiveFailures", consecutiveFailures, metric.createContext(Map.of("job", job))); }); } 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 0d1c655eb01..26f718ae5ff 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 @@ -18,8 +18,6 @@ import com.yahoo.config.provision.TenantName; import com.yahoo.config.provision.zone.RoutingMethod; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.path.Path; -import com.yahoo.vespa.flags.Flags; -import com.yahoo.vespa.flags.InMemoryFlagSource; import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions; import com.yahoo.vespa.hosted.controller.api.application.v4.model.EndpointStatus; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; @@ -868,26 +866,20 @@ public class ControllerTest { .stream() .map(Endpoint::routingMethod) .collect(Collectors.toSet()); - ((InMemoryFlagSource) tester.controller().flagSource()).withBooleanFlag(Flags.ALLOW_DIRECT_ROUTING.id(), false); - // Without everything + // Without satisfying any requirement context.submit(applicationPackageBuilder.build()).deploy(); assertEquals(Set.of(RoutingMethod.shared), routingMethods.get()); - // Without Athenz service + // Without satisfying Athenz service requirement context.submit(applicationPackageBuilder.compileVersion(RoutingController.DIRECT_ROUTING_MIN_VERSION).build()) .deploy(); assertEquals(Set.of(RoutingMethod.shared), routingMethods.get()); - // Without feature flag - applicationPackageBuilder = applicationPackageBuilder.compileVersion(RoutingController.DIRECT_ROUTING_MIN_VERSION) - .athenzIdentity(AthenzDomain.from("domain"), AthenzService.from("service")); - context.submit(applicationPackageBuilder.build()).deploy(); - assertEquals(Set.of(RoutingMethod.shared), routingMethods.get()); - - // With everything required - ((InMemoryFlagSource) tester.controller().flagSource()).withBooleanFlag(Flags.ALLOW_DIRECT_ROUTING.id(), true); - context.submit(applicationPackageBuilder.build()).deploy(); + // Satisfying all requirements + context.submit(applicationPackageBuilder.compileVersion(RoutingController.DIRECT_ROUTING_MIN_VERSION) + .athenzIdentity(AthenzDomain.from("domain"), AthenzService.from("service")) + .build()).deploy(); assertEquals(Set.of(RoutingMethod.shared, RoutingMethod.sharedLayer4), routingMethods.get()); // Global endpoint is configured and includes directly routed endpoint name diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java index 8d244917760..d90eb715499 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java @@ -13,8 +13,6 @@ import com.yahoo.security.KeyAlgorithm; import com.yahoo.security.KeyUtils; import com.yahoo.security.SignatureAlgorithm; import com.yahoo.security.X509CertificateBuilder; -import com.yahoo.vespa.flags.Flags; -import com.yahoo.vespa.flags.InMemoryFlagSource; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.ControllerTester; import com.yahoo.vespa.hosted.controller.Instance; @@ -113,7 +111,6 @@ public class DeploymentContext { this.runner = tester.runner(); this.tester = tester; createTenantAndApplication(); - ((InMemoryFlagSource) tester.controller().flagSource()).withBooleanFlag(Flags.ALLOW_DIRECT_ROUTING.id(), true); } private void createTenantAndApplication() { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintainerTest.java index 4218e66703f..6a2feba1d47 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintainerTest.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.yahoo.config.provision.SystemName; +import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.ControllerTester; import com.yahoo.vespa.hosted.controller.integration.MetricsMock; import org.junit.Before; @@ -28,28 +29,47 @@ public class ControllerMaintainerTest { @Test public void only_runs_in_permitted_systems() { AtomicInteger executions = new AtomicInteger(); - maintainerIn(SystemName.cd, executions).run(); - maintainerIn(SystemName.main, executions).run(); + new TestControllerMaintainer(tester.controller(), SystemName.cd, executions).run(); + new TestControllerMaintainer(tester.controller(), SystemName.main, executions).run(); assertEquals(1, executions.get()); } @Test public void records_metric() { - maintainerIn(SystemName.main, new AtomicInteger()).run(); + TestControllerMaintainer maintainer = new TestControllerMaintainer(tester.controller(), SystemName.main, new AtomicInteger()); + maintainer.run(); + assertEquals(0L, consecutiveFailuresMetric()); + maintainer.success = false; + maintainer.run(); + maintainer.run(); + assertEquals(2L, consecutiveFailuresMetric()); + maintainer.success = true; + maintainer.run();; + assertEquals(0, consecutiveFailuresMetric()); + } + + private long consecutiveFailuresMetric() { MetricsMock metrics = (MetricsMock) tester.controller().metric(); - assertEquals(0L, metrics.getMetric((context) -> "MockMaintainer".equals(context.get("job")), - "maintenance.secondsSinceSuccess").get()); + return metrics.getMetric((context) -> "TestControllerMaintainer".equals(context.get("job")), + "maintenance.consecutiveFailures").get().longValue(); } - private ControllerMaintainer maintainerIn(SystemName system, AtomicInteger executions) { - return new ControllerMaintainer(tester.controller(), Duration.ofDays(1), - "MockMaintainer", EnumSet.of(system)) { - @Override - protected boolean maintain() { - executions.incrementAndGet(); - return true; - } - }; + private static class TestControllerMaintainer extends ControllerMaintainer { + + private final AtomicInteger executions; + private boolean success = true; + + public TestControllerMaintainer(Controller controller, SystemName system, AtomicInteger executions) { + super(controller, Duration.ofDays(1), null, EnumSet.of(system)); + this.executions = executions; + } + + @Override + protected boolean maintain() { + executions.incrementAndGet(); + return success; + } + } } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index 5bd1d3dcc60..37973a6d435 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -264,12 +264,6 @@ public class Flags { "Takes effect on redeploy", ZONE_ID, APPLICATION_ID); - public static final UnboundBooleanFlag ALLOW_DIRECT_ROUTING = defineFeatureFlag( - "publish-direct-routing-endpoint", true, - "Whether an application should receive a directly routed endpoint in its endpoint list", - "Takes effect immediately", - APPLICATION_ID); - public static final UnboundBooleanFlag NLB_PROXY_PROTOCOL = defineFeatureFlag( "nlb-proxy-protocol", false, "Configure NLB to use proxy protocol", diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintainer.java index 85477dad729..5f87cf9fd9b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintainer.java @@ -9,7 +9,6 @@ import com.yahoo.jdisc.Metric; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeRepository; -import java.time.Clock; import java.time.Duration; import java.util.List; import java.util.Map; @@ -25,8 +24,8 @@ public abstract class NodeRepositoryMaintainer extends Maintainer { private final NodeRepository nodeRepository; public NodeRepositoryMaintainer(NodeRepository nodeRepository, Duration interval, Metric metric) { - super(null, interval, nodeRepository.clock().instant(), nodeRepository.jobControl(), - jobMetrics(nodeRepository.clock(), metric), nodeRepository.database().cluster()); + super(null, interval, nodeRepository.clock().instant(), nodeRepository.jobControl(), jobMetrics(metric), + nodeRepository.database().cluster()); this.nodeRepository = nodeRepository; } @@ -45,10 +44,9 @@ public abstract class NodeRepositoryMaintainer extends Maintainer { .collect(Collectors.groupingBy(node -> node.allocation().get().owner())); } - private static JobMetrics jobMetrics(Clock clock, Metric metric) { - return new JobMetrics(clock, (job, instant) -> { - Duration sinceSuccess = Duration.between(instant, clock.instant()); - metric.set("maintenance.secondsSinceSuccess", sinceSuccess.getSeconds(), metric.createContext(Map.of("job", job))); + private static JobMetrics jobMetrics(Metric metric) { + return new JobMetrics((job, consecutiveFailures) -> { + metric.set("maintenance.consecutiveFailures", consecutiveFailures, metric.createContext(Map.of("job", job))); }); } diff --git a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/JobMetrics.java b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/JobMetrics.java index 4c05d46d782..a43e2156025 100644 --- a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/JobMetrics.java +++ b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/JobMetrics.java @@ -1,10 +1,7 @@ // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.concurrent.maintenance; -import java.time.Clock; -import java.time.Instant; import java.util.Map; -import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.function.BiConsumer; @@ -15,26 +12,29 @@ import java.util.function.BiConsumer; */ public class JobMetrics { - private final Clock clock; - private final BiConsumer<String, Instant> metricConsumer; + private final BiConsumer<String, Long> metricConsumer; - private final Map<String, Instant> successfulRuns = new ConcurrentHashMap<>(); + private final Map<String, Long> incompleteRuns = new ConcurrentHashMap<>(); - public JobMetrics(Clock clock, BiConsumer<String, Instant> metricConsumer) { - this.clock = Objects.requireNonNull(clock); + public JobMetrics(BiConsumer<String, Long> metricConsumer) { this.metricConsumer = metricConsumer; } + /** Record a run for given job */ + public void recordRunOf(String job) { + incompleteRuns.compute(job, (ignored, run) -> run == null ? 1 : ++run); + } + /** Record successful run of given job */ public void recordSuccessOf(String job) { - successfulRuns.put(job, clock.instant()); + incompleteRuns.put(job, 0L); } /** Forward metrics for given job to metric consumer */ public void forward(String job) { - Instant lastSuccess = successfulRuns.get(job); - if (lastSuccess != null) { - metricConsumer.accept(job, lastSuccess); + Long incompleteRuns = this.incompleteRuns.get(job); + if (incompleteRuns != null) { + metricConsumer.accept(job, incompleteRuns); } } diff --git a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java index 0385c27536d..eb9b91c812c 100644 --- a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java +++ b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java @@ -85,6 +85,7 @@ public abstract class Maintainer implements Runnable, AutoCloseable { public final void lockAndMaintain() { try (var lock = jobControl.lockJob(name())) { try { + jobMetrics.recordRunOf(name()); if (maintain()) jobMetrics.recordSuccessOf(name()); } finally { // Always forward metrics diff --git a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java index 47ed010e95e..2bfaad894a5 100644 --- a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java +++ b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java @@ -1,16 +1,14 @@ // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.concurrent.maintenance; -import com.yahoo.test.ManualClock; import org.junit.Test; import java.time.Duration; import java.time.Instant; import java.util.List; -import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.atomic.AtomicLong; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; /** * @author freva @@ -41,37 +39,31 @@ public class MaintainerTest { @Test public void success_metric() { - ManualClock clock = new ManualClock(); - AtomicReference<Instant> lastSuccess = new AtomicReference<>(); - JobMetrics jobMetrics = new JobMetrics(clock, (job, instant) -> lastSuccess.set(instant)); + AtomicLong consecutiveFailures = new AtomicLong(); + JobMetrics jobMetrics = new JobMetrics((job, count) -> consecutiveFailures.set(count)); TestMaintainer maintainer = new TestMaintainer(jobMetrics); - // Maintainer not successful yet + // Maintainer fails twice in a row maintainer.successOnNextRun(false).run(); - assertNull(lastSuccess.get()); + assertEquals(1, consecutiveFailures.get()); + maintainer.successOnNextRun(false).run(); + assertEquals(2, consecutiveFailures.get()); // Maintainer runs successfully - clock.advance(Duration.ofHours(1)); - Instant lastSuccess0 = clock.instant(); maintainer.successOnNextRun(true).run(); - assertEquals(lastSuccess0, lastSuccess.get()); + assertEquals(0, consecutiveFailures.get()); // Maintainer runs successfully again - clock.advance(Duration.ofHours(2)); - Instant lastSuccess1 = clock.instant(); maintainer.run(); - assertEquals(lastSuccess1, lastSuccess.get()); + assertEquals(0, consecutiveFailures.get()); // Maintainer throws - clock.advance(Duration.ofHours(5)); maintainer.throwOnNextRun(true).run(); - assertEquals("Time of successful run is unchanged", lastSuccess1, lastSuccess.get()); + assertEquals(1, consecutiveFailures.get()); // Maintainer recovers - clock.advance(Duration.ofHours(3)); - Instant lastSuccess2 = clock.instant(); maintainer.throwOnNextRun(false).run(); - assertEquals(lastSuccess2, lastSuccess.get()); + assertEquals(0, consecutiveFailures.get()); } } diff --git a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/TestMaintainer.java b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/TestMaintainer.java index 0ea24fb6c2b..5eae643fe40 100644 --- a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/TestMaintainer.java +++ b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/TestMaintainer.java @@ -1,7 +1,6 @@ // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.concurrent.maintenance; -import java.time.Clock; import java.time.Duration; /** @@ -22,7 +21,7 @@ class TestMaintainer extends Maintainer { } public TestMaintainer(String name, JobControl jobControl) { - this(name, jobControl, new JobMetrics(Clock.systemUTC(), (job, instant) -> {})); + this(name, jobControl, new JobMetrics((job, instant) -> {})); } public int totalRuns() { |