diff options
78 files changed, 1522 insertions, 664 deletions
diff --git a/bundle-plugin/src/test/java/com/yahoo/container/plugin/mojo/GenerateSourcesMojoTest.java b/bundle-plugin/src/test/java/com/yahoo/container/plugin/mojo/GenerateSourcesMojoTest.java index 9aaf06aa357..3b75d9dcb7d 100644 --- a/bundle-plugin/src/test/java/com/yahoo/container/plugin/mojo/GenerateSourcesMojoTest.java +++ b/bundle-plugin/src/test/java/com/yahoo/container/plugin/mojo/GenerateSourcesMojoTest.java @@ -22,4 +22,5 @@ public class GenerateSourcesMojoTest { System.out.println("defaultConfigGenVersion = " + actual); assertEquals(expected, actual); } + } diff --git a/clustercontroller-core/pom.xml b/clustercontroller-core/pom.xml index df34272ef9c..c3ccf39c906 100644 --- a/clustercontroller-core/pom.xml +++ b/clustercontroller-core/pom.xml @@ -125,7 +125,7 @@ <artifactId>maven-surefire-plugin</artifactId> <configuration> <forkCount>4</forkCount> - <rerunFailingTestsCount>3</rerunFailingTestsCount> + <rerunFailingTestsCount>5</rerunFailingTestsCount> </configuration> </plugin> </plugins> diff --git a/config-application-package/src/main/java/com/yahoo/config/model/application/provider/Bundle.java b/config-application-package/src/main/java/com/yahoo/config/model/application/provider/Bundle.java index 721f3fd3460..c4fb7c29e7f 100644 --- a/config-application-package/src/main/java/com/yahoo/config/model/application/provider/Bundle.java +++ b/config-application-package/src/main/java/com/yahoo/config/model/application/provider/Bundle.java @@ -56,13 +56,7 @@ public class Bundle { if (!bundleDir.isDirectory()) { return new ArrayList<>(); } - return Arrays.asList(bundleDir.listFiles( - new FilenameFilter() { - @Override - public boolean accept(File dir, String name) { - return name.endsWith(".jar"); - } - })); + return Arrays.asList(bundleDir.listFiles((dir, name) -> name.endsWith(".jar"))); } public List<DefEntry> getDefEntries() { diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java index 360968bacd5..92975e47f75 100644 --- a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java +++ b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java @@ -91,7 +91,6 @@ public interface ModelContext { @ModelFeatureFlag(owners = {"baldersheim"}) default int maxMergeQueueSize() { return 100; } @ModelFeatureFlag(owners = {"baldersheim"}) default boolean containerDumpHeapOnShutdownTimeout() { throw new UnsupportedOperationException("TODO specify default value"); } @ModelFeatureFlag(owners = {"baldersheim"}) default double containerShutdownTimeout() { throw new UnsupportedOperationException("TODO specify default value"); } - @ModelFeatureFlag(owners = {"geirst"}, removeAfter = "7.541") default boolean enableFeedBlockInDistributor() { return true; } @ModelFeatureFlag(owners = {"bjorncs", "tokle"}) default List<String> allowedAthenzProxyIdentities() { return List.of(); } @ModelFeatureFlag(owners = {"vekterli"}) default int maxActivationInhibitedOutOfSyncGroups() { return 0; } @ModelFeatureFlag(owners = {"hmusum"}) default String jvmOmitStackTraceInFastThrowOption(ClusterSpec.Type type) { return ""; } @@ -118,9 +117,7 @@ public interface ModelContext { @ModelFeatureFlag(owners = {"arnej"}) default boolean useQrserverServiceName() { return true; } @ModelFeatureFlag(owners = {"bjorncs", "baldersheim"}) default boolean enableJdiscPreshutdownCommand() { return true; } @ModelFeatureFlag(owners = {"arnej"}) default boolean avoidRenamingSummaryFeatures() { return false; } - @ModelFeatureFlag(owners = {"bjorncs", "baldersheim"}, removeAfter = "7.569") default boolean mergeGroupingResultInSearchInvoker() { return true; } @ModelFeatureFlag(owners = {"arnej"}) default boolean experimentalSdParsing() { return false; } - @ModelFeatureFlag(owners = {"hmusum"}, removeAfter = "7.571") default String adminClusterNodeArchitecture() { return adminClusterArchitecture().name(); } @ModelFeatureFlag(owners = {"hmusum"}) default Architecture adminClusterArchitecture() { return Architecture.getDefault(); } } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java index 893595befd3..029c0efb55f 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/RankProfile.java @@ -773,7 +773,7 @@ public class RankProfile implements Cloneable { input.getValue() + ", with type " + input.getValue() + "" + " but this input is already defined with type " + existingType + " in another profile this inherits"); - inputs.put(input.getKey(), input.getValue()); + allInputs.put(input.getKey(), input.getValue()); } } allInputs.putAll(inputs); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index 47519853ed0..ff1a4b6cc5f 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -1184,7 +1184,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { String message = "Invalid or misplaced JVM options in services.xml: " + String.join(",", invalidOptions) + "." + " See https://docs.vespa.ai/en/reference/services-container.html#jvm"; - if (failDeploymentWithInvalidJvmOptions) + if (failDeploymentWithInvalidJvmOptions && isHosted) throw new IllegalArgumentException(message); else logger.logApplicationPackage(WARNING, message); @@ -1250,7 +1250,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { String message = "Invalid or misplaced JVM GC options in services.xml: " + String.join(",", options) + "." + " See https://docs.vespa.ai/en/reference/services-container.html#jvm"; - if (failDeploymentWithInvalidJvmOptions) + if (failDeploymentWithInvalidJvmOptions && isHosted) throw new IllegalArgumentException(message); else logger.logApplicationPackage(WARNING, message); diff --git a/config-model/src/test/derived/neuralnet_noqueryprofile/neuralnet.sd b/config-model/src/test/derived/neuralnet_noqueryprofile/neuralnet.sd index 4241e9dab85..9069f59bbe3 100644 --- a/config-model/src/test/derived/neuralnet_noqueryprofile/neuralnet.sd +++ b/config-model/src/test/derived/neuralnet_noqueryprofile/neuralnet.sd @@ -69,7 +69,7 @@ search neuralnet { } - rank-profile defaultRankProfile inherits default { + rank-profile default { inputs { query(W_0) tensor(x[9],hidden[9]) @@ -80,6 +80,10 @@ search neuralnet { query(b_out) tensor(out[1]) } + } + + rank-profile defaultRankProfile inherits default { + constants { maxSignedSixtyFourBitInteger: 9223372036854775807 } diff --git a/config-model/src/test/derived/neuralnet_noqueryprofile/rank-profiles.cfg b/config-model/src/test/derived/neuralnet_noqueryprofile/rank-profiles.cfg index 9c3cfd28b9a..f5134dd15f9 100644 --- a/config-model/src/test/derived/neuralnet_noqueryprofile/rank-profiles.cfg +++ b/config-model/src/test/derived/neuralnet_noqueryprofile/rank-profiles.cfg @@ -1,4 +1,16 @@ rankprofile[].name "default" +rankprofile[].fef.property[].name "vespa.type.query.W_0" +rankprofile[].fef.property[].value "tensor(hidden[9],x[9])" +rankprofile[].fef.property[].name "vespa.type.query.b_0" +rankprofile[].fef.property[].value "tensor(hidden[9])" +rankprofile[].fef.property[].name "vespa.type.query.W_1" +rankprofile[].fef.property[].value "tensor(hidden[9],out[9])" +rankprofile[].fef.property[].name "vespa.type.query.b_1" +rankprofile[].fef.property[].value "tensor(out[9])" +rankprofile[].fef.property[].name "vespa.type.query.W_out" +rankprofile[].fef.property[].value "tensor(out[9])" +rankprofile[].fef.property[].name "vespa.type.query.b_out" +rankprofile[].fef.property[].value "tensor(out[1])" rankprofile[].name "unranked" rankprofile[].fef.property[].name "vespa.rank.firstphase" rankprofile[].fef.property[].value "value(0)" diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Mail.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Mail.java index b586b97ddf0..36b6e251fbc 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Mail.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Mail.java @@ -5,6 +5,7 @@ import com.google.common.collect.ImmutableList; import java.util.Collection; import java.util.Objects; +import java.util.Optional; /** * A message with a subject and a nonempty set of recipients. @@ -16,18 +17,29 @@ public class Mail { private final Collection<String> recipients; private final String subject; private final String message; + private final Optional<String> htmlMessage; public Mail(Collection<String> recipients, String subject, String message) { + this(recipients, subject, message, Optional.empty()); + } + + public Mail(Collection<String> recipients, String subject, String message, String htmlMessage) { + this(recipients, subject, message, Optional.of(htmlMessage)); + } + + Mail(Collection<String> recipients, String subject, String message, Optional<String> htmlMessage) { if (recipients.isEmpty()) throw new IllegalArgumentException("Empty recipient list is not allowed."); recipients.forEach(Objects::requireNonNull); this.recipients = ImmutableList.copyOf(recipients); this.subject = Objects.requireNonNull(subject); this.message = Objects.requireNonNull(message); + this.htmlMessage = Objects.requireNonNull(htmlMessage); } public Collection<String> recipients() { return recipients; } public String subject() { return subject; } public String message() { return message; } + public Optional<String> htmlMessage() { return htmlMessage; } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java index 935ba17eed6..7ac7a36d742 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/zone/ZoneRegistry.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.controller.api.integration.zone; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.ApplicationName; import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; @@ -85,6 +86,9 @@ public interface ZoneRegistry { URI dashboardUrl(TenantName id); /** Returns a URL which displays information about the given application. */ + URI dashboardUrl(TenantName tenantName, ApplicationName applicationName); + + /** Returns a URL which displays information about the given application instance. */ URI dashboardUrl(ApplicationId id); /** Returns a URL which displays information about the given job run. */ 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 613e3413ae8..6907747646e 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 @@ -93,6 +93,8 @@ import java.util.Optional; import java.util.OptionalInt; import java.util.Set; import java.util.TreeMap; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.function.Predicate; import java.util.logging.Level; @@ -471,8 +473,10 @@ public class ApplicationController { RevisionId revision = run.versions().sourceRevision().filter(__ -> deploySourceVersions).orElse(run.versions().targetRevision()); ApplicationPackage applicationPackage = new ApplicationPackage(applicationStore.get(deployment, revision)); + AtomicReference<RevisionId> lastRevision = new AtomicReference<>(); try (Mutex lock = lock(applicationId)) { LockedApplication application = new LockedApplication(requireApplication(applicationId), lock); + application.get().revisions().last().map(ApplicationVersion::id).ifPresent(lastRevision::set); Instance instance = application.get().require(job.application().instance()); if ( ! applicationPackage.trustedCertificates().isEmpty() @@ -492,22 +496,25 @@ public class ApplicationController { var quotaUsage = deploymentQuotaUsage(zone, job.application()); // For direct deployments use the full deployment ID, but otherwise use just the tenant and application as - // the source since it's the same application, so it should have the same warnings - NotificationSource source = zone.environment().isManuallyDeployed() ? - NotificationSource.from(deployment) : NotificationSource.from(applicationId); - - @SuppressWarnings("deprecation") - List<String> warnings = Optional.ofNullable(result.prepareResponse().log) - .map(logs -> logs.stream() - .filter(log -> log.applicationPackage) - .filter(log -> LogLevel.parse(log.level).intValue() >= Level.WARNING.intValue()) - .map(log -> log.message) - .sorted() - .distinct() - .collect(Collectors.toList())) - .orElseGet(List::of); - if (warnings.isEmpty()) controller.notificationsDb().removeNotification(source, Notification.Type.applicationPackage); - else controller.notificationsDb().setNotification(source, Notification.Type.applicationPackage, Notification.Level.warning, warnings); + // the source since it's the same application, so it should have the same warnings. + // These notifications are only updated when the last submitted revision is deployed here. + NotificationSource source = zone.environment().isManuallyDeployed() + ? NotificationSource.from(deployment) + : revision.equals(lastRevision.get()) ? NotificationSource.from(applicationId) : null; + if (source != null) { + @SuppressWarnings("deprecation") + List<String> warnings = Optional.ofNullable(result.prepareResponse().log) + .map(logs -> logs.stream() + .filter(log -> log.applicationPackage) + .filter(log -> LogLevel.parse(log.level).intValue() >= Level.WARNING.intValue()) + .map(log -> log.message) + .sorted() + .distinct() + .collect(Collectors.toList())) + .orElseGet(List::of); + if (warnings.isEmpty()) controller.notificationsDb().removeNotification(source, Notification.Type.applicationPackage); + else controller.notificationsDb().setNotification(source, Notification.Type.applicationPackage, Notification.Level.warning, warnings); + } lockApplicationOrThrow(applicationId, application -> store(application.with(job.application().instance(), @@ -542,7 +549,7 @@ public class ApplicationController { controller.jobController().deploymentStatus(application.get()); for (Notification notification : controller.notificationsDb().listNotifications(NotificationSource.from(application.get().id()), true)) { - if ( ! notification.source().instance().map(declaredInstances::contains).orElse(false)) + if ( ! notification.source().instance().map(declaredInstances::contains).orElse(true)) controller.notificationsDb().removeNotifications(notification.source()); if (notification.source().instance().isPresent() && ! notification.source().zoneId().map(application.get().require(notification.source().instance().get()).deployments()::containsKey).orElse(false)) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java index ed794a9d929..0d39703b70d 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java @@ -66,9 +66,9 @@ public class ApplicationPackage { private static final String trustedCertificatesFile = "security/clients.pem"; private static final String buildMetaFile = "build-meta.json"; - private static final String deploymentFile = "deployment.xml"; + static final String deploymentFile = "deployment.xml"; private static final String validationOverridesFile = "validation-overrides.xml"; - private static final String servicesFile = "services.xml"; + static final String servicesFile = "services.xml"; private final String contentHash; private final String bundleHash; @@ -212,7 +212,7 @@ public class ApplicationPackage { entry -> entry.getValue().get()))); } - static byte[] filesZip(Map<String, byte[]> files) { + public static byte[] filesZip(Map<String, byte[]> files) { try (ZipBuilder zipBuilder = new ZipBuilder(files.values().stream().mapToInt(bytes -> bytes.length).sum() + 512)) { files.forEach(zipBuilder::add); zipBuilder.close(); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/TestPackage.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/TestPackage.java new file mode 100644 index 00000000000..fb352848911 --- /dev/null +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/TestPackage.java @@ -0,0 +1,318 @@ +package com.yahoo.vespa.hosted.controller.application.pkg; + +import com.yahoo.config.application.api.DeploymentInstanceSpec; +import com.yahoo.config.application.api.DeploymentSpec; +import com.yahoo.config.application.api.DeploymentSpec.Step; +import com.yahoo.config.provision.AthenzDomain; +import com.yahoo.config.provision.AthenzService; +import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.NodeResources; +import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.path.Path; +import com.yahoo.security.KeyAlgorithm; +import com.yahoo.security.KeyUtils; +import com.yahoo.security.SignatureAlgorithm; +import com.yahoo.security.X509CertificateBuilder; +import com.yahoo.security.X509CertificateUtils; +import com.yahoo.text.Text; +import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; +import com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite; +import com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterId; +import com.yahoo.vespa.hosted.controller.config.ControllerConfig; +import com.yahoo.vespa.hosted.controller.config.ControllerConfig.Steprunner.Testerapp; +import com.yahoo.yolean.Exceptions; + +import javax.security.auth.x500.X500Principal; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.math.BigInteger; +import java.security.KeyPair; +import java.security.cert.X509Certificate; +import java.time.Duration; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.UUID; +import java.util.jar.JarInputStream; +import java.util.jar.Manifest; +import java.util.regex.Pattern; + +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite.production; +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite.staging; +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite.staging_setup; +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite.system; +import static com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackage.deploymentFile; +import static com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackage.servicesFile; +import static com.yahoo.vespa.hosted.controller.application.pkg.ZipEntries.transferAndWrite; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.stream.Collectors.groupingBy; +import static java.util.stream.Collectors.mapping; +import static java.util.stream.Collectors.toList; + +/** + * Validation and manipulation of test package. + * + * @author jonmv + */ +public class TestPackage { + + // Must match exactly the advertised resources of an AWS instance type. Also consider that the container + // will have ~1.8 GB less memory than equivalent resources in AWS (VESPA-16259). + static final NodeResources DEFAULT_TESTER_RESOURCES_AWS = new NodeResources(2, 8, 50, 0.3, NodeResources.DiskSpeed.any); + static final NodeResources DEFAULT_TESTER_RESOURCES = new NodeResources(1, 4, 50, 0.3, NodeResources.DiskSpeed.any); + + private final ApplicationPackage applicationPackage; + private final X509Certificate certificate; + + public TestPackage(byte[] testPackage, boolean isPublicSystem, RunId id, Testerapp testerApp, + DeploymentSpec spec, Instant certificateValidFrom, Duration certificateValidDuration) { + + // Copy contents of submitted application-test.zip, and ensure required directories exist within the zip. + Map<String, byte[]> entries = new HashMap<>(); + entries.put("artifacts/.ignore-" + UUID.randomUUID(), new byte[0]); + entries.put("tests/.ignore-" + UUID.randomUUID(), new byte[0]); + + entries.put(servicesFile, + servicesXml( ! isPublicSystem, + certificateValidFrom != null, + testerResourcesFor(id.type().zone(), spec.requireInstance(id.application().instance())), + testerApp)); + + entries.put(deploymentFile, + deploymentXml(id.tester(), + spec.athenzDomain(), + spec.requireInstance(id.application().instance()) + .athenzService(id.type().zone().environment(), id.type().zone().region()))); + + if (certificateValidFrom != null) { + KeyPair keyPair = KeyUtils.generateKeypair(KeyAlgorithm.RSA, 2048); + X500Principal subject = new X500Principal("CN=" + id.tester().id().toFullString() + "." + id.type() + "." + id.number()); + this.certificate = X509CertificateBuilder.fromKeypair(keyPair, + subject, + certificateValidFrom, + certificateValidFrom.plus(certificateValidDuration), + SignatureAlgorithm.SHA512_WITH_RSA, + BigInteger.valueOf(1)) + .build(); + entries.put("artifacts/key", KeyUtils.toPem(keyPair.getPrivate()).getBytes(UTF_8)); + entries.put("artifacts/cert", X509CertificateUtils.toPem(certificate).getBytes(UTF_8)); + } + else { + this.certificate = null; + } + + ByteArrayOutputStream buffer = new ByteArrayOutputStream(testPackage.length + 10_000); + transferAndWrite(buffer, new ByteArrayInputStream(testPackage), entries); + this.applicationPackage = new ApplicationPackage(buffer.toByteArray()); + } + + public ApplicationPackage asApplicationPackage() { + return applicationPackage; + } + + public X509Certificate certificate() { + return Objects.requireNonNull(certificate); + } + + public static TestSummary validateTests(DeploymentSpec spec, byte[] testPackage) { + return validateTests(expectedSuites(spec.steps()), testPackage); + } + + static TestSummary validateTests(Collection<Suite> expectedSuites, byte[] testPackage) { + List<String> problems = new ArrayList<>(); + Set<Suite> suites = new LinkedHashSet<>(); + ZipEntries.from(testPackage, __ -> true, 0, false).asList().stream() + .map(entry -> Path.fromString(entry.name())) + .collect(groupingBy(path -> path.elements().size() > 1 ? path.elements().get(0) : "", + mapping(path -> (path.elements().size() > 1 ? path.getChildPath() : path).getRelative(), toList()))) + .forEach((directory, paths) -> { + switch (directory) { + case "components": { + for (String path : paths) { + if (path.endsWith("-tests.jar")) { + try { + byte[] testsJar = ZipEntries.readFile(testPackage, "components/" + path, 1 << 30); + Manifest manifest = new JarInputStream(new ByteArrayInputStream(testsJar)).getManifest(); + for (String suite : manifest.getMainAttributes().getValue("X-JDisc-Test-Bundle-Categories").split(",")) + switch (suite.trim()) { + case "SystemTest": suites.add(system); break; + case "StagingSetup": suites.add(staging_setup); break; + case "StagingTest": suites.add(staging); break; + case "ProductionTest": suites.add(production); break; + default: problems.add("unexpected test suite name '" + suite + "' in bundle manifest"); + } + } + catch (Exception e) { + problems.add("failed reading test bundle manifest: " + Exceptions.toMessageString(e)); + } + } + } + } + break; + case "tests": { + if (paths.stream().anyMatch(Pattern.compile("system-test/.+\\.json").asMatchPredicate())) suites.add(system); + if (paths.stream().anyMatch(Pattern.compile("staging-setup/.+\\.json").asMatchPredicate())) suites.add(staging_setup); + if (paths.stream().anyMatch(Pattern.compile("staging-test/.+\\.json").asMatchPredicate())) suites.add(staging); + if (paths.stream().anyMatch(Pattern.compile("production-test/.+\\.json").asMatchPredicate())) suites.add(production); + } + break; + case "artifacts": { + if (paths.stream().anyMatch(Pattern.compile(".+-tests.jar").asMatchPredicate())) + suites.addAll(expectedSuites); // ಠ_ಠ+ + for (String forbidden : List.of("key", "cert")) + if (paths.contains(forbidden)) + problems.add("test package contains 'artifacts/" + forbidden + + "'; this conflicts with credentials used to run tests in Vespa Cloud"); + } + break; + } + }); + + if (expectedSuites.contains(system) && ! suites.contains(system)) + problems.add("test package has no system tests, but <test /> is declared in deployment.xml"); + + if (suites.contains(staging) != suites.contains(staging_setup)) + problems.add("test package has " + (suites.contains(staging) ? "staging tests" : "staging setup") + + ", so it should also include " + (suites.contains(staging) ? "staging setup" : "staging tests")); + else if (expectedSuites.contains(staging) && ! suites.contains(staging)) + problems.add("test package has no staging setup and tests, but <staging /> is declared in deployment.xml"); + + if (suites.contains(production) != expectedSuites.contains(production)) + problems.add("test package has " + (suites.contains(production) ? "" : "no ") + "production tests, " + + "but " + (suites.contains(production) ? "no " : "") + "production tests are declared in deployment.xml"); + + if ( ! problems.isEmpty()) + problems.add("see https://docs.vespa.ai/en/testing.html for details on how to write system tests for Vespa"); + + return new TestSummary(problems, suites); + } + + public static NodeResources testerResourcesFor(ZoneId zone, DeploymentInstanceSpec spec) { + NodeResources nodeResources = spec.steps().stream() + .filter(step -> step.concerns(zone.environment())) + .findFirst() + .flatMap(step -> step.zones().get(0).testerFlavor()) + .map(NodeResources::fromLegacyName) + .orElse(zone.region().value().contains("aws-") ? DEFAULT_TESTER_RESOURCES_AWS + : DEFAULT_TESTER_RESOURCES); + return nodeResources.with(NodeResources.DiskSpeed.any); + } + + /** Returns the generated services.xml content for the tester application. */ + public static byte[] servicesXml(boolean systemUsesAthenz, boolean useTesterCertificate, + NodeResources resources, ControllerConfig.Steprunner.Testerapp config) { + int jdiscMemoryGb = 2; // 2Gb memory for tester application (excessive?). + int jdiscMemoryPct = (int) Math.ceil(100 * jdiscMemoryGb / resources.memoryGb()); + + // Of the remaining memory, split 50/50 between Surefire running the tests and the rest + int testMemoryMb = (int) (1024 * (resources.memoryGb() - jdiscMemoryGb) / 2); + + String resourceString = Text.format("<resources vcpu=\"%.2f\" memory=\"%.2fGb\" disk=\"%.2fGb\" disk-speed=\"%s\" storage-type=\"%s\"/>", + resources.vcpu(), resources.memoryGb(), resources.diskGb(), resources.diskSpeed().name(), resources.storageType().name()); + + String runtimeProviderClass = config.runtimeProviderClass(); + String tenantCdBundle = config.tenantCdBundle(); + + String servicesXml = + "<?xml version='1.0' encoding='UTF-8'?>\n" + + "<services xmlns:deploy='vespa' version='1.0'>\n" + + " <container version='1.0' id='tester'>\n" + + "\n" + + " <component id=\"com.yahoo.vespa.hosted.testrunner.TestRunner\" bundle=\"vespa-testrunner-components\">\n" + + " <config name=\"com.yahoo.vespa.hosted.testrunner.test-runner\">\n" + + " <artifactsPath>artifacts</artifactsPath>\n" + + " <surefireMemoryMb>" + testMemoryMb + "</surefireMemoryMb>\n" + + " <useAthenzCredentials>" + systemUsesAthenz + "</useAthenzCredentials>\n" + + " <useTesterCertificate>" + useTesterCertificate + "</useTesterCertificate>\n" + + " </config>\n" + + " </component>\n" + + "\n" + + " <handler id=\"com.yahoo.vespa.testrunner.TestRunnerHandler\" bundle=\"vespa-osgi-testrunner\">\n" + + " <binding>http://*/tester/v1/*</binding>\n" + + " </handler>\n" + + "\n" + + " <component id=\"" + runtimeProviderClass + "\" bundle=\"" + tenantCdBundle + "\" />\n" + + "\n" + + " <component id=\"com.yahoo.vespa.testrunner.JunitRunner\" bundle=\"vespa-osgi-testrunner\">\n" + + " <config name=\"com.yahoo.vespa.testrunner.junit-test-runner\">\n" + + " <artifactsPath>artifacts</artifactsPath>\n" + + " <useAthenzCredentials>" + systemUsesAthenz + "</useAthenzCredentials>\n" + + " </config>\n" + + " </component>\n" + + "\n" + + " <component id=\"com.yahoo.vespa.testrunner.VespaCliTestRunner\" bundle=\"vespa-osgi-testrunner\">\n" + + " <config name=\"com.yahoo.vespa.testrunner.vespa-cli-test-runner\">\n" + + " <artifactsPath>artifacts</artifactsPath>\n" + + " <testsPath>tests</testsPath>\n" + + " <useAthenzCredentials>" + systemUsesAthenz + "</useAthenzCredentials>\n" + + " </config>\n" + + " </component>\n" + + "\n" + + " <nodes count=\"1\">\n" + + " <jvm allocated-memory=\"" + jdiscMemoryPct + "%\"/>\n" + + " " + resourceString + "\n" + + " </nodes>\n" + + " </container>\n" + + "</services>\n"; + + return servicesXml.getBytes(UTF_8); + } + + /** Returns a dummy deployment xml which sets up the service identity for the tester, if present. */ + public static byte[] deploymentXml(TesterId id, Optional<AthenzDomain> athenzDomain, Optional<AthenzService> athenzService) { + String deploymentSpec = + "<?xml version='1.0' encoding='UTF-8'?>\n" + + "<deployment version=\"1.0\" " + + athenzDomain.map(domain -> "athenz-domain=\"" + domain.value() + "\" ").orElse("") + + athenzService.map(service -> "athenz-service=\"" + service.value() + "\" ").orElse("") + ">" + + " <instance id=\"" + id.id().instance().value() + "\" />" + + "</deployment>"; + return deploymentSpec.getBytes(UTF_8); + } + + static Set<Suite> expectedSuites(List<Step> steps) { + Set<Suite> suites = new HashSet<>(); + if (steps.isEmpty()) return suites; + for (Step step : steps) { + if (step.isTest()) { + if (step.concerns(Environment.prod)) suites.add(production); + if (step.concerns(Environment.test)) suites.add(system); + if (step.concerns(Environment.staging)) { suites.add(staging); suites.add(staging_setup); } + } + else + suites.addAll(expectedSuites(step.steps())); + } + return suites; + } + + + public static class TestSummary { + + private final List<String> problems; + private final List<Suite> suites; + + public TestSummary(List<String> problems, Set<Suite> suites) { + this.problems = List.copyOf(problems); + this.suites = List.copyOf(suites); + } + + public List<String> problems() { + return problems; + } + + public List<Suite> suites() { + return suites; + } + + } + +} diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ZipEntries.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ZipEntries.java index a6cb7f23fc3..8392a77bad5 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ZipEntries.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ZipEntries.java @@ -13,6 +13,8 @@ import java.io.OutputStream; import java.io.UncheckedIOException; import java.util.ArrayList; import java.util.List; +import java.util.Map; +import java.util.Map.Entry; import java.util.Objects; import java.util.Optional; import java.util.function.Predicate; @@ -35,19 +37,28 @@ public class ZipEntries { /** Copies the zipped content from in to out, adding/overwriting an entry with the given name and content. */ public static void transferAndWrite(OutputStream out, InputStream in, String name, byte[] content) { + transferAndWrite(out, in, Map.of(name, content)); + } + + /** Copies the zipped content from in to out, adding/overwriting/removing (on {@code null}) entries as specified. */ + public static void transferAndWrite(OutputStream out, InputStream in, Map<String, byte[]> entries) { try (ZipOutputStream zipOut = new ZipOutputStream(out); ZipInputStream zipIn = new ZipInputStream(in)) { for (ZipEntry entry = zipIn.getNextEntry(); entry != null; entry = zipIn.getNextEntry()) { - if (entry.getName().equals(name)) + if (entries.containsKey(entry.getName())) continue; zipOut.putNextEntry(new ZipEntry(entry.getName())); zipIn.transferTo(zipOut); zipOut.closeEntry(); } - zipOut.putNextEntry(new ZipEntry(name)); - zipOut.write(content); - zipOut.closeEntry(); + for (Entry<String, byte[]> entry : entries.entrySet()) { + if (entry.getValue() != null) { + zipOut.putNextEntry(new ZipEntry(entry.getKey())); + zipOut.write(entry.getValue()); + zipOut.closeEntry(); + } + } } catch (IOException e) { throw new UncheckedIOException(e); @@ -76,6 +87,10 @@ public class ZipEntries { return new ZipEntries(entries); } + public static byte[] readFile(byte[] zip, String name, int maxEntrySizeInBytes) { + return from(zip, name::equals, maxEntrySizeInBytes, true).asList().get(0).contentOrThrow(); + } + public List<ZipEntryWithContent> asList() { return entries; } public static class ZipEntryWithContent { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java index 9e551c7ce78..52e5431b552 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java @@ -3,16 +3,12 @@ package com.yahoo.vespa.hosted.controller.deployment; import ai.vespa.http.DomainName; import com.yahoo.component.Version; -import com.yahoo.config.application.api.DeploymentInstanceSpec; import com.yahoo.config.application.api.DeploymentSpec; import com.yahoo.config.application.api.Notifications; import com.yahoo.config.application.api.Notifications.When; import com.yahoo.config.provision.ApplicationId; -import com.yahoo.config.provision.AthenzDomain; -import com.yahoo.config.provision.AthenzService; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.HostName; -import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.zone.RoutingMethod; import com.yahoo.config.provision.zone.ZoneId; @@ -22,7 +18,6 @@ import com.yahoo.security.KeyUtils; import com.yahoo.security.SignatureAlgorithm; import com.yahoo.security.X509CertificateBuilder; import com.yahoo.security.X509CertificateUtils; -import com.yahoo.text.Text; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.Instance; @@ -38,7 +33,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RevisionId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud; -import com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterId; import com.yahoo.vespa.hosted.controller.api.integration.organization.DeploymentFailureMails; import com.yahoo.vespa.hosted.controller.api.integration.organization.Mail; import com.yahoo.vespa.hosted.controller.application.ActivateResult; @@ -46,7 +40,7 @@ import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.application.Endpoint; import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId; import com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackage; -import com.yahoo.vespa.hosted.controller.config.ControllerConfig; +import com.yahoo.vespa.hosted.controller.application.pkg.TestPackage; import com.yahoo.vespa.hosted.controller.maintenance.JobRunner; import com.yahoo.vespa.hosted.controller.notification.Notification; import com.yahoo.vespa.hosted.controller.notification.NotificationSource; @@ -124,13 +118,6 @@ public class InternalStepRunner implements StepRunner { private static final Logger logger = Logger.getLogger(InternalStepRunner.class.getName()); - static final NodeResources DEFAULT_TESTER_RESOURCES = - new NodeResources(1, 4, 50, 0.3, NodeResources.DiskSpeed.any); - // Must match exactly the advertised resources of an AWS instance type. Also consider that the container - // will have ~1.8 GB less memory than equivalent resources in AWS (VESPA-16259). - static final NodeResources DEFAULT_TESTER_RESOURCES_AWS = - new NodeResources(2, 8, 50, 0.3, NodeResources.DiskSpeed.any); - private final Controller controller; private final TestConfigSerializer testConfigSerializer; private final DeploymentFailureMails mails; @@ -907,48 +894,20 @@ public class InternalStepRunner implements StepRunner { private ApplicationPackage testerPackage(RunId id) { RevisionId revision = controller.jobController().run(id).get().versions().targetRevision(); DeploymentSpec spec = controller.applications().requireApplication(TenantAndApplicationId.from(id.application())).deploymentSpec(); - - ZoneId zone = id.type().zone(); + byte[] testZip = controller.applications().applicationStore().getTester(id.application().tenant(), + id.application().application(), revision); boolean useTesterCertificate = useTesterCertificate(id); - byte[] servicesXml = servicesXml( ! controller.system().isPublic(), - useTesterCertificate, - testerResourcesFor(zone, spec.requireInstance(id.application().instance())), - controller.controllerConfig().steprunner().testerapp()); - byte[] testPackage = controller.applications().applicationStore().getTester(id.application().tenant(), id.application().application(), revision); - byte[] deploymentXml = deploymentXml(id.tester(), - spec.athenzDomain(), - spec.requireInstance(id.application().instance()).athenzService(zone.environment(), zone.region())); - - try (ZipBuilder zipBuilder = new ZipBuilder(testPackage.length + servicesXml.length + deploymentXml.length + 1000)) { - // Copy contents of submitted application-test.zip, and ensure required directories exist within the zip. - zipBuilder.add(testPackage); - zipBuilder.add("artifacts/.ignore-" + UUID.randomUUID(), new byte[0]); - zipBuilder.add("tests/.ignore-" + UUID.randomUUID(), new byte[0]); - - zipBuilder.add("services.xml", servicesXml); - zipBuilder.add("deployment.xml", deploymentXml); - if (useTesterCertificate) - appendAndStoreCertificate(zipBuilder, id); - - zipBuilder.close(); - return new ApplicationPackage(zipBuilder.toByteArray()); - } - } + TestPackage testPackage = new TestPackage(testZip, + controller.system().isPublic(), + id, + controller.controllerConfig().steprunner().testerapp(), + spec, + useTesterCertificate ? controller.clock().instant() : null, + timeouts.testerCertificate()); + if (useTesterCertificate) controller.jobController().storeTesterCertificate(id, testPackage.certificate()); - private void appendAndStoreCertificate(ZipBuilder zipBuilder, RunId id) { - KeyPair keyPair = KeyUtils.generateKeypair(KeyAlgorithm.RSA, 2048); - X500Principal subject = new X500Principal("CN=" + id.tester().id().toFullString() + "." + id.type() + "." + id.number()); - X509Certificate certificate = X509CertificateBuilder.fromKeypair(keyPair, - subject, - controller.clock().instant(), - controller.clock().instant().plus(timeouts.testerCertificate()), - SignatureAlgorithm.SHA512_WITH_RSA, - BigInteger.valueOf(1)) - .build(); - controller.jobController().storeTesterCertificate(id, certificate); - zipBuilder.add("artifacts/key", KeyUtils.toPem(keyPair.getPrivate()).getBytes(UTF_8)); - zipBuilder.add("artifacts/cert", X509CertificateUtils.toPem(certificate).getBytes(UTF_8)); + return testPackage.asApplicationPackage(); } private DeploymentId getTesterDeploymentId(RunId runId) { @@ -956,90 +915,6 @@ public class InternalStepRunner implements StepRunner { return new DeploymentId(runId.tester().id(), zoneId); } - static NodeResources testerResourcesFor(ZoneId zone, DeploymentInstanceSpec spec) { - NodeResources nodeResources = spec.steps().stream() - .filter(step -> step.concerns(zone.environment())) - .findFirst() - .flatMap(step -> step.zones().get(0).testerFlavor()) - .map(NodeResources::fromLegacyName) - .orElse(zone.region().value().contains("aws-") ? - DEFAULT_TESTER_RESOURCES_AWS : DEFAULT_TESTER_RESOURCES); - return nodeResources.with(NodeResources.DiskSpeed.any); - } - - /** Returns the generated services.xml content for the tester application. */ - static byte[] servicesXml(boolean systemUsesAthenz, boolean useTesterCertificate, - NodeResources resources, ControllerConfig.Steprunner.Testerapp config) { - int jdiscMemoryGb = 2; // 2Gb memory for tester application (excessive?). - int jdiscMemoryPct = (int) Math.ceil(100 * jdiscMemoryGb / resources.memoryGb()); - - // Of the remaining memory, split 50/50 between Surefire running the tests and the rest - int testMemoryMb = (int) (1024 * (resources.memoryGb() - jdiscMemoryGb) / 2); - - String resourceString = Text.format( - "<resources vcpu=\"%.2f\" memory=\"%.2fGb\" disk=\"%.2fGb\" disk-speed=\"%s\" storage-type=\"%s\"/>", - resources.vcpu(), resources.memoryGb(), resources.diskGb(), resources.diskSpeed().name(), resources.storageType().name()); - - String runtimeProviderClass = config.runtimeProviderClass(); - String tenantCdBundle = config.tenantCdBundle(); - - String servicesXml = - "<?xml version='1.0' encoding='UTF-8'?>\n" + - "<services xmlns:deploy='vespa' version='1.0'>\n" + - " <container version='1.0' id='tester'>\n" + - "\n" + - " <component id=\"com.yahoo.vespa.hosted.testrunner.TestRunner\" bundle=\"vespa-testrunner-components\">\n" + - " <config name=\"com.yahoo.vespa.hosted.testrunner.test-runner\">\n" + - " <artifactsPath>artifacts</artifactsPath>\n" + - " <surefireMemoryMb>" + testMemoryMb + "</surefireMemoryMb>\n" + - " <useAthenzCredentials>" + systemUsesAthenz + "</useAthenzCredentials>\n" + - " <useTesterCertificate>" + useTesterCertificate + "</useTesterCertificate>\n" + - " </config>\n" + - " </component>\n" + - "\n" + - " <handler id=\"com.yahoo.vespa.testrunner.TestRunnerHandler\" bundle=\"vespa-osgi-testrunner\">\n" + - " <binding>http://*/tester/v1/*</binding>\n" + - " </handler>\n" + - "\n" + - " <component id=\"" + runtimeProviderClass + "\" bundle=\"" + tenantCdBundle + "\" />\n" + - "\n" + - " <component id=\"com.yahoo.vespa.testrunner.JunitRunner\" bundle=\"vespa-osgi-testrunner\">\n" + - " <config name=\"com.yahoo.vespa.testrunner.junit-test-runner\">\n" + - " <artifactsPath>artifacts</artifactsPath>\n" + - " <useAthenzCredentials>" + systemUsesAthenz + "</useAthenzCredentials>\n" + - " </config>\n" + - " </component>\n" + - "\n" + - " <component id=\"com.yahoo.vespa.testrunner.VespaCliTestRunner\" bundle=\"vespa-osgi-testrunner\">\n" + - " <config name=\"com.yahoo.vespa.testrunner.vespa-cli-test-runner\">\n" + - " <artifactsPath>artifacts</artifactsPath>\n" + - " <testsPath>tests</testsPath>\n" + - " <useAthenzCredentials>" + systemUsesAthenz + "</useAthenzCredentials>\n" + - " </config>\n" + - " </component>\n" + - "\n" + - " <nodes count=\"1\">\n" + - " <jvm allocated-memory=\"" + jdiscMemoryPct + "%\"/>\n" + - " " + resourceString + "\n" + - " </nodes>\n" + - " </container>\n" + - "</services>\n"; - - return servicesXml.getBytes(UTF_8); - } - - /** Returns a dummy deployment xml which sets up the service identity for the tester, if present. */ - private static byte[] deploymentXml(TesterId id, Optional<AthenzDomain> athenzDomain, Optional<AthenzService> athenzService) { - String deploymentSpec = - "<?xml version='1.0' encoding='UTF-8'?>\n" + - "<deployment version=\"1.0\" " + - athenzDomain.map(domain -> "athenz-domain=\"" + domain.value() + "\" ").orElse("") + - athenzService.map(service -> "athenz-service=\"" + service.value() + "\" ").orElse("") + ">" + - " <instance id=\"" + id.id().instance().value() + "\" />" + - "</deployment>"; - return deploymentSpec.getBytes(UTF_8); - } - /** Logger which logs to a {@link JobController}, as well as to the parent class' {@link Logger}. */ private class DualLogger { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java index 69d9ba504a5..b0966f7db21 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java @@ -28,6 +28,11 @@ import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId; import com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackage; import com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackageDiff; +import com.yahoo.vespa.hosted.controller.application.pkg.TestPackage; +import com.yahoo.vespa.hosted.controller.application.pkg.TestPackage.TestSummary; +import com.yahoo.vespa.hosted.controller.notification.Notification; +import com.yahoo.vespa.hosted.controller.notification.Notification.Type; +import com.yahoo.vespa.hosted.controller.notification.NotificationSource; import com.yahoo.vespa.hosted.controller.persistence.BufferedLogStore; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; import com.yahoo.vespa.hosted.controller.versions.VespaVersion; @@ -433,8 +438,15 @@ public class JobController { }); } finally { - for (Mutex lock : locks) - lock.close(); + for (Mutex lock : locks) { + try { + lock.close(); + } catch (Throwable t) { + log.log(WARNING, "Failed to close the lock " + lock + ": the lock may or may not " + + "have been released in ZooKeeper, and if not this controller " + + "must be restarted to release the lock", t); + } + } } } @@ -489,6 +501,16 @@ public class JobController { application = application.withRevisions(revisions -> revisions.with(version.get())); application = withPrunedPackages(application); + TestSummary testSummary = TestPackage.validateTests(applicationPackage.deploymentSpec(), testPackageBytes); + if (testSummary.problems().isEmpty()) + controller.notificationsDb().removeNotification(NotificationSource.from(id), + Type.testPackage); + else + controller.notificationsDb().setNotification(NotificationSource.from(id), + Type.testPackage, + Notification.Level.warning, + testSummary.problems()); + applications.storeWithUpdatedConfig(application, applicationPackage); applications.deploymentTrigger().triggerNewRevision(id); }); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notification/Notification.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notification/Notification.java index c39ee031e27..38cb09355ca 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notification/Notification.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notification/Notification.java @@ -66,10 +66,13 @@ public class Notification { } public enum Type { - /** Related to contents of application package, e.g. usage of deprecated features/syntax */ + /** Related to contents of application package, e.g., usage of deprecated features/syntax */ applicationPackage, - /** Related to deployment of application, e.g. system test failure, node allocation failure, internal errors, etc. */ + /** Related to contents of application test package, e.g., mismatch between deployment spec and provided tests */ + testPackage, + + /** Related to deployment of application, e.g., system test failure, node allocation failure, internal errors, etc. */ deployment, /** Application cluster is (near) external feed blocked */ diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notify/Notifier.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notify/Notifier.java index 7692752f3ca..6b14872b07d 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notify/Notifier.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/notify/Notifier.java @@ -95,27 +95,38 @@ public class Notifier { private Mail mailOf(Notification n, Collection<String> recipients) { var source = n.source(); - var subject = Text.format("[%s] %s Vespa Notification for %s - %s", n.level().toString().toUpperCase(), n.type().name(), source.tenant(), source.application()); + var subject = Text.format("[%s] %s Vespa Notification for %s", n.level().toString().toUpperCase(), n.type().name(), applicationIdSource(source)); var body = new StringBuilder(); body.append("Source: ").append(n.source().toString()).append("\n") .append("\n") .append(String.join("\n", n.messages())) .append("\n") .append(url(source).toString()); - return new Mail(recipients, subject.toString(), body.toString()); + return new Mail(recipients, subject, body.toString()); + } + + private String applicationIdSource(NotificationSource source) { + StringBuilder sb = new StringBuilder(); + sb.append(source.tenant().value()); + source.application().ifPresent(applicationName -> sb.append(".").append(applicationName.value())); + source.instance().ifPresent(instanceName -> sb.append(".").append(instanceName.value())); + return sb.toString(); } private URI url(NotificationSource source) { - if (source.application().isPresent() && source.instance().isPresent()) { - if (source.jobType().isPresent() && source.runNumber().isPresent()) { - return zoneRegistry.dashboardUrl( - new RunId(ApplicationId.from(source.tenant(), - source.application().get(), - source.instance().get()), - source.jobType().get(), - source.runNumber().getAsLong())); + if (source.application().isPresent()) { + if (source.instance().isPresent()) { + if (source.jobType().isPresent() && source.runNumber().isPresent()) { + return zoneRegistry.dashboardUrl( + new RunId(ApplicationId.from(source.tenant(), + source.application().get(), + source.instance().get()), + source.jobType().get(), + source.runNumber().getAsLong())); + } + return zoneRegistry.dashboardUrl(ApplicationId.from(source.tenant(), source.application().get(), source.instance().get())); } - return zoneRegistry.dashboardUrl(ApplicationId.from(source.tenant(), source.application().get(), source.instance().get())); + return zoneRegistry.dashboardUrl(source.tenant(), source.application().get()); } return zoneRegistry.dashboardUrl(source.tenant()); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/NotificationsSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/NotificationsSerializer.java index c882681632e..570dbdd870e 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/NotificationsSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/NotificationsSerializer.java @@ -92,6 +92,7 @@ public class NotificationsSerializer { private static String asString(Notification.Type type) { switch (type) { case applicationPackage: return "applicationPackage"; + case testPackage: return "testPackage"; case deployment: return "deployment"; case feedBlock: return "feedBlock"; case reindex: return "reindex"; @@ -102,6 +103,7 @@ public class NotificationsSerializer { private static Notification.Type typeFrom(Inspector field) { switch (field.asString()) { case "applicationPackage": return Notification.Type.applicationPackage; + case "testPackage": return Notification.Type.testPackage; case "deployment": return Notification.Type.deployment; case "feedBlock": return Notification.Type.feedBlock; case "reindex": return Notification.Type.reindex; 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 c07794ea39c..4df53be57f2 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 @@ -745,6 +745,7 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler { private static String notificationTypeAsString(Notification.Type type) { switch (type) { case applicationPackage: return "applicationPackage"; + case testPackage: return "testPackage"; case deployment: return "deployment"; case feedBlock: return "feedBlock"; case reindex: return "reindex"; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java index dff379a2249..de03bbfb767 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java @@ -225,6 +225,7 @@ class JobControllerApiHandlerHelper { case aborted: return "aborted"; case error: return "error"; case testFailure: return "testFailure"; + case noTests: return "noTests"; case endpointCertificateTimeout: return "endpointCertificateTimeout"; case nodeAllocationFailure: return "nodeAllocationFailure"; case installationFailed: return "installationFailed"; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/billing/BillingApiHandlerV2.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/billing/BillingApiHandlerV2.java index db9c6845183..612b584c7c0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/billing/BillingApiHandlerV2.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/billing/BillingApiHandlerV2.java @@ -31,6 +31,7 @@ import java.time.Clock; import java.time.Instant; import java.time.LocalDate; import java.time.ZoneOffset; +import java.time.chrono.ChronoZonedDateTime; import java.time.format.DateTimeFormatter; import java.time.temporal.ChronoUnit; import java.util.Comparator; @@ -179,7 +180,7 @@ public class BillingApiHandlerV2 extends RestApiRequestHandler<BillingApiHandler private Slime tenantUsage(RestApi.RequestContext requestContext) { var tenantName = TenantName.from(requestContext.pathParameters().getStringOrThrow("tenant")); var tenant = tenants.require(tenantName, CloudTenant.class); - var untilAt = untilParameter(requestContext).orElseGet(this::startOfDayTomorrowUTC); + var untilAt = untilParameter(requestContext); var usage = billing.createUncommittedBill(tenant.name(), untilAt.atZone(ZoneOffset.UTC).toLocalDate()); var slime = new Slime(); @@ -190,7 +191,7 @@ public class BillingApiHandlerV2 extends RestApiRequestHandler<BillingApiHandler // --------- ACCOUNTANT API ---------- private Slime accountant(RestApi.RequestContext requestContext) { - var untilAt = untilParameter(requestContext).orElseGet(this::startOfDayTomorrowUTC); + var untilAt = untilParameter(requestContext); var usagePerTenant = billing.createUncommittedBills(untilAt.atZone(ZoneOffset.UTC).toLocalDate()); var response = new Slime(); @@ -211,7 +212,7 @@ public class BillingApiHandlerV2 extends RestApiRequestHandler<BillingApiHandler private Slime previewBill(RestApi.RequestContext requestContext) { var tenantName = TenantName.from(requestContext.pathParameters().getStringOrThrow("tenant")); var tenant = tenants.require(tenantName, CloudTenant.class); - var untilAt = untilParameter(requestContext).orElseGet(this::startOfDayTodayUTC); + var untilAt = untilParameter(requestContext); var usage = billing.createUncommittedBill(tenant.name(), untilAt.atZone(ZoneOffset.UTC).toLocalDate()); @@ -319,10 +320,13 @@ public class BillingApiHandlerV2 extends RestApiRequestHandler<BillingApiHandler // ---------- END INVOICE RENDERING ---------- - private Optional<Instant> untilParameter(RestApi.RequestContext ctx) { + private Instant untilParameter(RestApi.RequestContext ctx) { return ctx.queryParameters().getString("until") .map(LocalDate::parse) - .map(date -> date.plusDays(1).atStartOfDay(ZoneOffset.UTC).toInstant()); + .map(date -> date.plusDays(1)) + .map(date -> date.atStartOfDay(ZoneOffset.UTC)) + .map(ChronoZonedDateTime::toInstant) + .orElseGet(this::startOfDayTomorrowUTC); } private Instant startOfDayTodayUTC() { 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 e9619297a2f..20b64419f28 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 @@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.controller; import com.google.common.collect.Sets; import com.yahoo.component.Version; import com.yahoo.config.application.api.DeploymentSpec; +import com.yahoo.config.application.api.Notifications; import com.yahoo.config.application.api.ValidationId; import com.yahoo.config.application.api.ValidationOverrides; import com.yahoo.config.provision.ApplicationId; @@ -38,6 +39,10 @@ import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder; import com.yahoo.vespa.hosted.controller.deployment.DeploymentContext; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import com.yahoo.vespa.hosted.controller.integration.ZoneApiMock; +import com.yahoo.vespa.hosted.controller.notification.Notification; +import com.yahoo.vespa.hosted.controller.notification.Notification.Level; +import com.yahoo.vespa.hosted.controller.notification.Notification.Type; +import com.yahoo.vespa.hosted.controller.notification.NotificationSource; import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb; import com.yahoo.vespa.hosted.controller.routing.RoutingStatus; import com.yahoo.vespa.hosted.controller.routing.context.DeploymentRoutingContext; @@ -1104,6 +1109,27 @@ public class ControllerTest { } @Test + public void testTestPackageWarnings() { + String deploymentXml = "<deployment version='1.0'>\n" + + " <prod>\n" + + " <region>us-west-1</region>\n" + + " </prod>\n" + + "</deployment>\n"; + ApplicationPackage applicationPackage = ApplicationPackageBuilder.fromDeploymentXml(deploymentXml); + byte[] testPackage = ApplicationPackage.filesZip(Map.of("tests/staging-test/foo.json", new byte[0])); + var app = tester.newDeploymentContext(); + tester.jobs().submit(app.application().id(), Optional.empty(), Optional.empty(), Optional.empty(), 1, + applicationPackage, testPackage, Optional.empty(), 0); + assertEquals(List.of(new Notification(tester.clock().instant(), + Type.testPackage, + Level.warning, + NotificationSource.from(app.application().id()), + List.of("test package has staging tests, so it should also include staging setup", + "see https://docs.vespa.ai/en/testing.html for details on how to write system tests for Vespa"))), + tester.controller().notificationsDb().listNotifications(NotificationSource.from(app.application().id()), true)); + } + + @Test public void testCompileVersion() { DeploymentContext context = tester.newDeploymentContext(); ApplicationPackage applicationPackage = new ApplicationPackageBuilder().region("us-west-1").build(); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java index 4e155e937b9..99e22302c73 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java @@ -24,24 +24,24 @@ import static org.junit.Assert.fail; */ public class ApplicationPackageTest { - private static final String deploymentXml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + - "<deployment version=\"1.0\">\n" + - " <test />\n" + - " <prod>\n" + - " <parallel>\n" + - " <region active=\"true\">us-central-1</region>\n" + - " </parallel>\n" + - " </prod>\n" + - "</deployment>\n"; - - private static final String servicesXml = "<services version='1.0' xmlns:deploy=\"vespa\" xmlns:preprocess=\"properties\">\n" + - " <preprocess:include file='jdisc.xml' />\n" + - " <content version='1.0' if='foo' />\n" + - " <content version='1.0' id='foo' deploy:environment='staging prod' deploy:region='us-east-3 us-central-1'>\n" + - " <preprocess:include file='content/content.xml' />\n" + - " </content>\n" + - " <preprocess:include file='not_found.xml' required='false' />\n" + - "</services>\n"; + static final String deploymentXml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" + + "<deployment version=\"1.0\">\n" + + " <test />\n" + + " <prod>\n" + + " <parallel>\n" + + " <region active=\"true\">us-central-1</region>\n" + + " </parallel>\n" + + " </prod>\n" + + "</deployment>\n"; + + static final String servicesXml = "<services version='1.0' xmlns:deploy=\"vespa\" xmlns:preprocess=\"properties\">\n" + + " <preprocess:include file='jdisc.xml' />\n" + + " <content version='1.0' if='foo' />\n" + + " <content version='1.0' id='foo' deploy:environment='staging prod' deploy:region='us-east-3 us-central-1'>\n" + + " <preprocess:include file='content/content.xml' />\n" + + " </content>\n" + + " <preprocess:include file='not_found.xml' required='false' />\n" + + "</services>\n"; private static final String jdiscXml = "<container id='stateless' version='1.0' />\n"; diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/TestPackageTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/TestPackageTest.java new file mode 100644 index 00000000000..8588bb9ea16 --- /dev/null +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/TestPackageTest.java @@ -0,0 +1,157 @@ +package com.yahoo.vespa.hosted.controller.application.pkg; + +import com.yahoo.config.application.api.DeploymentSpec; +import com.yahoo.config.provision.NodeResources; +import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.vespa.hosted.controller.application.pkg.TestPackage.TestSummary; +import com.yahoo.vespa.hosted.controller.config.ControllerConfig; +import org.junit.Test; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.List; +import java.util.Map; +import java.util.jar.JarOutputStream; +import java.util.zip.ZipEntry; + +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite.production; +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite.staging; +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite.staging_setup; +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Suite.system; +import static com.yahoo.vespa.hosted.controller.application.pkg.TestPackage.validateTests; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertEquals; + +/** + * @author jonmv + */ +public class TestPackageTest { + + static byte[] testsJar(String... suites) throws IOException { + String manifest = "Manifest-Version: 1.0\n" + + "Created-By: vespa container maven plugin\n" + + "Build-Jdk-Spec: 17\n" + + "Bundle-ManifestVersion: 2\n" + + "Bundle-SymbolicName: canary-application-test\n" + + "Bundle-Version: 1.0.1\n" + + "Bundle-Name: Test & verification application for Vespa\n" + + "X-JDisc-Test-Bundle-Version: 1.0\n" + + "Bundle-Vendor: Yahoo!\n" + + "Bundle-ClassPath: .,dependencies/fest-assert-1.4.jar,dependencies/fest-u\n" + + " til-1.1.6.jar\n" + + "Import-Package: ai.vespa.feed.client;version=\"[1.0.0,2)\",ai.vespa.hosted\n" + + " .cd;version=\"[1.0.0,2)\",com.yahoo.config;version=\"[1.0.0,2)\",com.yahoo.\n" + + " container.jdisc;version=\"[1.0.0,2)\",com.yahoo.jdisc.http;version=\"[1.0.\n" + + " 0,2)\",com.yahoo.slime;version=\"[1.0.0,2)\",java.awt.image;version=\"[0.0.\n" + + " 0,1)\",java.awt;version=\"[0.0.0,1)\",java.beans;version=\"[0.0.0,1)\",java.\n" + + " io;version=\"[0.0.0,1)\",java.lang.annotation;version=\"[0.0.0,1)\",java.la\n" + + " ng.reflect;version=\"[0.0.0,1)\",java.lang;version=\"[0.0.0,1)\",java.math;\n" + + " version=\"[0.0.0,1)\",java.net.http;version=\"[0.0.0,1)\",java.net;version=\n" + + " \"[0.0.0,1)\",java.nio.file;version=\"[0.0.0,1)\",java.security;version=\"[0\n" + + " .0.0,1)\",java.text;version=\"[0.0.0,1)\",java.time.temporal;version=\"[0.0\n" + + " .0,1)\",java.time;version=\"[0.0.0,1)\",java.util.concurrent;version=\"[0.0\n" + + " .0,1)\",java.util.function;version=\"[0.0.0,1)\",java.util.stream;version=\n" + + " \"[0.0.0,1)\",java.util;version=\"[0.0.0,1)\",javax.imageio;version=\"[0.0.0\n" + + " ,1)\",org.junit.jupiter.api;version=\"[5.8.1,6)\"\n" + + "X-JDisc-Test-Bundle-Categories: " + String.join(",", suites) + "\n" + + "\n"; + + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + try (JarOutputStream out = new JarOutputStream(buffer)) { + write("META-INF/MANIFEST.MF", manifest, out); + write("dependencies/foo.jar", "bar", out); + write("META-INF/maven/ai.vespa.test/app/pom.xml", "<project />", out); + write("ai/vespa/test/Test.class", "baz", out); + } + return buffer.toByteArray(); + } + + static void write(String name, String content, JarOutputStream out) throws IOException { + out.putNextEntry(new ZipEntry(name)); + out.write(content.getBytes(UTF_8)); + out.closeEntry(); + } + + @Test + public void testBundleValidation() throws IOException { + byte[] testZip = ApplicationPackage.filesZip(Map.of("components/foo-tests.jar", testsJar("SystemTest", "StagingSetup", "ProductionTest"), + "artifacts/key", new byte[0])); + TestSummary summary = validateTests(List.of(system), testZip); + + assertEquals(List.of(system, staging_setup, production), summary.suites()); + assertEquals(List.of("test package contains 'artifacts/key'; this conflicts with credentials used to run tests in Vespa Cloud", + "test package has staging setup, so it should also include staging tests", + "test package has production tests, but no production tests are declared in deployment.xml", + "see https://docs.vespa.ai/en/testing.html for details on how to write system tests for Vespa"), + summary.problems()); + } + + @Test + public void testFatTestsValidation() { + byte[] testZip = ApplicationPackage.filesZip(Map.of("artifacts/foo-tests.jar", new byte[0])); + TestSummary summary = validateTests(List.of(staging, production), testZip); + + assertEquals(List.of(staging, production), summary.suites()); + assertEquals(List.of("test package has staging tests, so it should also include staging setup", + "see https://docs.vespa.ai/en/testing.html for details on how to write system tests for Vespa"), + summary.problems()); + } + + @Test + public void testBasicTestsValidation() { + byte[] testZip = ApplicationPackage.filesZip(Map.of("tests/staging-test/foo.json", new byte[0], + "tests/staging-setup/foo.json", new byte[0])); + TestSummary summary = validateTests(List.of(system, production), testZip); + assertEquals(List.of(staging_setup, staging), summary.suites()); + assertEquals(List.of("test package has no system tests, but <test /> is declared in deployment.xml", + "test package has no production tests, but production tests are declared in deployment.xml", + "see https://docs.vespa.ai/en/testing.html for details on how to write system tests for Vespa"), + summary.problems()); + } + + @Test + public void generates_correct_tester_flavor() { + DeploymentSpec spec = DeploymentSpec.fromXml("<deployment version='1.0' athenz-domain='domain' athenz-service='service'>\n" + + " <instance id='first'>\n" + + " <test tester-flavor=\"d-6-16-100\" />\n" + + " <prod>\n" + + " <region active=\"true\">us-west-1</region>\n" + + " <test>us-west-1</test>\n" + + " </prod>\n" + + " </instance>\n" + + " <instance id='second'>\n" + + " <test />\n" + + " <staging />\n" + + " <prod tester-flavor=\"d-6-16-100\">\n" + + " <parallel>\n" + + " <region active=\"true\">us-east-3</region>\n" + + " <region active=\"true\">us-central-1</region>\n" + + " </parallel>\n" + + " <region active=\"true\">us-west-1</region>\n" + + " <test>us-west-1</test>\n" + + " </prod>\n" + + " </instance>\n" + + "</deployment>\n"); + + NodeResources firstResources = TestPackage.testerResourcesFor(ZoneId.from("prod", "us-west-1"), spec.requireInstance("first")); + assertEquals(TestPackage.DEFAULT_TESTER_RESOURCES, firstResources); + + NodeResources secondResources = TestPackage.testerResourcesFor(ZoneId.from("prod", "us-west-1"), spec.requireInstance("second")); + assertEquals(6, secondResources.vcpu(), 1e-9); + assertEquals(16, secondResources.memoryGb(), 1e-9); + assertEquals(100, secondResources.diskGb(), 1e-9); + } + + @Test + public void generates_correct_services_xml() throws IOException { + assertEquals(Files.readString(Paths.get("src/test/resources/test_runner_services.xml-cd")), + new String(TestPackage.servicesXml(true, + false, + new NodeResources(2, 12, 75, 1, NodeResources.DiskSpeed.fast, NodeResources.StorageType.local), + new ControllerConfig.Steprunner.Testerapp.Builder().build()), + UTF_8)); + } + +} diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java index 0dde6bd882f..031cdaa84ae 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunnerTest.java @@ -13,9 +13,6 @@ import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.slime.Inspector; import com.yahoo.slime.SlimeUtils; import com.yahoo.vespa.hosted.controller.ControllerTester; -import com.yahoo.vespa.hosted.controller.api.application.v4.model.configserverbindings.ConfigChangeActions; -import com.yahoo.vespa.hosted.controller.api.application.v4.model.configserverbindings.RestartAction; -import com.yahoo.vespa.hosted.controller.api.application.v4.model.configserverbindings.ServiceInfo; import com.yahoo.vespa.hosted.controller.api.integration.LogEntry; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServerException; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node; @@ -27,6 +24,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud. import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockMailer; import com.yahoo.vespa.hosted.controller.application.SystemApplication; import com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackage; +import com.yahoo.vespa.hosted.controller.application.pkg.TestPackage; import com.yahoo.vespa.hosted.controller.config.ControllerConfig; import com.yahoo.vespa.hosted.controller.integration.ZoneApiMock; import com.yahoo.vespa.hosted.controller.maintenance.JobRunner; @@ -35,6 +33,7 @@ import org.junit.Test; import java.io.IOException; import java.io.UncheckedIOException; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -47,8 +46,6 @@ import java.util.List; import java.util.Optional; import java.util.concurrent.Executors; import java.util.concurrent.Future; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Supplier; import static com.yahoo.vespa.hosted.controller.api.integration.LogEntry.Type.error; import static com.yahoo.vespa.hosted.controller.api.integration.LogEntry.Type.info; @@ -63,6 +60,7 @@ import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.success; import static com.yahoo.vespa.hosted.controller.deployment.Step.Status.failed; import static com.yahoo.vespa.hosted.controller.deployment.Step.Status.succeeded; import static com.yahoo.vespa.hosted.controller.deployment.Step.Status.unfinished; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.time.temporal.ChronoUnit.SECONDS; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -556,62 +554,4 @@ public class InternalStepRunnerTest { "3554970337.947820\t17491290-v6-1.ostk.bm2.prod.ne1.yahoo.com\t5480\tcontainer\tstdout\tinfo\tERROR: Bundle canary-application [71] Unable to get module class path. (java.lang.NullPointerException)\n" + "3554970337.947845\t17491290-v6-1.ostk.bm2.prod.ne1.yahoo.com\t5480\tcontainer\tstderr\twarning\tjava.lang.NullPointerException\\n\\tat org.apache.felix.framework.BundleRevisionImpl.calculateContentPath(BundleRevisionImpl.java:438)\\n\\tat org.apache.felix.framework.BundleRevisionImpl.initializeContentPath(BundleRevisionImpl.java:371)"; - @Test - public void generates_correct_tester_flavor() { - DeploymentSpec spec = DeploymentSpec.fromXml("<deployment version='1.0' athenz-domain='domain' athenz-service='service'>\n" + - " <instance id='first'>\n" + - " <test tester-flavor=\"d-6-16-100\" />\n" + - " <prod>\n" + - " <region active=\"true\">us-west-1</region>\n" + - " <test>us-west-1</test>\n" + - " </prod>\n" + - " </instance>\n" + - " <instance id='second'>\n" + - " <test />\n" + - " <staging />\n" + - " <prod tester-flavor=\"d-6-16-100\">\n" + - " <parallel>\n" + - " <region active=\"true\">us-east-3</region>\n" + - " <region active=\"true\">us-central-1</region>\n" + - " </parallel>\n" + - " <region active=\"true\">us-west-1</region>\n" + - " <test>us-west-1</test>\n" + - " </prod>\n" + - " </instance>\n" + - "</deployment>\n"); - - NodeResources firstResources = InternalStepRunner.testerResourcesFor(ZoneId.from("prod", "us-west-1"), spec.requireInstance("first")); - assertEquals(InternalStepRunner.DEFAULT_TESTER_RESOURCES, firstResources); - - NodeResources secondResources = InternalStepRunner.testerResourcesFor(ZoneId.from("prod", "us-west-1"), spec.requireInstance("second")); - assertEquals(6, secondResources.vcpu(), 1e-9); - assertEquals(16, secondResources.memoryGb(), 1e-9); - assertEquals(100, secondResources.diskGb(), 1e-9); - } - - @Test - public void generates_correct_services_xml() { - generates_correct_services_xml("test_runner_services.xml-cd"); - } - - private void generates_correct_services_xml(String filenameExpectedOutput) { - ControllerConfig.Steprunner.Testerapp config = new ControllerConfig.Steprunner.Testerapp.Builder().build(); - assertFile(filenameExpectedOutput, - new String(InternalStepRunner.servicesXml( - true, - false, - new NodeResources(2, 12, 75, 1, NodeResources.DiskSpeed.fast, NodeResources.StorageType.local), - config))); - } - - private void assertFile(String resourceName, String actualContent) { - try { - Path path = Paths.get("src/test/resources/").resolve(resourceName); - String expectedContent = new String(Files.readAllBytes(path)); - assertEquals(expectedContent, actualContent); - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } - } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java index a4b17239626..5b8e25cbfe8 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java @@ -3,6 +3,7 @@ package com.yahoo.vespa.hosted.controller.integration; import com.yahoo.component.AbstractComponent; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.ApplicationName; import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; @@ -200,6 +201,11 @@ public class ZoneRegistryMock extends AbstractComponent implements ZoneRegistry } @Override + public URI dashboardUrl(TenantName tenantName, ApplicationName applicationName) { + return URI.create("https://dashboard.tld/" + tenantName + "/" + applicationName); + } + + @Override public URI dashboardUrl(TenantName tenantName) { return URI.create("https://dashboard.tld/" + tenantName); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/NotificationsSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/NotificationsSerializerTest.java index 1b82f622773..370b1cbe02c 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/NotificationsSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/NotificationsSerializerTest.java @@ -16,7 +16,6 @@ import java.io.IOException; import java.time.Instant; import java.util.List; -import static com.yahoo.config.provision.SystemName.main; import static org.junit.Assert.assertEquals; /** diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java index ac8bdafa2bd..d2698afdc48 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java @@ -8,10 +8,13 @@ import com.yahoo.slime.SlimeUtils; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServerException; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RevisionId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.TestReport; +import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId; import com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackage; import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder; import com.yahoo.vespa.hosted.controller.deployment.DeploymentContext; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; +import com.yahoo.vespa.hosted.controller.notification.Notification.Type; +import com.yahoo.vespa.hosted.controller.notification.NotificationSource; import org.junit.Test; import java.io.ByteArrayOutputStream; 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 b458e1fa662..2de58660b0c 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -224,7 +224,7 @@ public class Flags { public static final UnboundStringFlag JDK_VERSION = defineStringFlag( "jdk-version", "11", - List.of("hmusum"), "2021-10-25", "2022-05-01", + List.of("hmusum"), "2021-10-25", "2022-05-15", "JDK version to use on host and inside containers. Note application-id dimension only applies for container, " + "while hostname and node type applies for host.", "Takes effect on restart for Docker container and on next host-admin tick for host", @@ -262,8 +262,8 @@ public class Flags { ZONE_ID, APPLICATION_ID); public static final UnboundBooleanFlag FAIL_DEPLOYMENT_WITH_INVALID_JVM_OPTIONS = defineFeatureFlag( - "fail-deployment-with-invalid-jvm-options", false, - List.of("hmusum"), "2021-12-20", "2022-05-01", + "fail-deployment-with-invalid-jvm-options", true, + List.of("hmusum"), "2021-12-20", "2022-05-15", "Whether to fail deployments with invalid JVM options in services.xml", "Takes effect at redeployment", ZONE_ID, APPLICATION_ID); @@ -385,7 +385,7 @@ public class Flags { public static final UnboundBooleanFlag USE_ZSTD_IN_FILE_DISTRIBUTION = defineFeatureFlag( "use-zstd-in-file-distribution", false, - List.of("hmusum"), "2022-04-07", "2022-05-07", + List.of("hmusum"), "2022-04-07", "2022-06-01", "Whether to use zstd compression for data sent with file distribution", "Takes effect immediately", ZONE_ID, APPLICATION_ID); diff --git a/integration/intellij/build.gradle b/integration/intellij/build.gradle index 6bc385a983c..1c2dae46d87 100644 --- a/integration/intellij/build.gradle +++ b/integration/intellij/build.gradle @@ -36,7 +36,7 @@ compileJava { } group 'ai.vespa' -version '1.1.4' // Also update pom.xml version if this is changed +version '1.1.5' // Also update pom.xml version if this is changed sourceCompatibility = 11 diff --git a/integration/intellij/pom.xml b/integration/intellij/pom.xml index 07488ed3d93..1c973b84d37 100644 --- a/integration/intellij/pom.xml +++ b/integration/intellij/pom.xml @@ -9,7 +9,7 @@ <relativePath>../parent/pom.xml</relativePath> </parent> <artifactId>vespa-intellij</artifactId> <!-- Not used - plugin is build by gradle --> - <version>1.1.4</version> <!-- See copy-zip below, which depends on this being the same as the v. in build.gradle --> + <version>1.1.5</version> <!-- See copy-zip below, which depends on this being the same as the v. in build.gradle --> <description> Maven wrapper for the gradle build of this IntelliJ plugin. </description> diff --git a/integration/intellij/src/main/bnf/ai/vespa/intellij/schema/parser/sd.bnf b/integration/intellij/src/main/bnf/ai/vespa/intellij/schema/parser/sd.bnf index cb4bc024c86..b0e0e5e61dc 100644 --- a/integration/intellij/src/main/bnf/ai/vespa/intellij/schema/parser/sd.bnf +++ b/integration/intellij/src/main/bnf/ai/vespa/intellij/schema/parser/sd.bnf @@ -111,7 +111,7 @@ AnnotationFieldDefinition ::= field IdentifierVal type FieldTypeName '{' '}' // The *Expr alternatives are consumed greedily so order matters. //------------------------- RankingExpression ::= LiteralTensorExpr | FilePathExpr | ParenthesisedExpr | BooleanExpr | ArithmeticExpr | IfFunctionExpr | - QueryDefinitionExpr | FunctionCallExpr | InListRankingExpr | PrimitiveExpr | SliceExpr | LambdaExpr + QueryDefinitionExpr | FunctionCallOrLambdaExpr | InListRankingExpr | PrimitiveExpr | SliceExpr FilePathExpr ::= file ':' (FilePath | WordWrapper) @@ -129,7 +129,9 @@ ArithmeticOperator ::= '+' | '-' | '*' | '/' | '%' | '^' | "||" | "&&" QueryDefinitionExpr ::= QueryDefinition | ItemRawScoreDefinition -FunctionCallExpr ::= IdentifierWithDashVal '(' RankingExpression (COMMA RankingExpression)* ')' ('.' IdentifierWithDashVal)? +// Rough parsing but hard to do better due to greediness: If this is a lambda arg expressions must be identifiers +FunctionCallOrLambdaExpr ::= IdentifierWithDashVal '(' RankingExpression (COMMA RankingExpression)* ')' ('.' IdentifierWithDashVal)? + ParenthesisedExpr? // This turns the function call into a lambda ParenthesisedExpr ::= '(' RankingExpression ')' @@ -152,7 +154,7 @@ TensorValue ::= CellAddress ':' RankingExpression CellAddress ::= Label | FullTensorAddress -LambdaExp ::= FunctionCallExpr ParenthesisedExpr +LambdaExpr ::= IdentifierWithDashVal '(' IdentifierVal (COMMA IdentifierVal)* ')' ParenthesisedExpr //------------------------- //-- Rank Profile rules --- diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/RegistryCredentials.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/RegistryCredentials.java index 1ac61cd6c1f..29934e3d1aa 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/RegistryCredentials.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/RegistryCredentials.java @@ -10,16 +10,14 @@ import java.util.Objects; */ public class RegistryCredentials { - public static final RegistryCredentials none = new RegistryCredentials("", "", ""); + public static final RegistryCredentials none = new RegistryCredentials("", ""); private final String username; private final String password; - private final String registryAddress; - public RegistryCredentials(String username, String password, String registryAddress) { + public RegistryCredentials(String username, String password) { this.username = Objects.requireNonNull(username); this.password = Objects.requireNonNull(password); - this.registryAddress = Objects.requireNonNull(registryAddress); } public String username() { @@ -30,28 +28,23 @@ public class RegistryCredentials { return password; } - public String registryAddress() { - return registryAddress; - } - @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; RegistryCredentials that = (RegistryCredentials) o; return username.equals(that.username) && - password.equals(that.password) && - registryAddress.equals(that.registryAddress); + password.equals(that.password); } @Override public int hashCode() { - return Objects.hash(username, password, registryAddress); + return Objects.hash(username, password); } @Override public String toString() { - return "registry credentials for " + registryAddress + " [username=" + username + ",password=" + password + "]"; + return "registry credentials [username=" + username + ",password=<hidden>]"; } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java index 75977da369c..61e777a9576 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java @@ -27,9 +27,6 @@ import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer; import com.yahoo.vespa.hosted.node.admin.maintenance.identity.CredentialsMaintainer; import com.yahoo.vespa.hosted.node.admin.maintenance.servicedump.VespaServiceDumper; import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException; -import com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder; -import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath; -import com.yahoo.vespa.hosted.node.admin.task.util.fs.ContainerPath; import java.time.Clock; import java.time.Duration; @@ -140,31 +137,6 @@ public class NodeAgentImpl implements NodeAgent { if (loopThread != null) throw new IllegalStateException("Can not re-start a node agent."); - // TODO: Remove after this has rolled out everywhere - int[] stats = new int[]{0, 0, 0}; - ContainerPath vespaHome = initialContext.paths().underVespaHome(""); - FileFinder.files(initialContext.paths().of("/")).forEachPath(path -> { - UnixPath unixPath = new UnixPath(path); - - String permissions = unixPath.getPermissions(); - if (!permissions.endsWith("---")) { - unixPath.setPermissions(permissions.substring(0, 6) + "---"); - stats[0]++; - } - - if (path.startsWith(vespaHome) && unixPath.getOwnerId() != initialContext.users().vespa().uid()) { - unixPath.setOwnerId(initialContext.users().vespa().uid()); - stats[1]++; - } - - if (path.startsWith(vespaHome) && unixPath.getGroupId() != initialContext.users().vespa().gid()) { - unixPath.setGroupId(initialContext.users().vespa().gid()); - stats[2]++; - } - }); - if (stats[0] + stats[1] + stats[2] > 0) - initialContext.log(logger, "chmod %d, chown UID %d, chown GID %d files", stats[0], stats[1], stats[2]); - loopThread = new Thread(() -> { while (!terminated.get()) { try { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ApplicationPatcher.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ApplicationPatcher.java index cb3c6c0f5f4..7b5146955fb 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ApplicationPatcher.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ApplicationPatcher.java @@ -13,6 +13,7 @@ import com.yahoo.vespa.hosted.provision.applications.Application; import java.io.IOException; import java.io.InputStream; import java.io.UncheckedIOException; +import java.time.Duration; /** * A class which can take a partial JSON node/v2 application JSON structure and apply it to an application object. @@ -33,7 +34,8 @@ public class ApplicationPatcher implements AutoCloseable { } catch (IOException e) { throw new UncheckedIOException("Error reading request body", e); } - this.lock = nodeRepository.nodes().lock(applicationId); + // Use same timeout for acquiring lock as client timeout for patch request + this.lock = nodeRepository.nodes().lock(applicationId, Duration.ofSeconds(30)); try { this.application = nodeRepository.applications().require(applicationId); } @@ -47,7 +49,7 @@ public class ApplicationPatcher implements AutoCloseable { public Application apply() { inspector.traverse((String name, Inspector value) -> { try { - application = applyField(application, name, value, inspector); + application = applyField(application, name, value); } catch (IllegalArgumentException e) { throw new IllegalArgumentException("Could not set field '" + name + "'", e); } @@ -65,7 +67,7 @@ public class ApplicationPatcher implements AutoCloseable { lock.close(); } - private Application applyField(Application application, String name, Inspector value, Inspector root) { + private Application applyField(Application application, String name, Inspector value) { switch (name) { case "currentReadShare" : return application.with(application.status().withCurrentReadShare(asDouble(value))); diff --git a/searchcore/src/tests/proton/matching/query_test.cpp b/searchcore/src/tests/proton/matching/query_test.cpp index 7f0232120e7..1c31cb1d1ad 100644 --- a/searchcore/src/tests/proton/matching/query_test.cpp +++ b/searchcore/src/tests/proton/matching/query_test.cpp @@ -1138,19 +1138,19 @@ Test::global_filter_is_calculated_and_handled() uint32_t docid_limit = 10; { // global filter is not wanted GlobalFilterBlueprint bp(result, false); - auto res = Query::handle_global_filter(bp, docid_limit, 0, 1); + auto res = Query::handle_global_filter(bp, docid_limit, 0, 1, nullptr); EXPECT_FALSE(res); EXPECT_FALSE(bp.filter); } { // estimated_hit_ratio < global_filter_lower_limit GlobalFilterBlueprint bp(result, true); - auto res = Query::handle_global_filter(bp, docid_limit, 0.31, 1); + auto res = Query::handle_global_filter(bp, docid_limit, 0.31, 1, nullptr); EXPECT_FALSE(res); EXPECT_FALSE(bp.filter); } { // estimated_hit_ratio <= global_filter_upper_limit GlobalFilterBlueprint bp(result, true); - auto res = Query::handle_global_filter(bp, docid_limit, 0, 0.3); + auto res = Query::handle_global_filter(bp, docid_limit, 0, 0.3, nullptr); EXPECT_TRUE(res); EXPECT_TRUE(bp.filter); EXPECT_TRUE(bp.filter->has_filter()); @@ -1163,7 +1163,7 @@ Test::global_filter_is_calculated_and_handled() } { // estimated_hit_ratio > global_filter_upper_limit GlobalFilterBlueprint bp(result, true); - auto res = Query::handle_global_filter(bp, docid_limit, 0, 0.29); + auto res = Query::handle_global_filter(bp, docid_limit, 0, 0.29, nullptr); EXPECT_TRUE(res); EXPECT_TRUE(bp.filter); EXPECT_FALSE(bp.filter->has_filter()); diff --git a/searchcore/src/tests/proton/server/disk_mem_usage_filter/disk_mem_usage_filter_test.cpp b/searchcore/src/tests/proton/server/disk_mem_usage_filter/disk_mem_usage_filter_test.cpp index db32c1e77c4..dd38765c4f0 100644 --- a/searchcore/src/tests/proton/server/disk_mem_usage_filter/disk_mem_usage_filter_test.cpp +++ b/searchcore/src/tests/proton/server/disk_mem_usage_filter/disk_mem_usage_filter_test.cpp @@ -18,12 +18,7 @@ struct DiskMemUsageFilterTest : public ::testing::Test DiskMemUsageFilterTest() : _filter(HwInfo(HwInfo::Disk(100, false, false), HwInfo::Memory(1000), HwInfo::Cpu(0))) { - _filter.setDiskUsedSize(20); - _filter.setMemoryStats(vespalib::ProcessMemoryStats(297, - 298, - 299, - 300, - 42)); + _filter.set_resource_usage(TransientResourceUsage(), vespalib::ProcessMemoryStats(297, 298, 299, 300, 42), 20); } void testWrite(const vespalib::string &exp) { @@ -41,16 +36,12 @@ struct DiskMemUsageFilterTest : public ::testing::Test } void triggerDiskLimit() { - _filter.setDiskUsedSize(90); + _filter.set_resource_usage(_filter.get_transient_resource_usage(), _filter.getMemoryStats(), 90); } void triggerMemoryLimit() { - _filter.setMemoryStats(vespalib::ProcessMemoryStats(897, - 898, - 899, - 900, - 43)); + _filter.set_resource_usage(TransientResourceUsage(), vespalib::ProcessMemoryStats(897, 898, 899, 900, 43), _filter.getDiskUsedSize()); } }; @@ -123,7 +114,7 @@ TEST_F(DiskMemUsageFilterTest, both_disk_limit_and_memory_limit_can_be_reached) TEST_F(DiskMemUsageFilterTest, transient_and_non_transient_disk_usage_tracked_in_usage_state_and_metrics) { - _filter.set_transient_resource_usage({15, 0}); + _filter.set_resource_usage({15, 0}, _filter.getMemoryStats(), _filter.getDiskUsedSize()); EXPECT_DOUBLE_EQ(0.15, _filter.usageState().transient_disk_usage()); EXPECT_DOUBLE_EQ(0.15, _filter.get_metrics().transient_disk_usage()); EXPECT_DOUBLE_EQ(0.05, _filter.usageState().non_transient_disk_usage()); @@ -132,7 +123,7 @@ TEST_F(DiskMemUsageFilterTest, transient_and_non_transient_disk_usage_tracked_in TEST_F(DiskMemUsageFilterTest, transient_and_non_transient_memory_usage_tracked_in_usage_state_and_metrics) { - _filter.set_transient_resource_usage({0, 100}); + _filter.set_resource_usage({0, 100}, _filter.getMemoryStats(), _filter.getDiskUsedSize()); EXPECT_DOUBLE_EQ(0.1, _filter.usageState().transient_memory_usage()); EXPECT_DOUBLE_EQ(0.1, _filter.get_metrics().transient_memory_usage()); EXPECT_DOUBLE_EQ(0.2, _filter.usageState().non_transient_memory_usage()); diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_master.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_master.cpp index 3ff0a7d1808..624922eb27b 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_master.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/match_master.cpp @@ -109,7 +109,7 @@ MatchMaster::match(search::engine::Trace & trace, double match_time_s = 0.0; std::unique_ptr<vespalib::slime::Inserter> inserter; if (trace.shouldTrace(4)) { - inserter = std::make_unique<vespalib::slime::ArrayInserter>(trace.createCursor("match_threads").setArray("threads")); + inserter = std::make_unique<vespalib::slime::ArrayInserter>(trace.createCursor("query_execution").setArray("threads")); } for (size_t i = 0; i < threadState.size(); ++i) { const MatchThread & matchThread = *threadState[i]; diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp index 37635825295..3d8d56f0150 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp @@ -3,12 +3,15 @@ #include "match_tools.h" #include "querynodes.h" #include <vespa/searchcorespi/index/indexsearchable.h> +#include <vespa/searchlib/attribute/attribute_blueprint_params.h> +#include <vespa/searchlib/attribute/attribute_operation.h> +#include <vespa/searchlib/attribute/diversity.h> +#include <vespa/searchlib/engine/trace.h> #include <vespa/searchlib/fef/indexproperties.h> #include <vespa/searchlib/fef/ranksetup.h> -#include <vespa/searchlib/engine/trace.h> -#include <vespa/searchlib/attribute/diversity.h> -#include <vespa/searchlib/attribute/attribute_operation.h> -#include <vespa/searchlib/attribute/attribute_blueprint_params.h> +#include <vespa/vespalib/data/slime/cursor.h> +#include <vespa/vespalib/data/slime/inject.h> +#include <vespa/vespalib/data/slime/inserter.h> #include <vespa/vespalib/util/issue.h> #include <vespa/log/log.h> @@ -167,7 +170,7 @@ MatchToolsFactory(QueryLimiter & queryLimiter, const vespalib::Doom & doom, ISearchContext & searchContext, IAttributeContext & attributeContext, - search::engine::Trace & trace, + search::engine::Trace & root_trace, vespalib::stringref queryStack, const vespalib::string & location, const ViewResolver & viewResolver, @@ -188,34 +191,35 @@ MatchToolsFactory(QueryLimiter & queryLimiter, _diversityParams(), _valid(false) { - trace.addEvent(4, "MTF: Start"); + search::engine::Trace trace(root_trace.getRelativeTime(), root_trace.getLevel()); + trace.addEvent(4, "Start query setup"); _query.setWhiteListBlueprint(metaStore.createWhiteListBlueprint()); - trace.addEvent(5, "MTF: Build query"); + trace.addEvent(5, "Deserialize and build query tree"); _valid = _query.buildTree(queryStack, location, viewResolver, indexEnv, rankSetup.split_unpacking_iterators(), rankSetup.delay_unpacking_iterators()); if (_valid) { _query.extractTerms(_queryEnv.terms()); _query.extractLocations(_queryEnv.locations()); - trace.addEvent(5, "MTF: reserve handles"); + trace.addEvent(5, "Build query execution plan"); _query.reserveHandles(_requestContext, searchContext, _mdl); + trace.addEvent(5, "Optimize query execution plan"); _query.optimize(); - trace.addEvent(4, "MTF: Fetch Postings"); + trace.addEvent(4, "Perform dictionary lookups and posting lists initialization"); _query.fetchPostings(); if (is_search) { - trace.addEvent(5, "MTF: Handle Global Filters"); double lower_limit = GlobalFilterLowerLimit::lookup(rankProperties, rankSetup.get_global_filter_lower_limit()); double upper_limit = GlobalFilterUpperLimit::lookup(rankProperties, rankSetup.get_global_filter_upper_limit()); - _query.handle_global_filter(searchContext.getDocIdLimit(), lower_limit, upper_limit); + _query.handle_global_filter(searchContext.getDocIdLimit(), lower_limit, upper_limit, trace); } _query.freeze(); - trace.addEvent(5, "MTF: prepareSharedState"); + trace.addEvent(5, "Prepare shared state for multi-threaded rank executors"); _rankSetup.prepareSharedState(_queryEnv, _queryEnv.getObjectStore()); _diversityParams = extractDiversityParams(_rankSetup, rankProperties); DegradationParams degradationParams = extractDegradationParams(_rankSetup, rankProperties); if (degradationParams.enabled()) { - trace.addEvent(5, "MTF: Build MatchPhaseLimiter"); + trace.addEvent(5, "Setup match phase limiter"); _match_limiter = std::make_unique<MatchPhaseLimiter>(metaStore.getCommittedDocIdLimit(), searchContext.getAttributes(), _requestContext, degradationParams, _diversityParams); } @@ -223,7 +227,11 @@ MatchToolsFactory(QueryLimiter & queryLimiter, if ( ! _match_limiter) { _match_limiter = std::make_unique<NoMatchPhaseLimiter>(); } - trace.addEvent(4, "MTF: Complete"); + trace.addEvent(4, "Complete query setup"); + if (root_trace.shouldTrace(4)) { + vespalib::slime::ObjectInserter inserter(root_trace.createCursor("query_setup"), "traces"); + vespalib::slime::inject(trace.getTraces(), inserter); + } } MatchToolsFactory::~MatchToolsFactory() = default; diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_tools.h b/searchcore/src/vespa/searchcore/proton/matching/match_tools.h index 4b5f9cf76cc..a7d39a0c3e8 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_tools.h +++ b/searchcore/src/vespa/searchcore/proton/matching/match_tools.h @@ -114,7 +114,7 @@ public: const vespalib::Doom & softDoom, ISearchContext &searchContext, search::attribute::IAttributeContext &attributeContext, - search::engine::Trace & trace, + search::engine::Trace & root_trace, vespalib::stringref queryStack, const vespalib::string &location, const ViewResolver &viewResolver, diff --git a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp index e945bbb850b..756af216988 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp @@ -176,7 +176,7 @@ namespace { void traceQuery(uint32_t traceLevel, Trace & trace, const Query & query) { if (traceLevel <= trace.getLevel()) { if (query.peekRoot()) { - vespalib::slime::ObjectInserter inserter(trace.createCursor("blueprint"), "optimized"); + vespalib::slime::ObjectInserter inserter(trace.createCursor("query_execution_plan"), "optimized"); query.peekRoot()->asSlime(inserter); } } diff --git a/searchcore/src/vespa/searchcore/proton/matching/query.cpp b/searchcore/src/vespa/searchcore/proton/matching/query.cpp index 95fe846a088..84671ec72c7 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/query.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/query.cpp @@ -1,15 +1,16 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include "query.h" #include "blueprintbuilder.h" #include "matchdatareservevisitor.h" +#include "query.h" #include "resolveviewvisitor.h" -#include "termdataextractor.h" #include "sameelementmodifier.h" +#include "termdataextractor.h" #include "unpacking_iterators_optimizer.h" #include <vespa/document/datatype/positiondatatype.h> -#include <vespa/searchlib/common/geo_location_spec.h> #include <vespa/searchlib/common/geo_location_parser.h> +#include <vespa/searchlib/common/geo_location_spec.h> +#include <vespa/searchlib/engine/trace.h> #include <vespa/searchlib/parsequery/stackdumpiterator.h> #include <vespa/searchlib/queryeval/intermediate_blueprints.h> #include <vespa/vespalib/util/issue.h> @@ -27,19 +28,19 @@ using search::fef::IIndexEnvironment; using search::fef::ITermData; using search::fef::MatchData; using search::fef::MatchDataLayout; +using search::query::LocationTerm; using search::query::Node; using search::query::QueryTreeCreator; using search::query::Weight; using search::queryeval::AndBlueprint; using search::queryeval::AndNotBlueprint; -using search::queryeval::RankBlueprint; -using search::queryeval::IntermediateBlueprint; using search::queryeval::Blueprint; using search::queryeval::IRequestContext; +using search::queryeval::IntermediateBlueprint; +using search::queryeval::RankBlueprint; using search::queryeval::SearchIterator; -using search::query::LocationTerm; -using vespalib::string; using vespalib::Issue; +using vespalib::string; using std::vector; namespace proton::matching { @@ -244,12 +245,14 @@ Query::fetchPostings() } void -Query::handle_global_filter(uint32_t docid_limit, double global_filter_lower_limit, double global_filter_upper_limit) +Query::handle_global_filter(uint32_t docid_limit, double global_filter_lower_limit, double global_filter_upper_limit, + search::engine::Trace& trace) { - if (!handle_global_filter(*_blueprint, docid_limit, global_filter_lower_limit, global_filter_upper_limit)) { + if (!handle_global_filter(*_blueprint, docid_limit, global_filter_lower_limit, global_filter_upper_limit, &trace)) { return; } // optimized order may change after accounting for global filter: + trace.addEvent(5, "Optimize query execution plan to account for global filter"); _blueprint = Blueprint::optimize(std::move(_blueprint)); LOG(debug, "blueprint after handle_global_filter:\n%s\n", _blueprint->asString().c_str()); // strictness may change if optimized order changed: @@ -257,7 +260,9 @@ Query::handle_global_filter(uint32_t docid_limit, double global_filter_lower_lim } bool -Query::handle_global_filter(Blueprint& blueprint, uint32_t docid_limit, double global_filter_lower_limit, double global_filter_upper_limit) +Query::handle_global_filter(Blueprint& blueprint, uint32_t docid_limit, + double global_filter_lower_limit, double global_filter_upper_limit, + search::engine::Trace* trace) { using search::queryeval::GlobalFilter; double estimated_hit_ratio = blueprint.getState().hit_ratio(docid_limit); @@ -265,24 +270,37 @@ Query::handle_global_filter(Blueprint& blueprint, uint32_t docid_limit, double g return false; } - LOG(debug, "docid_limit=%d, estimated_hit_ratio=%1.2f, global_filter_lower_limit=%1.2f, global_filter_upper_limit=%1.2f", - docid_limit, estimated_hit_ratio, global_filter_lower_limit, global_filter_upper_limit); if (estimated_hit_ratio < global_filter_lower_limit) { + if (trace && trace->shouldTrace(5)) { + trace->addEvent(5, vespalib::make_string("Skip calculate global filter (estimated_hit_ratio (%f) < lower_limit (%f))", + estimated_hit_ratio, global_filter_lower_limit)); + } return false; } + std::shared_ptr<GlobalFilter> global_filter; if (estimated_hit_ratio <= global_filter_upper_limit) { + if (trace && trace->shouldTrace(5)) { + trace->addEvent(5, vespalib::make_string("Calculate global filter (estimated_hit_ratio (%f) <= upper_limit (%f))", + estimated_hit_ratio, global_filter_upper_limit)); + } auto constraint = Blueprint::FilterConstraint::UPPER_BOUND; bool strict = true; auto filter_iterator = blueprint.createFilterSearch(strict, constraint); filter_iterator->initRange(1, docid_limit); auto white_list = filter_iterator->get_hits(1); - auto global_filter = GlobalFilter::create(std::move(white_list)); - blueprint.set_global_filter(*global_filter); + global_filter = GlobalFilter::create(std::move(white_list)); } else { - auto no_filter = GlobalFilter::create(); - blueprint.set_global_filter(*no_filter); + if (trace && trace->shouldTrace(5)) { + trace->addEvent(5, vespalib::make_string("Create match all global filter (estimated_hit_ratio (%f) > upper_limit (%f))", + estimated_hit_ratio, global_filter_upper_limit)); + } + global_filter = GlobalFilter::create(); + } + if (trace) { + trace->addEvent(5, "Handle global filter in query execution plan"); } + blueprint.set_global_filter(*global_filter); return true; } diff --git a/searchcore/src/vespa/searchcore/proton/matching/query.h b/searchcore/src/vespa/searchcore/proton/matching/query.h index 29bca310502..09eaed5c4a9 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/query.h +++ b/searchcore/src/vespa/searchcore/proton/matching/query.h @@ -10,6 +10,8 @@ #include <vespa/searchlib/queryeval/blueprint.h> #include <vespa/searchlib/queryeval/irequestcontext.h> +namespace search::engine { class Trace; } + namespace proton::matching { class ViewResolver; @@ -93,7 +95,8 @@ public: void optimize(); void fetchPostings(); - void handle_global_filter(uint32_t docid_limit, double global_filter_lower_limit, double global_filter_upper_limit); + void handle_global_filter(uint32_t docid_limit, double global_filter_lower_limit, double global_filter_upper_limit, + search::engine::Trace& trace); /** * Calculates and handles the global filter if needed by the blueprint tree. @@ -109,7 +112,8 @@ public: * @return whether the global filter was set on the blueprint. */ static bool handle_global_filter(Blueprint& blueprint, uint32_t docid_limit, - double global_filter_lower_limit, double global_filter_upper_limit); + double global_filter_lower_limit, double global_filter_upper_limit, + search::engine::Trace* trace); void freeze(); diff --git a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.cpp b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.cpp index 8928045b814..54eea6565cb 100644 --- a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.cpp @@ -180,26 +180,11 @@ DiskMemUsageFilter::DiskMemUsageFilter(const HwInfo &hwInfo) DiskMemUsageFilter::~DiskMemUsageFilter() = default; void -DiskMemUsageFilter::setMemoryStats(vespalib::ProcessMemoryStats memoryStats_in) -{ - Guard guard(_lock); - _memoryStats = memoryStats_in; - recalcState(guard); -} - -void -DiskMemUsageFilter::setDiskUsedSize(uint64_t diskUsedSizeBytes) -{ - Guard guard(_lock); - _diskUsedSizeBytes = diskUsedSizeBytes; - recalcState(guard); -} - -void -DiskMemUsageFilter::set_transient_resource_usage(const TransientResourceUsage& transient_usage) -{ +DiskMemUsageFilter::set_resource_usage(const TransientResourceUsage& transient_usage, vespalib::ProcessMemoryStats memoryStats, uint64_t diskUsedSizeBytes) { Guard guard(_lock); _transient_usage = transient_usage; + _memoryStats = memoryStats; + _diskUsedSizeBytes = diskUsedSizeBytes; recalcState(guard); } diff --git a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.h b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.h index ac328c3ddea..16e4d1d6869 100644 --- a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.h +++ b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.h @@ -65,9 +65,7 @@ private: public: DiskMemUsageFilter(const HwInfo &hwInfo); ~DiskMemUsageFilter() override; - void setMemoryStats(vespalib::ProcessMemoryStats memoryStats_in); - void setDiskUsedSize(uint64_t diskUsedSizeBytes); - void set_transient_resource_usage(const TransientResourceUsage& transient_usage); + void set_resource_usage(const TransientResourceUsage& transient_usage, vespalib::ProcessMemoryStats memoryStats, uint64_t diskUsedSizeBytes); void setConfig(Config config); vespalib::ProcessMemoryStats getMemoryStats() const; uint64_t getDiskUsedSize() const; diff --git a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_sampler.cpp b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_sampler.cpp index 7054c7077c8..6030fb3cceb 100644 --- a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_sampler.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_sampler.cpp @@ -33,25 +33,32 @@ DiskMemUsageSampler::setConfig(const Config &config) _periodicTimer->reset(); _filter.setConfig(config.filterConfig); _sampleInterval = config.sampleInterval; - sampleUsage(); + sampleAndReportUsage(); _lastSampleTime = vespalib::steady_clock::now(); vespalib::duration maxInterval = std::min(vespalib::duration(1s), _sampleInterval); _periodicTimer->scheduleAtFixedRate(makeLambdaTask([this]() { if (_filter.acceptWriteOperation() && (vespalib::steady_clock::now() < (_lastSampleTime + _sampleInterval))) { return; } - sampleUsage(); + sampleAndReportUsage(); _lastSampleTime = vespalib::steady_clock::now(); }), maxInterval, maxInterval); } void -DiskMemUsageSampler::sampleUsage() +DiskMemUsageSampler::sampleAndReportUsage() { - sampleMemoryUsage(); - sampleDiskUsage(); - sample_transient_resource_usage(); + TransientResourceUsage transientUsage = sample_transient_resource_usage(); + /* It is important that transient resource usage is sampled first. This prevents + * a false positive where we report a too high disk or memory usage causing + * either feed blocked, or an alert due to metric spike. + * A false negative is less of a problem, as it will only be a short drop in the metric, + * and a short period of allowed feed. The latter will be very rare as you are rarely feed blocked anyway. + */ + vespalib::ProcessMemoryStats memoryStats = sampleMemoryUsage(); + uint64_t diskUsage = sampleDiskUsage(); + _filter.set_resource_usage(transientUsage, memoryStats, diskUsage); } namespace { @@ -106,22 +113,22 @@ sampleDiskUsageInDirectory(const fs::path &path) } -void +uint64_t DiskMemUsageSampler::sampleDiskUsage() { const auto &disk = _filter.getHwInfo().disk(); - _filter.setDiskUsedSize(disk.shared() ? - sampleDiskUsageInDirectory(_path) : - sampleDiskUsageOnFileSystem(_path, disk)); + return disk.shared() + ? sampleDiskUsageInDirectory(_path) + : sampleDiskUsageOnFileSystem(_path, disk); } -void +vespalib::ProcessMemoryStats DiskMemUsageSampler::sampleMemoryUsage() { - _filter.setMemoryStats(vespalib::ProcessMemoryStats::create()); + return vespalib::ProcessMemoryStats::create(); } -void +TransientResourceUsage DiskMemUsageSampler::sample_transient_resource_usage() { TransientResourceUsage transient_usage; @@ -131,7 +138,7 @@ DiskMemUsageSampler::sample_transient_resource_usage() transient_usage.merge(provider->get_transient_resource_usage()); } } - _filter.set_transient_resource_usage(transient_usage); + return transient_usage; } void diff --git a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_sampler.h b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_sampler.h index 7475282e718..fa8ac48fa1f 100644 --- a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_sampler.h +++ b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_sampler.h @@ -17,18 +17,18 @@ class ITransientResourceUsageProvider; * Class to sample disk and memory usage used for filtering write operations. */ class DiskMemUsageSampler { - DiskMemUsageFilter _filter; - std::filesystem::path _path; - vespalib::duration _sampleInterval; - vespalib::steady_time _lastSampleTime; + DiskMemUsageFilter _filter; + std::filesystem::path _path; + vespalib::duration _sampleInterval; + vespalib::steady_time _lastSampleTime; std::unique_ptr<vespalib::ScheduledExecutor> _periodicTimer; std::mutex _lock; std::vector<std::shared_ptr<const ITransientResourceUsageProvider>> _transient_usage_providers; - void sampleUsage(); - void sampleDiskUsage(); - void sampleMemoryUsage(); - void sample_transient_resource_usage(); + void sampleAndReportUsage(); + uint64_t sampleDiskUsage(); + vespalib::ProcessMemoryStats sampleMemoryUsage(); + TransientResourceUsage sample_transient_resource_usage(); public: struct Config { DiskMemUsageFilter::Config filterConfig; diff --git a/searchlib/src/vespa/searchlib/engine/trace.h b/searchlib/src/vespa/searchlib/engine/trace.h index 1940af5cf38..709dc05d93c 100644 --- a/searchlib/src/vespa/searchlib/engine/trace.h +++ b/searchlib/src/vespa/searchlib/engine/trace.h @@ -85,6 +85,7 @@ public: vespalib::string toString() const; bool hasTrace() const { return static_cast<bool>(_trace); } Cursor & getRoot() const { return root(); } + Cursor & getTraces() const { return traces(); } vespalib::Slime & getSlime() const { return slime(); } bool shouldTrace(uint32_t level) const { return level <= _level; } uint32_t getLevel() const { return _level; } diff --git a/searchlib/src/vespa/searchlib/features/attributefeature.cpp b/searchlib/src/vespa/searchlib/features/attributefeature.cpp index 485f9af90d0..10cca2213e2 100644 --- a/searchlib/src/vespa/searchlib/features/attributefeature.cpp +++ b/searchlib/src/vespa/searchlib/features/attributefeature.cpp @@ -137,16 +137,16 @@ public: }; /** - * Implements the executor for fetching values from a single or array attribute vector + * Implements the executor for fetching values from an array attribute vector */ template <typename BaseType> -class MultiAttributeExecutor final : public fef::FeatureExecutor { +class ArrayAttributeExecutor final : public fef::FeatureExecutor { private: using ArrayReadView = attribute::IArrayReadView<BaseType>; const ArrayReadView* _array_read_view; uint32_t _idx; public: - MultiAttributeExecutor(const ArrayReadView* array_read_view, uint32_t idx) : _array_read_view(array_read_view), _idx(idx) { } + ArrayAttributeExecutor(const ArrayReadView* array_read_view, uint32_t idx) : _array_read_view(array_read_view), _idx(idx) { } void execute(uint32_t docId) override; void handle_bind_outputs(vespalib::ArrayRef<fef::NumberOrObject> outputs_in) override { fef::FeatureExecutor::handle_bind_outputs(outputs_in); @@ -239,7 +239,7 @@ SingleAttributeExecutor<T>::execute(uint32_t docId) template <typename BaseType> void -MultiAttributeExecutor<BaseType>::execute(uint32_t docId) +ArrayAttributeExecutor<BaseType>::execute(uint32_t docId) { auto values = _array_read_view->get_values(docId); auto o = outputs().get_bound(); @@ -330,10 +330,10 @@ private: }; template <typename T> -struct MultiValueExecutorCreator { +struct ArrayExecutorCreator { using ArrayReadView = attribute::IArrayReadView<typename T::BaseType>; - using ExecType = MultiAttributeExecutor<typename T::BaseType>; - MultiValueExecutorCreator() : _array_read_view(nullptr) {} + using ExecType = ArrayAttributeExecutor<typename T::BaseType>; + ArrayExecutorCreator() : _array_read_view(nullptr) {} bool handle(vespalib::Stash &stash, const IAttributeVector *attribute) { auto multi_value_attribute = attribute->as_multi_value_attribute(); if (multi_value_attribute != nullptr) { @@ -414,19 +414,19 @@ createAttributeExecutor(uint32_t numOutputs, const IAttributeVector *attribute, return stash.create<AttributeExecutor<ConstCharContent>>(attribute, idx); } else if (attribute->isIntegerType()) { if (basicType == BasicType::INT32) { - MultiValueExecutorCreator<IntegerAttributeTemplate<int32_t>> creator; + ArrayExecutorCreator<IntegerAttributeTemplate<int32_t>> creator; if (creator.handle(stash, attribute)) return creator.create(stash, idx); } else if (basicType == BasicType::INT64) { - MultiValueExecutorCreator<IntegerAttributeTemplate<int64_t>> creator; + ArrayExecutorCreator<IntegerAttributeTemplate<int64_t>> creator; if (creator.handle(stash, attribute)) return creator.create(stash, idx); } return stash.create<AttributeExecutor<IntegerContent>>(attribute, idx); } else { // FLOAT if (basicType == BasicType::DOUBLE) { - MultiValueExecutorCreator<FloatingPointAttributeTemplate<double>> creator; + ArrayExecutorCreator<FloatingPointAttributeTemplate<double>> creator; if (creator.handle(stash, attribute)) return creator.create(stash, idx); } else { - MultiValueExecutorCreator<FloatingPointAttributeTemplate<float>> creator; + ArrayExecutorCreator<FloatingPointAttributeTemplate<float>> creator; if (creator.handle(stash, attribute)) return creator.create(stash, idx); } return stash.create<AttributeExecutor<FloatContent>>(attribute, idx); diff --git a/searchsummary/src/tests/docsummary/attributedfw/attributedfw_test.cpp b/searchsummary/src/tests/docsummary/attributedfw/attributedfw_test.cpp index 9f6ca8f6b57..67d505582d8 100644 --- a/searchsummary/src/tests/docsummary/attributedfw/attributedfw_test.cpp +++ b/searchsummary/src/tests/docsummary/attributedfw/attributedfw_test.cpp @@ -59,6 +59,8 @@ public: } _writer = AttributeDFWFactory::create(_attrs.mgr(), field_name, filter_elements, _matching_elems_fields); _writer->setIndex(0); + EXPECT_TRUE(_writer->setFieldWriterStateIndex(0)); + _state._fieldWriterStates.resize(1); _field_name = field_name; _state._attributes.resize(1); _state._attributes[0] = _state._attrCtx->getAttribute(field_name); @@ -77,6 +79,7 @@ public: _callback.clear(); _callback.add_matching_elements(docid, _field_name, matching_elems); _state._matching_elements = std::unique_ptr<MatchingElements>(); + _state._fieldWriterStates[0] = nullptr; // Force new state to pick up changed matching elements expect_field(exp_slime_as_json, docid); } }; diff --git a/searchsummary/src/vespa/searchsummary/docsummary/array_attribute_combiner_dfw.cpp b/searchsummary/src/vespa/searchsummary/docsummary/array_attribute_combiner_dfw.cpp index 8eba167b9cf..ff5c2c5e05b 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/array_attribute_combiner_dfw.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/array_attribute_combiner_dfw.cpp @@ -9,6 +9,8 @@ #include <vespa/searchlib/common/matching_elements.h> #include <vespa/searchlib/common/matching_elements_fields.h> #include <vespa/vespalib/data/slime/cursor.h> +#include <vespa/vespalib/util/stash.h> +#include <algorithm> #include <cassert> using search::attribute::IAttributeContext; @@ -21,7 +23,8 @@ namespace { class ArrayAttributeFieldWriterState : public DocsumFieldWriterState { - std::vector<std::unique_ptr<AttributeFieldWriter>> _writers; + // AttributeFieldWriter instances are owned by stash passed to constructor + std::vector<AttributeFieldWriter*> _writers; const vespalib::string& _field_name; const MatchingElements* const _matching_elements; @@ -29,6 +32,7 @@ public: ArrayAttributeFieldWriterState(const std::vector<vespalib::string> &fieldNames, const std::vector<vespalib::string> &attributeNames, IAttributeContext &context, + vespalib::Stash& stash, const vespalib::string &field_name, const MatchingElements* matching_elements, bool is_map_of_scalar); @@ -40,6 +44,7 @@ public: ArrayAttributeFieldWriterState::ArrayAttributeFieldWriterState(const std::vector<vespalib::string> &fieldNames, const std::vector<vespalib::string> &attributeNames, IAttributeContext &context, + vespalib::Stash& stash, const vespalib::string &field_name, const MatchingElements *matching_elements, bool is_map_of_scalar) @@ -53,7 +58,7 @@ ArrayAttributeFieldWriterState::ArrayAttributeFieldWriterState(const std::vector for (uint32_t field = 0; field < fields; ++field) { const IAttributeVector *attr = context.getAttribute(attributeNames[field]); if (attr != nullptr) { - _writers.emplace_back(AttributeFieldWriter::create(fieldNames[field], *attr, is_map_of_scalar)); + _writers.emplace_back(&AttributeFieldWriter::create(fieldNames[field], *attr, stash, is_map_of_scalar)); } } } @@ -74,10 +79,7 @@ ArrayAttributeFieldWriterState::insertField(uint32_t docId, vespalib::slime::Ins { uint32_t elems = 0; for (auto &writer : _writers) { - writer->fetch(docId); - if (elems < writer->size()) { - elems = writer->size(); - } + elems = std::max(elems, writer->fetch(docId)); } if (elems == 0) { return; @@ -118,11 +120,10 @@ ArrayAttributeCombinerDFW::ArrayAttributeCombinerDFW(const vespalib::string &fie ArrayAttributeCombinerDFW::~ArrayAttributeCombinerDFW() = default; -std::unique_ptr<DocsumFieldWriterState> -ArrayAttributeCombinerDFW::allocFieldWriterState(IAttributeContext &context, const MatchingElements* matching_elements) +DocsumFieldWriterState* +ArrayAttributeCombinerDFW::allocFieldWriterState(IAttributeContext &context, vespalib::Stash &stash, const MatchingElements* matching_elements) { - return std::make_unique<ArrayAttributeFieldWriterState>(_fields, _attributeNames, context, - _fieldName, matching_elements, _is_map_of_scalar); + return &stash.create<ArrayAttributeFieldWriterState>(_fields, _attributeNames, context, stash, _fieldName, matching_elements, _is_map_of_scalar); } } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/array_attribute_combiner_dfw.h b/searchsummary/src/vespa/searchsummary/docsummary/array_attribute_combiner_dfw.h index c36140c5220..18b4fd34e66 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/array_attribute_combiner_dfw.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/array_attribute_combiner_dfw.h @@ -22,7 +22,7 @@ class ArrayAttributeCombinerDFW : public AttributeCombinerDFW std::vector<vespalib::string> _attributeNames; bool _is_map_of_scalar; - std::unique_ptr<DocsumFieldWriterState> allocFieldWriterState(search::attribute::IAttributeContext &context, const MatchingElements* matching_elements) override; + DocsumFieldWriterState* allocFieldWriterState(search::attribute::IAttributeContext &context, vespalib::Stash &stash, const MatchingElements* matching_elements) override; public: ArrayAttributeCombinerDFW(const vespalib::string &fieldName, const StructFieldsResolver& fields_resolver, diff --git a/searchsummary/src/vespa/searchsummary/docsummary/attribute_combiner_dfw.cpp b/searchsummary/src/vespa/searchsummary/docsummary/attribute_combiner_dfw.cpp index 01fd688f9e0..79c11b20479 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/attribute_combiner_dfw.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/attribute_combiner_dfw.cpp @@ -57,13 +57,13 @@ AttributeCombinerDFW::create(const vespalib::string &fieldName, IAttributeContex void AttributeCombinerDFW::insertField(uint32_t docid, GetDocsumsState *state, ResType, vespalib::slime::Inserter &target) { - auto &fieldWriterState = state->_fieldWriterStates[_stateIndex]; + auto& fieldWriterState = state->_fieldWriterStates[_stateIndex]; if (!fieldWriterState) { const MatchingElements *matching_elements = nullptr; if (_filter_elements) { matching_elements = &state->get_matching_elements(*_matching_elems_fields); } - fieldWriterState = allocFieldWriterState(*state->_attrCtx, matching_elements); + fieldWriterState = allocFieldWriterState(*state->_attrCtx, state->get_stash(), matching_elements); } fieldWriterState->insertField(docid, target); } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/attribute_combiner_dfw.h b/searchsummary/src/vespa/searchsummary/docsummary/attribute_combiner_dfw.h index f0fed55b5f0..c1742595745 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/attribute_combiner_dfw.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/attribute_combiner_dfw.h @@ -10,6 +10,8 @@ class MatchingElementsFields; } namespace search::attribute { class IAttributeContext; } +namespace vespalib { class Stash; } + namespace search::docsummary { class DocsumFieldWriterState; @@ -29,7 +31,7 @@ protected: AttributeCombinerDFW(const vespalib::string &fieldName, bool filter_elements, std::shared_ptr<MatchingElementsFields> matching_elems_fields); protected: - virtual std::unique_ptr<DocsumFieldWriterState> allocFieldWriterState(search::attribute::IAttributeContext &context, const MatchingElements* matching_elements) = 0; + virtual DocsumFieldWriterState* allocFieldWriterState(search::attribute::IAttributeContext &context, vespalib::Stash& stash, const MatchingElements* matching_elements) = 0; public: ~AttributeCombinerDFW() override; bool IsGenerated() const override; diff --git a/searchsummary/src/vespa/searchsummary/docsummary/attribute_field_writer.cpp b/searchsummary/src/vespa/searchsummary/docsummary/attribute_field_writer.cpp index e8777a28775..5668ab82d06 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/attribute_field_writer.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/attribute_field_writer.cpp @@ -2,22 +2,22 @@ #include "attribute_field_writer.h" #include <vespa/searchcommon/attribute/attributecontent.h> +#include <vespa/searchcommon/attribute/i_multi_value_attribute.h> #include <vespa/searchcommon/common/undefinedvalues.h> #include <vespa/vespalib/data/slime/cursor.h> +#include <vespa/vespalib/util/stash.h> #include <cassert> using search::attribute::BasicType; using search::attribute::IAttributeVector; using search::attribute::getUndefined; +using search::attribute::IArrayReadView; using vespalib::slime::Cursor; namespace search::docsummary { -AttributeFieldWriter::AttributeFieldWriter(vespalib::Memory fieldName, - const IAttributeVector &attr) - : _fieldName(fieldName), - _attr(attr), - _size(0) +AttributeFieldWriter::AttributeFieldWriter(vespalib::Memory fieldName) + : _fieldName(fieldName) { } @@ -25,78 +25,88 @@ AttributeFieldWriter::~AttributeFieldWriter() = default; namespace { -template <class Content> +template <class BasicType> class WriteField : public AttributeFieldWriter { protected: - Content _content; + const IArrayReadView<BasicType>* _array_read_view; + vespalib::ConstArrayRef<BasicType> _content; - WriteField(vespalib::Memory fieldName, const IAttributeVector &attr); + WriteField(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash &stash); ~WriteField() override; private: - void fetch(uint32_t docId) override; + uint32_t fetch(uint32_t docId) override; }; -class WriteStringField : public WriteField<search::attribute::ConstCharContent> +class WriteStringField : public WriteField<const char*> { public: - WriteStringField(vespalib::Memory fieldName, - const IAttributeVector &attr); + WriteStringField(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash); ~WriteStringField() override; void print(uint32_t idx, Cursor &cursor) override; }; -class WriteStringFieldNeverSkip : public WriteField<search::attribute::ConstCharContent> +class WriteStringFieldNeverSkip : public WriteField<const char*> { public: - WriteStringFieldNeverSkip(vespalib::Memory fieldName, - const IAttributeVector &attr) - : WriteField(fieldName, attr) {} + WriteStringFieldNeverSkip(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash) + : WriteField(fieldName, attr, stash) {} ~WriteStringFieldNeverSkip() override {} void print(uint32_t idx, Cursor &cursor) override; }; -class WriteFloatField : public WriteField<search::attribute::FloatContent> +template <typename BasicType> +class WriteFloatField : public WriteField<BasicType> { public: - WriteFloatField(vespalib::Memory fieldName, - const IAttributeVector &attr); + WriteFloatField(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash); ~WriteFloatField() override; void print(uint32_t idx, Cursor &cursor) override; }; -class WriteIntField : public WriteField<search::attribute::IntegerContent> +template <typename BasicType> +class WriteIntField : public WriteField<BasicType> { - IAttributeVector::largeint_t _undefined; public: - WriteIntField(vespalib::Memory fieldName, - const IAttributeVector &attr, - IAttributeVector::largeint_t undefined); + WriteIntField(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash); ~WriteIntField() override; void print(uint32_t idx, Cursor &cursor) override; }; -template <class Content> -WriteField<Content>::WriteField(vespalib::Memory fieldName, const IAttributeVector &attr) - : AttributeFieldWriter(fieldName, attr), +template <typename BasicType> +const search::attribute::IArrayReadView<BasicType>* +make_array_read_view(const IAttributeVector& attribute, vespalib::Stash& stash) +{ + auto multi_value_attribute = attribute.as_multi_value_attribute(); + if (multi_value_attribute != nullptr) { + return multi_value_attribute->make_read_view(search::attribute::IMultiValueAttribute::ArrayTag<BasicType>(), stash); + } + return nullptr; +} + +template <class BasicType> +WriteField<BasicType>::WriteField(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash) + : AttributeFieldWriter(fieldName), + _array_read_view(make_array_read_view<BasicType>(attr, stash)), _content() { } -template <class Content> -WriteField<Content>::~WriteField() = default; +template <class BasicType> +WriteField<BasicType>::~WriteField() = default; -template <class Content> -void -WriteField<Content>::fetch(uint32_t docId) +template <class BasicType> +uint32_t +WriteField<BasicType>::fetch(uint32_t docId) { - _content.fill(_attr, docId); - _size = _content.size(); + if (_array_read_view) { + _content = _array_read_view->get_values(docId); + } + return _content.size(); } -WriteStringField::WriteStringField(vespalib::Memory fieldName, - const IAttributeVector &attr) - : WriteField(fieldName, attr) +WriteStringField::WriteStringField(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash) + : WriteField(fieldName, attr, stash) { } @@ -105,7 +115,7 @@ WriteStringField::~WriteStringField() = default; void WriteStringField::print(uint32_t idx, Cursor &cursor) { - if (idx < _size) { + if (idx < _content.size()) { const char *s = _content[idx]; if (s[0] != '\0') { cursor.setString(_fieldName, vespalib::Memory(s)); @@ -116,7 +126,7 @@ WriteStringField::print(uint32_t idx, Cursor &cursor) void WriteStringFieldNeverSkip::print(uint32_t idx, Cursor &cursor) { - if (idx < _size) { + if (idx < _content.size()) { const char *s = _content[idx]; cursor.setString(_fieldName, vespalib::Memory(s)); } else { @@ -124,68 +134,71 @@ WriteStringFieldNeverSkip::print(uint32_t idx, Cursor &cursor) } } -WriteFloatField::WriteFloatField(vespalib::Memory fieldName, - const IAttributeVector &attr) - : WriteField(fieldName, attr) +template <typename BasicType> +WriteFloatField<BasicType>::WriteFloatField(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash) + : WriteField<BasicType>(fieldName, attr, stash) { } -WriteFloatField::~WriteFloatField() = default; +template <typename BasicType> +WriteFloatField<BasicType>::~WriteFloatField() = default; +template <typename BasicType> void -WriteFloatField::print(uint32_t idx, Cursor &cursor) +WriteFloatField<BasicType>::print(uint32_t idx, Cursor &cursor) { - if (idx < _size) { - double val = _content[idx]; + if (idx < this->_content.size()) { + double val = this->_content[idx]; if (!search::attribute::isUndefined(val)) { - cursor.setDouble(_fieldName, val); + cursor.setDouble(this->_fieldName, val); } } } -WriteIntField::WriteIntField(vespalib::Memory fieldName, - const IAttributeVector &attr, - IAttributeVector::largeint_t undefined) - : WriteField(fieldName, attr), - _undefined(undefined) +template <typename BasicType> +WriteIntField<BasicType>::WriteIntField(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash) + : WriteField<BasicType>(fieldName, attr, stash) { } -WriteIntField::~WriteIntField() = default; +template <typename BasicType> +WriteIntField<BasicType>::~WriteIntField() = default; +template <typename BasicType> void -WriteIntField::print(uint32_t idx, Cursor &cursor) +WriteIntField<BasicType>::print(uint32_t idx, Cursor &cursor) { - if (idx < _size) { - auto val = _content[idx]; - if (val != _undefined) { - cursor.setLong(_fieldName, _content[idx]); + if (idx < this->_content.size()) { + auto val = this->_content[idx]; + if (val != getUndefined<BasicType>()) { + cursor.setLong(this->_fieldName, val); } } } } -std::unique_ptr<AttributeFieldWriter> -AttributeFieldWriter::create(vespalib::Memory fieldName, const IAttributeVector &attr, bool keep_empty_strings) +AttributeFieldWriter& +AttributeFieldWriter::create(vespalib::Memory fieldName, const IAttributeVector& attr, vespalib::Stash& stash, bool keep_empty_strings) { switch (attr.getBasicType()) { case BasicType::INT8: - return std::make_unique<WriteIntField>(fieldName, attr, getUndefined<int8_t>()); + return stash.create<WriteIntField<int8_t>>(fieldName, attr, stash); case BasicType::INT16: - return std::make_unique<WriteIntField>(fieldName, attr, getUndefined<int16_t>()); + return stash.create<WriteIntField<int16_t>>(fieldName, attr, stash); case BasicType::INT32: - return std::make_unique<WriteIntField>(fieldName, attr, getUndefined<int32_t>()); + return stash.create<WriteIntField<int32_t>>(fieldName, attr, stash); case BasicType::INT64: - return std::make_unique<WriteIntField>(fieldName, attr, getUndefined<int64_t>()); + return stash.create<WriteIntField<int64_t>>(fieldName, attr, stash); case BasicType::FLOAT: + return stash.create<WriteFloatField<float>>(fieldName, attr, stash); case BasicType::DOUBLE: - return std::make_unique<WriteFloatField>(fieldName, attr); + return stash.create<WriteFloatField<double>>(fieldName, attr, stash); case BasicType::STRING: if (keep_empty_strings) { - return std::make_unique<WriteStringFieldNeverSkip>(fieldName, attr); + return stash.create<WriteStringFieldNeverSkip>(fieldName, attr, stash); } else { - return std::make_unique<WriteStringField>(fieldName, attr); + return stash.create<WriteStringField>(fieldName, attr, stash); } default: assert(false); diff --git a/searchsummary/src/vespa/searchsummary/docsummary/attribute_field_writer.h b/searchsummary/src/vespa/searchsummary/docsummary/attribute_field_writer.h index eb108a9681d..5c04328d01e 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/attribute_field_writer.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/attribute_field_writer.h @@ -6,6 +6,7 @@ #include <memory> namespace search::attribute { class IAttributeVector; } +namespace vespalib { class Stash; } namespace vespalib::slime { struct Cursor; } namespace search::docsummary { @@ -20,16 +21,13 @@ class AttributeFieldWriter { protected: const vespalib::Memory _fieldName; - const search::attribute::IAttributeVector &_attr; - size_t _size; - AttributeFieldWriter(vespalib::Memory fieldName, - const search::attribute::IAttributeVector &attr); + AttributeFieldWriter(vespalib::Memory fieldName); public: virtual ~AttributeFieldWriter(); - virtual void fetch(uint32_t docId) = 0; + virtual uint32_t fetch(uint32_t docId) = 0; virtual void print(uint32_t idx, vespalib::slime::Cursor &cursor) = 0; - static std::unique_ptr<AttributeFieldWriter> create(vespalib::Memory fieldName, const search::attribute::IAttributeVector &attr, bool keep_empty_strings = false); - uint32_t size() const { return _size; } + // Create a new attribute field writer which is owned by stash + static AttributeFieldWriter& create(vespalib::Memory fieldName, const search::attribute::IAttributeVector& attr, vespalib::Stash& stash, bool keep_empty_strings = false); }; } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/attributedfw.cpp b/searchsummary/src/vespa/searchsummary/docsummary/attributedfw.cpp index 448feedac80..eb4f1a19e06 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/attributedfw.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/attributedfw.cpp @@ -3,12 +3,12 @@ #include "attributedfw.h" #include "docsumstate.h" #include "docsumwriter.h" +#include "docsum_field_writer_state.h" #include <vespa/eval/eval/value.h> #include <vespa/eval/eval/value_codec.h> -#include <vespa/searchcommon/attribute/iattributecontext.h> +#include <vespa/searchcommon/attribute/i_multi_value_attribute.h> +#include <vespa/searchcommon/attribute/multi_value_traits.h> #include <vespa/searchlib/attribute/iattributemanager.h> -#include <vespa/searchlib/attribute/integerbase.h> -#include <vespa/searchlib/attribute/stringbase.h> #include <vespa/searchlib/common/matching_elements.h> #include <vespa/searchlib/common/matching_elements_fields.h> #include <vespa/searchlib/tensor/i_tensor_attribute.h> @@ -23,6 +23,8 @@ using namespace search; using search::attribute::BasicType; using search::attribute::IAttributeContext; using search::attribute::IAttributeVector; +using search::attribute::IMultiValueAttribute; +using search::attribute::IMultiValueReadView; using vespalib::Memory; using vespalib::slime::Cursor; using vespalib::slime::Inserter; @@ -140,145 +142,219 @@ SingleAttrDFW::insertField(uint32_t docid, GetDocsumsState * state, ResType type //----------------------------------------------------------------------------- -template <typename DataType> -class MultiAttrDFW : public AttrDFW { -private: - bool _is_weighted_set; - bool _filter_elements; - std::shared_ptr<MatchingElementsFields> _matching_elems_fields; +template <typename MultiValueType> +const IMultiValueReadView<MultiValueType>* +make_read_view(const IAttributeVector& attribute, vespalib::Stash& stash) +{ + auto multi_value_attribute = attribute.as_multi_value_attribute(); + if (multi_value_attribute != nullptr) { + return multi_value_attribute->make_read_view(IMultiValueAttribute::MultiValueTag<MultiValueType>(), stash); + } + return nullptr; +} +class EmptyWriterState : public DocsumFieldWriterState +{ public: - explicit MultiAttrDFW(const vespalib::string& attr_name, bool is_weighted_set, - bool filter_elements, std::shared_ptr<MatchingElementsFields> matching_elems_fields) - : AttrDFW(attr_name), - _is_weighted_set(is_weighted_set), - _filter_elements(filter_elements), - _matching_elems_fields(std::move(matching_elems_fields)) - { - if (filter_elements && _matching_elems_fields) { - _matching_elems_fields->add_field(attr_name); - } - } - void insertField(uint32_t docid, GetDocsumsState* state, ResType type, Inserter& target) override; + EmptyWriterState() = default; + ~EmptyWriterState() = default; + void insertField(uint32_t, Inserter&) override { } }; -void -set(const vespalib::string & value, Symbol itemSymbol, Cursor & cursor) +template <typename MultiValueType> +class MultiAttrDFWState : public DocsumFieldWriterState { - cursor.setString(itemSymbol, value); -} + const vespalib::string& _field_name; + const IMultiValueReadView<MultiValueType>* _read_view; + const MatchingElements* _matching_elements; +public: + MultiAttrDFWState(const vespalib::string& field_name, const IAttributeVector& attr, vespalib::Stash& stash, const MatchingElements* matching_elements); + ~MultiAttrDFWState() override; + void insertField(uint32_t docid, Inserter& target) override; +}; -void -append(const IAttributeVector::WeightedString & element, Cursor& arr) -{ - arr.addString(element.getValue()); -} -void -set(int64_t value, Symbol itemSymbol, Cursor & cursor) +template <typename MultiValueType> +MultiAttrDFWState<MultiValueType>::MultiAttrDFWState(const vespalib::string& field_name, const IAttributeVector& attr, vespalib::Stash& stash, const MatchingElements* matching_elements) + : _field_name(field_name), + _read_view(make_read_view<MultiValueType>(attr, stash)), + _matching_elements(matching_elements) { - cursor.setLong(itemSymbol, value); } -void -append(const IAttributeVector::WeightedInt & element, Cursor& arr) -{ - arr.addLong(element.getValue()); -} +template <typename MultiValueType> +MultiAttrDFWState<MultiValueType>::~MultiAttrDFWState() = default; +template <typename V> void -set(double value, Symbol itemSymbol, Cursor & cursor) +set_value(V value, Symbol item_symbol, Cursor& cursor) { - cursor.setDouble(itemSymbol, value); + if constexpr (std::is_same_v<V, const char*>) { + cursor.setString(item_symbol, value); + } else if constexpr(std::is_floating_point_v<V>) { + cursor.setDouble(item_symbol, value); + } else { + cursor.setLong(item_symbol, value); + } } +template <typename V> void -append(const IAttributeVector::WeightedFloat & element, Cursor& arr) +append_value(V value, Cursor& arr) { - arr.addDouble(element.getValue()); + if constexpr (std::is_same_v<V, const char*>) { + arr.addString(value); + } else if constexpr(std::is_floating_point_v<V>) { + arr.addDouble(value); + } else { + arr.addLong(value); + } } Memory ITEM("item"); Memory WEIGHT("weight"); -template <typename DataType> +template <typename MultiValueType> void -MultiAttrDFW<DataType>::insertField(uint32_t docid, GetDocsumsState* state, ResType, Inserter& target) +MultiAttrDFWState<MultiValueType>::insertField(uint32_t docid, Inserter& target) { - const auto& attr = get_attribute(*state); - uint32_t entries = attr.getValueCount(docid); - if (entries == 0) { - return; // Don't insert empty fields + using ValueType = multivalue::ValueType_t<MultiValueType>; + if (!_read_view) { + return; } + auto elements = _read_view->get_values(docid); + if (elements.empty()) { + return; + } + Cursor &arr = target.insertArray(elements.size()); - std::vector<DataType> elements(entries); - entries = std::min(entries, attr.get(docid, elements.data(), entries)); - Cursor &arr = target.insertArray(entries); - - if (_filter_elements) { - const auto& matching_elems = state->get_matching_elements(*_matching_elems_fields) - .get_matching_elements(docid, getAttributeName()); - if (!matching_elems.empty() && matching_elems.back() < entries) { - if (_is_weighted_set) { + if (_matching_elements) { + const auto& matching_elems = _matching_elements->get_matching_elements(docid, _field_name); + if (!matching_elems.empty() && matching_elems.back() < elements.size()) { + if constexpr (multivalue::is_WeightedValue_v<MultiValueType>) { Symbol itemSymbol = arr.resolve(ITEM); Symbol weightSymbol = arr.resolve(WEIGHT); for (uint32_t id_to_keep : matching_elems) { - const DataType & element = elements[id_to_keep]; + auto& element = elements[id_to_keep]; Cursor& elemC = arr.addObject(); - set(element.getValue(), itemSymbol, elemC); - elemC.setLong(weightSymbol, element.getWeight()); + set_value<ValueType>(element.value(), itemSymbol, elemC); + elemC.setLong(weightSymbol, element.weight()); } } else { for (uint32_t id_to_keep : matching_elems) { - append(elements[id_to_keep], arr); + append_value<ValueType>(elements[id_to_keep], arr); } } } } else { - if (_is_weighted_set) { + if constexpr (multivalue::is_WeightedValue_v<MultiValueType>) { Symbol itemSymbol = arr.resolve(ITEM); Symbol weightSymbol = arr.resolve(WEIGHT); for (const auto & element : elements) { Cursor& elemC = arr.addObject(); - set(element.getValue(), itemSymbol, elemC); - elemC.setLong(weightSymbol, element.getWeight()); + set_value<ValueType>(element.value(), itemSymbol, elemC); + elemC.setLong(weightSymbol, element.weight()); } } else { for (const auto & element : elements) { - append(element, arr); + append_value<ValueType>(element, arr); } } } } -std::unique_ptr<IDocsumFieldWriter> -create_multi_writer(const IAttributeVector& attr, - bool filter_elements, - std::shared_ptr<MatchingElementsFields> matching_elems_fields) +class MultiAttrDFW : public AttrDFW { +private: + bool _filter_elements; + uint32_t _state_index; // index into _fieldWriterStates in GetDocsumsState + std::shared_ptr<MatchingElementsFields> _matching_elems_fields; + +public: + explicit MultiAttrDFW(const vespalib::string& attr_name, bool filter_elements, std::shared_ptr<MatchingElementsFields> matching_elems_fields) + : AttrDFW(attr_name), + _filter_elements(filter_elements), + _matching_elems_fields(std::move(matching_elems_fields)) + { + if (filter_elements && _matching_elems_fields) { + _matching_elems_fields->add_field(attr_name); + } + } + bool setFieldWriterStateIndex(uint32_t fieldWriterStateIndex) override; + void insertField(uint32_t docid, GetDocsumsState* state, ResType type, Inserter& target) override; +}; + +bool +MultiAttrDFW::setFieldWriterStateIndex(uint32_t fieldWriterStateIndex) +{ + _state_index = fieldWriterStateIndex; + return true; +} + +template <typename DataType> +DocsumFieldWriterState* +make_field_writer_state_helper(const vespalib::string& field_name, const IAttributeVector& attr, vespalib::Stash& stash, const MatchingElements* matching_elements) { - auto type = attr.getBasicType(); bool is_weighted_set = attr.hasWeightedSetType(); + if (is_weighted_set) { + return &stash.create<MultiAttrDFWState<multivalue::WeightedValue<DataType>>>(field_name, attr, stash, matching_elements); + } else { + return &stash.create<MultiAttrDFWState<DataType>>(field_name, attr, stash, matching_elements); + } +} + +DocsumFieldWriterState* +make_field_writer_state(const vespalib::string& field_name, const IAttributeVector& attr, vespalib::Stash& stash, const MatchingElements* matching_elements) +{ + auto type = attr.getBasicType(); switch (type) { - case BasicType::NONE: - case BasicType::STRING: { - return std::make_unique<MultiAttrDFW<IAttributeVector::WeightedString>>(attr.getName(), is_weighted_set, - filter_elements, std::move(matching_elems_fields)); + case BasicType::Type::STRING: + return make_field_writer_state_helper<const char*>(field_name, attr, stash, matching_elements); + case BasicType::Type::INT8: + return make_field_writer_state_helper<int8_t>(field_name, attr, stash, matching_elements); + case BasicType::Type::INT16: + return make_field_writer_state_helper<int16_t>(field_name, attr, stash, matching_elements); + case BasicType::Type::INT32: + return make_field_writer_state_helper<int32_t>(field_name, attr, stash, matching_elements); + case BasicType::Type::INT64: + return make_field_writer_state_helper<int64_t>(field_name, attr, stash, matching_elements); + case BasicType::Type::FLOAT: + return make_field_writer_state_helper<float>(field_name, attr, stash, matching_elements); + case BasicType::Type::DOUBLE: + return make_field_writer_state_helper<double>(field_name, attr, stash, matching_elements); + default: + ; + } + return &stash.create<EmptyWriterState>(); +} + +void +MultiAttrDFW::insertField(uint32_t docid, GetDocsumsState *state, ResType, vespalib::slime::Inserter &target) +{ + auto& field_writer_state = state->_fieldWriterStates[_state_index]; + if (!field_writer_state) { + const MatchingElements *matching_elements = nullptr; + if (_filter_elements) { + matching_elements = &state->get_matching_elements(*_matching_elems_fields); + } + const auto& attr = get_attribute(*state); + field_writer_state = make_field_writer_state(getAttributeName(), attr, state->get_stash(), matching_elements); } - case BasicType::BOOL: - case BasicType::UINT2: - case BasicType::UINT4: + field_writer_state->insertField(docid, target); +} + +std::unique_ptr<IDocsumFieldWriter> +create_multi_writer(const IAttributeVector& attr, bool filter_elements, std::shared_ptr<MatchingElementsFields> matching_elems_fields) +{ + auto type = attr.getBasicType(); + switch (type) { + case BasicType::STRING: case BasicType::INT8: case BasicType::INT16: case BasicType::INT32: - case BasicType::INT64: { - return std::make_unique<MultiAttrDFW<IAttributeVector::WeightedInt>>(attr.getName(), is_weighted_set, - filter_elements, std::move(matching_elems_fields)); - } + case BasicType::INT64: case BasicType::FLOAT: - case BasicType::DOUBLE: { - return std::make_unique<MultiAttrDFW<IAttributeVector::WeightedFloat>>(attr.getName(), is_weighted_set, - filter_elements, std::move(matching_elems_fields)); - } + case BasicType::DOUBLE: + return std::make_unique<MultiAttrDFW>(attr.getName(), filter_elements, std::move(matching_elems_fields)); default: // should not happen LOG(error, "Bad value for attribute type: %u", type); diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp b/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp index 4f1634042c1..e538af3839e 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp @@ -32,6 +32,7 @@ GetDocsumsState::GetDocsumsState(GetDocsumsStateCallback &callback) _dynteaser(), _attrCtx(), _attributes(), + _stash(), _fieldWriterStates(), _parsedLocations(), _summaryFeatures(nullptr), diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.h b/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.h index 85d24d25c62..c25aca15200 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.h @@ -7,6 +7,7 @@ #include <vespa/searchlib/common/featureset.h> #include <vespa/searchlib/common/geo_location_spec.h> #include <vespa/vespalib/util/jsonwriter.h> +#include <vespa/vespalib/util/stash.h> namespace juniper { class Config; @@ -73,7 +74,11 @@ public: std::unique_ptr<search::attribute::IAttributeContext> _attrCtx; std::vector<const search::attribute::IAttributeVector *> _attributes; - std::vector<std::unique_ptr<DocsumFieldWriterState>> _fieldWriterStates; +private: + vespalib::Stash _stash; +public: + // DocsumFieldWriterState instances are owned by _stash + std::vector<DocsumFieldWriterState*> _fieldWriterStates; // used by AbsDistanceDFW std::vector<search::common::GeoLocationSpec> _parsedLocations; @@ -87,7 +92,7 @@ public: // used by RankFeaturesDFW FeatureSet::SP _rankFeatures; - // Used by AttributeCombinerDFW when filtering is enabled + // Used by AttributeCombinerDFW and MultiAttrDFW when filtering is enabled std::unique_ptr<search::MatchingElements> _matching_elements; GetDocsumsState(const GetDocsumsState &) = delete; @@ -97,6 +102,7 @@ public: const MatchingElements &get_matching_elements(const MatchingElementsFields &matching_elems_fields); vespalib::JSONStringer & jsonStringer(); + vespalib::Stash& get_stash() noexcept { return _stash; } private: // Only used by rank/summary features, so make it lazy std::unique_ptr<vespalib::JSONStringer> _jsonStringer; diff --git a/searchsummary/src/vespa/searchsummary/docsummary/struct_map_attribute_combiner_dfw.cpp b/searchsummary/src/vespa/searchsummary/docsummary/struct_map_attribute_combiner_dfw.cpp index ee9ca92c15b..aec55977546 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/struct_map_attribute_combiner_dfw.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/struct_map_attribute_combiner_dfw.cpp @@ -9,6 +9,8 @@ #include <vespa/searchlib/common/matching_elements.h> #include <vespa/searchlib/common/matching_elements_fields.h> #include <vespa/vespalib/data/slime/cursor.h> +#include <vespa/vespalib/util/stash.h> +#include <algorithm> #include <cassert> using search::attribute::IAttributeContext; @@ -24,16 +26,19 @@ vespalib::Memory valueName("value"); class StructMapAttributeFieldWriterState : public DocsumFieldWriterState { - std::unique_ptr<AttributeFieldWriter> _keyWriter; - std::vector<std::unique_ptr<AttributeFieldWriter>> _valueWriters; - const vespalib::string& _field_name; - const MatchingElements* const _matching_elements; + // AttributeFieldWriter instance is owned by stash passed to constructor + AttributeFieldWriter* _keyWriter; + // AttributeFieldWriter instances are owned by stash passed to constructor + std::vector<AttributeFieldWriter*> _valueWriters; + const vespalib::string& _field_name; + const MatchingElements* const _matching_elements; public: StructMapAttributeFieldWriterState(const vespalib::string &keyAttributeName, const std::vector<vespalib::string> &valueFieldNames, const std::vector<vespalib::string> &valueAttributeNames, IAttributeContext &context, + vespalib::Stash& stash, const vespalib::string &field_name, const MatchingElements* matching_elements); ~StructMapAttributeFieldWriterState() override; @@ -45,24 +50,25 @@ StructMapAttributeFieldWriterState::StructMapAttributeFieldWriterState(const ves const std::vector<vespalib::string> &valueFieldNames, const std::vector<vespalib::string> &valueAttributeNames, IAttributeContext &context, - const vespalib::string& field_name, + vespalib::Stash& stash, + const vespalib::string& field_name, const MatchingElements *matching_elements) : DocsumFieldWriterState(), - _keyWriter(), + _keyWriter(nullptr), _valueWriters(), _field_name(field_name), _matching_elements(matching_elements) { const IAttributeVector *attr = context.getAttribute(keyAttributeName); if (attr != nullptr) { - _keyWriter = AttributeFieldWriter::create(keyName, *attr, true); + _keyWriter = &AttributeFieldWriter::create(keyName, *attr, stash, true); } size_t fields = valueFieldNames.size(); _valueWriters.reserve(fields); for (uint32_t field = 0; field < fields; ++field) { attr = context.getAttribute(valueAttributeNames[field]); if (attr != nullptr) { - _valueWriters.emplace_back(AttributeFieldWriter::create(valueFieldNames[field], *attr)); + _valueWriters.emplace_back(&AttributeFieldWriter::create(valueFieldNames[field], *attr, stash)); } } } @@ -87,16 +93,10 @@ StructMapAttributeFieldWriterState::insertField(uint32_t docId, vespalib::slime: { uint32_t elems = 0; if (_keyWriter) { - _keyWriter->fetch(docId); - if (elems < _keyWriter->size()) { - elems = _keyWriter->size(); - } + elems = _keyWriter->fetch(docId); } for (auto &valueWriter : _valueWriters) { - valueWriter->fetch(docId); - if (elems < valueWriter->size()) { - elems = valueWriter->size(); - } + elems = std::max(elems, valueWriter->fetch(docId)); } if (elems == 0) { return; @@ -137,10 +137,10 @@ StructMapAttributeCombinerDFW::StructMapAttributeCombinerDFW(const vespalib::str StructMapAttributeCombinerDFW::~StructMapAttributeCombinerDFW() = default; -std::unique_ptr<DocsumFieldWriterState> -StructMapAttributeCombinerDFW::allocFieldWriterState(IAttributeContext &context, const MatchingElements* matching_elements) +DocsumFieldWriterState* +StructMapAttributeCombinerDFW::allocFieldWriterState(IAttributeContext &context, vespalib::Stash& stash, const MatchingElements* matching_elements) { - return std::make_unique<StructMapAttributeFieldWriterState>(_keyAttributeName, _valueFields, _valueAttributeNames, context, _fieldName, matching_elements); + return &stash.create<StructMapAttributeFieldWriterState>(_keyAttributeName, _valueFields, _valueAttributeNames, context, stash, _fieldName, matching_elements); } } diff --git a/searchsummary/src/vespa/searchsummary/docsummary/struct_map_attribute_combiner_dfw.h b/searchsummary/src/vespa/searchsummary/docsummary/struct_map_attribute_combiner_dfw.h index 84e89477fd2..74ba61dce14 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/struct_map_attribute_combiner_dfw.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/struct_map_attribute_combiner_dfw.h @@ -21,7 +21,7 @@ class StructMapAttributeCombinerDFW : public AttributeCombinerDFW std::vector<vespalib::string> _valueFields; std::vector<vespalib::string> _valueAttributeNames; - std::unique_ptr<DocsumFieldWriterState> allocFieldWriterState(search::attribute::IAttributeContext &context, const MatchingElements* matching_elements) override; + DocsumFieldWriterState* allocFieldWriterState(search::attribute::IAttributeContext &context, vespalib::Stash& stash, const MatchingElements* matching_elements) override; public: StructMapAttributeCombinerDFW(const vespalib::string &fieldName, const StructFieldsResolver& fields_resolver, diff --git a/vespa-application-maven-plugin/src/main/java/com/yahoo/container/plugin/mojo/ApplicationMojo.java b/vespa-application-maven-plugin/src/main/java/com/yahoo/container/plugin/mojo/ApplicationMojo.java index b3fd107631b..62bb5c53f8c 100644 --- a/vespa-application-maven-plugin/src/main/java/com/yahoo/container/plugin/mojo/ApplicationMojo.java +++ b/vespa-application-maven-plugin/src/main/java/com/yahoo/container/plugin/mojo/ApplicationMojo.java @@ -76,10 +76,14 @@ public class ApplicationMojo extends AbstractMojo { throw new IllegalArgumentException("compile version (" + compileVersion + ") cannot be higher than parent version (" + parentVersion + ")"); } - String metaData = String.format("{\n \"compileVersion\": \"%s\",\n \"buildTime\": %d,\n \"parentVersion\": %s\n}", + String metaData = String.format("{\n" + + " \"compileVersion\": \"%s\",\n" + + " \"buildTime\": %d,\n" + + " \"parentVersion\": %s\n" + + "}", compileVersion, System.currentTimeMillis(), - parentVersion); + parentVersion == null ? null : "\"" + parentVersion + "\""); try { Files.write(applicationDestination.toPath().resolve("build-meta.json"), metaData.getBytes(StandardCharsets.UTF_8)); diff --git a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/JunitRunner.java b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/JunitRunner.java index c59c693a789..8a0446ac2e6 100644 --- a/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/JunitRunner.java +++ b/vespa-osgi-testrunner/src/main/java/com/yahoo/vespa/testrunner/JunitRunner.java @@ -153,7 +153,7 @@ public class JunitRunner extends AbstractComponent implements TestRunner { try { var jsonDescriptor = IOUtils.readAll(resource.openStream(), Charset.defaultCharset()).trim(); testDescriptor = TestDescriptor.fromJsonString(jsonDescriptor); - logger.info( "Test classes in bundle :" + testDescriptor.toString()); + logger.info("Test classes in bundle: " + testDescriptor); return Optional.of(testDescriptor); } catch (IOException e) { return Optional.empty(); diff --git a/vespalib/CMakeLists.txt b/vespalib/CMakeLists.txt index c3c7fee2cef..7796ce7df28 100644 --- a/vespalib/CMakeLists.txt +++ b/vespalib/CMakeLists.txt @@ -12,8 +12,9 @@ vespa_define_module( APPS src/apps/make_fixture_macros src/apps/vespa-detect-hostname - src/apps/vespa-validate-hostname src/apps/vespa-drop-file-from-cache + src/apps/vespa-tsan-digest + src/apps/vespa-validate-hostname TESTS src/tests/alloc diff --git a/vespalib/src/apps/vespa-tsan-digest/.gitignore b/vespalib/src/apps/vespa-tsan-digest/.gitignore new file mode 100644 index 00000000000..ca770e0e9c3 --- /dev/null +++ b/vespalib/src/apps/vespa-tsan-digest/.gitignore @@ -0,0 +1 @@ +/vespa-tsan-digest diff --git a/vespalib/src/apps/vespa-tsan-digest/CMakeLists.txt b/vespalib/src/apps/vespa-tsan-digest/CMakeLists.txt new file mode 100644 index 00000000000..3214d833783 --- /dev/null +++ b/vespalib/src/apps/vespa-tsan-digest/CMakeLists.txt @@ -0,0 +1,9 @@ +# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +vespa_add_executable(vespalib_vespa-tsan-digest_app + SOURCES + tsan_digest.cpp + OUTPUT_NAME vespa-tsan-digest + INSTALL bin + DEPENDS + vespalib +) diff --git a/vespalib/src/apps/vespa-tsan-digest/tsan_digest.cpp b/vespalib/src/apps/vespa-tsan-digest/tsan_digest.cpp new file mode 100644 index 00000000000..bebb32ac1ec --- /dev/null +++ b/vespalib/src/apps/vespa-tsan-digest/tsan_digest.cpp @@ -0,0 +1,278 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include <vespa/vespalib/stllike/string.h> +#include <vespa/vespalib/util/stringfmt.h> +#include <vespa/vespalib/util/size_literals.h> +#include <xxhash.h> +#include <cassert> +#include <vector> +#include <map> +#include <memory> +#include <algorithm> +#include <unistd.h> +#include <string.h> + +using vespalib::make_string_short::fmt; + +constexpr auto npos = vespalib::string::npos; + +//----------------------------------------------------------------------------- + +size_t trace_limit = 7; + +//----------------------------------------------------------------------------- + +class Hasher { +private: + XXH3_state_t *_state; +public: + Hasher() : _state(XXH3_createState()) { assert(_state != nullptr && "Out of memory!"); } + ~Hasher() { XXH3_freeState(_state); } + void reset() { XXH3_64bits_reset(_state); } + void update(const char *buf, size_t len) { XXH3_64bits_update(_state, buf, len); } + uint64_t get() const { return XXH3_64bits_digest(_state); } +}; + +uint64_t get_hash(const std::vector<vespalib::string> &list) { + static Hasher hasher; + hasher.reset(); + for (const auto &item: list) { + hasher.update(item.data(), item.size()); + } + return hasher.get(); +} + +//----------------------------------------------------------------------------- + +class StackTrace { +private: + vespalib::string _heading; + std::vector<vespalib::string> _frames; + uint64_t _hash; +public: + StackTrace(const vespalib::string &heading) noexcept + : _heading(heading), _frames(), _hash() {} + void add_frame(const vespalib::string &frame) { + _frames.push_back(frame); + } + void done() { _hash = get_hash(_frames); } + uint64_t hash() const { return _hash; } + void dump(FILE *dst) const { + fprintf(dst, "%s\n", _heading.c_str()); + for (const auto &frame: _frames) { + fprintf(dst, "%s\n", frame.c_str()); + } + fprintf(dst, "\n"); + } +}; + +vespalib::string make_trace_heading(const vespalib::string &line) { + auto pos = line.find(" at 0x"); + if ((pos != npos) && (line.find("Location") == npos)) { + return line.substr(0, pos); + } + return line; +} + +std::vector<StackTrace> extract_traces(const std::vector<vespalib::string> &lines, size_t cutoff) { + std::vector<StackTrace> result; + for (size_t i = 1; (i < lines.size()) && (result.size() < cutoff); ++i) { + auto pos = lines[i].find("#0 "); + if (pos != npos) { + size_t start = i; + result.emplace_back(make_trace_heading(lines[i - 1])); + result.back().add_frame(lines[i]); + for (i = i + 1; i < lines.size(); ++i) { + if (((i - start) > trace_limit) || + (lines[i].find("#") == npos)) + { + break; + } + result.back().add_frame(lines[i]); + } + result.back().done(); + } + } + return result; +}; + +//----------------------------------------------------------------------------- + +enum class ReportType { UNKNOWN, RACE }; + +ReportType detect_report_type(const std::vector<vespalib::string> &lines) { + for (const auto &line: lines) { + if (starts_with(line, "WARNING: ThreadSanitizer: data race")) { + return ReportType::RACE; + } + } + return ReportType::UNKNOWN; +} + +//----------------------------------------------------------------------------- + +bool is_delimiter(const vespalib::string &line) { + // TSAN delimiter is 18=, look for at least 16= + return (line.find("================") < line.size()); +} + +void dump_delimiter(FILE *dst) { + fprintf(dst, "==================\n"); +} + +//----------------------------------------------------------------------------- + +struct Report { + using UP = std::unique_ptr<Report>; + virtual vespalib::string make_key() const = 0; + virtual void add(Report::UP report) = 0; + virtual size_t count() const = 0; + virtual void dump(FILE *dst) const = 0; + virtual ~Report() {} +}; + +class RawReport : public Report { +private: + std::vector<vespalib::string> _lines; +public: + RawReport(const std::vector<vespalib::string> &lines) + : _lines(lines) {} + vespalib::string make_key() const override { + return fmt("raw:%zu", get_hash(_lines)); + } + void add(Report::UP) override { + fprintf(stderr, "WARNING: hash collision for raw report\n"); + } + size_t count() const override { return 1; } + void dump(FILE *dst) const override { + for (const auto &line: _lines) { + fprintf(dst, "%s\n", line.c_str()); + } + } +}; + +class RaceReport : public Report { +private: + StackTrace _trace1; + StackTrace _trace2; + size_t _total; + size_t _inverted; + +public: + RaceReport(const StackTrace &trace1, const StackTrace &trace2) + : _trace1(trace1), _trace2(trace2), _total(1), _inverted(0) {} + + vespalib::string make_key() const override { + if (_trace2.hash() < _trace1.hash()) { + return fmt("race:%zu,%zu", _trace2.hash(), _trace1.hash()); + } + return fmt("race:%zu,%zu", _trace1.hash(), _trace2.hash()); + } + void add(Report::UP report) override { + // should have correct type due to key prefix + const RaceReport &rhs = dynamic_cast<RaceReport&>(*report); + ++_total; + if (_trace1.hash() != rhs._trace1.hash()) { + ++_inverted; + } + } + size_t count() const override { return _total; } + void dump(FILE *dst) const override { + fprintf(dst, "WARNING: ThreadSanitizer: data race\n"); + _trace1.dump(dst); + _trace2.dump(dst); + fprintf(dst, "INFO: total: %zu (inverted: %zu)\n", _total, _inverted); + } +}; + +//----------------------------------------------------------------------------- + +size_t total_reports = 0; +std::map<vespalib::string,Report::UP> reports; + +void handle_report(std::unique_ptr<Report> report) { + ++total_reports; + auto [pos, first] = reports.try_emplace(report->make_key(), std::move(report)); + if (!first) { + assert(report && "should still be valid"); + pos->second->add(std::move(report)); + } +} + +void make_report(const std::vector<vespalib::string> &lines) { + auto type = detect_report_type(lines); + if (type == ReportType::RACE) { + auto traces = extract_traces(lines, 2); + if (traces.size() == 2) { + return handle_report(std::make_unique<RaceReport>(traces[0], traces[1])); + } + } + return handle_report(std::make_unique<RawReport>(lines)); +} + +void handle_line(const vespalib::string &line) { + static bool inside = false; + static std::vector<vespalib::string> lines; + if (is_delimiter(line)) { + inside = !inside; + if (!inside && !lines.empty()) { + make_report(lines); + lines.clear(); + } + } else if (inside) { + lines.push_back(line); + } +} + +void read_input() { + char buf[64_Ki]; + bool eof = false; + vespalib::string line; + while (!eof) { + ssize_t res = read(STDIN_FILENO, buf, sizeof(buf)); + if (res < 0) { + throw fmt("error reading stdin: %s", strerror(errno)); + } + eof = (res == 0); + for (int i = 0; i < res; ++i) { + if (buf[i] == '\n') { + handle_line(line); + line.clear(); + } else { + line.push_back(buf[i]); + } + } + } + if (!line.empty()) { + handle_line(line); + } +} + +void write_output() { + std::vector<Report*> list; + list.reserve(reports.size()); + for (const auto &[key, value]: reports) { + list.push_back(value.get()); + } + std::sort(list.begin(), list.end(), + [](const auto &a, const auto &b) { + return (a->count() > b->count()); + }); + for (const auto *report: list) { + dump_delimiter(stdout); + report->dump(stdout); + dump_delimiter(stdout); + } + fprintf(stderr, "%zu reports in, %zu reports out\n", total_reports, reports.size()); +} + +int main(int, char **) { + try { + read_input(); + write_output(); + } catch (vespalib::string &err) { + fprintf(stderr, "%s\n", err.c_str()); + return 1; + } + return 0; +} diff --git a/yolean/abi-spec.json b/yolean/abi-spec.json index 45ba75d736d..6285cc54118 100644 --- a/yolean/abi-spec.json +++ b/yolean/abi-spec.json @@ -202,6 +202,7 @@ "methods": [ "public void <init>(com.yahoo.yolean.concurrent.ResourceFactory)", "public void <init>(java.util.function.Supplier)", + "public void preallocate(int)", "public final java.lang.Object alloc()", "public final void free(java.lang.Object)", "public java.util.Iterator iterator()" @@ -258,8 +259,9 @@ ], "methods": [ "public void <init>(com.yahoo.yolean.concurrent.ResourceFactory)", - "public final java.lang.Object alloc()", - "public final void free(java.lang.Object)", + "public void <init>(java.util.function.Supplier)", + "public java.lang.Object alloc()", + "public void free(java.lang.Object)", "public java.util.Iterator iterator()" ], "fields": [] diff --git a/yolean/src/main/java/com/yahoo/yolean/concurrent/ConcurrentResourcePool.java b/yolean/src/main/java/com/yahoo/yolean/concurrent/ConcurrentResourcePool.java index fae01e8ac34..bdd059f3e17 100644 --- a/yolean/src/main/java/com/yahoo/yolean/concurrent/ConcurrentResourcePool.java +++ b/yolean/src/main/java/com/yahoo/yolean/concurrent/ConcurrentResourcePool.java @@ -18,7 +18,9 @@ public class ConcurrentResourcePool<T> implements Iterable<T> { private final Queue<T> pool = new ConcurrentLinkedQueue<>(); private final Supplier<T> factory; - // TODO: Deprecate + /** @deprecated Use {@link ConcurrentResourcePool(Supplier)} instead */ + @Deprecated(forRemoval = true, since = "7") + @SuppressWarnings("removal") public ConcurrentResourcePool(ResourceFactory<T> factory) { this.factory = factory.asSupplier(); } @@ -27,6 +29,12 @@ public class ConcurrentResourcePool<T> implements Iterable<T> { this.factory = factory; } + public void preallocate(int instances) { + for (int i = 0; i < instances; i++) { + pool.offer(factory.get()); + } + } + /** * Allocates an instance of the resource to the requestor. * The resource will be allocated exclusively to the requestor until it calls free(instance). diff --git a/yolean/src/main/java/com/yahoo/yolean/concurrent/ResourceFactory.java b/yolean/src/main/java/com/yahoo/yolean/concurrent/ResourceFactory.java index 584647198a5..cc9acf69684 100644 --- a/yolean/src/main/java/com/yahoo/yolean/concurrent/ResourceFactory.java +++ b/yolean/src/main/java/com/yahoo/yolean/concurrent/ResourceFactory.java @@ -6,7 +6,7 @@ import java.util.function.Supplier; /** * @author baldersheim */ -// TODO: Deprecate +@Deprecated(forRemoval = true, since = "7") public abstract class ResourceFactory<T> { public abstract T create(); diff --git a/yolean/src/main/java/com/yahoo/yolean/concurrent/ResourcePool.java b/yolean/src/main/java/com/yahoo/yolean/concurrent/ResourcePool.java index 75f8c961349..895fa890beb 100644 --- a/yolean/src/main/java/com/yahoo/yolean/concurrent/ResourcePool.java +++ b/yolean/src/main/java/com/yahoo/yolean/concurrent/ResourcePool.java @@ -4,6 +4,7 @@ package com.yahoo.yolean.concurrent; import java.util.ArrayDeque; import java.util.Deque; import java.util.Iterator; +import java.util.function.Supplier; /** * <p>This implements a simple stack based resource pool. If you are out of resources new are allocated from the @@ -15,17 +16,24 @@ import java.util.Iterator; public final class ResourcePool<T> implements Iterable<T> { private final Deque<T> pool = new ArrayDeque<>(); - private final ResourceFactory<T> factory; + private final Supplier<T> factory; + /** @deprecated Use {@link ResourcePool( Supplier )} instead */ + @Deprecated(forRemoval = true, since = "7") + @SuppressWarnings("removal") public ResourcePool(ResourceFactory<T> factory) { + this(factory.asSupplier()); + } + + public ResourcePool(Supplier<T> factory) { this.factory = factory; } - public final T alloc() { - return pool.isEmpty() ? factory.create() : pool.pop(); + public T alloc() { + return pool.isEmpty() ? factory.get() : pool.pop(); } - public final void free(T e) { + public void free(T e) { pool.push(e); } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java index 6f914a8e9a3..05294a5435b 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/Lock.java @@ -9,6 +9,8 @@ import com.yahoo.vespa.curator.stats.ThreadLockStats; import org.apache.curator.framework.recipes.locks.InterProcessLock; import java.time.Duration; +import java.util.HashMap; +import java.util.Map; import java.util.concurrent.TimeUnit; /** @@ -21,6 +23,10 @@ import java.util.concurrent.TimeUnit; */ public class Lock implements Mutex { + private final Object monitor = new Object(); + private long nextSequenceNumber = 0; + private final Map<Long, Long> reentriesByThreadId = new HashMap<>(); + private final InterProcessLock mutex; private final String lockPath; @@ -52,14 +58,43 @@ public class Lock implements Mutex { throw new UncheckedTimeoutException("Timed out after waiting " + timeout + " to acquire lock '" + lockPath + "'"); } - threadLockStats.lockAcquired(); + + invoke(+1L, threadLockStats::lockAcquired); + } + + @FunctionalInterface + private interface TriConsumer { + void accept(String lockId, long reentryCountDiff, Map<Long, Long> reentriesByThreadId); + } + + // TODO(hakon): Remove once debugging is unnecessary + private void invoke(long reentryCountDiff, TriConsumer consumer) { + long threadId = Thread.currentThread().getId(); + final long sequenceNumber; + final Map<Long, Long> reentriesByThreadIdCopy; + synchronized (monitor) { + sequenceNumber = nextSequenceNumber++; + reentriesByThreadId.merge(threadId, reentryCountDiff, (oldValue, argumentValue) -> { + long sum = oldValue + argumentValue /* == reentryCountDiff */; + if (sum == 0) { + // Remove from map + return null; + } else { + return sum; + } + }); + reentriesByThreadIdCopy = Map.copyOf(reentriesByThreadId); + } + + String lockId = Integer.toHexString(System.identityHashCode(this)); + consumer.accept(lockId, sequenceNumber, reentriesByThreadIdCopy); } @Override public void close() { ThreadLockStats threadLockStats = LockStats.getForCurrentThread(); // Update metrics now before release() to avoid double-counting time in locked state. - threadLockStats.preRelease(); + invoke(-1L, threadLockStats::preRelease); try { mutex.release(); threadLockStats.postRelease(); @@ -72,6 +107,10 @@ public class Lock implements Mutex { protected String lockPath() { return lockPath; } + @Override + public String toString() { + return "Lock{" + lockPath + "}"; + } } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockAttempt.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockAttempt.java index 887e2cd2700..1bbd3c7c734 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockAttempt.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockAttempt.java @@ -66,7 +66,7 @@ public class LockAttempt { public String getLockPath() { return lockPath; } public Instant getTimeAcquiredWasInvoked() { return callAcquireInstant; } public Duration getAcquireTimeout() { return timeout; } - public boolean getReentry() { return reentry; } + public boolean isReentry() { return reentry; } public LockState getLockState() { return lockState; } public Optional<Instant> getTimeLockWasAcquired() { return lockAcquiredInstant; } public boolean isAcquiring() { return lockAcquiredInstant.isEmpty(); } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java index e4f78a4f9e9..ecb344dedb9 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/LockStats.java @@ -64,28 +64,33 @@ public class LockStats { } /** Must be invoked only after the first and non-reentry acquisition of the lock. */ - void notifyOfThreadHoldingLock(Thread currentThread, String lockPath) { + void notifyOfThreadHoldingLock(Thread currentThread, String lockPath, String lockId, + long sequenceNumber, Map<Long, Long> reentriesByThreadId) { Thread oldThread = lockPathsHeld.put(lockPath, currentThread); if (oldThread != null) { getLockMetrics(lockPath).incrementAcquireWithoutReleaseCount(); - logger.warning("Thread " + currentThread.getName() + - " reports it has the lock on " + lockPath + ", but thread " + oldThread.getName() + - " has not reported it released the lock"); + logger.warning("Thread " + currentThread.getName() + " reports it has the lock on " + + lockPath + ", but thread " + oldThread.getName() + + " has not reported it released the lock. " + lockId + "#" + sequenceNumber + + ", reentries by thread ID = " + reentriesByThreadId); } } /** Must be invoked only before the last and non-reentry release of the lock. */ - void notifyOfThreadReleasingLock(Thread currentThread, String lockPath) { + void notifyOfThreadReleasingLock(Thread currentThread, String lockPath, String lockId, + long sequenceNumber, Map<Long, Long> reentriesByThreadId) { Thread oldThread = lockPathsHeld.remove(lockPath); if (oldThread == null) { getLockMetrics(lockPath).incrementNakedReleaseCount(); - logger.warning("Thread " + currentThread.getName() + - " is releasing the lock " + lockPath + ", but nobody owns that lock"); + logger.warning("Thread " + currentThread.getName() + " is releasing the lock " + lockPath + + ", but nobody owns that lock. " + lockId + "#" + sequenceNumber + + ", reentries by thread ID = " + reentriesByThreadId); } else if (oldThread != currentThread) { getLockMetrics(lockPath).incrementForeignReleaseCount(); logger.warning("Thread " + currentThread.getName() + - " is releasing the lock " + lockPath + ", but it was owned by thread " - + oldThread.getName()); + " is releasing the lock " + lockPath + ", but it was owned by thread " + + oldThread.getName() + ". " + lockId + "#" + sequenceNumber + + ", reentries by thread ID = " + reentriesByThreadId); } } diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java index d4511bd04fb..7f8eafdcf7f 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/stats/ThreadLockStats.java @@ -6,6 +6,7 @@ import com.yahoo.vespa.curator.Lock; import java.time.Duration; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentLinkedDeque; @@ -97,7 +98,7 @@ public class ThreadLockStats { } /** Mutable method (see class doc) */ - public void lockAcquired() { + public void lockAcquired(String lockId, long sequenceNumber, Map<Long, Long> reentriesByThreadId) { withLastLockAttempt(lockAttempt -> { // Note on the order of lockAcquired() vs notifyOfThreadHoldingLock(): When the latter is // invoked, other threads may query e.g. isAcquired() on the lockAttempt, which would @@ -105,19 +106,21 @@ public class ThreadLockStats { // but seems better to ensure LockAttempt is updated first. lockAttempt.lockAcquired(); - if (!lockAttempt.getReentry()) { - LockStats.getGlobal().notifyOfThreadHoldingLock(thread, lockAttempt.getLockPath()); + if (!lockAttempt.isReentry()) { + LockStats.getGlobal().notifyOfThreadHoldingLock(thread, lockAttempt.getLockPath(), + lockId, sequenceNumber, reentriesByThreadId); } }); } /** Mutable method (see class doc) */ - public void preRelease() { + public void preRelease(String lockId, long sequenceNumber, Map<Long, Long> reentriesByThreadId) { withLastLockAttempt(lockAttempt -> { // Note on the order of these two statement: Same concerns apply here as in lockAcquired(). - if (!lockAttempt.getReentry()) { - LockStats.getGlobal().notifyOfThreadReleasingLock(thread, lockAttempt.getLockPath()); + if (!lockAttempt.isReentry()) { + LockStats.getGlobal().notifyOfThreadReleasingLock(thread, lockAttempt.getLockPath(), + lockId, sequenceNumber, reentriesByThreadId); } lockAttempt.preRelease(); diff --git a/zookeeper-command-line-client/pom.xml b/zookeeper-command-line-client/pom.xml index 236bd5245a9..5dfc5ab22be 100644 --- a/zookeeper-command-line-client/pom.xml +++ b/zookeeper-command-line-client/pom.xml @@ -66,7 +66,11 @@ <artifactId>snappy-java</artifactId> <scope>compile</scope> </dependency> - + <dependency> + <!-- Needed by zookeeper, which only has an optional dependency. --> + <groupId>com.github.spotbugs</groupId> + <artifactId>spotbugs-annotations</artifactId> + </dependency> </dependencies> <build> <plugins> diff --git a/zookeeper-command-line-client/src/main/java/com/yahoo/vespa/zookeeper/cli/Main.java b/zookeeper-command-line-client/src/main/java/com/yahoo/vespa/zookeeper/cli/Main.java index 7f7965f82eb..e10404cf2c7 100644 --- a/zookeeper-command-line-client/src/main/java/com/yahoo/vespa/zookeeper/cli/Main.java +++ b/zookeeper-command-line-client/src/main/java/com/yahoo/vespa/zookeeper/cli/Main.java @@ -3,8 +3,8 @@ package com.yahoo.vespa.zookeeper.cli; import com.yahoo.vespa.zookeeper.client.ZkClientConfigBuilder; import org.apache.zookeeper.ZooKeeperMain; +import org.apache.zookeeper.util.ServiceUtils; import org.slf4j.impl.SimpleLogger; - import java.io.IOException; /** @@ -20,6 +20,7 @@ public class Main { new ZkClientConfigBuilder() .toConfigProperties() .forEach(System::setProperty); + ServiceUtils.setSystemExitProcedure(System::exit); ZooKeeperMain.main(args); } } |