diff options
96 files changed, 1103 insertions, 954 deletions
diff --git a/build_settings.cmake b/build_settings.cmake index 597f2cd42ae..63535062c9b 100644 --- a/build_settings.cmake +++ b/build_settings.cmake @@ -108,7 +108,7 @@ if (VESPA_USE_SANITIZER) set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-sanitize=vptr") endif() endif() -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_C_FLAGS} ${CXX_SPECIFIC_WARN_OPTS} -std=c++2a -fdiagnostics-color=auto ${EXTRA_CXX_FLAGS}") +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_C_FLAGS} ${CXX_SPECIFIC_WARN_OPTS} -fdiagnostics-color=auto ${EXTRA_CXX_FLAGS}") if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ") else() @@ -147,9 +147,8 @@ SET(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -rdynamic" ) message("-- CMAKE_SHARED_LINKER_FLAGS is ${CMAKE_SHARED_LINKER_FLAGS}") -# Use C++ 17 -# TODO renable when cmake 3.8 is out. -# set(CMAKE_CXX_STANDARD 17) +# Use C++ 20 +set(CMAKE_CXX_STANDARD 20) # Always build shared libs if not explicitly specified set(BUILD_SHARED_LIBS ON) diff --git a/cloud-tenant-base-dependencies-enforcer/pom.xml b/cloud-tenant-base-dependencies-enforcer/pom.xml index 166236f91a0..5d5c63934d3 100644 --- a/cloud-tenant-base-dependencies-enforcer/pom.xml +++ b/cloud-tenant-base-dependencies-enforcer/pom.xml @@ -40,8 +40,8 @@ <aopalliance.version>1.0</aopalliance.version> <guava.version>27.1-jre</guava.version> <guice.version>4.2.3</guice.version> - <jackson2.version>2.15.0</jackson2.version> - <jackson-databind.version>2.15.0</jackson-databind.version> + <jackson2.version>2.15.2</jackson2.version> + <jackson-databind.version>2.15.2</jackson-databind.version> <javax.inject.version>1</javax.inject.version> <javax.servlet-api.version>3.1.0</javax.servlet-api.version> <javax.ws.rs-api.version>2.0.1</javax.ws.rs-api.version> diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java index 0e39b7b5c3a..2b55b1f1d10 100644 --- a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java +++ b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java @@ -11,12 +11,14 @@ import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.ClusterSpec; +import com.yahoo.config.provision.DataplaneToken; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.Zone; import com.yahoo.vespa.model.container.ApplicationContainerCluster; import java.net.URI; import java.security.cert.X509Certificate; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Optional; @@ -85,6 +87,7 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea private boolean allowUserFilters = true; private boolean allowMoreThanOneContentGroupDown = false; private boolean enableConditionalPutRemoveWriteRepair = false; + private List<DataplaneToken> dataplaneTokens; @Override public ModelContext.FeatureFlags featureFlags() { return this; } @Override public boolean multitenant() { return multitenant; } @@ -144,6 +147,7 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea @Override public boolean enableGlobalPhase() { return true; } // Enable global-phase by default for unit tests only @Override public boolean allowMoreThanOneContentGroupDown(ClusterSpec.Id id) { return allowMoreThanOneContentGroupDown; } @Override public boolean enableConditionalPutRemoveWriteRepair() { return enableConditionalPutRemoveWriteRepair; } + @Override public List<DataplaneToken> dataplaneTokens() { return dataplaneTokens; } public TestProperties sharedStringRepoNoReclaim(boolean sharedStringRepoNoReclaim) { this.sharedStringRepoNoReclaim = sharedStringRepoNoReclaim; @@ -384,6 +388,11 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea public TestProperties setAllowUserFilters(boolean b) { this.allowUserFilters = b; return this; } + public TestProperties setDataplaneTokens(Collection<DataplaneToken> tokens) { + this.dataplaneTokens = List.copyOf(tokens); + return this; + } + public static class Spec implements ConfigServerSpec { private final String hostName; diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Client.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Client.java index 7707949714e..1388e9647a6 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Client.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Client.java @@ -1,6 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.model.container.http; +import com.yahoo.config.provision.DataplaneToken; + import java.security.cert.X509Certificate; import java.util.List; @@ -10,19 +12,22 @@ import java.util.List; * @author mortent */ public class Client { - private String id; - private List<String> permissions; - private List<X509Certificate> certificates; - private boolean internal; - - public Client(String id, List<String> permissions, List<X509Certificate> certificates) { - this(id, permissions, certificates, false); + private final String id; + private final List<String> permissions; + private final List<X509Certificate> certificates; + private final List<DataplaneToken> tokens; + private final boolean internal; + + public Client(String id, List<String> permissions, List<X509Certificate> certificates, List<DataplaneToken> tokens) { + this(id, permissions, certificates, tokens, false); } - private Client(String id, List<String> permissions, List<X509Certificate> certificates, boolean internal) { + private Client(String id, List<String> permissions, List<X509Certificate> certificates, List<DataplaneToken> tokens, + boolean internal) { this.id = id; - this.permissions = permissions; - this.certificates = certificates; + this.permissions = List.copyOf(permissions); + this.certificates = List.copyOf(certificates); + this.tokens = List.copyOf(tokens); this.internal = internal; } @@ -38,11 +43,13 @@ public class Client { return certificates; } + public List<DataplaneToken> tokens() { return tokens; } + public boolean internal() { return internal; } public static Client internalClient(List<X509Certificate> certificates) { - return new Client("_internal", List.of("read","write"), certificates, true); + return new Client("_internal", List.of("read","write"), certificates, List.of(), true); } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/CloudDataPlaneFilter.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/CloudDataPlaneFilter.java index 6767a61d02b..2217b58c508 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/CloudDataPlaneFilter.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/CloudDataPlaneFilter.java @@ -4,6 +4,8 @@ package com.yahoo.vespa.model.container.xml; import com.yahoo.component.ComponentSpecification; import com.yahoo.component.chain.dependencies.Dependencies; import com.yahoo.component.chain.model.ChainedComponentModel; +import com.yahoo.config.model.deploy.DeployState; +import com.yahoo.config.provision.DataplaneToken; import com.yahoo.container.bundle.BundleInstantiationSpecification; import com.yahoo.jdisc.http.filter.security.cloud.config.CloudDataPlaneFilterConfig; import com.yahoo.security.X509CertificateUtils; @@ -11,6 +13,7 @@ import com.yahoo.vespa.model.container.ApplicationContainerCluster; import com.yahoo.vespa.model.container.http.Client; import com.yahoo.vespa.model.container.http.Filter; +import java.util.Collection; import java.util.List; class CloudDataPlaneFilter extends Filter implements CloudDataPlaneFilterConfig.Producer { @@ -18,13 +21,17 @@ class CloudDataPlaneFilter extends Filter implements CloudDataPlaneFilterConfig. private static final String CLASS = "com.yahoo.jdisc.http.filter.security.cloud.CloudDataPlaneFilter"; private static final String BUNDLE = "jdisc-security-filters"; - private final ApplicationContainerCluster cluster; - private final boolean legacyMode; + private final Collection<Client> clients; + private final boolean clientsLegacyMode; + private final String tokenContext; - CloudDataPlaneFilter(ApplicationContainerCluster cluster, boolean legacyMode) { + CloudDataPlaneFilter(ApplicationContainerCluster cluster, DeployState state) { super(model()); - this.cluster = cluster; - this.legacyMode = legacyMode; + this.clients = List.copyOf(cluster.getClients()); + this.clientsLegacyMode = cluster.clientsLegacyMode(); + // Token domain must be identical to the domain used for generating the tokens + this.tokenContext = "Vespa Cloud tenant data plane:%s" + .formatted(state.getProperties().applicationId().tenant().value()); } private static ChainedComponentModel model() { @@ -36,18 +43,26 @@ class CloudDataPlaneFilter extends Filter implements CloudDataPlaneFilterConfig. @Override public void getConfig(CloudDataPlaneFilterConfig.Builder builder) { - if (legacyMode) { + if (clientsLegacyMode) { builder.legacyMode(true); } else { - List<Client> clients = cluster.getClients(); - builder.legacyMode(false); - List<CloudDataPlaneFilterConfig.Clients.Builder> clientsList = clients.stream() + var clientsCfg = clients.stream() .map(x -> new CloudDataPlaneFilterConfig.Clients.Builder() .id(x.id()) .certificates(X509CertificateUtils.toPem(x.certificates())) + .tokens(tokensConfig(x.tokens())) .permissions(x.permissions())) .toList(); - builder.clients(clientsList); + builder.clients(clientsCfg).legacyMode(false).tokenContext(tokenContext); } } + + private static List<CloudDataPlaneFilterConfig.Clients.Tokens.Builder> tokensConfig(Collection<DataplaneToken> tokens) { + return tokens.stream() + .map(token -> new CloudDataPlaneFilterConfig.Clients.Tokens.Builder() + .id(token.tokenId()) + .fingerprints(token.versions().stream().map(DataplaneToken.Version::fingerprint).toList()) + .checkAccessHashes(token.versions().stream().map(DataplaneToken.Version::checkAccessHash).toList())) + .toList(); + } } 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 9fda25bcb00..2b5232eba8c 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 @@ -29,6 +29,7 @@ import com.yahoo.config.provision.AthenzService; import com.yahoo.config.provision.Capacity; import com.yahoo.config.provision.ClusterMembership; import com.yahoo.config.provision.ClusterSpec; +import com.yahoo.config.provision.DataplaneToken; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.NodeType; @@ -121,6 +122,7 @@ import java.util.Optional; import java.util.OptionalInt; import java.util.Set; import java.util.function.Consumer; +import java.util.function.Function; import java.util.logging.Level; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -471,7 +473,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { var dataplanePort = getDataplanePort(deployState); // Setup secure filter chain var secureChain = new HttpFilterChain("cloud-data-plane-secure", HttpFilterChain.Type.SYSTEM); - secureChain.addInnerComponent(new CloudDataPlaneFilter(cluster, cluster.clientsLegacyMode())); + secureChain.addInnerComponent(new CloudDataPlaneFilter(cluster, deployState)); cluster.getHttp().getFilterChains().add(secureChain); // Set cloud data plane filter as default request filter chain for data plane connector cluster.getHttp().getHttpServer().orElseThrow().getConnectorFactories().stream() @@ -505,15 +507,16 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { Element clientsElement = XML.getChild(spec, "clients"); boolean legacyMode = false; if (clientsElement == null) { - Client defaultClient = new Client("default", - List.of(), - getCertificates(app.getFile(Path.fromString("security/clients.pem")))); - clients = List.of(defaultClient); + clients = List.of(new Client( + "default", List.of(), getCertificates(app.getFile(Path.fromString("security/clients.pem"))), List.of())); legacyMode = true; } else { clients = XML.getChildren(clientsElement, "client").stream() - .map(this::getClient) + .flatMap(elem -> getClient(elem, deployState).stream()) .toList(); + boolean atLeastOneClientWithCertificate = clients.stream().anyMatch(client -> !client.certificates().isEmpty()); + if (!atLeastOneClientWithCertificate) + throw new IllegalArgumentException("At least one client must require a certificate"); } List<X509Certificate> operatorAndTesterCertificates = deployState.getProperties().operatorCertificates(); @@ -522,22 +525,56 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { cluster.setClients(legacyMode, clients); } - private Client getClient(Element clientElement) { - String id = XML.attribute("id", clientElement).orElseThrow(); - if (id.startsWith("_")) throw new IllegalArgumentException("Invalid client id '%s', id cannot start with '_'".formatted(id)); + private Optional<Client> getClient(Element clientElement, DeployState state) { + String clientId = XML.attribute("id", clientElement).orElseThrow(); + if (clientId.startsWith("_")) throw new IllegalArgumentException("Invalid client id '%s', id cannot start with '_'".formatted(clientId)); List<String> permissions = XML.attribute("permissions", clientElement) .map(p -> p.split(",")).stream() .flatMap(Arrays::stream) .toList(); - List<X509Certificate> x509Certificates = XML.getChildren(clientElement, "certificate").stream() - .map(certElem -> Path.fromString(certElem.getAttribute("file"))) - .map(path -> app.getFile(path)) - .filter(ApplicationFile::exists) - .map(this::getCertificates) - .flatMap(Collection::stream) + var certificates = XML.getChildren(clientElement, "certificate").stream() + .flatMap(certElem -> { + var file = app.getFile(Path.fromString(certElem.getAttribute("file"))); + if (!file.exists()) { + throw new IllegalArgumentException("Certificate file '%s' for client '%s' does not exist" + .formatted(file.getPath().getRelative(), clientId)); + } + return getCertificates(file).stream(); + }) + .toList(); + // A client cannot use both tokens and certificates + if (!certificates.isEmpty()) return Optional.of(new Client(clientId, permissions, certificates, List.of())); + + var knownTokens = state.getProperties().dataplaneTokens().stream() + .collect(Collectors.toMap(DataplaneToken::tokenId, Function.identity())); + + var referencedTokens = XML.getChildren(clientElement, "token").stream() + .map(elem -> { + var tokenId = elem.getAttribute("id"); + var token = knownTokens.get(tokenId); + if (token == null) + throw new IllegalArgumentException( + "Token '%s' for client '%s' does not exist".formatted(tokenId, clientId)); + return token; + }) + .filter(token -> { + boolean empty = token.versions().isEmpty(); + if (empty) + log.logApplicationPackage( + WARNING, "Token '%s' for client '%s' has no activate versions" + .formatted(token.tokenId(), clientId)); + return !empty; + }) .toList(); - return new Client(id, permissions, x509Certificates); + + // Don't include 'client' that refers to token without versions + if (referencedTokens.isEmpty()) { + log.log(Level.INFO, "Skipping client '%s' as it does not refer to any activate tokens".formatted(clientId)); + return Optional.empty(); + } + + return Optional.of(new Client(clientId, permissions, List.of(), referencedTokens)); } private List<X509Certificate> getCertificates(ApplicationFile file) { diff --git a/config-model/src/main/resources/schema/containercluster.rnc b/config-model/src/main/resources/schema/containercluster.rnc index b8c02b013aa..285911549d7 100644 --- a/config-model/src/main/resources/schema/containercluster.rnc +++ b/config-model/src/main/resources/schema/containercluster.rnc @@ -140,9 +140,11 @@ Clients = element clients { Client = element client { ComponentId & attribute permissions { string } & - element certificate { - attribute file { string } - }+ + ( + element certificate { attribute file { string } }+ + | + element token { attribute id { string } }+ + ) } # SEARCH: diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java index b955ada20d9..d8dfe204453 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/CertificateRemovalChangeValidatorTest.java @@ -31,9 +31,9 @@ public class CertificateRemovalChangeValidatorTest { void validate() { Instant now = LocalDate.parse("2000-01-01", DateTimeFormatter.ISO_DATE).atStartOfDay().atZone(ZoneOffset.UTC).toInstant(); - Client c1 = new Client("c1", List.of(), List.of(certificate("cn=c1"))); - Client c2 = new Client("c2", List.of(), List.of(certificate("cn=c2"))); - Client c3 = new Client("c3", List.of(), List.of(certificate("cn=c3"))); + Client c1 = new Client("c1", List.of(), List.of(certificate("cn=c1")), List.of()); + Client c2 = new Client("c2", List.of(), List.of(certificate("cn=c2")), List.of()); + Client c3 = new Client("c3", List.of(), List.of(certificate("cn=c3")), List.of()); Client internal = Client.internalClient(List.of(certificate("cn=internal"))); CertificateRemovalChangeValidator validator = new CertificateRemovalChangeValidator(); diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/CloudDataPlaneFilterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/CloudDataPlaneFilterTest.java index 94d92b355f9..5bb0254f1cc 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/CloudDataPlaneFilterTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/CloudDataPlaneFilterTest.java @@ -6,6 +6,7 @@ import com.yahoo.config.model.builder.xml.test.DomBuilderTest; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.deploy.TestProperties; import com.yahoo.config.model.test.MockApplicationPackage; +import com.yahoo.config.provision.DataplaneToken; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.SystemName; @@ -36,12 +37,14 @@ import java.security.KeyPair; import java.security.cert.X509Certificate; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.Collection; import java.util.List; import java.util.Optional; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertIterableEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -118,6 +121,41 @@ public class CloudDataPlaneFilterTest extends ContainerModelBuilderTestBase { } @Test + void generates_correct_config_for_tokens() throws IOException { + var certFile = securityFolder.resolve("foo.pem"); + var clusterElem = DomBuilderTest.parse( + """ + <container version='1.0'> + <clients> + <client id="foo" permissions="read,write"> + <certificate file="%s"/> + </client> + <client id="bar" permissions="read"> + <token id="my-token"/> + </client> + </clients> + </container> + """ + .formatted(applicationFolder.toPath().relativize(certFile).toString())); + createCertificate(certFile); + buildModel(clusterElem); + + var cfg = root.getConfig(CloudDataPlaneFilterConfig.class, cloudDataPlaneFilterConfigId); + var tokenClient = cfg.clients().stream().filter(c -> c.id().equals("bar")).findAny().orElse(null); + assertNotNull(tokenClient); + assertEquals(List.of("read"), tokenClient.permissions()); + var expectedTokenCfg = tokenConfig( + "my-token", List.of("myfingerprint1", "myfingerprint2"), List.of("myaccesshash1", "myaccesshash2")); + assertEquals(List.of(expectedTokenCfg), tokenClient.tokens()); + } + + private static CloudDataPlaneFilterConfig.Clients.Tokens tokenConfig( + String id, Collection<String> fingerprints, Collection<String> accessCheckHashes) { + return new CloudDataPlaneFilterConfig.Clients.Tokens.Builder() + .id(id).fingerprints(fingerprints).checkAccessHashes(accessCheckHashes).build(); + } + + @Test public void it_rejects_files_without_certificates() throws IOException { Path certFile = securityFolder.resolve("foo.pem"); Element clusterElem = DomBuilderTest.parse( @@ -189,6 +227,9 @@ public class CloudDataPlaneFilterTest extends ContainerModelBuilderTestBase { .properties( new TestProperties() .setEndpointCertificateSecrets(Optional.of(new EndpointCertificateSecrets("CERT", "KEY"))) + .setDataplaneTokens(List.of(new DataplaneToken("my-token", List.of( + new DataplaneToken.Version("myfingerprint1", "myaccesshash1"), + new DataplaneToken.Version("myfingerprint2", "myaccesshash2"))))) .setHostedVespa(true)) .zone(new Zone(SystemName.PublicCd, Environment.dev, RegionName.defaultName())) .build(); diff --git a/config-model/src/test/schema-test-files/services.xml b/config-model/src/test/schema-test-files/services.xml index 8806a4e082a..e5cb7e8ef54 100644 --- a/config-model/src/test/schema-test-files/services.xml +++ b/config-model/src/test/schema-test-files/services.xml @@ -218,6 +218,13 @@ <certificate file="security/file1.pem" /> <certificate file="security/file2.pem" /> </client> + <client id="client3" permissions="read"> + <token id="my-token-1" /> + <token id="my-token-2" /> + </client> + <client id="client4" permissions="write"> + <token id="my-token-3" /> + </client> </clients> <document-processing> diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/ApplicationId.java b/config-provisioning/src/main/java/com/yahoo/config/provision/ApplicationId.java index c8712064cc0..49e0b0f478d 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/ApplicationId.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/ApplicationId.java @@ -5,8 +5,11 @@ import com.yahoo.cloud.config.ApplicationIdConfig; import java.util.Comparator; import java.util.Objects; +import java.util.logging.Logger; import java.util.regex.Pattern; +import static java.util.logging.Level.SEVERE; + /** * A complete, immutable identification of an application instance. * @@ -16,6 +19,8 @@ import java.util.regex.Pattern; */ public class ApplicationId implements Comparable<ApplicationId> { + private static final Logger log = Logger.getLogger(ApplicationId.class.getName()); + static final Pattern namePattern = Pattern.compile("[a-zA-Z0-9_-]{1,256}"); private static final ApplicationId global = new ApplicationId(TenantName.from("hosted-vespa"), @@ -53,19 +58,15 @@ public class ApplicationId implements Comparable<ApplicationId> { return new ApplicationId(TenantName.from(tenant), ApplicationName.from(application), InstanceName.from(instance)); } - public static ApplicationId fromSerializedForm(String idString) { - String[] parts = idString.split(":"); - if (parts.length < 3) - throw new IllegalArgumentException("Application ids must be on the form tenant:application:instance, but was " + idString); - - return from(parts[0], parts[1], parts[2]); - } + public static ApplicationId fromSerializedForm(String idString) { return fromIdString(idString, ":"); } - public static ApplicationId fromFullString(String idString) { - String[] parts = idString.split("\\."); - if (parts.length < 3) - throw new IllegalArgumentException("Application ids must be on the form tenant.application.instance, but was " + idString); + public static ApplicationId fromFullString(String idString) { return fromIdString(idString, "."); } + private static ApplicationId fromIdString(String idString, String splitCharacter) { + String[] parts = idString.split(Pattern.quote(splitCharacter)); + if (parts.length != 3) + throw new IllegalArgumentException("Application ids must be on the form tenant" + + splitCharacter + "application" + splitCharacter + "instance, but was " + idString); return from(parts[0], parts[1], parts[2]); } diff --git a/config-provisioning/src/test/java/com/yahoo/config/provision/ApplicationIdTest.java b/config-provisioning/src/test/java/com/yahoo/config/provision/ApplicationIdTest.java index 0a60b868bde..c4896a8d78b 100644 --- a/config-provisioning/src/test/java/com/yahoo/config/provision/ApplicationIdTest.java +++ b/config-provisioning/src/test/java/com/yahoo/config/provision/ApplicationIdTest.java @@ -18,7 +18,6 @@ import static org.junit.jupiter.api.Assertions.assertThrows; /** * @author Ulf Lilleengen * @author vegard - * @since 5.1 */ public class ApplicationIdTest { @@ -81,9 +80,17 @@ public class ApplicationIdTest { @Test void require_that_invalid_idstring_throws_exception() { - assertThrows(IllegalArgumentException.class, () -> { - ApplicationId.fromSerializedForm("foo:baz"); - }); + var e = assertThrows(IllegalArgumentException.class, () -> ApplicationId.fromSerializedForm("foo:baz")); + assertEquals("Application ids must be on the form tenant:application:instance, but was foo:baz", e.getMessage()); + + e = assertThrows(IllegalArgumentException.class, () -> ApplicationId.fromFullString("foo.baz")); + assertEquals("Application ids must be on the form tenant.application.instance, but was foo.baz", e.getMessage()); + + e = assertThrows(IllegalArgumentException.class, () -> ApplicationId.fromSerializedForm("foo:baz:bar:xyzzy")); + assertEquals("Application ids must be on the form tenant:application:instance, but was foo:baz:bar:xyzzy", e.getMessage()); + + e = assertThrows(IllegalArgumentException.class, () -> ApplicationId.fromFullString("foo.baz.bar.xyzzy")); + assertEquals("Application ids must be on the form tenant.application.instance, but was foo.baz.bar.xyzzy", e.getMessage()); } @Test diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/state/CoredumpGatherer.java b/container-core/src/main/java/com/yahoo/container/jdisc/state/CoredumpGatherer.java deleted file mode 100644 index c82f0cb436f..00000000000 --- a/container-core/src/main/java/com/yahoo/container/jdisc/state/CoredumpGatherer.java +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.container.jdisc.state; - -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.node.ObjectNode; -import com.yahoo.vespa.defaults.Defaults; - -import java.io.IOException; -import java.io.UncheckedIOException; -import java.nio.file.NoSuchFileException; -import java.nio.file.Path; -import java.time.Instant; -import java.util.stream.Stream; - -/** - * @author olaa - */ -public class CoredumpGatherer { - - private static final ObjectMapper jsonMapper = new ObjectMapper(); - - private static final Path COREDUMP_PATH = Path.of(Defaults.getDefaults().underVespaHome("var/crash/processing")); - - public static JsonNode gatherCoredumpMetrics(FileWrapper fileWrapper) { - int coredumps = getNumberOfCoredumps(fileWrapper); - ObjectNode packet = jsonMapper.createObjectNode(); - packet.put("status_code", coredumps == 0 ? 0 : 1); - packet.put("status_msg", coredumps == 0 ? "OK" : String.format("Found %d coredump(s)", coredumps)); - packet.put("timestamp", Instant.now().getEpochSecond()); - packet.put("application", "system-coredumps-processing"); - return packet; - } - - private static int getNumberOfCoredumps(FileWrapper fileWrapper) { - try (Stream<Path> stream = fileWrapper.walkTree(COREDUMP_PATH)){ - return (int) stream - .filter(fileWrapper::isRegularFile) - .count(); - } catch (NoSuchFileException e) { - return 0; - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } -} diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/state/MetricGatherer.java b/container-core/src/main/java/com/yahoo/container/jdisc/state/MetricGatherer.java index fbe6c5c0aec..103a4363ac2 100644 --- a/container-core/src/main/java/com/yahoo/container/jdisc/state/MetricGatherer.java +++ b/container-core/src/main/java/com/yahoo/container/jdisc/state/MetricGatherer.java @@ -16,7 +16,6 @@ public class MetricGatherer { static List<JsonNode> getAdditionalMetrics() { FileWrapper fileWrapper = new FileWrapper(); List<JsonNode> packetList = new ArrayList<>(); - packetList.add(CoredumpGatherer.gatherCoredumpMetrics(fileWrapper)); if (System.getProperty("os.name").contains("nux")) packetList.add(HostLifeGatherer.getHostLifePacket(fileWrapper)); return packetList; diff --git a/container-core/src/main/java/com/yahoo/container/jdisc/state/MetricsPacketsHandler.java b/container-core/src/main/java/com/yahoo/container/jdisc/state/MetricsPacketsHandler.java index df4f4bcb9ea..e06c7c8aa32 100644 --- a/container-core/src/main/java/com/yahoo/container/jdisc/state/MetricsPacketsHandler.java +++ b/container-core/src/main/java/com/yahoo/container/jdisc/state/MetricsPacketsHandler.java @@ -51,25 +51,20 @@ public class MetricsPacketsHandler extends AbstractRequestHandler { static final String APPLICATION_KEY = "application"; static final String TIMESTAMP_KEY = "timestamp"; - static final String STATUS_CODE_KEY = "status_code"; - static final String STATUS_MSG_KEY = "status_msg"; static final String METRICS_KEY = "metrics"; static final String DIMENSIONS_KEY = "dimensions"; static final String PACKET_SEPARATOR = "\n\n"; - private final StateMonitor monitor; private final Timer timer; private final SnapshotProvider snapshotProvider; private final String applicationName; private final String hostDimension; @Inject - public MetricsPacketsHandler(StateMonitor monitor, - Timer timer, + public MetricsPacketsHandler(Timer timer, ComponentRegistry<SnapshotProvider> snapshotProviders, MetricsPacketsHandlerConfig config) { - this.monitor = monitor; this.timer = timer; snapshotProvider = getSnapshotProviderOrThrow(snapshotProviders); applicationName = config.application(); @@ -105,7 +100,7 @@ public class MetricsPacketsHandler extends AbstractRequestHandler { return buildPrometheusOutput(); } - String output = jsonToString(getStatusPacket()) + getAllMetricsPackets() + "\n"; + String output = getAllMetricsPackets() + "\n"; return output.getBytes(StandardCharsets.UTF_8); } catch (JsonProcessingException e) { throw new RuntimeException("Bad JSON construction.", e); @@ -117,7 +112,6 @@ public class MetricsPacketsHandler extends AbstractRequestHandler { private byte[] getMetricsArray() throws JsonProcessingException { ObjectNode root = jsonMapper.createObjectNode(); ArrayNode jsonArray = jsonMapper.createArrayNode(); - jsonArray.add(getStatusPacket()); getPacketsForSnapshot(getSnapshot(), applicationName, timer.currentTimeMillis()) .forEach(jsonArray::add); MetricGatherer.getAdditionalMetrics().forEach(jsonArray::add); @@ -133,19 +127,6 @@ public class MetricsPacketsHandler extends AbstractRequestHandler { return PrometheusHelper.buildPrometheusOutput(getSnapshot(), applicationName, timer.currentTimeMillis()); } - /** - * Exactly one status packet is added to the response. - */ - private JsonNode getStatusPacket() { - ObjectNode packet = jsonMapper.createObjectNode(); - packet.put(APPLICATION_KEY, applicationName); - - StateMonitor.Status status = monitor.status(); - packet.put(STATUS_CODE_KEY, status.ordinal()); - packet.put(STATUS_MSG_KEY, status.name()); - return packet; - } - private static String jsonToString(JsonNode jsonObject) throws JsonProcessingException { return jsonMapper.writerWithDefaultPrettyPrinter() .writeValueAsString(jsonObject); @@ -154,9 +135,11 @@ public class MetricsPacketsHandler extends AbstractRequestHandler { private String getAllMetricsPackets() throws JsonProcessingException { StringBuilder ret = new StringBuilder(); List<JsonNode> metricsPackets = getPacketsForSnapshot(getSnapshot(), applicationName, timer.currentTimeMillis()); + String delimiter = ""; for (JsonNode packet : metricsPackets) { - ret.append(PACKET_SEPARATOR); // For legibility and parsing in unit tests + ret.append(delimiter); // For legibility and parsing in unit tests ret.append(jsonToString(packet)); + delimiter = PACKET_SEPARATOR; } return ret.toString(); } diff --git a/container-core/src/main/java/com/yahoo/jdisc/http/ssl/impl/ConfiguredSslContextFactoryProvider.java b/container-core/src/main/java/com/yahoo/jdisc/http/ssl/impl/ConfiguredSslContextFactoryProvider.java index b99bc007b32..8e2f080d4ce 100644 --- a/container-core/src/main/java/com/yahoo/jdisc/http/ssl/impl/ConfiguredSslContextFactoryProvider.java +++ b/container-core/src/main/java/com/yahoo/jdisc/http/ssl/impl/ConfiguredSslContextFactoryProvider.java @@ -111,13 +111,16 @@ public class ConfiguredSslContextFactoryProvider implements SslProvider { private static boolean hasNeither(String a, String b) { return a.isBlank() && b.isBlank(); } Optional<String> getCaCertificates(ConnectorConfig.Ssl sslConfig) { + var sb = new StringBuilder(); + if (sslConfig.caCertificateFile().isBlank() && sslConfig.caCertificate().isBlank()) return Optional.empty(); if (!sslConfig.caCertificate().isBlank()) { - return Optional.of(sslConfig.caCertificate()); - } else if (!sslConfig.caCertificateFile().isBlank()) { - return Optional.of(readToString(sslConfig.caCertificateFile())); - } else { - return Optional.empty(); + sb.append(sslConfig.caCertificate()); + } + if (!sslConfig.caCertificateFile().isBlank()) { + if (sb.length() > 0) sb.append('\n'); + sb.append(readToString(sslConfig.caCertificateFile())); } + return Optional.of(sb.toString()); } private static String getPrivateKey(ConnectorConfig.Ssl config) { diff --git a/container-core/src/test/java/com/yahoo/container/jdisc/state/CoredumpGathererTest.java b/container-core/src/test/java/com/yahoo/container/jdisc/state/CoredumpGathererTest.java deleted file mode 100644 index 5cec6c471fe..00000000000 --- a/container-core/src/test/java/com/yahoo/container/jdisc/state/CoredumpGathererTest.java +++ /dev/null @@ -1,42 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.container.jdisc.state; - -import com.fasterxml.jackson.databind.JsonNode; -import org.junit.jupiter.api.Test; - -import java.nio.file.Path; -import java.util.stream.Stream; - -import static org.junit.jupiter.api.Assertions.assertEquals; - - -/** - * @author olaa - */ -public class CoredumpGathererTest { - - @Test - void finds_one_coredump() { - JsonNode packet = CoredumpGatherer.gatherCoredumpMetrics(new MockFileWrapper()); - - assertEquals("system-coredumps-processing", packet.get("application").textValue()); - assertEquals(1, packet.get("status_code").intValue()); - assertEquals("Found 1 coredump(s)", packet.get("status_msg").textValue()); - - } - - static class MockFileWrapper extends FileWrapper { - - - @Override - Stream<Path> walkTree(Path path) { - return Stream.of(Path.of("dummy-path")); - } - - @Override - boolean isRegularFile(Path path) { - return true; - } - } - -} diff --git a/container-core/src/test/java/com/yahoo/container/jdisc/state/MetricsPacketsHandlerTest.java b/container-core/src/test/java/com/yahoo/container/jdisc/state/MetricsPacketsHandlerTest.java index 434622852e9..3f5c31e5e7f 100644 --- a/container-core/src/test/java/com/yahoo/container/jdisc/state/MetricsPacketsHandlerTest.java +++ b/container-core/src/test/java/com/yahoo/container/jdisc/state/MetricsPacketsHandlerTest.java @@ -15,8 +15,6 @@ import static com.yahoo.container.jdisc.state.MetricsPacketsHandler.APPLICATION_ import static com.yahoo.container.jdisc.state.MetricsPacketsHandler.DIMENSIONS_KEY; import static com.yahoo.container.jdisc.state.MetricsPacketsHandler.METRICS_KEY; import static com.yahoo.container.jdisc.state.MetricsPacketsHandler.PACKET_SEPARATOR; -import static com.yahoo.container.jdisc.state.MetricsPacketsHandler.STATUS_CODE_KEY; -import static com.yahoo.container.jdisc.state.MetricsPacketsHandler.STATUS_MSG_KEY; import static com.yahoo.container.jdisc.state.MetricsPacketsHandler.TIMESTAMP_KEY; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -35,30 +33,17 @@ public class MetricsPacketsHandlerTest extends StateHandlerTestBase { public void setupHandler() { metricsPacketsHandlerConfig = new MetricsPacketsHandlerConfig(new MetricsPacketsHandlerConfig.Builder() .application(APPLICATION_NAME).hostname(HOST_DIMENSION)); - metricsPacketsHandler = new MetricsPacketsHandler(monitor, timer, snapshotProviderRegistry, metricsPacketsHandlerConfig); + metricsPacketsHandler = new MetricsPacketsHandler(timer, snapshotProviderRegistry, metricsPacketsHandlerConfig); testDriver = new RequestHandlerTestDriver(metricsPacketsHandler); } @Test - void status_packet_is_returned_prior_to_first_snapshot() throws Exception { - String response = requestAsString("http://localhost/metrics-packets"); - - List<JsonNode> packets = toJsonPackets(response); - assertEquals(1, packets.size()); - - JsonNode statusPacket = packets.get(0); - assertEquals(APPLICATION_NAME, statusPacket.get(APPLICATION_KEY).asText(), statusPacket.toString()); - assertEquals(0, statusPacket.get(STATUS_CODE_KEY).asInt(), statusPacket.toString()); - assertEquals("up", statusPacket.get(STATUS_MSG_KEY).asText(), statusPacket.toString()); - } - - @Test void metrics_are_included_after_snapshot() throws Exception { createSnapshotWithCountMetric("counter", 1, null); List<JsonNode> packets = incrementTimeAndGetJsonPackets(); - assertEquals(2, packets.size()); + assertEquals(1, packets.size()); - JsonNode counterPacket = packets.get(1); + JsonNode counterPacket = packets.get(0); assertCountMetric(counterPacket, "counter.count", 1); } @@ -66,7 +51,7 @@ public class MetricsPacketsHandlerTest extends StateHandlerTestBase { void metadata_is_included_in_each_metrics_packet() throws Exception { createSnapshotWithCountMetric("counter", 1, null); List<JsonNode> packets = incrementTimeAndGetJsonPackets(); - JsonNode counterPacket = packets.get(1); + JsonNode counterPacket = packets.get(0); assertTrue(counterPacket.has(TIMESTAMP_KEY)); assertTrue(counterPacket.has(APPLICATION_KEY)); @@ -77,7 +62,7 @@ public class MetricsPacketsHandlerTest extends StateHandlerTestBase { void timestamp_resolution_is_in_seconds() throws Exception { createSnapshotWithCountMetric("counter", 1, null); List<JsonNode> packets = incrementTimeAndGetJsonPackets(); - JsonNode counterPacket = packets.get(1); + JsonNode counterPacket = packets.get(0); assertEquals(SNAPSHOT_INTERVAL / 1000L, counterPacket.get(TIMESTAMP_KEY).asLong()); } @@ -90,7 +75,7 @@ public class MetricsPacketsHandlerTest extends StateHandlerTestBase { snapshotProvider.setSnapshot(snapshot); List<JsonNode> packets = incrementTimeAndGetJsonPackets(); - JsonNode gaugeMetric = packets.get(1).get(METRICS_KEY); + JsonNode gaugeMetric = packets.get(0).get(METRICS_KEY); assertEquals(0.2, gaugeMetric.get("gauge.last").asDouble(), 0.1); assertEquals(0.2, gaugeMetric.get("gauge.average").asDouble(), 0.1); @@ -103,7 +88,7 @@ public class MetricsPacketsHandlerTest extends StateHandlerTestBase { createSnapshotWithCountMetric("counter", 1, context); List<JsonNode> packets = incrementTimeAndGetJsonPackets(); - JsonNode counterPacket = packets.get(1); + JsonNode counterPacket = packets.get(0); assertTrue(counterPacket.has(DIMENSIONS_KEY)); JsonNode dimensions = counterPacket.get(DIMENSIONS_KEY); @@ -119,8 +104,8 @@ public class MetricsPacketsHandlerTest extends StateHandlerTestBase { snapshotProvider.setSnapshot(snapshot); List<JsonNode> packets = incrementTimeAndGetJsonPackets(); - assertEquals(2, packets.size()); - JsonNode countersPacket = packets.get(1); + assertEquals(1, packets.size()); + JsonNode countersPacket = packets.get(0); assertEquals("value1", countersPacket.get(DIMENSIONS_KEY).get("dim1").asText()); assertCountMetric(countersPacket, "counter1.count", 1); @@ -137,7 +122,7 @@ public class MetricsPacketsHandlerTest extends StateHandlerTestBase { snapshotProvider.setSnapshot(snapshot); List<JsonNode> packets = incrementTimeAndGetJsonPackets(); - assertEquals(3, packets.size()); + assertEquals(2, packets.size()); } @Test @@ -150,7 +135,7 @@ public class MetricsPacketsHandlerTest extends StateHandlerTestBase { snapshotProvider.setSnapshot(snapshot); var packets = incrementTimeAndGetJsonPackets(); - assertEquals(3, packets.size()); + assertEquals(2, packets.size()); packets.forEach(packet -> { if (!packet.has(DIMENSIONS_KEY)) return; diff --git a/container-dependency-versions/pom.xml b/container-dependency-versions/pom.xml index a511e14de0a..92d06a5c1bd 100644 --- a/container-dependency-versions/pom.xml +++ b/container-dependency-versions/pom.xml @@ -243,8 +243,8 @@ <error-prone-annotations.version>2.18.0</error-prone-annotations.version> <guava.version>27.1-jre</guava.version> <guice.version>4.2.3</guice.version> - <jackson2.version>2.15.0</jackson2.version> - <jackson-databind.version>2.15.0</jackson-databind.version> + <jackson2.version>2.15.2</jackson2.version> + <jackson-databind.version>2.15.2</jackson-databind.version> <javax.inject.version>1</javax.inject.version> <javax.servlet-api.version>3.1.0</javax.servlet-api.version> <javax.ws.rs-api.version>2.0.1</javax.ws.rs-api.version> diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java b/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java index 83e793dace2..6d871b7283f 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/DataplaneProxyService.java @@ -99,7 +99,7 @@ public class DataplaneProxyService extends AbstractComponent { try { Process stopCommand = new ProcessBuilder().command( "nginx", - "-s", "reload" + "-s", "stop" ).start(); int exitCode = stopCommand.waitFor(); if (exitCode != 0) { diff --git a/container-search/src/main/java/com/yahoo/search/logging/LoggerEntry.java b/container-search/src/main/java/com/yahoo/search/logging/LoggerEntry.java index a7ef0b9300f..80ff967f779 100644 --- a/container-search/src/main/java/com/yahoo/search/logging/LoggerEntry.java +++ b/container-search/src/main/java/com/yahoo/search/logging/LoggerEntry.java @@ -123,10 +123,7 @@ public class LoggerEntry { } public Builder blob(String blob) { - byte[] bytes = Utf8.toBytes(blob); - this.blob = ByteBuffer.allocate(bytes.length); - this.blob.put(bytes); - return this; + return this.blob(Utf8.toBytes(blob)); } public Builder track(String track) { diff --git a/container-search/src/main/java/com/yahoo/search/searchers/QueryValidator.java b/container-search/src/main/java/com/yahoo/search/searchers/QueryValidator.java index 0b435c2e32d..a4abab3cbef 100644 --- a/container-search/src/main/java/com/yahoo/search/searchers/QueryValidator.java +++ b/container-search/src/main/java/com/yahoo/search/searchers/QueryValidator.java @@ -31,7 +31,7 @@ public class QueryValidator extends Searcher { public Result search(Query query, Execution execution) { var session = execution.context().schemaInfo().newSession(query); ToolBox.visit(new TermSearchValidator(session), query.getModel().getQueryTree().getRoot()); - // ToolBox.visit(new PrefixSearchValidator(session), query.getModel().getQueryTree().getRoot()); TODO: Enable check and QueryValidatorPrefixTest + ToolBox.visit(new PrefixSearchValidator(session), query.getModel().getQueryTree().getRoot()); return execution.search(query); } diff --git a/container-search/src/test/java/com/yahoo/search/searchers/test/QueryValidatorPrefixTest.java b/container-search/src/test/java/com/yahoo/search/searchers/test/QueryValidatorPrefixTest.java index b653e4d97aa..fdc8c773cf2 100644 --- a/container-search/src/test/java/com/yahoo/search/searchers/test/QueryValidatorPrefixTest.java +++ b/container-search/src/test/java/com/yahoo/search/searchers/test/QueryValidatorPrefixTest.java @@ -9,7 +9,6 @@ import com.yahoo.search.schema.SchemaInfo; import com.yahoo.search.searchchain.Execution; import com.yahoo.search.searchers.QueryValidator; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.Disabled; import java.util.List; @@ -21,7 +20,6 @@ import static org.junit.jupiter.api.Assertions.fail; */ public class QueryValidatorPrefixTest { - @Disabled @Test void testPrefixRequiresAttribute() { var indexing = new Cluster.Builder("indexing").addSchema("test1").build(); diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/application/v4/model/DeploymentData.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/application/v4/model/DeploymentData.java index 5a55768ad2c..b23b93cba78 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/application/v4/model/DeploymentData.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/application/v4/model/DeploymentData.java @@ -10,6 +10,7 @@ import com.yahoo.vespa.athenz.api.AthenzDomain; import com.yahoo.vespa.hosted.controller.api.integration.billing.Quota; import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMetadata; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ContainerEndpoint; +import com.yahoo.vespa.hosted.controller.api.integration.dataplanetoken.DataplaneTokenVersions; import com.yahoo.vespa.hosted.controller.api.integration.secrets.TenantSecretStore; import com.yahoo.yolean.concurrent.Memoized; @@ -42,6 +43,7 @@ public class DeploymentData { private final List<TenantSecretStore> tenantSecretStores; private final List<X509Certificate> operatorCertificates; private final Supplier<Optional<CloudAccount>> cloudAccount; + private final List<DataplaneTokenVersions> dataPlaneTokens; private final boolean dryRun; public DeploymentData(ApplicationId instance, ZoneId zone, Supplier<InputStream> applicationPackage, Version platform, @@ -53,6 +55,7 @@ public class DeploymentData { List<TenantSecretStore> tenantSecretStores, List<X509Certificate> operatorCertificates, Supplier<Optional<CloudAccount>> cloudAccount, + List<DataplaneTokenVersions> dataPlaneTokens, boolean dryRun) { this.instance = requireNonNull(instance); this.zone = requireNonNull(zone); @@ -66,6 +69,7 @@ public class DeploymentData { this.tenantSecretStores = List.copyOf(requireNonNull(tenantSecretStores)); this.operatorCertificates = List.copyOf(requireNonNull(operatorCertificates)); this.cloudAccount = new Memoized<>(requireNonNull(cloudAccount)); + this.dataPlaneTokens = dataPlaneTokens; this.dryRun = dryRun; } @@ -117,6 +121,10 @@ public class DeploymentData { return cloudAccount.get(); } + public List<DataplaneTokenVersions> dataPlaneTokens() { + return dataPlaneTokens; + } + public boolean isDryRun() { return dryRun; } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dataplanetoken/package-info.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dataplanetoken/package-info.java new file mode 100644 index 00000000000..09cc3a84cd3 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/dataplanetoken/package-info.java @@ -0,0 +1,5 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +@ExportPackage +package com.yahoo.vespa.hosted.controller.api.integration.dataplanetoken; + +import com.yahoo.osgi.annotation.ExportPackage;
\ No newline at end of file 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 eedc94c729c..c3e1ff1dbf2 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 @@ -41,6 +41,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.configserver.Deployment import com.yahoo.vespa.hosted.controller.api.integration.configserver.DeploymentResult.LogEntry; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node; import com.yahoo.vespa.hosted.controller.api.integration.configserver.NodeFilter; +import com.yahoo.vespa.hosted.controller.api.integration.dataplanetoken.DataplaneTokenVersions; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationStore; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ArtifactRepository; @@ -677,9 +678,10 @@ public class ApplicationController { operatorCertificates = Stream.concat(operatorCertificates.stream(), testerCertificate.stream()).toList(); } Supplier<Optional<CloudAccount>> cloudAccount = () -> decideCloudAccountOf(deployment, applicationPackage.truncatedPackage().deploymentSpec()); + List<DataplaneTokenVersions> dataplaneTokenVersions = controller.dataplaneTokenService().listTokens(application.tenant()); DeploymentData deploymentData = new DeploymentData(application, zone, applicationPackage::zipStream, platform, endpoints, endpointCertificateMetadata, dockerImageRepo, domain, - deploymentQuota, tenantSecretStores, operatorCertificates, cloudAccount, dryRun); + deploymentQuota, tenantSecretStores, operatorCertificates, cloudAccount, dataplaneTokenVersions, dryRun); ConfigServer.PreparedApplication preparedApplication = configServer.deploy(deploymentData); return new DeploymentDataAndResult(deploymentData, preparedApplication.deploymentResult()); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java index c50cc6051e1..81362018939 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java @@ -13,6 +13,9 @@ import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.zone.RoutingMethod; import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.vespa.flags.BooleanFlag; +import com.yahoo.vespa.flags.FetchVector; +import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ContainerEndpoint; import com.yahoo.vespa.hosted.controller.api.integration.dns.Record; @@ -71,6 +74,7 @@ public class RoutingController { private final Controller controller; private final RoutingPolicies routingPolicies; private final RotationRepository rotationRepository; + private final BooleanFlag createTokenEndpoint; public RoutingController(Controller controller, RotationsConfig rotationsConfig) { this.controller = Objects.requireNonNull(controller, "controller must be non-null"); @@ -78,6 +82,7 @@ public class RoutingController { this.rotationRepository = new RotationRepository(Objects.requireNonNull(rotationsConfig, "rotationsConfig must be non-null"), controller.applications(), controller.curator()); + this.createTokenEndpoint = Flags.ENABLE_DATAPLANE_PROXY.bindTo(controller.flagSource()); } /** Create a routing context for given deployment */ @@ -109,11 +114,12 @@ public class RoutingController { /** Read and return zone-scoped endpoints for given deployment */ public EndpointList readEndpointsOf(DeploymentId deployment) { + boolean addTokenEndpoint = createTokenEndpoint.with(FetchVector.Dimension.APPLICATION_ID, deployment.applicationId().serializedForm()).value(); Set<Endpoint> endpoints = new LinkedHashSet<>(); // To discover the cluster name for a zone-scoped endpoint, we need to read routing policies for (var policy : routingPolicies.read(deployment)) { RoutingMethod routingMethod = controller.zoneRegistry().routingMethod(policy.id().zone()); - endpoints.addAll(policy.zoneEndpointsIn(controller.system(), routingMethod)); + endpoints.addAll(policy.zoneEndpointsIn(controller.system(), routingMethod, addTokenEndpoint)); endpoints.add(policy.regionEndpointIn(controller.system(), routingMethod)); } return EndpointList.copyOf(endpoints); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java index 80b52e0c7a4..7b2e8d0f4ed 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java @@ -45,10 +45,11 @@ public class Endpoint { private final Scope scope; private final boolean legacy; private final RoutingMethod routingMethod; + private boolean tokenEndpoint; private Endpoint(TenantAndApplicationId application, Optional<InstanceName> instanceName, EndpointId id, ClusterSpec.Id cluster, URI url, URI legacyRegionalUrl, List<Target> targets, Scope scope, Port port, boolean legacy, - RoutingMethod routingMethod, boolean certificateName) { + RoutingMethod routingMethod, boolean certificateName, boolean tokenEndpoint) { Objects.requireNonNull(application, "application must be non-null"); Objects.requireNonNull(instanceName, "instanceName must be non-null"); Objects.requireNonNull(cluster, "cluster must be non-null"); @@ -66,6 +67,7 @@ public class Endpoint { this.scope = requireScope(scope, routingMethod); this.legacy = legacy; this.routingMethod = routingMethod; + this.tokenEndpoint = tokenEndpoint; } /** @@ -353,6 +355,10 @@ public class Endpoint { return targets; } + public boolean isTokenEndpoint() { + return tokenEndpoint; + } + /** An endpoint's scope */ public enum Scope { @@ -477,6 +483,7 @@ public class Endpoint { private RoutingMethod routingMethod = RoutingMethod.sharedLayer4; private boolean legacy = false; private boolean certificateName = false; + private boolean tokenEndpoint = false; private EndpointBuilder(TenantAndApplicationId application, Optional<InstanceName> instance) { this.application = Objects.requireNonNull(application); @@ -544,6 +551,11 @@ public class Endpoint { return this; } + public EndpointBuilder tokenEndpoint() { + this.tokenEndpoint = true; + return this; + } + /** Sets the port of this */ public EndpointBuilder on(Port port) { this.port = port; @@ -576,7 +588,8 @@ public class Endpoint { if (routingMethod.isDirect() && !port.isDefault()) { throw new IllegalArgumentException("Routing method " + routingMethod + " can only use default port"); } - URI url = createUrl(endpointOrClusterAsString(endpointId, cluster), + String prefix = tokenEndpoint ? "token-" : ""; + URI url = createUrl(prefix + endpointOrClusterAsString(endpointId, cluster), Objects.requireNonNull(application, "application must be non-null"), Objects.requireNonNull(instance, "instance must be non-null"), Objects.requireNonNull(targets, "targets must be non-null"), @@ -604,7 +617,8 @@ public class Endpoint { port, legacy, routingMethod, - certificateName); + certificateName, + tokenEndpoint); } private Scope requireUnset(Scope scope) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/dataplanetoken/DataplaneTokenService.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/dataplanetoken/DataplaneTokenService.java index 731806bd53a..b3e5f663317 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/dataplanetoken/DataplaneTokenService.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/dataplanetoken/DataplaneTokenService.java @@ -14,7 +14,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.dataplanetoken.FingerPr import com.yahoo.vespa.hosted.controller.api.integration.dataplanetoken.TokenId; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; -import java.nio.charset.StandardCharsets; import java.security.Principal; import java.util.List; import java.util.Objects; @@ -56,7 +55,7 @@ public class DataplaneTokenService { * @return a DataplaneToken containing the secret generated token */ public DataplaneToken generateToken(TenantName tenantName, TokenId tokenId, Principal principal) { - TokenDomain tokenDomain = TokenDomain.of(tenantName.value()); + TokenDomain tokenDomain = TokenDomain.of("Vespa Cloud tenant data plane:%s".formatted(tenantName.value())); Token token = TokenGenerator.generateToken(tokenDomain, TOKEN_PREFIX, TOKEN_BYTES); TokenCheckHash checkHash = TokenCheckHash.of(token, CHECK_HASH_BYTES); DataplaneTokenVersions.Version newTokenVersion = new DataplaneTokenVersions.Version( diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java index 7a4d9edf66c..cd632917842 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java @@ -7,6 +7,9 @@ import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.zone.RoutingMethod; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.transaction.Mutex; +import com.yahoo.vespa.flags.BooleanFlag; +import com.yahoo.vespa.flags.FetchVector; +import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.api.identifiers.ClusterId; @@ -43,6 +46,8 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.stream.Collectors; /** @@ -53,12 +58,16 @@ import java.util.stream.Collectors; */ public class RoutingPolicies { + private static final Logger LOG = Logger.getLogger(RoutingPolicies.class.getName()); + private final Controller controller; private final CuratorDb db; + private final BooleanFlag createTokenEndpoint; public RoutingPolicies(Controller controller) { this.controller = Objects.requireNonNull(controller, "controller must be non-null"); this.db = controller.curator(); + this.createTokenEndpoint = Flags.ENABLE_DATAPLANE_PROXY.bindTo(controller.flagSource()); try (var lock = db.lockRoutingPolicies()) { // Update serialized format for (var policy : db.readRoutingPolicies().entrySet()) { db.writeRoutingPolicies(policy.getKey(), policy.getValue()); @@ -122,8 +131,8 @@ public class RoutingPolicies { instancePolicies = removePoliciesUnreferencedBy(allocation, instancePolicies, lock); applicationPolicies = applicationPolicies.replace(instance, instancePolicies); - updateGlobalDnsOf(instancePolicies, inactiveZones, owner, lock); - updateApplicationDnsOf(applicationPolicies, inactiveZones, owner, lock); + updateGlobalDnsOf(instancePolicies, Optional.of(deployment), inactiveZones, owner, lock); + updateApplicationDnsOf(applicationPolicies, inactiveZones, deployment, owner, lock); } } @@ -134,7 +143,7 @@ public class RoutingPolicies { controller.clock().instant()))); Map<ApplicationId, RoutingPolicyList> allPolicies = readAll().groupingBy(policy -> policy.id().owner()); allPolicies.forEach((instance, policies) -> { - updateGlobalDnsOf(policies, Set.of(), Optional.of(TenantAndApplicationId.from(instance)), lock); + updateGlobalDnsOf(policies, Optional.empty(), Set.of(), Optional.of(TenantAndApplicationId.from(instance)), lock); }); } } @@ -150,36 +159,64 @@ public class RoutingPolicies { var newPolicy = policy.with(RoutingStatus.create(value, agent, controller.clock().instant())); updatedPolicies.put(policy.id(), newPolicy); } - RoutingPolicyList effectivePolicies = RoutingPolicyList.copyOf(updatedPolicies.values()); Map<ApplicationId, RoutingPolicyList> policiesByInstance = effectivePolicies.groupingBy(policy -> policy.id().owner()); - policiesByInstance.forEach((owner, instancePolicies) -> db.writeRoutingPolicies(owner, instancePolicies.asList())); policiesByInstance.forEach((ignored, instancePolicies) -> updateGlobalDnsOf(instancePolicies, + Optional.of(deployment), Set.of(), ownerOf(deployment), lock)); - updateApplicationDnsOf(effectivePolicies, Set.of(), ownerOf(deployment), lock); + updateApplicationDnsOf(effectivePolicies, Set.of(), deployment, ownerOf(deployment), lock); + policiesByInstance.forEach((owner, instancePolicies) -> db.writeRoutingPolicies(owner, instancePolicies.asList())); } } /** Update global DNS records for given policies */ - private void updateGlobalDnsOf(RoutingPolicyList instancePolicies, Set<ZoneId> inactiveZones, - Optional<TenantAndApplicationId> owner, @SuppressWarnings("unused") Mutex lock) { + private void updateGlobalDnsOf(RoutingPolicyList instancePolicies, Optional<DeploymentId> deployment, + Set<ZoneId> inactiveZones, Optional<TenantAndApplicationId> owner, + @SuppressWarnings("unused") Mutex lock) { Map<RoutingId, List<RoutingPolicy>> routingTable = instancePolicies.asInstanceRoutingTable(); for (Map.Entry<RoutingId, List<RoutingPolicy>> routeEntry : routingTable.entrySet()) { RoutingId routingId = routeEntry.getKey(); controller.routing().readDeclaredEndpointsOf(routingId.instance()) .named(routingId.endpointId(), Endpoint.Scope.global) .not().requiresRotation() - .forEach(endpoint -> updateGlobalDnsOf(endpoint, inactiveZones, routeEntry.getValue(), owner)); + .forEach(endpoint -> updateGlobalDnsOf(endpoint, inactiveZones, routeEntry.getValue(), deployment, owner)); } } /** Update global DNS records for given global endpoint */ - private void updateGlobalDnsOf(Endpoint endpoint, Set<ZoneId> inactiveZones, List<RoutingPolicy> policies, Optional<TenantAndApplicationId> owner) { + private void updateGlobalDnsOf(Endpoint endpoint, Set<ZoneId> inactiveZones, List<RoutingPolicy> policies, + Optional<DeploymentId> deployment, Optional<TenantAndApplicationId> owner) { if (endpoint.scope() != Endpoint.Scope.global) throw new IllegalArgumentException("Endpoint " + endpoint + " is not global"); - // Create a weighted ALIAS per region, pointing to all zones within the same region + if (deployment.isPresent() && !endpoint.deployments().contains(deployment.get())) return; + Collection<RegionEndpoint> regionEndpoints = computeRegionEndpoints(policies, inactiveZones); + Set<AliasTarget> latencyTargets = new LinkedHashSet<>(); + Set<AliasTarget> inactiveLatencyTargets = new LinkedHashSet<>(); + for (var regionEndpoint : regionEndpoints) { + if (regionEndpoint.active()) { + latencyTargets.add(regionEndpoint.target()); + } else { + inactiveLatencyTargets.add(regionEndpoint.target()); + } + } + + // Refuse removal of last target in an endpoint. We do this because removing 100% of the ALIAS records would + // cause the application endpoint to stop resolving entirely (NXDOMAIN). + if (latencyTargets.isEmpty() && !inactiveLatencyTargets.isEmpty()) { + if (deployment.isPresent()) { + throw new IllegalArgumentException("Cannot deactivate routing for " + deployment.get() + + " as it's the last remaining active deployment in " + endpoint); + } else { + // Operator is deactivating routing for entire zone, but this endpoint only has one target + LOG.log(Level.WARNING, "Cannot deactivate routing for " + endpoint + " because it has only one " + + "active zone. Leaving it in"); + return; + } + } + + // Create a weighted ALIAS per region, pointing to all zones within the same region regionEndpoints.forEach(regionEndpoint -> { if ( ! regionEndpoint.zoneAliasTargets().isEmpty()) { controller.nameServiceForwarder().createAlias(RecordName.from(regionEndpoint.target().name().value()), @@ -196,23 +233,6 @@ public class RoutingPolicies { }); // Create global latency-based ALIAS pointing to each per-region weighted ALIAS - Set<AliasTarget> latencyTargets = new LinkedHashSet<>(); - Set<AliasTarget> inactiveLatencyTargets = new LinkedHashSet<>(); - for (var regionEndpoint : regionEndpoints) { - if (regionEndpoint.active()) { - latencyTargets.add(regionEndpoint.target()); - } else { - inactiveLatencyTargets.add(regionEndpoint.target()); - } - } - - // If all targets are configured OUT, all targets are kept IN. We do this because otherwise removing 100% of - // the ALIAS records would cause the global endpoint to stop resolving entirely (NXDOMAIN). - if (latencyTargets.isEmpty() && !inactiveLatencyTargets.isEmpty()) { - latencyTargets.addAll(inactiveLatencyTargets); - inactiveLatencyTargets.clear(); - } - controller.nameServiceForwarder().createAlias(RecordName.from(endpoint.dnsName()), latencyTargets, Priority.normal, owner); inactiveLatencyTargets.forEach(t -> controller.nameServiceForwarder() .removeRecords(Record.Type.ALIAS, @@ -254,7 +274,8 @@ public class RoutingPolicies { private void updateApplicationDnsOf(RoutingPolicyList routingPolicies, Set<ZoneId> inactiveZones, - Optional<TenantAndApplicationId> owner, @SuppressWarnings("unused") Mutex lock) { + DeploymentId deployment, Optional<TenantAndApplicationId> owner, + @SuppressWarnings("unused") Mutex lock) { // In the context of single deployment (which this is) there is only one routing policy per routing ID. I.e. // there is no scenario where more than one deployment within an instance can be a member the same // application-level endpoint. However, to allow this in the future the routing table remains @@ -281,8 +302,7 @@ public class RoutingPolicies { Set<Target> inactiveTargets = inactiveTargetsByEndpoint.computeIfAbsent(endpoint, (k) -> new LinkedHashSet<>()); if (isConfiguredOut(zonePolicy, policy, inactiveZones)) { inactiveTargets.add(Target.weighted(policy, target)); - } - else { + } else { activeTargets.add(Target.weighted(policy, target)); } } @@ -290,39 +310,49 @@ public class RoutingPolicies { } } - // If all targets are configured OUT, all targets are kept IN. We do this because otherwise removing 100% of - // the ALIAS records would cause the application endpoint to stop resolving entirely (NXDOMAIN). + // Refuse removal of last target in an endpoint. We do this because removing 100% of the ALIAS records would + // cause the application endpoint to stop resolving entirely (NXDOMAIN). targetsByEndpoint.forEach((endpoint, targets) -> { - if (targets.isEmpty()) targets.addAll(inactiveTargetsByEndpoint.remove(endpoint)); + if (targets.isEmpty()) { + throw new IllegalArgumentException("Cannot deactivate routing for " + deployment + + " as it's the last remaining active deployment in " + endpoint); + } }); + // Create DNS records for active targets targetsByEndpoint.forEach((applicationEndpoint, targets) -> { // Where multiple zones are permitted, they all have the same routing policy, and nameServiceForwarder (below). ZoneId targetZone = applicationEndpoint.targets().iterator().next().deployment().zoneId(); Set<AliasTarget> aliasTargets = new LinkedHashSet<>(); Set<DirectTarget> directTargets = new LinkedHashSet<>(); for (Target target : targets) { - if (target.aliasOrDirectTarget() instanceof AliasTarget at) aliasTargets.add(at); - else directTargets.add((DirectTarget) target.aliasOrDirectTarget()); + if (!target.deployment().equals(deployment)) continue; // Do not update target not matching this deployment + if (target.aliasOrDirectTarget() instanceof AliasTarget at) { + aliasTargets.add(at); + } else { + directTargets.add((DirectTarget) target.aliasOrDirectTarget()); + } } - - if ( ! aliasTargets.isEmpty()) { + if (!aliasTargets.isEmpty()) { nameServiceForwarderIn(targetZone).createAlias( RecordName.from(applicationEndpoint.dnsName()), aliasTargets, Priority.normal, owner); nameServiceForwarderIn(targetZone).removeRecords(Type.ALIAS, RecordName.from(applicationEndpoint.legacyRegionalDnsName()), Priority.normal, owner); } - if ( ! directTargets.isEmpty()) { + if (!directTargets.isEmpty()) { nameServiceForwarderIn(targetZone).createDirect( RecordName.from(applicationEndpoint.dnsName()), directTargets, Priority.normal, owner); nameServiceForwarderIn(targetZone).removeRecords(Type.DIRECT, RecordName.from(applicationEndpoint.legacyRegionalDnsName()), Priority.normal, owner); } }); + + // Remove DNS records for inactive targets inactiveTargetsByEndpoint.forEach((applicationEndpoint, targets) -> { // Where multiple zones are permitted, they all have the same routing policy, and nameServiceForwarder. ZoneId targetZone = applicationEndpoint.targets().iterator().next().deployment().zoneId(); targets.forEach(target -> { + if (!target.deployment().equals(deployment)) return; // Do not update target not matching this deployment nameServiceForwarderIn(targetZone).removeRecords(target.type(), RecordName.from(applicationEndpoint.dnsName()), target.data(), @@ -369,7 +399,8 @@ public class RoutingPolicies { /** Update zone DNS record for given policy */ private void updateZoneDnsOf(RoutingPolicy policy, LoadBalancer loadBalancer, DeploymentId deploymentId) { - for (var endpoint : policy.zoneEndpointsIn(controller.system(), RoutingMethod.exclusive)) { + boolean addTokenEndpoint = createTokenEndpoint.with(FetchVector.Dimension.APPLICATION_ID, deploymentId.applicationId().serializedForm()).value(); + for (var endpoint : policy.zoneEndpointsIn(controller.system(), RoutingMethod.exclusive, addTokenEndpoint)) { var name = RecordName.from(endpoint.dnsName()); var record = policy.canonicalName().isPresent() ? new Record(Record.Type.CNAME, name, RecordData.fqdn(policy.canonicalName().get().value())) : @@ -381,6 +412,7 @@ public class RoutingPolicies { private void setPrivateDns(Endpoint endpoint, LoadBalancer loadBalancer, DeploymentId deploymentId) { if (loadBalancer.service().isEmpty()) return; + if (endpoint.isTokenEndpoint()) return; controller.serviceRegistry().vpcEndpointService() .setPrivateDns(DomainName.of(endpoint.dnsName()), new ClusterId(deploymentId, endpoint.cluster()), @@ -437,12 +469,13 @@ public class RoutingPolicies { * @return the updated policies */ private RoutingPolicyList removePoliciesUnreferencedBy(LoadBalancerAllocation allocation, RoutingPolicyList instancePolicies, @SuppressWarnings("unused") Mutex lock) { + boolean addTokenEndpoint = createTokenEndpoint.with(FetchVector.Dimension.APPLICATION_ID, allocation.deployment.applicationId().serializedForm()).value(); Map<RoutingPolicyId, RoutingPolicy> newPolicies = new LinkedHashMap<>(instancePolicies.asMap()); Set<RoutingPolicyId> activeIds = allocation.asPolicyIds(); RoutingPolicyList removable = instancePolicies.deployment(allocation.deployment) .not().matching(policy -> activeIds.contains(policy.id())); for (var policy : removable) { - for (var endpoint : policy.zoneEndpointsIn(controller.system(), RoutingMethod.exclusive)) { + for (var endpoint : policy.zoneEndpointsIn(controller.system(), RoutingMethod.exclusive, addTokenEndpoint)) { nameServiceForwarderIn(allocation.deployment.zoneId()).removeRecords(Record.Type.CNAME, RecordName.from(endpoint.dnsName()), Priority.normal, @@ -676,16 +709,16 @@ public class RoutingPolicies { } /** Denotes record data (record rhs) of either an ALIAS or a DIRECT target */ - private record Target(Record.Type type, RecordData data, Object aliasOrDirectTarget) { + private record Target(Record.Type type, RecordData data, DeploymentId deployment, Object aliasOrDirectTarget) { static Target weighted(RoutingPolicy policy, Endpoint.Target endpointTarget) { if (policy.ipAddress().isPresent()) { var wt = new WeightedDirectTarget(RecordData.from(policy.ipAddress().get()), endpointTarget.deployment().zoneId(), endpointTarget.weight()); - return new Target(Record.Type.DIRECT, wt.recordData(), wt); + return new Target(Record.Type.DIRECT, wt.recordData(), endpointTarget.deployment(), wt); } var wt = new WeightedAliasTarget(policy.canonicalName().get(), policy.dnsZone().get(), endpointTarget.deployment().zoneId().value(), endpointTarget.weight()); - return new Target(Record.Type.ALIAS, RecordData.fqdn(wt.name().value()), wt); + return new Target(Record.Type.ALIAS, RecordData.fqdn(wt.name().value()), endpointTarget.deployment(), wt); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicy.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicy.java index b4d83b7ded6..fb8f5e8e129 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicy.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicy.java @@ -104,9 +104,15 @@ public record RoutingPolicy(RoutingPolicyId id, } /** Returns the zone endpoints of this */ - public List<Endpoint> zoneEndpointsIn(SystemName system, RoutingMethod routingMethod) { + public List<Endpoint> zoneEndpointsIn(SystemName system, RoutingMethod routingMethod, boolean includeTokenEndpoint) { DeploymentId deployment = new DeploymentId(id.owner(), id.zone()); - return List.of(endpoint(routingMethod).target(id.cluster(), deployment).in(system)); + Endpoint zoneEndpoint = endpoint(routingMethod).target(id.cluster(), deployment).in(system); + if (includeTokenEndpoint) { + Endpoint tokenEndpoint = endpoint(routingMethod).target(id.cluster(), deployment).tokenEndpoint().in(system); + return List.of(zoneEndpoint, tokenEndpoint); + } else { + return List.of(zoneEndpoint); + } } /** Returns the region endpoint of this */ 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 874d9468941..475da3224cd 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 @@ -1545,7 +1545,7 @@ public class ControllerTest { DeploymentId deployment = context.deploymentIdIn(ZoneId.from("prod", "us-west-1")); DeploymentData deploymentData = new DeploymentData(deployment.applicationId(), deployment.zoneId(), InputStream::nullInputStream, Version.fromString("6.1"), Set.of(), Optional::empty, Optional.empty(), Optional.empty(), - Quota::unlimited, List.of(), List.of(), Optional::empty, false); + Quota::unlimited, List.of(), List.of(), Optional::empty, List.of(),false); tester.configServer().deploy(deploymentData); assertTrue(tester.configServer().application(deployment.applicationId(), deployment.zoneId()).isPresent()); tester.controller().applications().deactivate(deployment.applicationId(), deployment.zoneId()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java index 772877de8e3..0233db50ac6 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java @@ -16,6 +16,7 @@ import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.zone.RoutingMethod; import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.hosted.controller.ControllerTester; import com.yahoo.vespa.hosted.controller.Instance; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; @@ -34,6 +35,8 @@ 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.dns.NameServiceQueue; +import com.yahoo.vespa.hosted.controller.dns.RemoveRecords; import com.yahoo.vespa.hosted.controller.integration.ZoneApiMock; import com.yahoo.vespa.hosted.rotation.config.RotationsConfig; import org.junit.jupiter.api.Test; @@ -50,12 +53,15 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.stream.Collector; import java.util.stream.Collectors; +import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; /** * @author mortent @@ -117,6 +123,16 @@ public class RoutingPoliciesTest { tester.assertTargets(context1.instanceId(), EndpointId.of("r1"), 0, zone1); tester.assertTargets(context1.instanceId(), EndpointId.of("r2"), 1, zone1, zone2, zone3); + // Ensure test deployment only updates endpoints of which it is a member + context1.submit(applicationPackage2) + .runJob(DeploymentContext.systemTest); + NameServiceQueue queue = tester.controllerTester().controller().curator().readNameServiceQueue(); + assertEquals(List.of(new RemoveRecords(Optional.of(TenantAndApplicationId.from(context1.instanceId())), + Record.Type.CNAME, + RecordName.from("app1.tenant1.us-east-1.test.vespa.oath.cloud"))), + queue.requests()); + context1.completeRollout(); + // Another application is deployed with a single cluster and global endpoint var endpoint4 = "r0.app2.tenant1.global.vespa.oath.cloud"; tester.provisionLoadBalancers(1, context2.instanceId(), zone1, zone2); @@ -323,6 +339,28 @@ public class RoutingPoliciesTest { } @Test + void zone_token_endpoints() { + var tester = new RoutingPoliciesTester(); + tester.enableTokenEndpoint(true); + + var context1 = tester.newDeploymentContext("tenant1", "app1", "default"); + + // Deploy application + tester.provisionLoadBalancers(1, context1.instanceId(), false, zone1, zone2); + context1.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy(); + + // Deployment creates records and policies for all clusters in all zones + Set<String> expectedRecords = Set.of( + "c0.app1.tenant1.us-west-1.vespa.oath.cloud", + "token-c0.app1.tenant1.us-west-1.vespa.oath.cloud", + "c0.app1.tenant1.us-central-1.vespa.oath.cloud", + "token-c0.app1.tenant1.us-central-1.vespa.oath.cloud" + ); + assertEquals(expectedRecords, tester.recordNames()); + assertEquals(2, tester.policiesOf(context1.instanceId()).size()); + } + + @Test void zone_routing_policies_without_dns_update() { var tester = new RoutingPoliciesTester(new DeploymentTester(), false); var context = tester.newDeploymentContext("tenant1", "app1", "default"); @@ -730,37 +768,47 @@ public class RoutingPoliciesTest { context.flushDnsUpdates(); tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone2); - // Setting other deployment out implicitly sets all deployments in. Weight is set to zero, but that has no - // impact on routing decisions when the weight sum is zero - tester.routingPolicies().setRoutingStatus(context.deploymentIdIn(zone2), RoutingStatus.Value.out, - RoutingStatus.Agent.tenant); + // Setting remaining deployment out is rejected + try { + tester.routingPolicies().setRoutingStatus(context.deploymentIdIn(zone2), RoutingStatus.Value.out, + RoutingStatus.Agent.tenant); + } catch (IllegalArgumentException e) { + assertEquals("Cannot deactivate routing for tenant1.app1 in prod.us-central-1 as it's the last remaining active deployment in endpoint https://r0.app1.tenant1.global.vespa.oath.cloud/ [scope=global, legacy=false, routingMethod=exclusive]", e.getMessage()); + } context.flushDnsUpdates(); - tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, ImmutableMap.of(zone1, 0L, zone2, 0L)); + tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone2); - // One inactive deployment is put back in. Global DNS record now points to the only active deployment + // Inactive deployment is put back in. Global DNS record now points to all deployments tester.routingPolicies().setRoutingStatus(context.deploymentIdIn(zone1), RoutingStatus.Value.in, RoutingStatus.Agent.tenant); context.flushDnsUpdates(); + tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2); + + // One deployment is deactivated again + tester.routingPolicies().setRoutingStatus(context.deploymentIdIn(zone2), RoutingStatus.Value.out, + RoutingStatus.Agent.tenant); + context.flushDnsUpdates(); tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1); - // Setting zone (containing active deployment) out puts all deployments in + // Operator deactivates routing for entire zone where deployment only has that zone activated. This does not + // change status for the deployment as it's the only one left tester.routingPolicies().setRoutingStatus(zone1, RoutingStatus.Value.out); context.flushDnsUpdates(); assertEquals(RoutingStatus.Value.out, tester.routingPolicies().read(zone1).routingStatus().value()); - tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, ImmutableMap.of(zone1, 0L, zone2, 0L)); - - // Setting zone back in removes the currently inactive deployment - tester.routingPolicies().setRoutingStatus(zone1, RoutingStatus.Value.in); - context.flushDnsUpdates(); tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1); - // Inactive deployment is set in + // Inactive deployment is set in which allows the zone-wide status to take effect tester.routingPolicies().setRoutingStatus(context.deploymentIdIn(zone2), RoutingStatus.Value.in, RoutingStatus.Agent.tenant); context.flushDnsUpdates(); for (var policy : tester.routingPolicies().read(context.instanceId())) { assertSame(RoutingStatus.Value.in, policy.routingStatus().value()); } + tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone2); + + // Zone-wide status is changed to in + tester.routingPolicies().setRoutingStatus(zone1, RoutingStatus.Value.in); + context.flushDnsUpdates(); tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2); } @@ -790,8 +838,15 @@ public class RoutingPoliciesTest { tester.provisionLoadBalancers(2, mainInstance, zone); } + // Application endpoints are not created until production jobs run + betaContext.submit(applicationPackage) + .runJob(DeploymentContext.systemTest); + assertEquals(Set.of("beta.app1.tenant1.us-east-1.test.vespa.oath.cloud"), tester.recordNames()); + betaContext.runJob(DeploymentContext.stagingTest); + assertEquals(Set.of("beta.app1.tenant1.us-east-3.staging.vespa.oath.cloud"), tester.recordNames()); + // Deploy both instances - betaContext.submit(applicationPackage).deploy(); + betaContext.completeRollout(); // Application endpoint points to both instances with correct weights DeploymentId betaZone5 = betaContext.deploymentIdIn(zone5); @@ -845,6 +900,15 @@ public class RoutingPoliciesTest { .readDeclaredEndpointsOf(application) .named(EndpointId.of("a1"), Endpoint.Scope.application).isEmpty(), "Endpoint removed"); + + // Ensure test deployment only updates endpoint of which it is a member + betaContext.submit(applicationPackage) + .runJob(DeploymentContext.systemTest); + NameServiceQueue queue = tester.controllerTester().controller().curator().readNameServiceQueue(); + assertEquals(List.of(new RemoveRecords(Optional.of(TenantAndApplicationId.from(betaContext.instanceId())), + Record.Type.CNAME, + RecordName.from("beta.app1.tenant1.us-east-1.test.vespa.oath.cloud"))), + queue.requests()); } @Test @@ -890,15 +954,17 @@ public class RoutingPoliciesTest { // Changing routing status for remaining deployments adds back all deployments, because removing all deployments // puts all IN tester.routingPolicies().setRoutingStatus(betaZone1, RoutingStatus.Value.out, RoutingStatus.Agent.tenant); - tester.routingPolicies().setRoutingStatus(mainZone2, RoutingStatus.Value.out, RoutingStatus.Agent.tenant); - betaContext.flushDnsUpdates(); - tester.assertTargets(application, EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0, - Map.of(betaZone1, 2, - mainZone1, 8, - mainZone2, 9)); + try { + tester.routingPolicies().setRoutingStatus(mainZone2, RoutingStatus.Value.out, RoutingStatus.Agent.tenant); + fail("Expected exception"); + } catch (IllegalArgumentException e) { + assertEquals("Cannot deactivate routing for tenant1.app1.main in prod.south as it's the last remaining active deployment in endpoint https://a0.app1.tenant1.a.vespa.oath.cloud/ [scope=application, legacy=false, routingMethod=exclusive]", + e.getMessage()); + } - // Activating main deployment allows us to deactivate the beta deployment + // Re-activating one zone allows us to take out another tester.routingPolicies().setRoutingStatus(mainZone1, RoutingStatus.Value.in, RoutingStatus.Agent.tenant); + tester.routingPolicies().setRoutingStatus(mainZone2, RoutingStatus.Value.out, RoutingStatus.Agent.tenant); betaContext.flushDnsUpdates(); tester.assertTargets(application, EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0, Map.of(mainZone1, 8)); @@ -1068,6 +1134,10 @@ public class RoutingPoliciesTest { .toList(); } + void enableTokenEndpoint(boolean enabled) { + tester.controllerTester().flagSource().withBooleanFlag(Flags.ENABLE_DATAPLANE_PROXY.id(), enabled); + } + /** Assert that an application endpoint points to given targets and weights */ private void assertTargets(TenantAndApplicationId application, EndpointId endpointId, ClusterSpec.Id cluster, int loadBalancerId, Map<DeploymentId, Integer> deploymentWeights) { diff --git a/document/src/vespa/document/select/CMakeLists.txt b/document/src/vespa/document/select/CMakeLists.txt index f74e4a6abc9..ca41f19aaf0 100644 --- a/document/src/vespa/document/select/CMakeLists.txt +++ b/document/src/vespa/document/select/CMakeLists.txt @@ -52,4 +52,6 @@ elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND CMAKE_CXX_COMPILER_VERSION VER set_source_files_properties(${BISON_DocSelParser_OUTPUTS} PROPERTIES COMPILE_OPTIONS "-Wno-unused-but-set-variable;-Wno-deprecated-copy-with-user-provided-copy") elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") set_source_files_properties(${BISON_DocSelParser_OUTPUTS} PROPERTIES COMPILE_OPTIONS "-Wno-deprecated-copy-with-user-provided-copy") +elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang") + set_source_files_properties(${BISON_DocSelParser_OUTPUTS} PROPERTIES COMPILE_OPTIONS "-Wno-unused-but-set-variable") endif() 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 fba3db4cdf7..c1e12db3a81 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -50,7 +50,7 @@ public class Flags { public static final UnboundBooleanFlag IPV6_IN_GCP = defineFeatureFlag( "ipv6-in-gcp", false, - List.of("hakonhall"), "2023-05-15", "2023-06-15", + List.of("hakonhall"), "2023-05-15", "2023-07-15", "Provision GCP hosts with external IPv6 addresses", "Takes effect on the next host provisioning"); diff --git a/hosted-tenant-base/pom.xml b/hosted-tenant-base/pom.xml index adde2a32720..6fc8b6b18b8 100644 --- a/hosted-tenant-base/pom.xml +++ b/hosted-tenant-base/pom.xml @@ -38,7 +38,7 @@ <maven-compiler-plugin.version>3.10.1</maven-compiler-plugin.version> <maven-surefire-plugin.version>2.22.2</maven-surefire-plugin.version> <maven-dependency-plugin.version>3.5.0</maven-dependency-plugin.version> - <jackson2.version>2.15.0</jackson2.version> + <jackson2.version>2.15.2</jackson2.version> <!-- NOTE: this must not be overriden by users, and must be in sync with junit version specified in 'tenant-cd-api' --> <vespa.junit.version>5.8.1</vespa.junit.version> <test.categories>!integration</test.categories> diff --git a/maven-plugins/allowed-maven-dependencies.txt b/maven-plugins/allowed-maven-dependencies.txt index f2334a6ef00..d504357b07c 100644 --- a/maven-plugins/allowed-maven-dependencies.txt +++ b/maven-plugins/allowed-maven-dependencies.txt @@ -3,9 +3,9 @@ #[non-test] # Contains dependencies that are not used exclusively in 'test' scope aopalliance:aopalliance:1.0 -com.fasterxml.jackson.core:jackson-annotations:2.15.0 -com.fasterxml.jackson.core:jackson-core:2.15.0 -com.fasterxml.jackson.core:jackson-databind:2.15.0 +com.fasterxml.jackson.core:jackson-annotations:2.15.2 +com.fasterxml.jackson.core:jackson-core:2.15.2 +com.fasterxml.jackson.core:jackson-databind:2.15.2 com.google.errorprone:error_prone_annotations:2.18.0 com.google.guava:failureaccess:1.0.1 com.google.guava:guava:27.1-jre diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/yamas/YamasHandler.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/yamas/YamasHandler.java index eb635d8c641..ef23a5ad070 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/yamas/YamasHandler.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/yamas/YamasHandler.java @@ -52,7 +52,7 @@ public class YamasHandler extends HttpHandlerBase { NodeDimensions nodeDimensions) { super(executor); valuesFetcher = new ValuesFetcher(metricsManager, vespaServices, metricsConsumers); - this.nodeMetricGatherer = new NodeMetricGatherer(metricsManager, vespaServices, applicationDimensions, nodeDimensions); + this.nodeMetricGatherer = new NodeMetricGatherer(metricsManager, applicationDimensions, nodeDimensions); this.metricsConsumers = metricsConsumers; } diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/yamas/YamasResponse.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/yamas/YamasResponse.java index 49f5036b3fd..6c94de49140 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/yamas/YamasResponse.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/yamas/YamasResponse.java @@ -29,7 +29,7 @@ public class YamasResponse extends HttpResponse { @Override public void render(OutputStream outputStream) throws IOException { - YamasJsonUtil.toJson(metrics, outputStream, true); + YamasJsonUtil.toJson(metrics, outputStream, false); } } diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonUtil.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonUtil.java index 13b2a8d859c..5086846293b 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonUtil.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonUtil.java @@ -107,7 +107,7 @@ public class YamasJsonUtil { for (int i = 0; i < metrics.size() - 1; i++) { toJson(metrics.get(i), generator, addStatus); } - toJson(metrics.get(metrics.size() - 1), generator, true); + toJson(metrics.get(metrics.size() - 1), generator, addStatus); generator.writeEndArray(); generator.writeEndObject(); generator.close(); diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/prometheus/PrometheusUtil.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/prometheus/PrometheusUtil.java index 5fe9af24c3d..c2d05a8636e 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/prometheus/PrometheusUtil.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/metric/model/prometheus/PrometheusUtil.java @@ -57,14 +57,6 @@ public class PrometheusUtil { sampleList.add(new Sample(metricName, labels, labelValues, metric.getValue().doubleValue(), packet.timestamp * 1000)); } } - // convert status message to 0,1 metric - var firstPacket = packets.get(0); - String statusMetricName = serviceName + "_status"; - // MetricsPacket status 0 means OK, but it's the opposite in Prometheus. - double statusMetricValue = (firstPacket.statusCode == 0) ? 1.0 : 0.0; - List<Sample> sampleList = singletonList(new Sample(statusMetricName, emptyList(), emptyList(), - statusMetricValue, firstPacket.timestamp * 1000)); - metricFamilySamples.add(new MetricFamilySamples(statusMetricName, Collector.Type.UNTYPED, "status of service", sampleList)); })); return new PrometheusModel(metricFamilySamples); diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/NodeMetricGatherer.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/NodeMetricGatherer.java index 02785674103..3da7aef0a12 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/NodeMetricGatherer.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/NodeMetricGatherer.java @@ -10,10 +10,8 @@ import ai.vespa.metricsproxy.metric.model.MetricId; import ai.vespa.metricsproxy.metric.model.MetricsPacket; import ai.vespa.metricsproxy.metric.model.ServiceId; import ai.vespa.metricsproxy.service.SystemPollerProvider; -import ai.vespa.metricsproxy.service.VespaServices; import com.fasterxml.jackson.databind.JsonNode; import com.yahoo.component.annotation.Inject; -import com.yahoo.container.jdisc.state.CoredumpGatherer; import com.yahoo.container.jdisc.state.FileWrapper; import com.yahoo.container.jdisc.state.HostLifeGatherer; @@ -21,9 +19,6 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; import java.util.Set; -import java.util.stream.Collectors; - -import static ai.vespa.metricsproxy.node.ServiceHealthGatherer.gatherServiceHealthMetrics; /** * Fetches miscellaneous system metrics for node, including @@ -35,15 +30,13 @@ import static ai.vespa.metricsproxy.node.ServiceHealthGatherer.gatherServiceHeal */ public class NodeMetricGatherer { - private final VespaServices vespaServices; private final ApplicationDimensions applicationDimensions; private final NodeDimensions nodeDimensions; private final MetricsManager metricsManager; @Inject - public NodeMetricGatherer(MetricsManager metricsManager, VespaServices vespaServices, ApplicationDimensions applicationDimensions, NodeDimensions nodeDimensions) { + public NodeMetricGatherer(MetricsManager metricsManager, ApplicationDimensions applicationDimensions, NodeDimensions nodeDimensions) { this.metricsManager = metricsManager; - this.vespaServices = vespaServices; this.applicationDimensions = applicationDimensions; this.nodeDimensions = nodeDimensions; } @@ -51,10 +44,7 @@ public class NodeMetricGatherer { public List<MetricsPacket> gatherMetrics() { FileWrapper fileWrapper = new FileWrapper(); List<MetricsPacket.Builder> metricPacketBuilders = new ArrayList<>(); - metricPacketBuilders.addAll(gatherServiceHealthMetrics(vespaServices)); - JsonNode coredumpPacket = CoredumpGatherer.gatherCoredumpMetrics(fileWrapper); - addObjectToBuilders(metricPacketBuilders, coredumpPacket); if (SystemPollerProvider.runningOnLinux()) { JsonNode packet = HostLifeGatherer.getHostLifePacket(fileWrapper); addObjectToBuilders(metricPacketBuilders, packet); @@ -71,8 +61,6 @@ public class NodeMetricGatherer { protected static void addObjectToBuilders(List<MetricsPacket.Builder> builders, JsonNode object) { MetricsPacket.Builder builder = new MetricsPacket.Builder(ServiceId.toServiceId(object.get("application").textValue())); builder.timestamp(object.get("timestamp").longValue()); - if (object.has("status_code")) builder.statusCode(object.get("status_code").intValue()); - if (object.has("status_msg")) builder.statusMessage(object.get("status_msg").textValue()); if (object.has("metrics")) { JsonNode metrics = object.get("metrics"); Iterator<?> keys = metrics.fieldNames(); diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/ServiceHealthGatherer.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/ServiceHealthGatherer.java deleted file mode 100644 index d09f2aff3e5..00000000000 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/node/ServiceHealthGatherer.java +++ /dev/null @@ -1,36 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package ai.vespa.metricsproxy.node; - -import ai.vespa.metricsproxy.metric.HealthMetric; -import ai.vespa.metricsproxy.metric.model.ConsumerId; -import ai.vespa.metricsproxy.metric.model.DimensionId; -import ai.vespa.metricsproxy.metric.model.MetricsPacket; -import ai.vespa.metricsproxy.service.VespaServices; - -import java.time.Instant; -import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; - -/** - * @author olaa - */ -public class ServiceHealthGatherer { - - protected static List<MetricsPacket.Builder> gatherServiceHealthMetrics(VespaServices vespaServices) { - return vespaServices.getVespaServices() - .stream() - .map(service -> { - HealthMetric healt = service.getHealth(); - return new MetricsPacket.Builder(service.getMonitoringName()) - .timestamp(Instant.now().getEpochSecond()) - .statusMessage(healt.getStatus().status) - .statusCode(healt.getStatus().code) - .putDimension(DimensionId.toDimensionId("instance"), service.getInstanceName()) - .putDimension(DimensionId.toDimensionId("metrictype"), "health") - .addConsumers(Set.of(ConsumerId.toConsumerId("Vespa"))); - }) - .toList(); - } - -} diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/prometheus/PrometheusHandlerTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/prometheus/PrometheusHandlerTest.java index 2c216272022..90536ddcfdb 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/prometheus/PrometheusHandlerTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/prometheus/PrometheusHandlerTest.java @@ -60,23 +60,12 @@ public class PrometheusHandlerTest extends HttpHandlerTestBase { } @Test - public void response_contains_node_status() { - assertTrue(valuesResponse.contains("vespa_node_status 1.0")); - } - - @Test public void response_contains_node_metrics() { String cpu = getLine(valuesResponse, CPU_METRIC + "{"); assertTrue(cpu.contains("} 12.345")); // metric value } @Test - public void response_contains_service_status() { - assertTrue(valuesResponse.contains("vespa_dummy_status 1.0")); - assertTrue(valuesResponse.contains("vespa_down_service_status 0.0")); - } - - @Test public void response_contains_service_metrics() { String dummy0 = getLine(valuesResponse, DummyService.NAME + "0"); assertTrue(dummy0.contains("c_test")); // metric name diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/yamas/YamasHandlerTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/yamas/YamasHandlerTest.java index a4e61d5965e..346fc6a462b 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/yamas/YamasHandlerTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/http/yamas/YamasHandlerTest.java @@ -13,6 +13,7 @@ import java.util.List; import java.util.concurrent.Executors; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; public class YamasHandlerTest extends HttpHandlerTestBase { @@ -48,8 +49,9 @@ public class YamasHandlerTest extends HttpHandlerTestBase { } @Test - public void value_response_contains_coredump_metric() { - assertTrue(valuesResponse.contains("\"application\":\"system-coredumps-processing\",\"routing\":{\"yamas\":{\"namespaces\":[\"Vespa\"]}}")); + public void value_response_does_not_contain_status() { + assertFalse(valuesResponse.contains("status_code")); + assertFalse(valuesResponse.contains("status_msg")); } } diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonModelTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonModelTest.java index a5bc7a0877a..3e85166430d 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonModelTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonModelTest.java @@ -28,7 +28,7 @@ import static org.junit.Assert.assertTrue; */ public class YamasJsonModelTest { - private static final String EXPECTED_JSON = "{\"metrics\":[{\"status_code\":0,\"timestamp\":1400047900,\"application\":\"vespa.searchnode\",\"metrics\":{\"cpu\":55.5555555555555,\"memory_virt\":22222222222,\"memory_rss\":5555555555},\"dimensions\":{\"applicationName\":\"app\",\"tenantName\":\"tenant\",\"metrictype\":\"system\",\"instance\":\"searchnode\",\"applicationInstance\":\"default\",\"clustername\":\"cluster\"},\"routing\":{\"yamas\":{\"namespaces\":[\"Vespa\"]}},\"status_msg\":\"Data collected successfully\"}]}"; + private static final String EXPECTED_JSON = "{\"metrics\":[{\"timestamp\":1400047900,\"application\":\"vespa.searchnode\",\"metrics\":{\"cpu\":55.5555555555555,\"memory_virt\":22222222222,\"memory_rss\":5555555555},\"dimensions\":{\"applicationName\":\"app\",\"tenantName\":\"tenant\",\"metrictype\":\"system\",\"instance\":\"searchnode\",\"applicationInstance\":\"default\",\"clustername\":\"cluster\"},\"routing\":{\"yamas\":{\"namespaces\":[\"Vespa\"]}}}]}"; @Test public void array_definition_creates_correct_json() throws IOException { @@ -69,7 +69,7 @@ public class YamasJsonModelTest { assertEquals(5.555555555E9, metricsPacket.metrics().get(toMetricId("memory_rss")).doubleValue(), 0.1d); //Not using custom double rendrer // Serialize and verify - String string = YamasJsonUtil.toJson(List.of(metricsPacket), true); + String string = YamasJsonUtil.toJson(List.of(metricsPacket), false); assertEquals(EXPECTED_JSON, string); } diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonUtilTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonUtilTest.java index ebd80b38a42..133461e3658 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonUtilTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/metric/model/json/YamasJsonUtilTest.java @@ -31,15 +31,6 @@ public class YamasJsonUtilTest { private static ArrayNode metrics(List<MetricsPacket> packets, boolean addStatus) throws IOException { return (ArrayNode) jsonMapper.readTree(YamasJsonUtil.toJson(packets, addStatus)).get("metrics"); } - @Test - public void json_model_gets_null_status_by_default() throws IOException { - ArrayNode json = metrics(List.of(new MetricsPacket.Builder(toServiceId("foo")).build(), - new MetricsPacket.Builder(toServiceId("bar")).build()), false); - assertFalse(json.get(0).has("status_code")); - assertFalse(json.get(0).has("status_msg")); - assertTrue(json.get(1).has("status_code")); - assertTrue(json.get(1).has("status_msg")); - } @Test public void status_is_included_in_json_model_when_explicitly_asked_for() throws IOException { diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/node/NodeMetricGathererTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/node/NodeMetricGathererTest.java index 0de3526b40d..635365f9462 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/node/NodeMetricGathererTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/node/NodeMetricGathererTest.java @@ -30,8 +30,6 @@ public class NodeMetricGathererTest { MetricsPacket packet = builders.remove(0).build(); assertEquals("host_life", packet.service.id); - assertEquals(0, packet.statusCode); - assertEquals("OK", packet.statusMessage); assertEquals(123, packet.timestamp); assertEquals(12l, packet.metrics().get(MetricId.toMetricId("uptime"))); assertEquals(1l, packet.metrics().get(MetricId.toMetricId("alive"))); @@ -41,8 +39,6 @@ public class NodeMetricGathererTest { private JsonNode generateHostLifePacket() { ObjectNode jsonObject = jsonMapper.createObjectNode(); - jsonObject.put("status_code", 0); - jsonObject.put("status_msg", "OK"); jsonObject.put("timestamp", 123); jsonObject.put("application", "host_life"); ObjectNode metrics = jsonMapper.createObjectNode(); diff --git a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java index 1e7a398b3d0..142740356d7 100644 --- a/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java +++ b/metrics-proxy/src/test/java/ai/vespa/metricsproxy/rpc/RpcMetricsTest.java @@ -167,7 +167,6 @@ public class RpcMetricsTest { } } - verifyStatusMessage(metrics.get(metrics.size() - 1)); } private void verfiyMetricsFromServiceObject(VespaService service) { @@ -266,12 +265,4 @@ public class RpcMetricsTest { return returnValue; } - private static void verifyStatusMessage(JsonNode jsonObject) { - assertEquals(0, jsonObject.get("status_code").intValue()); - assertNotNull(jsonObject.get("status_msg")); - assertNotNull(jsonObject.get("application")); - assertNotNull(jsonObject.get("routing")); - assertEquals(4, jsonObject.size()); - } - } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/MetricsFetcher.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/MetricsFetcher.java index b9f16a27d73..e7e826fec91 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/MetricsFetcher.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/MetricsFetcher.java @@ -1,11 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.provision.autoscale; -import com.yahoo.collections.Pair; import com.yahoo.config.provision.ApplicationId; -import java.time.Instant; -import java.util.Collection; import java.util.concurrent.CompletableFuture; /** @@ -22,4 +19,6 @@ public interface MetricsFetcher { */ CompletableFuture<MetricsResponse> fetchMetrics(ApplicationId application); + void deconstruct(); + } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeMetricsDbMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeMetricsDbMaintainer.java index e28dac1c915..dd4839d131a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeMetricsDbMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeMetricsDbMaintainer.java @@ -62,6 +62,12 @@ public class NodeMetricsDbMaintainer extends NodeRepositoryMaintainer { } } + @Override + public void shutdown() { + super.shutdown(); + metricsFetcher.deconstruct(); + } + private void handleResponse(MetricsResponse response, Throwable exception, MutableInteger failures, diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockMetricsFetcher.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockMetricsFetcher.java index 97fea9648c6..6c6face7ba6 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockMetricsFetcher.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockMetricsFetcher.java @@ -10,6 +10,7 @@ import java.util.concurrent.CompletableFuture; /** * @author bratseth */ +@SuppressWarnings("unused") // Injected in container from test code (services.xml) public class MockMetricsFetcher implements MetricsFetcher { @Override @@ -17,4 +18,7 @@ public class MockMetricsFetcher implements MetricsFetcher { return CompletableFuture.completedFuture(MetricsResponse.empty()); } + @Override + public void deconstruct() {} + } diff --git a/parent/pom.xml b/parent/pom.xml index f68b2d0c068..d63e2f4d8f9 100644 --- a/parent/pom.xml +++ b/parent/pom.xml @@ -1078,7 +1078,7 @@ <dependency> <groupId>org.xerial.snappy</groupId> <artifactId>snappy-java</artifactId> - <version>1.1.7</version> + <version>1.1.10.1</version> </dependency> <dependency> <groupId>io.dropwizard.metrics</groupId> diff --git a/searchlib/src/tests/docstore/document_store/visitcache_test.cpp b/searchlib/src/tests/docstore/document_store/visitcache_test.cpp index 14e3c19fe33..3f80bb6004f 100644 --- a/searchlib/src/tests/docstore/document_store/visitcache_test.cpp +++ b/searchlib/src/tests/docstore/document_store/visitcache_test.cpp @@ -62,7 +62,7 @@ TEST("require that BlobSet can be built") { a.append(9, B("bbbbb",5)); verifyAB(a); CompressionConfig cfg(CompressionConfig::LZ4); - CompressedBlobSet ca(cfg, a); + CompressedBlobSet ca(cfg, std::move(a)); BlobSet b = ca.getBlobSet(); verifyAB(b); } diff --git a/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp b/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp index 1080d44f2fb..2bf2a38b7e6 100644 --- a/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp +++ b/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp @@ -368,13 +368,13 @@ TEST("test visit cache does not cache empty ones and is able to access some back IDataStore & datastore = store.getStore(); VisitCache visitCache(datastore, 100000, CompressionConfig::Type::LZ4); - EXPECT_EQUAL(0u, visitCache.read({1}).size()); + EXPECT_EQUAL(12u, visitCache.read({1}).bytesAllocated()); EXPECT_TRUE(visitCache.read({1}).empty()); datastore.write(1,1, A7, 7); - EXPECT_EQUAL(0u, visitCache.read({2}).size()); + EXPECT_EQUAL(12u, visitCache.read({2}).bytesAllocated()); CompressedBlobSet cbs = visitCache.read({1}); EXPECT_FALSE(cbs.empty()); - EXPECT_EQUAL(19u, cbs.size()); + EXPECT_EQUAL(19u, cbs.bytesAllocated()); BlobSet bs(cbs.getBlobSet()); EXPECT_EQUAL(7u, bs.get(1).size()); EXPECT_EQUAL(0, strncmp(A7, bs.get(1).c_str(), 7)); @@ -664,14 +664,14 @@ TEST("test that the integrated visit cache works.") { vcs.remove(17); TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 104, 97, BASE_SZ-671)); vcs.verifyVisit({7,9,17,19,67,88,89}, {7,9,19,67,88,89}, true); - TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 105, 98, BASE_SZ-89)); + TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 105, 98, BASE_SZ-70)); vcs.verifyVisit({41, 42}, true); - TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 106, 99, BASE_SZ+215)); + TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 106, 99, BASE_SZ+230)); vcs.verifyVisit({43, 44}, true); - TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 107, 100, BASE_SZ+520)); + TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 107, 100, BASE_SZ+540)); vcs.verifyVisit({41, 42, 43, 44}, true); - TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 108, 99, BASE_SZ+340)); + TEST_DO(verifyCacheStats(ds.getCacheStats(), 101, 108, 99, BASE_SZ+360)); } TEST("testWriteRead") { diff --git a/searchlib/src/tests/tensor/hnsw_index/hnsw_index_test.cpp b/searchlib/src/tests/tensor/hnsw_index/hnsw_index_test.cpp index f59c16c76f9..b238044a67b 100644 --- a/searchlib/src/tests/tensor/hnsw_index/hnsw_index_test.cpp +++ b/searchlib/src/tests/tensor/hnsw_index/hnsw_index_test.cpp @@ -855,6 +855,17 @@ TEST_F(HnswMultiIndexTest, docid_with_empty_tensor_can_be_removed) this->remove_document(1); } +TEST_F(HnswMultiIndexTest, docid_with_empty_tensor_can_be_removed_after_restart) +{ + this->init(false); + this->vectors.set(1, {}); + this->add_document(1); + auto data = this->save_index(); + this->init(false); + this->load_index(data); + this->remove_document(1); +} + TEST(LevelGeneratorTest, gives_various_levels) { InvLogLevelGenerator generator(4); diff --git a/searchlib/src/vespa/searchlib/docstore/documentstore.cpp b/searchlib/src/vespa/searchlib/docstore/documentstore.cpp index 49beed34f08..7d585007d76 100644 --- a/searchlib/src/vespa/searchlib/docstore/documentstore.cpp +++ b/searchlib/src/vespa/searchlib/docstore/documentstore.cpp @@ -437,7 +437,8 @@ DocumentStore::getFileChunkStats() const return _backingStore.getFileChunkStats(); } -CacheStats DocumentStore::getCacheStats() const { +CacheStats +DocumentStore::getCacheStats() const { CacheStats visitStats = _visitCache->getCacheStats(); CacheStats singleStats = _cache->get_stats(); singleStats.add_extra_misses(_uncached_lookups.load(std::memory_order_relaxed)); diff --git a/searchlib/src/vespa/searchlib/docstore/visitcache.cpp b/searchlib/src/vespa/searchlib/docstore/visitcache.cpp index c99bb50d4f8..b61cf49c438 100644 --- a/searchlib/src/vespa/searchlib/docstore/visitcache.cpp +++ b/searchlib/src/vespa/searchlib/docstore/visitcache.cpp @@ -2,12 +2,14 @@ #include "visitcache.h" #include "ibucketizer.h" -#include <vespa/vespalib/stllike/cache.hpp> -#include <vespa/vespalib/stllike/hash_map.hpp> +#include <vespa/vespalib/stllike/hash_set.h> +#include <vespa/vespalib/stllike/hash_map.h> +#include <vespa/vespalib/stllike/cache.h> #include <vespa/vespalib/data/databuffer.h> #include <vespa/vespalib/util/compressor.h> #include <vespa/vespalib/util/memory_allocator.h> -#include <algorithm> +#include <vespa/vespalib/stllike/cache.hpp> +#include <vespa/vespalib/stllike/hash_map.hpp> namespace search::docstore { @@ -17,14 +19,14 @@ using vespalib::DataBuffer; using vespalib::alloc::Alloc; using vespalib::alloc::MemoryAllocator; -KeySet::KeySet(uint32_t key) : - _keys() +KeySet::KeySet(uint32_t key) + : _keys() { _keys.push_back(key); } -KeySet::KeySet(const IDocumentStore::LidVector &keys) : - _keys(keys) +KeySet::KeySet(const IDocumentStore::LidVector &keys) + : _keys(keys) { std::sort(_keys.begin(), _keys.end()); } @@ -34,26 +36,26 @@ KeySet::contains(const KeySet &rhs) const { return std::includes(_keys.begin(), _keys.end(), rhs._keys.begin(), rhs._keys.end()); } -BlobSet::BlobSet() : - _positions(), - _buffer(Alloc::alloc(0, 16 * MemoryAllocator::HUGEPAGE_SIZE), 0) +BlobSet::BlobSet() + : _positions(), + _buffer(Alloc::alloc(0, 16 * MemoryAllocator::HUGEPAGE_SIZE), 0) { } BlobSet::~BlobSet() = default; namespace { -size_t getBufferSize(const BlobSet::Positions & p) { +size_t +getBufferSize(const BlobSet::Positions & p) { return p.empty() ? 0 : p.back().offset() + p.back().size(); } } -BlobSet::BlobSet(const Positions & positions, Alloc && buffer) : - _positions(positions), - _buffer(std::move(buffer), getBufferSize(_positions)) -{ -} +BlobSet::BlobSet(Positions positions, Alloc && buffer) noexcept + : _positions(std::move(positions)), + _buffer(std::move(buffer), getBufferSize(_positions)) +{ } void BlobSet::append(uint32_t lid, ConstBufferRef blob) { @@ -74,29 +76,30 @@ BlobSet::get(uint32_t lid) const return buf; } -CompressedBlobSet::CompressedBlobSet() : - _compression(CompressionConfig::Type::LZ4), - _positions(), - _buffer() -{ -} +CompressedBlobSet::CompressedBlobSet() noexcept + : _positions(), + _buffer(), + _used(0), + _compression(CompressionConfig::Type::LZ4) +{ } CompressedBlobSet::~CompressedBlobSet() = default; - -CompressedBlobSet::CompressedBlobSet(CompressionConfig compression, const BlobSet & uncompressed) : - _compression(compression.type), - _positions(uncompressed.getPositions()), - _buffer() +CompressedBlobSet::CompressedBlobSet(CompressionConfig compression, BlobSet uncompressed) + : _positions(uncompressed.stealPositions()), + _buffer(), + _used(0), + _compression(compression.type) { if ( ! _positions.empty() ) { DataBuffer compressed; ConstBufferRef org = uncompressed.getBuffer(); _compression = vespalib::compression::compress(compression, org, compressed, false); - _buffer = std::make_shared<vespalib::MallocPtr>(compressed.getDataLen()); - memcpy(*_buffer, compressed.getData(), compressed.getDataLen()); + _used = compressed.getDataLen(); + _buffer = std::make_shared<Alloc>(Alloc::alloc(_used)); + memcpy(_buffer->get(), compressed.getData(), _used); } else { - _buffer = std::make_shared<vespalib::MallocPtr>(); + _buffer = std::make_shared<Alloc>(); } } @@ -108,12 +111,13 @@ CompressedBlobSet::getBlobSet() const DataBuffer uncompressed(0, 1, Alloc::alloc(0, 16 * MemoryAllocator::HUGEPAGE_SIZE)); if ( ! _positions.empty() ) { decompress(_compression, getBufferSize(_positions), - ConstBufferRef(_buffer->c_str(), _buffer->size()), uncompressed, false); + ConstBufferRef(_buffer->get(), _used), uncompressed, false); } return BlobSet(_positions, std::move(uncompressed).stealBuffer()); } -size_t CompressedBlobSet::size() const { +size_t +CompressedBlobSet::bytesAllocated() const { return _positions.capacity() * sizeof(BlobSet::Positions::value_type) + _buffer->size(); } @@ -122,13 +126,10 @@ namespace { class VisitCollector : public IBufferVisitor { public: - VisitCollector() : - _blobSet() - { } + VisitCollector(BlobSet & blobSet) : _blobSet(blobSet) { } void visit(uint32_t lid, ConstBufferRef buf) override; - const BlobSet & getBlobSet() const { return _blobSet; } private: - BlobSet _blobSet; + BlobSet & _blobSet; }; void @@ -138,13 +139,52 @@ VisitCollector::visit(uint32_t lid, ConstBufferRef buf) { } } +struct ByteSize { + size_t operator() (const CompressedBlobSet & arg) const noexcept { return arg.bytesAllocated(); } +}; + } +using CacheParams = vespalib::CacheParam< + vespalib::LruParam<KeySet, CompressedBlobSet>, + VisitCache::BackingStore, + vespalib::zero<KeySet>, + ByteSize +>; + +/** + * This extends the default thread safe cache implementation so that + * it will correctly invalidate the cached sets when objects are removed/updated. + * It will also detect the addition of new objects to any of the sets upon first + * usage of the set and then invalidate and perform fresh visit of the backing store. + */ +class VisitCache::Cache : public vespalib::cache<CacheParams> { +public: + Cache(BackingStore & b, size_t maxBytes); + ~Cache() override; + CompressedBlobSet readSet(const KeySet & keys); + void removeKey(uint32_t key); + vespalib::MemoryUsage getStaticMemoryUsage() const override; +private: + void locateAndInvalidateOtherSubsets(const UniqueLock & cacheGuard, const KeySet & keys); + using IdSet = vespalib::hash_set<uint64_t>; + using Parent = vespalib::cache<CacheParams>; + using LidUniqueKeySetId = vespalib::hash_map<uint32_t, uint64_t>; + using IdKeySetMap = vespalib::hash_map<uint64_t, KeySet>; + IdSet findSetsContaining(const UniqueLock &, const KeySet & keys) const; + void onInsert(const K & key) override; + void onRemove(const K & key) override; + LidUniqueKeySetId _lid2Id; + IdKeySetMap _id2KeySet; +}; + bool VisitCache::BackingStore::read(const KeySet &key, CompressedBlobSet &blobs) const { - VisitCollector collector; + BlobSet blobSet; + blobSet.reserve(key.getKeys().size()); + VisitCollector collector(blobSet); _backingStore.read(key.getKeys(), collector); - blobs = CompressedBlobSet(_compression.load(std::memory_order_relaxed), collector.getBlobSet()); + blobs = CompressedBlobSet(_compression.load(std::memory_order_relaxed), std::move(blobSet)); return ! blobs.empty(); } @@ -157,8 +197,9 @@ VisitCache::BackingStore::reconfigure(CompressionConfig compression) { VisitCache::VisitCache(IDataStore &store, size_t cacheSize, CompressionConfig compression) : _store(store, compression), _cache(std::make_unique<Cache>(_store, cacheSize)) -{ -} +{ } + +VisitCache::~VisitCache() = default; void VisitCache::reconfigure(size_t cacheSize, CompressionConfig compression) { @@ -166,6 +207,10 @@ VisitCache::reconfigure(size_t cacheSize, CompressionConfig compression) { _cache->setCapacityBytes(cacheSize); } +vespalib::MemoryUsage +VisitCache::getStaticMemoryUsage() const { + return _cache->getStaticMemoryUsage(); +} VisitCache::Cache::IdSet VisitCache::Cache::findSetsContaining(const UniqueLock &, const KeySet & keys) const { @@ -237,8 +282,7 @@ VisitCache::Cache::removeKey(uint32_t subKey) { auto cacheGuard = getGuard(); const auto foundLid = _lid2Id.find(subKey); if (foundLid != _lid2Id.end()) { - K keySet = _id2KeySet[foundLid->second]; - invalidate(cacheGuard, keySet); + invalidate(cacheGuard, _id2KeySet[foundLid->second]); } } diff --git a/searchlib/src/vespa/searchlib/docstore/visitcache.h b/searchlib/src/vespa/searchlib/docstore/visitcache.h index baa594b8d28..2f312ec3011 100644 --- a/searchlib/src/vespa/searchlib/docstore/visitcache.h +++ b/searchlib/src/vespa/searchlib/docstore/visitcache.h @@ -3,14 +3,9 @@ #pragma once #include "idocumentstore.h" -#include <vespa/vespalib/stllike/cache.h> -#include <vespa/vespalib/stllike/hash_set.h> -#include <vespa/vespalib/stllike/hash_map.h> #include <vespa/vespalib/util/alloc.h> -#include <vespa/vespalib/util/memory.h> #include <vespa/vespalib/util/compressionconfig.h> #include <vespa/vespalib/objects/nbostream.h> -#include <vespa/document/util/bytebuffer.h> namespace search::docstore { @@ -19,7 +14,7 @@ namespace search::docstore { **/ class KeySet { public: - KeySet() : _keys() { } + KeySet() noexcept : _keys() { } KeySet(uint32_t key); explicit KeySet(const IDocumentStore::LidVector &keys); uint32_t hash() const noexcept { return _keys.empty() ? 0 : _keys[0]; } @@ -51,12 +46,14 @@ public: using Positions = std::vector<LidPosition>; BlobSet(); - BlobSet(const Positions & positions, vespalib::alloc::Alloc && buffer); - BlobSet(BlobSet &&) = default; - BlobSet &operator = (BlobSet &&) = default; + BlobSet(Positions positions, vespalib::alloc::Alloc && buffer) noexcept; + BlobSet(BlobSet &&) noexcept = default; + BlobSet &operator = (BlobSet &&) noexcept = default; ~BlobSet(); + void reserve(size_t elems) { _positions.reserve(elems);} void append(uint32_t lid, vespalib::ConstBufferRef blob); const Positions & getPositions() const { return _positions; } + Positions && stealPositions() { return std::move(_positions); } vespalib::ConstBufferRef get(uint32_t lid) const; vespalib::ConstBufferRef getBuffer() const { return vespalib::ConstBufferRef(_buffer.data(), _buffer.size()); } private: @@ -73,20 +70,22 @@ private: class CompressedBlobSet { public: using CompressionConfig = vespalib::compression::CompressionConfig; - CompressedBlobSet(); - CompressedBlobSet(CompressionConfig compression, const BlobSet & uncompressed); - CompressedBlobSet(CompressedBlobSet && rhs) = default; - CompressedBlobSet & operator=(CompressedBlobSet && rhs) = default; + CompressedBlobSet() noexcept; + CompressedBlobSet(CompressionConfig compression, BlobSet uncompressed); + CompressedBlobSet(CompressedBlobSet && rhs) noexcept = default; + CompressedBlobSet & operator=(CompressedBlobSet && rhs) noexcept = default; CompressedBlobSet(const CompressedBlobSet & rhs) = default; CompressedBlobSet & operator=(const CompressedBlobSet & rhs) = default; ~CompressedBlobSet(); - size_t size() const; + size_t bytesAllocated() const; bool empty() const { return _positions.empty(); } BlobSet getBlobSet() const; private: - CompressionConfig::Type _compression; + using Alloc = vespalib::alloc::Alloc; BlobSet::Positions _positions; - std::shared_ptr<vespalib::MallocPtr> _buffer; + std::shared_ptr<Alloc> _buffer; + uint32_t _used; + CompressionConfig::Type _compression; }; /** @@ -98,26 +97,27 @@ class VisitCache { public: using CompressionConfig = vespalib::compression::CompressionConfig; VisitCache(IDataStore &store, size_t cacheSize, CompressionConfig compression); + ~VisitCache(); CompressedBlobSet read(const IDocumentStore::LidVector & keys) const; void remove(uint32_t key); void invalidate(uint32_t key) { remove(key); } vespalib::CacheStats getCacheStats() const; - vespalib::MemoryUsage getStaticMemoryUsage() const { return _cache->getStaticMemoryUsage(); } + vespalib::MemoryUsage getStaticMemoryUsage() const; void reconfigure(size_t cacheSize, CompressionConfig compression); -private: + /** - * This implments the interface the cache uses when it has a cache miss. - * It wraps an IDataStore. Given a set of lids it will visit all objects - * and compress them as a complete set to maximize compression rate. - * As this is a readonly cache the write/erase methods are noops. - */ + * This implments the interface the cache uses when it has a cache miss. + * It wraps an IDataStore. Given a set of lids it will visit all objects + * and compress them as a complete set to maximize compression rate. + * As this is a readonly cache the write/erase methods are noops. + */ class BackingStore { public: - BackingStore(IDataStore &store, CompressionConfig compression) : - _backingStore(store), - _compression(compression) + BackingStore(IDataStore &store, CompressionConfig compression) + : _backingStore(store), + _compression(compression) { } bool read(const KeySet &key, CompressedBlobSet &blobs) const; void write(const KeySet &, const CompressedBlobSet &) { } @@ -128,40 +128,9 @@ private: IDataStore &_backingStore; std::atomic<CompressionConfig> _compression; }; +private: - using CacheParams = vespalib::CacheParam< - vespalib::LruParam<KeySet, CompressedBlobSet>, - BackingStore, - vespalib::zero<KeySet>, - vespalib::size<CompressedBlobSet> - >; - - /** - * This extends the default thread safe cache implementation so that - * it will correctly invalidate the cached sets when objects are removed/updated. - * It will also detect the addition of new objects to any of the sets upon first - * usage of the set and then invalidate and perform fresh visit of the backing store. - */ - class Cache : public vespalib::cache<CacheParams> { - public: - Cache(BackingStore & b, size_t maxBytes); - ~Cache() override; - CompressedBlobSet readSet(const KeySet & keys); - void removeKey(uint32_t key); - vespalib::MemoryUsage getStaticMemoryUsage() const override; - private: - void locateAndInvalidateOtherSubsets(const UniqueLock & cacheGuard, const KeySet & keys); - using IdSet = vespalib::hash_set<uint64_t>; - using Parent = vespalib::cache<CacheParams>; - using LidUniqueKeySetId = vespalib::hash_map<uint32_t, uint64_t>; - using IdKeySetMap = vespalib::hash_map<uint64_t, KeySet>; - IdSet findSetsContaining(const UniqueLock &, const KeySet & keys) const; - void onInsert(const K & key) override; - void onRemove(const K & key) override; - LidUniqueKeySetId _lid2Id; - IdKeySetMap _id2KeySet; - }; - + class Cache; BackingStore _store; std::unique_ptr<Cache> _cache; }; diff --git a/searchlib/src/vespa/searchlib/tensor/euclidean_distance.cpp b/searchlib/src/vespa/searchlib/tensor/euclidean_distance.cpp index 1b8924ab351..a76f220f1b4 100644 --- a/searchlib/src/vespa/searchlib/tensor/euclidean_distance.cpp +++ b/searchlib/src/vespa/searchlib/tensor/euclidean_distance.cpp @@ -35,7 +35,7 @@ using vespalib::BFloat16; template<typename AttributeCellType> class BoundEuclideanDistance : public BoundDistanceFunction { - using FloatType = std::conditional<std::is_same<AttributeCellType,BFloat16>::value,float,AttributeCellType>::type; + using FloatType = std::conditional_t<std::is_same<AttributeCellType,BFloat16>::value,float,AttributeCellType>; private: const vespalib::hwaccelrated::IAccelrated & _computer; mutable TemporaryVectorStore<FloatType> _tmpSpace; diff --git a/searchlib/src/vespa/searchlib/tensor/hnsw_nodeid_mapping.cpp b/searchlib/src/vespa/searchlib/tensor/hnsw_nodeid_mapping.cpp index 8c169e63f9a..a908ebd7210 100644 --- a/searchlib/src/vespa/searchlib/tensor/hnsw_nodeid_mapping.cpp +++ b/searchlib/src/vespa/searchlib/tensor/hnsw_nodeid_mapping.cpp @@ -82,14 +82,18 @@ HnswNodeidMapping::allocate_ids(uint32_t docid, uint32_t subspaces) vespalib::ConstArrayRef<uint32_t> HnswNodeidMapping::get_ids(uint32_t docid) const { - assert(docid < _refs.size()); + if (docid >= _refs.size()) { + return {}; + } return _nodeids.get(_refs[docid]); } void HnswNodeidMapping::free_ids(uint32_t docid) { - assert(docid < _refs.size()); + if (docid >= _refs.size()) { + return; + } EntryRef ref = _refs[docid]; if (!ref.valid()) { return; diff --git a/searchlib/src/vespa/searchlib/tensor/mips_distance_transform.cpp b/searchlib/src/vespa/searchlib/tensor/mips_distance_transform.cpp index 5ad3a044df6..392388b15a6 100644 --- a/searchlib/src/vespa/searchlib/tensor/mips_distance_transform.cpp +++ b/searchlib/src/vespa/searchlib/tensor/mips_distance_transform.cpp @@ -17,7 +17,7 @@ class BoundMipsDistanceFunction : public BoundDistanceFunction { const vespalib::ConstArrayRef<FloatType> _lhs_vector; const vespalib::hwaccelrated::IAccelrated & _computer; double _max_sq_norm; - using ExtraDimT = std::conditional<extra_dim,double,std::monostate>::type; + using ExtraDimT = std::conditional_t<extra_dim,double,std::monostate>; [[no_unique_address]] ExtraDimT _lhs_extra_dim; static const double *cast(const double * p) { return p; } diff --git a/storage/src/tests/common/dummystoragelink.h b/storage/src/tests/common/dummystoragelink.h index 8da92917c08..8e68f2e5a70 100644 --- a/storage/src/tests/common/dummystoragelink.h +++ b/storage/src/tests/common/dummystoragelink.h @@ -8,7 +8,6 @@ #include <string> #include <vector> #include <vespa/storage/common/storagelink.h> -#include <vespa/storage/common/bucketmessages.h> #include <vespa/storageapi/message/internal.h> namespace storage { diff --git a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp index bbe2af732f5..f39222722fc 100644 --- a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp +++ b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp @@ -1713,21 +1713,6 @@ TEST_F(FileStorManagerTest, set_bucket_active_state) { StorBucketDatabase::WrappedEntry entry(_node->getStorageBucketDatabase().get(bid, "foo")); EXPECT_TRUE(entry->info.isActive()); } - // Trigger bucket info to be read back into the database - { - auto cmd = std::make_shared<ReadBucketInfo>(makeDocumentBucket(bid)); - top.sendDown(cmd); - top.waitForMessages(1, _waitTime); - ASSERT_EQ(1, top.getNumReplies()); - auto reply = std::dynamic_pointer_cast<ReadBucketInfoReply>(top.getReply(0)); - top.reset(); - ASSERT_TRUE(reply.get()); - } - // Should not have lost active flag - { - StorBucketDatabase::WrappedEntry entry(_node->getStorageBucketDatabase().get(bid, "foo")); - EXPECT_TRUE(entry->info.isActive()); - } { auto cmd = std::make_shared<api::SetBucketStateCommand>( diff --git a/storage/src/vespa/storage/bucketdb/bucketmanager.h b/storage/src/vespa/storage/bucketdb/bucketmanager.h index ce60ec88bff..cb0bc6a9f95 100644 --- a/storage/src/vespa/storage/bucketdb/bucketmanager.h +++ b/storage/src/vespa/storage/bucketdb/bucketmanager.h @@ -11,7 +11,6 @@ #include "storbucketdb.h" #include <vespa/config/subscription/configuri.h> #include <vespa/storage/bucketdb/config-stor-bucketdb.h> -#include <vespa/storage/common/bucketmessages.h> #include <vespa/storage/common/servicelayercomponent.h> #include <vespa/storage/common/storagelinkqueued.h> #include <vespa/storageapi/message/bucket.h> diff --git a/storage/src/vespa/storage/common/CMakeLists.txt b/storage/src/vespa/storage/common/CMakeLists.txt index 4a712719d53..6165106f871 100644 --- a/storage/src/vespa/storage/common/CMakeLists.txt +++ b/storage/src/vespa/storage/common/CMakeLists.txt @@ -2,7 +2,6 @@ vespa_add_library(storage_common OBJECT SOURCES bucket_stripe_utils.cpp - bucketmessages.cpp content_bucket_space.cpp content_bucket_space_repo.cpp distributorcomponent.cpp diff --git a/storage/src/vespa/storage/common/bucketmessages.cpp b/storage/src/vespa/storage/common/bucketmessages.cpp deleted file mode 100644 index 1523ad1b0ef..00000000000 --- a/storage/src/vespa/storage/common/bucketmessages.cpp +++ /dev/null @@ -1,108 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#include "bucketmessages.h" -#include <vespa/vespalib/stllike/asciistream.h> -#include <ostream> - -using document::BucketSpace; - -namespace storage { - -ReadBucketList::ReadBucketList(BucketSpace bucketSpace) - : api::InternalCommand(ID), - _bucketSpace(bucketSpace) -{ } - -ReadBucketList::~ReadBucketList() = default; - -document::Bucket -ReadBucketList::getBucket() const -{ - return document::Bucket(_bucketSpace, document::BucketId()); -} - -void -ReadBucketList::print(std::ostream& out, bool verbose, const std::string& indent) const { - out << "ReadBucketList()"; - - if (verbose) { - out << " : "; - InternalCommand::print(out, true, indent); - } -} - -ReadBucketListReply::ReadBucketListReply(const ReadBucketList& cmd) - : api::InternalReply(ID, cmd), - _bucketSpace(cmd.getBucketSpace()) -{ } - -ReadBucketListReply::~ReadBucketListReply() = default; - -document::Bucket -ReadBucketListReply::getBucket() const -{ - return document::Bucket(_bucketSpace, document::BucketId()); -} - -void -ReadBucketListReply::print(std::ostream& out, bool verbose, const std::string& indent) const -{ - out << "ReadBucketListReply(" << _buckets.size() << " buckets)"; - if (verbose) { - out << " : "; - InternalReply::print(out, true, indent); - } -} - -std::unique_ptr<api::StorageReply> -ReadBucketList::makeReply() { - return std::make_unique<ReadBucketListReply>(*this); -} - -ReadBucketInfo::ReadBucketInfo(const document::Bucket &bucket) - : api::InternalCommand(ID), - _bucket(bucket) -{ } - -ReadBucketInfo::~ReadBucketInfo() = default; - -void -ReadBucketInfo::print(std::ostream& out, bool verbose, const std::string& indent) const -{ - out << "ReadBucketInfo(" << _bucket.getBucketId() << ")"; - - if (verbose) { - out << " : "; - InternalCommand::print(out, true, indent); - } -} - -vespalib::string -ReadBucketInfo::getSummary() const { - vespalib::string s("ReadBucketInfo("); - s.append(_bucket.toString()); - s.append(')'); - return s; -} - -ReadBucketInfoReply::ReadBucketInfoReply(const ReadBucketInfo& cmd) - : api::InternalReply(ID, cmd), - _bucket(cmd.getBucket()) -{ } - -ReadBucketInfoReply::~ReadBucketInfoReply() = default; -void -ReadBucketInfoReply::print(std::ostream& out, bool verbose, const std::string& indent) const { - out << "ReadBucketInfoReply()"; - if (verbose) { - out << " : "; - InternalReply::print(out, true, indent); - } -} - -std::unique_ptr<api::StorageReply> ReadBucketInfo::makeReply() { - return std::make_unique<ReadBucketInfoReply>(*this); -} - -} // storage - diff --git a/storage/src/vespa/storage/common/bucketmessages.h b/storage/src/vespa/storage/common/bucketmessages.h deleted file mode 100644 index ccee12938b2..00000000000 --- a/storage/src/vespa/storage/common/bucketmessages.h +++ /dev/null @@ -1,108 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#pragma once - -#include <vespa/persistence/spi/result.h> -#include <vespa/storageapi/message/internal.h> -#include <vespa/storageapi/buckets/bucketinfo.h> - -namespace storage { - -/** - * @class ReadBucketList - * @ingroup common - * - * @brief List buckets existing in a bucket space. - */ -class ReadBucketList : public api::InternalCommand { - document::BucketSpace _bucketSpace; - -public: - using UP = std::unique_ptr<ReadBucketList>; - static constexpr uint32_t ID = 2003; - - ReadBucketList(document::BucketSpace bucketSpace); - ~ReadBucketList(); - document::BucketSpace getBucketSpace() const { return _bucketSpace; } - document::Bucket getBucket() const override; - - std::unique_ptr<api::StorageReply> makeReply() override; - - void print(std::ostream& out, bool verbose, const std::string& indent) const override; -}; - - -/** - * @class ReadBucketListReply - * @ingroup common - */ -class ReadBucketListReply : public api::InternalReply { - document::BucketSpace _bucketSpace; - spi::BucketIdListResult::List _buckets; - -public: - using UP = std::unique_ptr<ReadBucketListReply>; - using SP = std::shared_ptr<ReadBucketListReply>; - static constexpr uint32_t ID = 2004; - - ReadBucketListReply(const ReadBucketList& cmd); - ~ReadBucketListReply(); - - document::BucketSpace getBucketSpace() const { return _bucketSpace; } - document::Bucket getBucket() const override; - - spi::BucketIdListResult::List& getBuckets() { return _buckets; } - const spi::BucketIdListResult::List& getBuckets() const { - return _buckets; - } - - void print(std::ostream& out, bool verbose, const std::string& indent) const override; -}; - -/** - * @class ReadBucketInfo - * @ingroup common - * - * @brief Get more detailed information about a set of buckets. - * - * The distributor wants some information for each bucket, that one - * have to open the bucket and read its headers to find. This class is - * used to retrieve such information. - */ -class ReadBucketInfo : public api::InternalCommand { - document::Bucket _bucket; - -public: - static constexpr uint32_t ID = 2005; - - ReadBucketInfo(const document::Bucket &bucket); - ~ReadBucketInfo(); - - document::Bucket getBucket() const override { return _bucket; } - - std::unique_ptr<api::StorageReply> makeReply() override; - - void print(std::ostream& out, bool verbose, const std::string& indent) const override; -private: - vespalib::string getSummary() const override; -}; - - -/** - * @class ReadBucketInfoReply - * @ingroup common - */ -class ReadBucketInfoReply : public api::InternalReply { - document::Bucket _bucket; - -public: - static constexpr uint32_t ID = 2006; - - ReadBucketInfoReply(const ReadBucketInfo& cmd); - ~ReadBucketInfoReply(); - - document::Bucket getBucket() const override { return _bucket; } - - void print(std::ostream& out, bool verbose, const std::string& indent) const override; -}; - -} // storage diff --git a/storage/src/vespa/storage/common/messagebucket.cpp b/storage/src/vespa/storage/common/messagebucket.cpp index 73b83936e98..286eef39e16 100644 --- a/storage/src/vespa/storage/common/messagebucket.cpp +++ b/storage/src/vespa/storage/common/messagebucket.cpp @@ -2,7 +2,6 @@ #include "messagebucket.h" #include "statusmessages.h" -#include "bucketmessages.h" #include <vespa/storageapi/message/bucket.h> #include <vespa/storageapi/message/bucketsplitting.h> #include <vespa/storageapi/message/persistence.h> @@ -60,10 +59,6 @@ getStorageMessageBucket(const api::StorageMessage& msg) return static_cast<const GetIterCommand&>(msg).getBucket(); case CreateIteratorCommand::ID: return static_cast<const CreateIteratorCommand&>(msg).getBucket(); - case ReadBucketList::ID: - return static_cast<const ReadBucketList&>(msg).getBucket(); - case ReadBucketInfo::ID: - return static_cast<const ReadBucketInfo&>(msg).getBucket(); case RecheckBucketInfoCommand::ID: return static_cast<const RecheckBucketInfoCommand&>(msg).getBucket(); case RunTaskCommand::ID: diff --git a/storage/src/vespa/storage/common/storagelink.cpp b/storage/src/vespa/storage/common/storagelink.cpp index 85ec26fda2f..2d566f1fc29 100644 --- a/storage/src/vespa/storage/common/storagelink.cpp +++ b/storage/src/vespa/storage/common/storagelink.cpp @@ -1,7 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "storagelink.h" -#include "bucketmessages.h" +#include <vespa/storageapi/messageapi/storagecommand.h> +#include <vespa/storageapi/messageapi/storagereply.h> #include <vespa/vespalib/util/backtrace.h> #include <sstream> #include <cassert> diff --git a/storage/src/vespa/storage/distributor/idealstatemanager.cpp b/storage/src/vespa/storage/distributor/idealstatemanager.cpp index 2c33bc490fe..cad141e76ed 100644 --- a/storage/src/vespa/storage/distributor/idealstatemanager.cpp +++ b/storage/src/vespa/storage/distributor/idealstatemanager.cpp @@ -8,7 +8,6 @@ #include "distributor_bucket_space.h" #include <vespa/vespalib/stllike/asciistream.h> #include <vespa/storageapi/message/persistence.h> -#include <vespa/storage/common/bucketmessages.h> #include <vespa/document/bucket/fixed_bucket_spaces.h> #include <vespa/vespalib/util/assert.h> #include <vespa/vespalib/stllike/hash_map.hpp> diff --git a/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp b/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp index 1d29a8795d5..33022c65e24 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp @@ -6,7 +6,6 @@ #include <vespa/storageapi/message/bucketsplitting.h> #include <vespa/storageapi/message/persistence.h> #include <vespa/storage/bucketdb/storbucketdb.h> -#include <vespa/storage/common/bucketmessages.h> #include <vespa/storage/common/statusmessages.h> #include <vespa/storage/common/messagebucket.h> #include <vespa/storage/persistence/asynchandler.h> @@ -666,7 +665,6 @@ FileStorHandlerImpl::remapMessage(api::StorageMessage& msg, const document::Buck } } break; - case ReadBucketInfo::ID: case RecheckBucketInfoCommand::ID: { LOG(debug, "While remapping load for bucket %s for reason %u, " diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp index da9d16c9356..d2c3cea44b0 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp @@ -4,7 +4,6 @@ #include "filestorhandlerimpl.h" #include <vespa/storageframework/generic/thread/thread.h> #include <vespa/storage/bucketdb/minimumusedbitstracker.h> -#include <vespa/storage/common/bucketmessages.h> #include <vespa/storage/common/content_bucket_space_repo.h> #include <vespa/storage/common/doneinitializehandler.h> #include <vespa/storage/common/hostreporter/hostinfo.h> @@ -755,21 +754,6 @@ FileStorManager::onInternal(const shared_ptr<api::InternalCommand>& msg) msg->getTrace().addChild(context.steal_trace()); return true; } - case ReadBucketList::ID: - { - shared_ptr<ReadBucketList> cmd(std::static_pointer_cast<ReadBucketList>(msg)); - handlePersistenceMessage(cmd); - return true; - } - case ReadBucketInfo::ID: - { - shared_ptr<ReadBucketInfo> cmd(std::static_pointer_cast<ReadBucketInfo>(msg)); - StorBucketDatabase::WrappedEntry entry(mapOperationToDisk(*cmd, cmd->getBucket())); - if (entry.exists()) { - handlePersistenceMessage(cmd); - } - return true; - } case RecheckBucketInfoCommand::ID: { shared_ptr<RecheckBucketInfoCommand> cmd(std::static_pointer_cast<RecheckBucketInfoCommand>(msg)); diff --git a/storage/src/vespa/storage/persistence/persistencehandler.cpp b/storage/src/vespa/storage/persistence/persistencehandler.cpp index 69f910d0910..00ab61f2304 100644 --- a/storage/src/vespa/storage/persistence/persistencehandler.cpp +++ b/storage/src/vespa/storage/persistence/persistencehandler.cpp @@ -100,10 +100,6 @@ PersistenceHandler::handleCommandSplitByType(api::StorageCommand& msg, MessageTr auto usage = vespalib::CpuUsage::use(CpuUsage::Category::READ); return _simpleHandler.handleCreateIterator(static_cast<CreateIteratorCommand&>(msg), std::move(tracker)); } - case ReadBucketList::ID: - return _simpleHandler.handleReadBucketList(static_cast<ReadBucketList&>(msg), std::move(tracker)); - case ReadBucketInfo::ID: - return _simpleHandler.handleReadBucketInfo(static_cast<ReadBucketInfo&>(msg), std::move(tracker)); case RecheckBucketInfoCommand::ID: return _splitJoinHandler.handleRecheckBucketInfo(static_cast<RecheckBucketInfoCommand&>(msg), std::move(tracker)); case RunTaskCommand::ID: diff --git a/storage/src/vespa/storage/persistence/simplemessagehandler.cpp b/storage/src/vespa/storage/persistence/simplemessagehandler.cpp index ea929bf8620..1ac0939c21e 100644 --- a/storage/src/vespa/storage/persistence/simplemessagehandler.cpp +++ b/storage/src/vespa/storage/persistence/simplemessagehandler.cpp @@ -131,29 +131,6 @@ SimpleMessageHandler::handleGetIter(GetIterCommand& cmd, MessageTracker::UP trac } MessageTracker::UP -SimpleMessageHandler::handleReadBucketList(ReadBucketList& cmd, MessageTracker::UP tracker) const -{ - tracker->setMetric(_env._metrics.readBucketList); - - spi::BucketIdListResult result(_spi.listBuckets(cmd.getBucketSpace())); - if (tracker->checkForError(result)) { - auto reply = std::make_shared<ReadBucketListReply>(cmd); - result.getList().swap(reply->getBuckets()); - tracker->setReply(reply); - } - - return tracker; -} - -MessageTracker::UP -SimpleMessageHandler::handleReadBucketInfo(ReadBucketInfo& cmd, MessageTracker::UP tracker) const -{ - tracker->setMetric(_env._metrics.readBucketInfo); - _env.updateBucketDatabase(cmd.getBucket(), _env.getBucketInfo(cmd.getBucket())); - return tracker; -} - -MessageTracker::UP SimpleMessageHandler::handleCreateIterator(CreateIteratorCommand& cmd, MessageTracker::UP tracker) const { tracker->setMetric(_env._metrics.createIterator); diff --git a/storage/src/vespa/storage/persistence/simplemessagehandler.h b/storage/src/vespa/storage/persistence/simplemessagehandler.h index a5a19772556..49432c1ccb7 100644 --- a/storage/src/vespa/storage/persistence/simplemessagehandler.h +++ b/storage/src/vespa/storage/persistence/simplemessagehandler.h @@ -4,7 +4,6 @@ #include "types.h" #include "messages.h" -#include <vespa/storage/common/bucketmessages.h> #include <vespa/storageapi/message/persistence.h> namespace document { class BucketIdFactory; } @@ -28,8 +27,6 @@ public: MessageTrackerUP handleRevert(api::RevertCommand& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleCreateIterator(CreateIteratorCommand& cmd, MessageTrackerUP tracker) const; MessageTrackerUP handleGetIter(GetIterCommand& cmd, MessageTrackerUP tracker) const; - MessageTrackerUP handleReadBucketList(ReadBucketList& cmd, MessageTrackerUP tracker) const; - MessageTrackerUP handleReadBucketInfo(ReadBucketInfo& cmd, MessageTrackerUP tracker) const; private: MessageTrackerUP handle_conditional_get(api::GetCommand& cmd, MessageTrackerUP tracker) const; diff --git a/vespa-dependencies-enforcer/allowed-maven-dependencies.txt b/vespa-dependencies-enforcer/allowed-maven-dependencies.txt index e04c19a8486..a57580c46b0 100644 --- a/vespa-dependencies-enforcer/allowed-maven-dependencies.txt +++ b/vespa-dependencies-enforcer/allowed-maven-dependencies.txt @@ -14,15 +14,15 @@ com.amazonaws:aws-java-sdk-ssm:1.12.460 com.amazonaws:aws-java-sdk-sts:1.12.460 com.amazonaws:jmespath-java:1.12.460 com.auth0:java-jwt:3.10.0 -com.fasterxml.jackson.core:jackson-annotations:2.15.0 -com.fasterxml.jackson.core:jackson-core:2.15.0 -com.fasterxml.jackson.core:jackson-databind:2.15.0 -com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.15.0 -com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.15.0 -com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.15.0 -com.fasterxml.jackson.jaxrs:jackson-jaxrs-base:2.15.0 -com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:2.15.0 -com.fasterxml.jackson.module:jackson-module-jaxb-annotations:2.15.0 +com.fasterxml.jackson.core:jackson-annotations:2.15.2 +com.fasterxml.jackson.core:jackson-core:2.15.2 +com.fasterxml.jackson.core:jackson-databind:2.15.2 +com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.15.2 +com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.15.2 +com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.15.2 +com.fasterxml.jackson.jaxrs:jackson-jaxrs-base:2.15.2 +com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:2.15.2 +com.fasterxml.jackson.module:jackson-module-jaxb-annotations:2.15.2 com.github.spotbugs:spotbugs-annotations:3.1.9 com.google.code.findbugs:jsr305:3.0.2 com.google.errorprone:error_prone_annotations:2.18.0 @@ -210,7 +210,7 @@ org.sonatype.sisu:sisu-guice:2.1.7:noaop org.sonatype.sisu:sisu-inject-bean:1.4.2 org.sonatype.sisu:sisu-inject-plexus:1.4.2 org.tukaani:xz:1.8 -org.xerial.snappy:snappy-java:1.1.7 +org.xerial.snappy:snappy-java:1.1.10.1 software.amazon.ion:ion-java:1.0.2 xerces:xercesImpl:2.12.2 xml-apis:xml-apis:1.4.01 diff --git a/vespalib/src/tests/datastore/array_store/array_store_test.cpp b/vespalib/src/tests/datastore/array_store/array_store_test.cpp index 37d5fc66c8b..c9f1230346c 100644 --- a/vespalib/src/tests/datastore/array_store/array_store_test.cpp +++ b/vespalib/src/tests/datastore/array_store/array_store_test.cpp @@ -1,6 +1,8 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include <vespa/vespalib/datastore/array_store.hpp> +#include <vespa/vespalib/datastore/array_store_dynamic_type_mapper.hpp> +#include <vespa/vespalib/datastore/dynamic_array_buffer_type.hpp> #include <vespa/vespalib/datastore/compaction_spec.h> #include <vespa/vespalib/datastore/compaction_strategy.h> #include <vespa/vespalib/gtest/gtest.h> @@ -27,38 +29,57 @@ namespace { constexpr float ALLOC_GROW_FACTOR = 0.2; +template <typename ElemT> +class MyArrayStoreSimpleTypeMapper : public ArrayStoreSimpleTypeMapper<ElemT> { +public: + MyArrayStoreSimpleTypeMapper(uint32_t, double) + : ArrayStoreSimpleTypeMapper<ElemT>() + { + } +}; + } -template <typename TestT, typename ElemT, typename RefT = EntryRefT<19> > +template <typename TestT, typename ElemT, typename RefT = EntryRefT<19>, typename TypeMapper = ArrayStoreDynamicTypeMapper<ElemT>> struct ArrayStoreTest : public TestT { using EntryRefType = RefT; - using ArrayStoreType = ArrayStore<ElemT, RefT>; + using ArrayStoreType = ArrayStore<ElemT, RefT, TypeMapper>; using LargeArray = typename ArrayStoreType::LargeArray; using ConstArrayRef = typename ArrayStoreType::ConstArrayRef; using ElemVector = std::vector<ElemT>; using value_type = ElemT; using ReferenceStore = vespalib::hash_map<EntryRef, ElemVector>; + using TypeMapperType = TypeMapper; + static constexpr bool simple_type_mapper = std::is_same_v<TypeMapperType,ArrayStoreSimpleTypeMapper<ElemT>>; + using TypeMapperWrappedType = std::conditional_t<simple_type_mapper,MyArrayStoreSimpleTypeMapper<ElemT>,TypeMapperType>; AllocStats stats; + TypeMapperWrappedType type_mapper; ArrayStoreType store; ReferenceStore refStore; generation_t generation; bool add_using_allocate; - ArrayStoreTest(uint32_t maxSmallArraySize = 3, bool enable_free_lists = true, bool add_using_allocate_in = false) - : store(ArrayStoreConfig(maxSmallArraySize, + double type_mapper_grow_factor; + ArrayStoreTest(uint32_t maxSmallArraySize = 3, bool enable_free_lists = true, bool add_using_allocate_in = false, double type_mapper_grow_factor_in = 2.0) + : type_mapper(maxSmallArraySize, type_mapper_grow_factor_in), + store(ArrayStoreConfig(maxSmallArraySize, ArrayStoreConfig::AllocSpec(16, RefT::offsetSize(), 8_Ki, ALLOC_GROW_FACTOR)).enable_free_lists(enable_free_lists), - std::make_unique<MemoryAllocatorObserver>(stats)), + std::make_unique<MemoryAllocatorObserver>(stats), + TypeMapperType(type_mapper)), refStore(), generation(1), - add_using_allocate(add_using_allocate_in) + add_using_allocate(add_using_allocate_in), + type_mapper_grow_factor(type_mapper_grow_factor_in) {} explicit ArrayStoreTest(const ArrayStoreConfig &storeCfg) - : store(storeCfg, std::make_unique<MemoryAllocatorObserver>(stats)), + : type_mapper(storeCfg.maxSmallArrayTypeId(), 2.0), + store(storeCfg, std::make_unique<MemoryAllocatorObserver>(stats), TypeMapperType(type_mapper)), refStore(), generation(1), - add_using_allocate(false) + add_using_allocate(false), + type_mapper_grow_factor(2.0) {} ~ArrayStoreTest() override; void assertAdd(const ElemVector &input) { @@ -163,39 +184,78 @@ struct ArrayStoreTest : public TestT } size_t elem_size() const { return sizeof(ElemT); } size_t largeArraySize() const { return sizeof(LargeArray); } + bool simple_buffers() const { return simple_type_mapper || type_mapper_grow_factor == 1.0; } }; -template <typename TestT, typename ElemT, typename RefT> -ArrayStoreTest<TestT, ElemT, RefT>::~ArrayStoreTest() = default; +template <typename TestT, typename ElemT, typename RefT, typename TypeMapper> +ArrayStoreTest<TestT, ElemT, RefT, TypeMapper>::~ArrayStoreTest() = default; -template <typename TestT, typename ElemT, typename RefT> +template <typename TestT, typename ElemT, typename RefT, typename TypeMapper> size_t -ArrayStoreTest<TestT, ElemT, RefT>::reference_store_count(EntryRef ref) const +ArrayStoreTest<TestT, ElemT, RefT, TypeMapper>::reference_store_count(EntryRef ref) const { return refStore.count(ref); } -struct TestParam { - bool add_using_allocate; - TestParam(bool add_using_allocate_in) : add_using_allocate(add_using_allocate_in) {} +struct SimpleTypeMapperAdd { + using TypeMapper = ArrayStoreSimpleTypeMapper<uint32_t>; + static constexpr bool add_using_allocate = false; + static constexpr double type_mapper_grow_factor = 1.0; }; -std::ostream& operator<<(std::ostream& os, const TestParam& param) -{ - os << (param.add_using_allocate ? "add_using_allocate" : "basic_add"); - return os; -} +struct SimpleTypeMapperAllocate { + using TypeMapper = ArrayStoreSimpleTypeMapper<uint32_t>; + static constexpr bool add_using_allocate = true; + static constexpr double type_mapper_grow_factor = 1.0; +}; + +struct DynamicTypeMapperAddGrow1 { + using TypeMapper = ArrayStoreDynamicTypeMapper<uint32_t>; + static constexpr bool add_using_allocate = false; + static constexpr double type_mapper_grow_factor = 1.0; +}; + +struct DynamicTypeMapperAllocateGrow1 { + using TypeMapper = ArrayStoreDynamicTypeMapper<uint32_t>; + static constexpr bool add_using_allocate = true; + static constexpr double type_mapper_grow_factor = 1.0; +}; + +struct DynamicTypeMapperAddGrow2 { + using TypeMapper = ArrayStoreDynamicTypeMapper<uint32_t>; + static constexpr bool add_using_allocate = false; + static constexpr double type_mapper_grow_factor = 2.0; +}; + +struct DynamicTypeMapperAllocateGrow2 { + using TypeMapper = ArrayStoreDynamicTypeMapper<uint32_t>; + static constexpr bool add_using_allocate = true; + static constexpr double type_mapper_grow_factor = 2.0; +}; -using NumberStoreTestWithParam = ArrayStoreTest<testing::TestWithParam<TestParam>, uint32_t>; +template <typename TypeMapper> +using NumberStoreTestWithParam = ArrayStoreTest<testing::Test, uint32_t, EntryRefT<19>, TypeMapper>; -struct NumberStoreTest : public NumberStoreTestWithParam { - NumberStoreTest() : NumberStoreTestWithParam(3, true, GetParam().add_using_allocate) {} +template <typename Param> +struct NumberStoreTest : public NumberStoreTestWithParam<typename Param::TypeMapper> { + using Parent = NumberStoreTestWithParam<typename Param::TypeMapper>; + NumberStoreTest() : Parent(3, true, Param::add_using_allocate, Param::type_mapper_grow_factor) {} }; -struct NumberStoreFreeListsDisabledTest : public NumberStoreTestWithParam { - NumberStoreFreeListsDisabledTest() : NumberStoreTestWithParam(3, false, GetParam().add_using_allocate) {} +template <typename Param> +struct NumberStoreFreeListsDisabledTest : public NumberStoreTestWithParam<typename Param::TypeMapper> { + using Parent = NumberStoreTestWithParam<typename Param::TypeMapper>; + NumberStoreFreeListsDisabledTest() : Parent(3, false, Param::add_using_allocate, Param::type_mapper_grow_factor) {} }; +using NumberStoreTestTypes = testing::Types<SimpleTypeMapperAdd, SimpleTypeMapperAllocate, + DynamicTypeMapperAddGrow1, DynamicTypeMapperAllocateGrow1, + DynamicTypeMapperAddGrow2, DynamicTypeMapperAllocateGrow2>; + +TYPED_TEST_SUITE(NumberStoreTest, NumberStoreTestTypes); + +TYPED_TEST_SUITE(NumberStoreFreeListsDisabledTest, NumberStoreTestTypes); + using NumberStoreBasicTest = ArrayStoreTest<testing::Test, uint32_t>; using StringStoreTest = ArrayStoreTest<testing::Test, std::string>; using SmallOffsetNumberStoreTest = ArrayStoreTest<testing::Test, uint32_t, EntryRefT<10>>; @@ -206,32 +266,54 @@ TEST(BasicStoreTest, test_with_trivial_and_non_trivial_types) EXPECT_FALSE(vespalib::can_skip_destruction<StringStoreTest::value_type>); } -INSTANTIATE_TEST_SUITE_P(NumberStoreMultiTest, - NumberStoreTest, - testing::Values(TestParam(false), TestParam(true)), - testing::PrintToStringParamName()); - -INSTANTIATE_TEST_SUITE_P(NumberStoreFreeListsDisabledMultiTest, - NumberStoreFreeListsDisabledTest, - testing::Values(TestParam(false), TestParam(true)), - testing::PrintToStringParamName()); - -TEST_P(NumberStoreTest, control_static_sizes) { +TYPED_TEST(NumberStoreTest, control_static_sizes) { static constexpr size_t sizeof_deque = vespalib::datastore::DataStoreBase::sizeof_entry_ref_hold_list_deque; - EXPECT_EQ(416u + sizeof_deque, sizeof(store)); - EXPECT_EQ(240u + sizeof_deque, sizeof(NumberStoreTest::ArrayStoreType::DataStoreType)); - EXPECT_EQ(112u, sizeof(NumberStoreTest::ArrayStoreType::SmallBufferType)); - MemoryUsage usage = store.getMemoryUsage(); - EXPECT_EQ(202140u, usage.allocatedBytes()); - EXPECT_EQ(197680u, usage.usedBytes()); + if constexpr (TestFixture::simple_type_mapper) { + EXPECT_EQ(416u + sizeof_deque, sizeof(this->store)); + } else { + EXPECT_EQ(464u + sizeof_deque, sizeof(this->store)); + } + EXPECT_EQ(240u + sizeof_deque, sizeof(typename TestFixture::ArrayStoreType::DataStoreType)); + EXPECT_EQ(112u, sizeof(typename TestFixture::ArrayStoreType::SmallBufferType)); + MemoryUsage usage = this->store.getMemoryUsage(); + if (this->simple_buffers()) { + EXPECT_EQ(202140u, usage.allocatedBytes()); + EXPECT_EQ(197680u, usage.usedBytes()); + } else { + EXPECT_EQ(202388u, usage.allocatedBytes()); + EXPECT_EQ(197568u, usage.usedBytes()); + } } -TEST_P(NumberStoreTest, add_and_get_small_arrays_of_trivial_type) +TYPED_TEST(NumberStoreTest, control_type_mapper) { - assertAdd({}); - assertAdd({1}); - assertAdd({2,3}); - assertAdd({3,4,5}); + if constexpr (TestFixture::simple_type_mapper) { + GTEST_SKIP() << "Skipping test due to using simple type mapper"; + } else { + EXPECT_EQ(3, this->type_mapper.get_max_small_array_type_id(1000)); + EXPECT_FALSE(this->type_mapper.is_dynamic_buffer(0)); + EXPECT_FALSE(this->type_mapper.is_dynamic_buffer(1)); + EXPECT_EQ(1, this->type_mapper.get_array_size(1)); + EXPECT_FALSE(this->type_mapper.is_dynamic_buffer(2)); + EXPECT_EQ(2, this->type_mapper.get_array_size(2)); + if (this->type_mapper_grow_factor == 1.0) { + EXPECT_FALSE(this->type_mapper.is_dynamic_buffer(3)); + EXPECT_EQ(3, this->type_mapper.get_array_size(3)); + EXPECT_EQ(0, this->type_mapper.count_dynamic_buffer_types(3)); + } else { + EXPECT_TRUE(this->type_mapper.is_dynamic_buffer(3)); + EXPECT_EQ(4, this->type_mapper.get_array_size(3)); + EXPECT_EQ(1, this->type_mapper.count_dynamic_buffer_types(3)); + } + } +} + +TYPED_TEST(NumberStoreTest, add_and_get_small_arrays_of_trivial_type) +{ + this->assertAdd({}); + this->assertAdd({1}); + this->assertAdd({2,3}); + this->assertAdd({3,4,5}); } TEST_F(StringStoreTest, add_and_get_small_arrays_of_non_trivial_type) @@ -242,60 +324,60 @@ TEST_F(StringStoreTest, add_and_get_small_arrays_of_non_trivial_type) assertAdd({"ddd", "eeee", "fffff"}); } -TEST_P(NumberStoreTest, add_and_get_large_arrays_of_simple_type) +TYPED_TEST(NumberStoreTest, add_and_get_large_arrays_of_simple_type) { - assertAdd({1,2,3,4}); - assertAdd({2,3,4,5,6}); + this->assertAdd({1,2,3,4,5}); + this->assertAdd({2,3,4,5,6,7}); } TEST_F(StringStoreTest, add_and_get_large_arrays_of_non_trivial_type) { - assertAdd({"aa", "bb", "cc", "dd"}); - assertAdd({"ddd", "eee", "ffff", "gggg", "hhhh"}); + assertAdd({"aa", "bb", "cc", "dd", "ee"}); + assertAdd({"ddd", "eee", "ffff", "gggg", "hhhh", "iiii"}); } -TEST_P(NumberStoreTest, entries_are_put_on_hold_when_a_small_array_is_removed) +TYPED_TEST(NumberStoreTest, entries_are_put_on_hold_when_a_small_array_is_removed) { - EntryRef ref = add({1,2,3}); - assertBufferState(ref, MemStats().used(1).hold(0)); - store.remove(ref); - assertBufferState(ref, MemStats().used(1).hold(1)); + EntryRef ref = this->add({1,2,3}); + this->assertBufferState(ref, MemStats().used(1).hold(0)); + this->store.remove(ref); + this->assertBufferState(ref, MemStats().used(1).hold(1)); } -TEST_P(NumberStoreTest, entries_are_put_on_hold_when_a_large_array_is_removed) +TYPED_TEST(NumberStoreTest, entries_are_put_on_hold_when_a_large_array_is_removed) { - EntryRef ref = add({1,2,3,4}); + EntryRef ref = this->add({1,2,3,4,5}); // Note: The first buffer has the first element reserved -> we expect 2 elements used here. - assertBufferState(ref, MemStats().used(2).hold(0).dead(1)); - store.remove(ref); - assertBufferState(ref, MemStats().used(2).hold(1).dead(1)); + this->assertBufferState(ref, MemStats().used(2).hold(0).dead(1)); + this->store.remove(ref); + this->assertBufferState(ref, MemStats().used(2).hold(1).dead(1)); } -TEST_P(NumberStoreTest, small_arrays_are_allocated_from_free_lists_when_enabled) { - assert_ref_reused({1,2,3}, {4,5,6}, true); +TYPED_TEST(NumberStoreTest, small_arrays_are_allocated_from_free_lists_when_enabled) { + this->assert_ref_reused({1,2,3}, {4,5,6}, true); } -TEST_P(NumberStoreTest, large_arrays_are_allocated_from_free_lists_when_enabled) { - assert_ref_reused({1,2,3,4}, {5,6,7,8}, true); +TYPED_TEST(NumberStoreTest, large_arrays_are_allocated_from_free_lists_when_enabled) { + this->assert_ref_reused({1,2,3,4,5}, {5,6,7,8,9}, true); } -TEST_P(NumberStoreFreeListsDisabledTest, small_arrays_are_NOT_allocated_from_free_lists_when_disabled) { - assert_ref_reused({1,2,3}, {4,5,6}, false); +TYPED_TEST(NumberStoreFreeListsDisabledTest, small_arrays_are_NOT_allocated_from_free_lists_when_disabled) { + this->assert_ref_reused({1,2,3}, {4,5,6}, false); } -TEST_P(NumberStoreFreeListsDisabledTest, large_arrays_are_NOT_allocated_from_free_lists_when_disabled) { - assert_ref_reused({1,2,3,4}, {5,6,7,8}, false); +TYPED_TEST(NumberStoreFreeListsDisabledTest, large_arrays_are_NOT_allocated_from_free_lists_when_disabled) { + this->assert_ref_reused({1,2,3,4,5}, {5,6,7,8,9}, false); } -TEST_P(NumberStoreTest, track_size_of_large_array_allocations_with_free_lists_enabled) { - EntryRef ref = add({1,2,3,4}); - assert_buffer_stats(ref, TestBufferStats().used(2).hold(0).dead(1).extra_used(16)); - remove({1,2,3,4}); - assert_buffer_stats(ref, TestBufferStats().used(2).hold(1).dead(1).extra_hold(16).extra_used(16)); - reclaim_memory(); - assert_buffer_stats(ref, TestBufferStats().used(2).hold(0).dead(2).extra_used(0)); - add({5,6,7,8,9}); - assert_buffer_stats(ref, TestBufferStats().used(2).hold(0).dead(1).extra_used(20)); +TYPED_TEST(NumberStoreTest, track_size_of_large_array_allocations_with_free_lists_enabled) { + EntryRef ref = this->add({1,2,3,4,5}); + this->assert_buffer_stats(ref, TestBufferStats().used(2).hold(0).dead(1).extra_used(20)); + this->remove({1,2,3,4,5}); + this->assert_buffer_stats(ref, TestBufferStats().used(2).hold(1).dead(1).extra_hold(20).extra_used(20)); + this->reclaim_memory(); + this->assert_buffer_stats(ref, TestBufferStats().used(2).hold(0).dead(2).extra_used(0)); + this->add({5,6,7,8,9,10}); + this->assert_buffer_stats(ref, TestBufferStats().used(2).hold(0).dead(1).extra_used(24)); } TEST_F(SmallOffsetNumberStoreTest, new_underlying_buffer_is_allocated_when_current_is_full) @@ -361,7 +443,8 @@ TEST_F(NumberStoreTwoSmallBufferTypesTest, buffer_with_most_dead_space_is_compac namespace { -void testCompaction(NumberStoreTest &f, bool compactMemory, bool compactAddressSpace) +template <typename Fixture> +void testCompaction(Fixture &f, bool compactMemory, bool compactAddressSpace) { EntryRef size1Ref = f.add({1}); EntryRef size2Ref = f.add({2,2}); @@ -422,52 +505,53 @@ void testCompaction(NumberStoreTest &f, bool compactMemory, bool compactAddressS } -TEST_P(NumberStoreTest, compactWorst_selects_on_only_memory) { - testCompaction(*this, true, false); +TYPED_TEST(NumberStoreTest, compactWorst_selects_on_only_memory) { + testCompaction<typename TestFixture::Parent>(*this, true, false); } -TEST_P(NumberStoreTest, compactWorst_selects_on_only_address_space) { - testCompaction(*this, false, true); +TYPED_TEST(NumberStoreTest, compactWorst_selects_on_only_address_space) { + testCompaction<typename TestFixture::Parent>(*this, false, true); } -TEST_P(NumberStoreTest, compactWorst_selects_on_both_memory_and_address_space) { - testCompaction(*this, true, true); +TYPED_TEST(NumberStoreTest, compactWorst_selects_on_both_memory_and_address_space) { + testCompaction<typename TestFixture::Parent>(*this, true, true); } -TEST_P(NumberStoreTest, compactWorst_selects_on_neither_memory_nor_address_space) { - testCompaction(*this, false, false); +TYPED_TEST(NumberStoreTest, compactWorst_selects_on_neither_memory_nor_address_space) { + testCompaction<typename TestFixture::Parent>(*this, false, false); } -TEST_P(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_small_arrays) +TYPED_TEST(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_small_arrays) { - MemStats exp(store.getMemoryUsage()); - add({1,2,3}); - assertMemoryUsage(exp.used(elem_size() * 3)); - remove({1,2,3}); - assertMemoryUsage(exp.hold(elem_size() * 3)); - reclaim_memory(); - assertMemoryUsage(exp.holdToDead(elem_size() * 3)); + MemStats exp(this->store.getMemoryUsage()); + this->add({1,2,3}); + uint32_t exp_entry_size = this->simple_buffers() ? (this->elem_size() * 3) : (this->elem_size() * 4 + 4); + this->assertMemoryUsage(exp.used(exp_entry_size)); + this->remove({1,2,3}); + this->assertMemoryUsage(exp.hold(exp_entry_size)); + this->reclaim_memory(); + this->assertMemoryUsage(exp.holdToDead(exp_entry_size)); } -TEST_P(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_large_arrays) +TYPED_TEST(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_large_arrays) { - MemStats exp(store.getMemoryUsage()); - add({1,2,3,4}); - assertMemoryUsage(exp.used(largeArraySize() + elem_size() * 4)); - remove({1,2,3,4}); - assertMemoryUsage(exp.hold(largeArraySize() + elem_size() * 4)); - reclaim_memory(); - assertMemoryUsage(exp.decUsed(elem_size() * 4).decHold(largeArraySize() + elem_size() * 4). - dead(largeArraySize())); + MemStats exp(this->store.getMemoryUsage()); + this->add({1,2,3,4,5}); + this->assertMemoryUsage(exp.used(this->largeArraySize() + this->elem_size() * 5)); + this->remove({1,2,3,4,5}); + this->assertMemoryUsage(exp.hold(this->largeArraySize() + this->elem_size() * 5)); + this->reclaim_memory(); + this->assertMemoryUsage(exp.decUsed(this->elem_size() * 5).decHold(this->largeArraySize() + this->elem_size() * 5). + dead(this->largeArraySize())); } -TEST_P(NumberStoreTest, address_space_usage_is_ratio_between_used_arrays_and_number_of_possible_arrays) +TYPED_TEST(NumberStoreTest, address_space_usage_is_ratio_between_used_arrays_and_number_of_possible_arrays) { - add({2,2}); - add({3,3,3}); + this->add({2,2}); + this->add({3,3,3}); // 1 array is reserved (buffer 0, offset 0). - EXPECT_EQ(3u, store.addressSpaceUsage().used()); - EXPECT_EQ(1u, store.addressSpaceUsage().dead()); + EXPECT_EQ(3u, this->store.addressSpaceUsage().used()); + EXPECT_EQ(1u, this->store.addressSpaceUsage().dead()); size_t fourgig = (1ull << 32); /* * Expected limit is sum of allocated arrays for active buffers and @@ -479,14 +563,18 @@ TEST_P(NumberStoreTest, address_space_usage_is_ratio_between_used_arrays_and_num * 16 * 3 * sizeof(int) = 192 -> 256. * allocated elements = 256 / sizeof(int) = 64. * limit = 64 / 3 = 21. + * + * For dynamic buffer 3, we have 16 * 5 * sizeof(int) => 320 -> 512 + * limit = 512 / (5 * 4) = 25 */ - size_t expLimit = fourgig - 4 * NumberStoreTest::EntryRefType::offsetSize() + 3 * 16 + 21; - EXPECT_EQ(static_cast<double>(2)/ expLimit, store.addressSpaceUsage().usage()); - EXPECT_EQ(expLimit, store.addressSpaceUsage().limit()); + size_t type_id_3_entries = this->simple_buffers() ? 21 : 25; + size_t expLimit = fourgig - 4 * TestFixture::EntryRefType::offsetSize() + 3 * 16 + type_id_3_entries; + EXPECT_EQ(static_cast<double>(2)/ expLimit, this->store.addressSpaceUsage().usage()); + EXPECT_EQ(expLimit, this->store.addressSpaceUsage().limit()); } -struct ByteStoreTest : public ArrayStoreTest<testing::Test, uint8_t> { - ByteStoreTest() : ArrayStoreTest<testing::Test, uint8_t>(ByteStoreTest::ArrayStoreType:: +struct ByteStoreTest : public ArrayStoreTest<testing::Test, uint8_t, EntryRefT<19>, ArrayStoreSimpleTypeMapper<uint8_t>> { + ByteStoreTest() : ArrayStoreTest<testing::Test, uint8_t, EntryRefT<19>, ArrayStoreSimpleTypeMapper<uint8_t>>(ByteStoreTest::ArrayStoreType:: optimizedConfigForHugePage(1023, vespalib::alloc::MemoryAllocator::HUGEPAGE_SIZE, vespalib::alloc::MemoryAllocator::PAGE_SIZE, @@ -503,9 +591,9 @@ TEST_F(ByteStoreTest, offset_in_EntryRefT_is_within_bounds_when_allocating_memor assertStoreContent(); } -TEST_P(NumberStoreTest, provided_memory_allocator_is_used) +TYPED_TEST(NumberStoreTest, provided_memory_allocator_is_used) { - EXPECT_EQ(AllocStats(4, 0), stats); + EXPECT_EQ(AllocStats(4, 0), this->stats); } GTEST_MAIN_RUN_ALL_TESTS() diff --git a/vespalib/src/tests/stllike/cache_test.cpp b/vespalib/src/tests/stllike/cache_test.cpp index 358a3591dbf..715fda1cbfd 100644 --- a/vespalib/src/tests/stllike/cache_test.cpp +++ b/vespalib/src/tests/stllike/cache_test.cpp @@ -136,4 +136,64 @@ TEST("testThatMultipleRemoveOnOverflowIsFine") { EXPECT_EQUAL(2924u, cache.sizeBytes()); } +class ExtendedCache : public cache< CacheParam<P, B> > { +public: + ExtendedCache(BackingStore & b, size_t maxBytes) + : cache<CacheParam<P, B>>(b, maxBytes), + _insert_count(0), + _remove_count(0) + {} + size_t _insert_count; + size_t _remove_count; +private: + void onRemove(const K &) override { + _remove_count++; + } + + void onInsert(const K &) override { + _insert_count++; + } +}; + +TEST("testOnInsertonRemoveCalledWhenFull") { + B m; + ExtendedCache cache(m, 200); + EXPECT_EQUAL(0u, cache._insert_count); + EXPECT_EQUAL(0u, cache._remove_count); + cache.write(1, "15 bytes string"); + EXPECT_EQUAL(1u, cache.size()); + EXPECT_EQUAL(80u, cache.sizeBytes()); + EXPECT_EQUAL(1u, cache._insert_count); + EXPECT_EQUAL(0u, cache._remove_count); + cache.write(2, "16 bytes stringg"); + EXPECT_EQUAL(2u, cache.size()); + EXPECT_EQUAL(160u, cache.sizeBytes()); + EXPECT_EQUAL(2u, cache._insert_count); + EXPECT_EQUAL(0u, cache._remove_count); + cache.write(3, "17 bytes stringgg"); + EXPECT_EQUAL(3u, cache.size()); + EXPECT_EQUAL(240u, cache.sizeBytes()); + EXPECT_EQUAL(3u, cache._insert_count); + EXPECT_EQUAL(0u, cache._remove_count); + EXPECT_TRUE(cache.hasKey(1)); + cache.write(4, "18 bytes stringggg"); + EXPECT_EQUAL(3u, cache.size()); + EXPECT_EQUAL(240u, cache.sizeBytes()); + EXPECT_EQUAL(4u, cache._insert_count); + EXPECT_EQUAL(1u, cache._remove_count); + EXPECT_FALSE(cache.hasKey(1)); + cache.invalidate(2); + EXPECT_EQUAL(2u, cache.size()); + EXPECT_EQUAL(160u, cache.sizeBytes()); + EXPECT_EQUAL(4u, cache._insert_count); + EXPECT_EQUAL(2u, cache._remove_count); + EXPECT_FALSE(cache.hasKey(2)); + cache.invalidate(3); + EXPECT_EQUAL(1u, cache.size()); + EXPECT_EQUAL(80u, cache.sizeBytes()); + EXPECT_EQUAL(4u, cache._insert_count); + EXPECT_EQUAL(3u, cache._remove_count); + EXPECT_FALSE(cache.hasKey(3)); +} + TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/vespalib/src/vespa/vespalib/datastore/allocator.h b/vespalib/src/vespa/vespalib/datastore/allocator.h index 30938bdc1c1..432232a8879 100644 --- a/vespalib/src/vespa/vespalib/datastore/allocator.h +++ b/vespalib/src/vespa/vespalib/datastore/allocator.h @@ -31,6 +31,8 @@ public: HandleType allocArray(ConstArrayRef array); HandleType allocArray(); + template <typename BufferType> + HandleType alloc_dynamic_array(ConstArrayRef array); }; } diff --git a/vespalib/src/vespa/vespalib/datastore/allocator.hpp b/vespalib/src/vespa/vespalib/datastore/allocator.hpp index fa97ef9a5f5..f80ba607ce7 100644 --- a/vespalib/src/vespa/vespalib/datastore/allocator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/allocator.hpp @@ -68,5 +68,30 @@ Allocator<EntryT, RefT>::allocArray() return HandleType(ref, buf); } +template <typename EntryT, typename RefT> +template <typename BufferType> +typename Allocator<EntryT, RefT>::HandleType +Allocator<EntryT, RefT>::alloc_dynamic_array(ConstArrayRef array) +{ + _store.ensure_buffer_capacity(_typeId, 1); + uint32_t buffer_id = _store.primary_buffer_id(_typeId); + BufferState &state = _store.getBufferState(buffer_id); + assert(state.isActive()); + auto max_array_size = state.getArraySize(); + assert(max_array_size >= array.size()); + RefT ref(state.size(), buffer_id); + auto entry_size = _store.get_entry_size(_typeId); + EntryT* buf = BufferType::get_entry(_store.getBuffer(ref.bufferId()), ref.offset(), entry_size); + for (size_t i = 0; i < array.size(); ++i) { + new (static_cast<void *>(buf + i)) EntryT(array[i]); + } + for (size_t i = array.size(); i < max_array_size; ++i) { + new (static_cast<void *>(buf + i)) EntryT(); + } + BufferType::set_dynamic_array_size(buf, entry_size, array.size()); + state.stats().pushed_back(1); + return HandleType(ref, buf); +} + } diff --git a/vespalib/src/vespa/vespalib/datastore/array_store.h b/vespalib/src/vespa/vespalib/datastore/array_store.h index 66e6c19fcb0..31b06b1f869 100644 --- a/vespalib/src/vespa/vespalib/datastore/array_store.h +++ b/vespalib/src/vespa/vespalib/datastore/array_store.h @@ -15,19 +15,32 @@ #include "large_array_buffer_type.h" #include "small_array_buffer_type.h" #include <vespa/vespalib/util/array.h> +#include <type_traits> namespace vespalib::datastore { /** - * Datastore for storing arrays of type ElemT that is accessed via a 32-bit EntryRef. + * Datastore for storing arrays of type ElemT that is accessed via a 32-bit + * EntryRef. * - * The default EntryRef type uses 19 bits for offset (524288 values) and 13 bits for buffer id (8192 buffers). + * The default EntryRef type uses 19 bits for offset (524288 values) and 13 + * bits for buffer id (8192 buffers). * - * Buffer type ids [1,maxSmallArrayTypeId] are used to allocate small arrays in datastore buffers. - * The default type mapper uses a 1-to-1 mapping between type id and array size. - * Buffer type id 0 is used to heap allocate large arrays as vespalib::Array instances. + * Buffer type ids [1,maxSmallArrayTypeId] are used to allocate small + * arrays in datastore buffers. * - * The max value of maxSmallArrayTypeId is (2^bufferBits - 1). + * The simple type mapper (ArrayStoreSimpleTypeMapper) uses a 1-to-1 + * mapping between type id and array size. + * + * If the type mapper has defined a DynamicBufferType type + * (e.g. ArrayStoreDynamicTypeMapper) then the last part of the buffer type + * ids range might be for dynamic buffers where maximum array size can + * grow exponentially as buffer type id increases. + * + * Buffer type id 0 is used to heap allocate large arrays as + * vespalib::Array instances. + * + * The max value of maxSmallArrayTypeId is (2^(bufferBits - 3) - 1). */ template <typename ElemT, typename RefT = EntryRefT<19>, typename TypeMapperT = ArrayStoreSimpleTypeMapper<ElemT> > class ArrayStore : public ICompactable @@ -43,6 +56,22 @@ public: using RefType = RefT; using SmallBufferType = typename TypeMapperT::SmallBufferType; using TypeMapper = TypeMapperT; + struct no_vector { }; + + template <class, class = void> + struct check_dynamic_buffer_type_member { + static constexpr bool value = false; + using vector_type = no_vector; + }; + + template <class T> + struct check_dynamic_buffer_type_member<T, std::void_t<typename T::DynamicBufferType>> { + static constexpr bool value = true; + using vector_type = std::vector<typename T::DynamicBufferType>; + }; + + static constexpr bool has_dynamic_buffer_type = check_dynamic_buffer_type_member<TypeMapper>::value; + using DynamicBufferTypeVector = typename check_dynamic_buffer_type_member<TypeMapper>::vector_type; private: uint32_t _largeArrayTypeId; uint32_t _maxSmallArrayTypeId; @@ -50,19 +79,31 @@ private: DataStoreType _store; TypeMapper _mapper; std::vector<SmallBufferType> _smallArrayTypes; + [[no_unique_address]] DynamicBufferTypeVector _dynamicArrayTypes; LargeBufferType _largeArrayType; CompactionSpec _compaction_spec; using generation_t = vespalib::GenerationHandler::generation_t; + BufferTypeBase* initArrayType(const ArrayStoreConfig &cfg, std::shared_ptr<alloc::MemoryAllocator> memory_allocator, uint32_t type_id); void initArrayTypes(const ArrayStoreConfig &cfg, std::shared_ptr<alloc::MemoryAllocator> memory_allocator); - EntryRef addSmallArray(ConstArrayRef array); - EntryRef allocate_small_array(size_t array_size); + EntryRef addSmallArray(ConstArrayRef array, uint32_t type_id); + EntryRef allocate_small_array(uint32_t type_id); + template <typename BufferType> + EntryRef add_dynamic_array(ConstArrayRef array, uint32_t type_id); + template <typename BufferType> + EntryRef allocate_dynamic_array(size_t array_size, uint32_t type_id); EntryRef addLargeArray(ConstArrayRef array); EntryRef allocate_large_array(size_t array_size); ConstArrayRef getSmallArray(RefT ref, size_t arraySize) const { const ElemT *buf = _store.template getEntryArray<ElemT>(ref, arraySize); return ConstArrayRef(buf, arraySize); } + template <typename BufferType> + ConstArrayRef get_dynamic_array(const void* buffer, size_t offset, uint32_t entry_size) const { + auto entry = BufferType::get_entry(buffer, offset, entry_size); + auto size = BufferType::get_dynamic_array_size(entry, entry_size); + return ConstArrayRef(entry, size); + } ConstArrayRef getLargeArray(RefT ref) const { const LargeArray *buf = _store.template getEntry<LargeArray>(ref); return ConstArrayRef(&(*buf)[0], buf->size()); @@ -80,6 +121,11 @@ public: RefT internalRef(ref); const BufferAndMeta & bufferAndMeta = _store.getBufferMeta(internalRef.bufferId()); if (bufferAndMeta.getTypeId() != _largeArrayTypeId) [[likely]] { + if constexpr (has_dynamic_buffer_type) { + if (_mapper.is_dynamic_buffer(bufferAndMeta.getTypeId())) [[unlikely]] { + return get_dynamic_array<typename TypeMapper::DynamicBufferType>(bufferAndMeta.get_buffer_acquire(), internalRef.offset(), bufferAndMeta.get_entry_size()); + } + } return getSmallArray(internalRef, bufferAndMeta.getArraySize()); } else { return getLargeArray(internalRef); diff --git a/vespalib/src/vespa/vespalib/datastore/array_store.hpp b/vespalib/src/vespa/vespalib/datastore/array_store.hpp index c0caab7b7db..8957e1f60aa 100644 --- a/vespalib/src/vespa/vespalib/datastore/array_store.hpp +++ b/vespalib/src/vespa/vespalib/datastore/array_store.hpp @@ -16,17 +16,35 @@ namespace vespalib::datastore { template <typename ElemT, typename RefT, typename TypeMapperT> +BufferTypeBase* +ArrayStore<ElemT, RefT, TypeMapperT>::initArrayType(const ArrayStoreConfig &cfg, std::shared_ptr<alloc::MemoryAllocator> memory_allocator, uint32_t type_id) +{ + const AllocSpec &spec = cfg.spec_for_type_id(type_id); + size_t array_size = _mapper.get_array_size(type_id); + if constexpr (has_dynamic_buffer_type) { + if (_mapper.is_dynamic_buffer(type_id)) { + return &_dynamicArrayTypes.emplace_back(array_size, spec, std::move(memory_allocator), _mapper); + } + } + return &_smallArrayTypes.emplace_back(array_size, spec, std::move(memory_allocator), _mapper); +} + +template <typename ElemT, typename RefT, typename TypeMapperT> void ArrayStore<ElemT, RefT, TypeMapperT>::initArrayTypes(const ArrayStoreConfig &cfg, std::shared_ptr<alloc::MemoryAllocator> memory_allocator) { _largeArrayTypeId = _store.addType(&_largeArrayType); assert(_largeArrayTypeId == 0); _smallArrayTypes.reserve(_maxSmallArrayTypeId); + if constexpr (has_dynamic_buffer_type) { + auto dynamic_buffer_types = _mapper.count_dynamic_buffer_types(_maxSmallArrayTypeId); + _smallArrayTypes.reserve(_maxSmallArrayTypeId - dynamic_buffer_types); + _dynamicArrayTypes.reserve(dynamic_buffer_types); + } else { + _smallArrayTypes.reserve(_maxSmallArrayTypeId); + } for (uint32_t type_id = 1; type_id <= _maxSmallArrayTypeId; ++type_id) { - const AllocSpec &spec = cfg.spec_for_type_id(type_id); - size_t arraySize = _mapper.get_array_size(type_id); - _smallArrayTypes.emplace_back(arraySize, spec, memory_allocator, _mapper); - uint32_t act_type_id = _store.addType(&_smallArrayTypes.back()); + uint32_t act_type_id = _store.addType(initArrayType(cfg, memory_allocator, type_id)); assert(type_id == act_type_id); } } @@ -80,7 +98,13 @@ ArrayStore<ElemT, RefT, TypeMapperT>::add(ConstArrayRef array) return EntryRef(); } if (array.size() <= _maxSmallArraySize) { - return addSmallArray(array); + uint32_t type_id = _mapper.get_type_id(array.size()); + if constexpr (has_dynamic_buffer_type) { + if (_mapper.is_dynamic_buffer(type_id)) [[unlikely]] { + return add_dynamic_array<typename TypeMapper::DynamicBufferType>(array, type_id); + } + } + return addSmallArray(array, type_id); } else { return addLargeArray(array); } @@ -94,7 +118,13 @@ ArrayStore<ElemT, RefT, TypeMapperT>::allocate(size_t array_size) return EntryRef(); } if (array_size <= _maxSmallArraySize) { - return allocate_small_array(array_size); + uint32_t type_id = _mapper.get_type_id(array_size); + if constexpr (has_dynamic_buffer_type) { + if (_mapper.is_dynamic_buffer(type_id)) [[unlikely]] { + return allocate_dynamic_array<typename TypeMapper::DynamicBufferType>(array_size, type_id); + } + } + return allocate_small_array(type_id); } else { return allocate_large_array(array_size); } @@ -102,22 +132,37 @@ ArrayStore<ElemT, RefT, TypeMapperT>::allocate(size_t array_size) template <typename ElemT, typename RefT, typename TypeMapperT> EntryRef -ArrayStore<ElemT, RefT, TypeMapperT>::addSmallArray(ConstArrayRef array) +ArrayStore<ElemT, RefT, TypeMapperT>::addSmallArray(ConstArrayRef array, uint32_t type_id) { - uint32_t typeId = _mapper.get_type_id(array.size()); using NoOpReclaimer = DefaultReclaimer<ElemT>; - return _store.template freeListAllocator<ElemT, NoOpReclaimer>(typeId).allocArray(array).ref; + return _store.template freeListAllocator<ElemT, NoOpReclaimer>(type_id).allocArray(array).ref; } template <typename ElemT, typename RefT, typename TypeMapperT> EntryRef -ArrayStore<ElemT, RefT, TypeMapperT>::allocate_small_array(size_t array_size) +ArrayStore<ElemT, RefT, TypeMapperT>::allocate_small_array(uint32_t type_id) { - uint32_t type_id = _mapper.get_type_id(array_size); return _store.template freeListRawAllocator<ElemT>(type_id).alloc(1).ref; } template <typename ElemT, typename RefT, typename TypeMapperT> +template <typename BufferType> +EntryRef +ArrayStore<ElemT, RefT, TypeMapperT>::add_dynamic_array(ConstArrayRef array, uint32_t type_id) +{ + using NoOpReclaimer = DefaultReclaimer<ElemT>; + return _store.template freeListAllocator<ElemT, NoOpReclaimer>(type_id).template alloc_dynamic_array<BufferType>(array).ref; +} + +template <typename ElemT, typename RefT, typename TypeMapperT> +template <typename BufferType> +EntryRef +ArrayStore<ElemT, RefT, TypeMapperT>::allocate_dynamic_array(size_t array_size, uint32_t type_id) +{ + return _store.template freeListRawAllocator<ElemT>(type_id).template alloc_dynamic_array<BufferType>(array_size).ref; +} + +template <typename ElemT, typename RefT, typename TypeMapperT> EntryRef ArrayStore<ElemT, RefT, TypeMapperT>::addLargeArray(ConstArrayRef array) { diff --git a/vespalib/src/vespa/vespalib/datastore/array_store_dynamic_type_mapper.h b/vespalib/src/vespa/vespalib/datastore/array_store_dynamic_type_mapper.h index e02f57eaea3..73c998e82a5 100644 --- a/vespalib/src/vespa/vespalib/datastore/array_store_dynamic_type_mapper.h +++ b/vespalib/src/vespa/vespalib/datastore/array_store_dynamic_type_mapper.h @@ -35,10 +35,13 @@ public: using DynamicBufferType = vespalib::datastore::DynamicArrayBufferType<ElemT>; using LargeBufferType = vespalib::datastore::LargeArrayBufferType<ElemT>; + ArrayStoreDynamicTypeMapper(); ArrayStoreDynamicTypeMapper(uint32_t max_buffer_type_id, double grow_factor); ~ArrayStoreDynamicTypeMapper(); void setup_array_sizes(uint32_t max_buffer_type_id, double grow_factor); size_t get_entry_size(uint32_t type_id) const; + bool is_dynamic_buffer(uint32_t type_id) const noexcept { return type_id > _max_static_array_buffer_type_id; } + uint32_t count_dynamic_buffer_types(uint32_t max_type_id) const noexcept { return (max_type_id > _max_static_array_buffer_type_id) ? (max_type_id - _max_static_array_buffer_type_id) : 0u; } }; extern template class ArrayStoreDynamicTypeMapper<char>; diff --git a/vespalib/src/vespa/vespalib/datastore/array_store_dynamic_type_mapper.hpp b/vespalib/src/vespa/vespalib/datastore/array_store_dynamic_type_mapper.hpp index f529ecccb46..e74cd92e6aa 100644 --- a/vespalib/src/vespa/vespalib/datastore/array_store_dynamic_type_mapper.hpp +++ b/vespalib/src/vespa/vespalib/datastore/array_store_dynamic_type_mapper.hpp @@ -11,6 +11,13 @@ namespace vespalib::datastore { template <typename ElemT> +ArrayStoreDynamicTypeMapper<ElemT>::ArrayStoreDynamicTypeMapper() + : ArrayStoreTypeMapper(), + _max_static_array_buffer_type_id(0) +{ +} + +template <typename ElemT> ArrayStoreDynamicTypeMapper<ElemT>::ArrayStoreDynamicTypeMapper(uint32_t max_buffer_type_id, double grow_factor) : ArrayStoreTypeMapper(), _max_static_array_buffer_type_id(0) diff --git a/vespalib/src/vespa/vespalib/datastore/bufferstate.h b/vespalib/src/vespa/vespalib/datastore/bufferstate.h index f714f8e24d5..289be32e19b 100644 --- a/vespalib/src/vespa/vespalib/datastore/bufferstate.h +++ b/vespalib/src/vespa/vespalib/datastore/bufferstate.h @@ -140,6 +140,7 @@ public: uint32_t getArraySize() const { return _arraySize; } BufferState * get_state_relaxed() { return _state.load(std::memory_order_relaxed); } const BufferState * get_state_acquire() const { return _state.load(std::memory_order_acquire); } + uint32_t get_entry_size() const { return get_state_acquire()->getTypeHandler()->entry_size(); } void setTypeId(uint32_t typeId) { _typeId = typeId; } void setArraySize(uint32_t arraySize) { _arraySize = arraySize; } void set_state(BufferState * state) { _state.store(state, std::memory_order_release); } diff --git a/vespalib/src/vespa/vespalib/datastore/datastorebase.h b/vespalib/src/vespa/vespalib/datastore/datastorebase.h index e5a38e3fd41..dbcdbeb12b9 100644 --- a/vespalib/src/vespa/vespalib/datastore/datastorebase.h +++ b/vespalib/src/vespa/vespalib/datastore/datastorebase.h @@ -184,12 +184,14 @@ public: */ virtual void reclaim_entry_refs(generation_t oldest_used_gen) = 0; + uint32_t get_entry_size(uint32_t type_id) { return _typeHandlers[type_id]->entry_size(); } + + void* getBuffer(uint32_t bufferId) { return _buffers[bufferId].get_buffer_relaxed(); } + protected: DataStoreBase(uint32_t numBuffers, uint32_t offset_bits, size_t max_entries); virtual ~DataStoreBase(); - void* getBuffer(uint32_t bufferId) { return _buffers[bufferId].get_buffer_relaxed(); } - struct EntryRefHoldElem { EntryRef ref; size_t num_entries; diff --git a/vespalib/src/vespa/vespalib/datastore/free_list_allocator.h b/vespalib/src/vespa/vespalib/datastore/free_list_allocator.h index dc2d1ea3c34..f488a4f0e0f 100644 --- a/vespalib/src/vespa/vespalib/datastore/free_list_allocator.h +++ b/vespalib/src/vespa/vespalib/datastore/free_list_allocator.h @@ -30,6 +30,8 @@ public: HandleType allocArray(ConstArrayRef array); HandleType allocArray(); + template <typename BufferType> + HandleType alloc_dynamic_array(ConstArrayRef array); }; } diff --git a/vespalib/src/vespa/vespalib/datastore/free_list_allocator.hpp b/vespalib/src/vespa/vespalib/datastore/free_list_allocator.hpp index 4e69db08a3c..6f3e0bc9911 100644 --- a/vespalib/src/vespa/vespalib/datastore/free_list_allocator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/free_list_allocator.hpp @@ -96,5 +96,25 @@ FreeListAllocator<EntryT, RefT, ReclaimerT>::allocArray() return HandleType(ref, buf); } +template <typename EntryT, typename RefT, typename ReclaimerT> +template <typename BufferType> +typename Allocator<EntryT, RefT>::HandleType +FreeListAllocator<EntryT, RefT, ReclaimerT>::alloc_dynamic_array(ConstArrayRef array) +{ + auto& free_list = _store.getFreeList(_typeId); + if (free_list.empty()) { + return ParentType::template alloc_dynamic_array<BufferType>(array); + } + RefT ref = free_list.pop_entry(); + assert(_store.getBufferState(ref.bufferId()).getArraySize() >= array.size()); + auto entry_size = _store.get_entry_size(_typeId); + EntryT* buf = BufferType::get_entry(_store.getBuffer(ref.bufferId()), ref.offset(), entry_size); + for (size_t i = 0; i < array.size(); ++i) { + *(buf + i) = array[i]; + } + BufferType::set_dynamic_array_size(buf, entry_size, array.size()); + return HandleType(ref, buf); +} + } diff --git a/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.h b/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.h index 29684267546..ead192156bd 100644 --- a/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.h +++ b/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.h @@ -28,6 +28,8 @@ public: FreeListRawAllocator(DataStoreBase &store, uint32_t typeId); HandleType alloc(size_t num_entries); + template <typename BufferType> + HandleType alloc_dynamic_array(size_t array_size); }; } diff --git a/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.hpp b/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.hpp index 7680cd8a9a5..c6d93e92828 100644 --- a/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.hpp @@ -29,5 +29,22 @@ FreeListRawAllocator<EntryT, RefT>::alloc(size_t num_entries) return HandleType(ref, entry); } +template <typename EntryT, typename RefT> +template <typename BufferType> +typename FreeListRawAllocator<EntryT, RefT>::HandleType +FreeListRawAllocator<EntryT, RefT>::alloc_dynamic_array(size_t array_size) +{ + auto& free_list = _store.getFreeList(_typeId); + if (free_list.empty()) { + return ParentType::template alloc_dynamic_array<BufferType>(array_size); + } + RefT ref = free_list.pop_entry(); + auto entry_size = _store.get_entry_size(_typeId); + assert(_store.getBufferState(ref.bufferId()).getArraySize() >= array_size); + EntryT* entry = BufferType::get_entry(_store.getBuffer(ref.bufferId()), ref.offset(), entry_size); + BufferType::set_dynamic_array_size(entry, entry_size, array_size); + return HandleType(ref, entry); +} + } diff --git a/vespalib/src/vespa/vespalib/datastore/raw_allocator.h b/vespalib/src/vespa/vespalib/datastore/raw_allocator.h index e7a59fadcf8..6af608164bc 100644 --- a/vespalib/src/vespa/vespalib/datastore/raw_allocator.h +++ b/vespalib/src/vespa/vespalib/datastore/raw_allocator.h @@ -29,6 +29,8 @@ public: return alloc(num_entries, 0); } HandleType alloc(size_t num_entries, size_t extra_entries); + template <typename BufferType> + HandleType alloc_dynamic_array(size_t array_size); }; } diff --git a/vespalib/src/vespa/vespalib/datastore/raw_allocator.hpp b/vespalib/src/vespa/vespalib/datastore/raw_allocator.hpp index 9de361a8b19..5dde8aaa622 100644 --- a/vespalib/src/vespa/vespalib/datastore/raw_allocator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/raw_allocator.hpp @@ -28,5 +28,23 @@ RawAllocator<EntryT, RefT>::alloc(size_t num_entries, size_t extra_entries) return HandleType(ref, buffer); } +template <typename EntryT, typename RefT> +template <typename BufferType> +typename RawAllocator<EntryT, RefT>::HandleType +RawAllocator<EntryT, RefT>::alloc_dynamic_array(size_t array_size) +{ + _store.ensure_buffer_capacity(_typeId, 1); + uint32_t buffer_id = _store.primary_buffer_id(_typeId); + BufferState &state = _store.getBufferState(buffer_id); + assert(state.isActive()); + assert(state.getArraySize() >= array_size); + RefT ref(state.size(), buffer_id); + auto entry_size = _store.get_entry_size(_typeId); + EntryT* buffer = BufferType::get_entry(_store.getBuffer(ref.bufferId()), ref.offset(), entry_size); + BufferType::set_dynamic_array_size(buffer, entry_size, array_size); + state.stats().pushed_back(1); + return HandleType(ref, buffer); +} + } diff --git a/vespalib/src/vespa/vespalib/stllike/lrucache_map.hpp b/vespalib/src/vespa/vespalib/stllike/lrucache_map.hpp index ca1b075d68b..f063f277e3e 100644 --- a/vespalib/src/vespa/vespalib/stllike/lrucache_map.hpp +++ b/vespalib/src/vespa/vespalib/stllike/lrucache_map.hpp @@ -204,9 +204,7 @@ lrucache_map<P>::removeOld() { (_tail != _head) && removeOldest(*last); last = & HashTable::getByInternalIndex(_tail)) { - _tail = last->second._prev; - HashTable::getByInternalIndex(_tail).second._next = LinkedValueBase::npos; - HashTable::erase(*this, HashTable::hash(last->first), HashTable::find(last->first)); + erase(last->first); } } } diff --git a/vespamalloc/src/vespamalloc/malloc/overload.h b/vespamalloc/src/vespamalloc/malloc/overload.h index 725ca761ec1..6583f742751 100644 --- a/vespamalloc/src/vespamalloc/malloc/overload.h +++ b/vespamalloc/src/vespamalloc/malloc/overload.h @@ -246,7 +246,7 @@ void* __libc_memalign(size_t align, size_t s) __THROW __attribute__((leaf void* __libc_memalign(size_t align, size_t s) __THROW __attribute__((leaf, malloc, alloc_size(2))) ALIAS("memalign"); #endif -int __posix_memalign(void** r, size_t a, size_t s) __THROW __nonnull((1)) ALIAS("posix_memalign"); +int __posix_memalign(void** r, size_t a, size_t s) __THROW __nonnull((1)) ALIAS("posix_memalign") __attribute((leaf)); #if __GLIBC_PREREQ(2, 33) struct mallinfo2 __libc_mallinfo2() __THROW ALIAS("mallinfo2"); diff --git a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java index 15431550d82..0702d01b9d0 100644 --- a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java +++ b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java @@ -115,7 +115,7 @@ public class Reconfigurer extends AbstractComponent { Duration delay = backoff.delay(attempt); now = Instant.now(); if (now.isBefore(end)) { - log.log(Level.WARNING, "Reconfiguration attempt " + attempt + " failed. Retrying in " + delay + + log.log(Level.INFO, "Reconfiguration attempt " + attempt + " failed. Retrying in " + delay + ", time left " + Duration.between(now, end) + ": " + Exceptions.toMessageString(e)); sleeper.sleep(delay); } |