diff options
109 files changed, 1479 insertions, 674 deletions
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java index 170547430cb..f778c2c2d0e 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java @@ -35,15 +35,7 @@ import java.util.stream.Stream; public class DeploymentSpec { /** The empty deployment spec, specifying no zones or rotation, and defaults for all settings */ - public static final DeploymentSpec empty = new DeploymentSpec(List.of(new DeploymentInstanceSpec(InstanceName.from("default"), - Collections.emptyList(), - UpgradePolicy.defaultPolicy, - Collections.emptyList(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - Notifications.none(), - List.of())), + public static final DeploymentSpec empty = new DeploymentSpec(List.of(), Optional.empty(), Optional.empty(), Optional.empty(), diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java index c14e6ce5966..faa7c8cf932 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java @@ -93,6 +93,8 @@ public class DeploymentSpecXmlReader { /** Reads a deployment spec from XML */ public DeploymentSpec read(String xmlForm) { Element root = XML.getDocument(xmlForm).getDocumentElement(); + if (isEmptySpec(root)) + return DeploymentSpec.empty; List<Step> steps = new ArrayList<>(); if ( ! containsTag(instanceTag, root)) { // deployment spec skipping explicit instance -> "default" instance @@ -417,6 +419,13 @@ public class DeploymentSpecXmlReader { "to control whether the region should receive production traffic"); } + private static boolean isEmptySpec(Element root) { + if ( ! XML.getChildren(root).isEmpty()) return false; + return root.getAttributes().getLength() == 0 + || root.getAttributes().getLength() == 1 && root.hasAttribute("version"); + } + + private static class MutableOptional<T> { private Optional<T> value = Optional.empty(); diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java index ac605add892..d0740b3e9b9 100644 --- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java +++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java @@ -422,14 +422,6 @@ public class DeploymentSpecTest { } @Test - public void testEmpty() { - assertFalse(DeploymentSpec.empty.requireInstance("default").globalServiceId().isPresent()); - assertTrue(DeploymentSpec.empty.requireInstance("default").steps().isEmpty()); - assertEquals(1, DeploymentSpec.empty.steps().size()); - assertEquals("<deployment version='1.0'/>", DeploymentSpec.empty.xmlForm()); - } - - @Test public void productionSpecWithParallelDeployments() { StringReader r = new StringReader( "<deployment>" + diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java index 50999759b77..b5e5946262a 100644 --- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java +++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java @@ -21,6 +21,7 @@ import static com.yahoo.config.application.api.Notifications.When.failing; import static com.yahoo.config.application.api.Notifications.When.failingCommit; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -317,9 +318,9 @@ public class DeploymentSpecWithoutInstanceTest { @Test public void testEmpty() { - assertFalse(DeploymentSpec.empty.requireInstance("default").globalServiceId().isPresent()); - assertEquals(1, DeploymentSpec.empty.steps().size()); - assertTrue(DeploymentSpec.empty.requireInstance("default").steps().isEmpty()); + assertEquals(0, DeploymentSpec.empty.steps().size()); + assertTrue(DeploymentSpec.empty.athenzDomain().isEmpty()); + assertTrue(DeploymentSpec.empty.athenzService().isEmpty()); assertEquals("<deployment version='1.0'/>", DeploymentSpec.empty.xmlForm()); } @@ -575,9 +576,15 @@ public class DeploymentSpecWithoutInstanceTest { } @Test - public void noNotifications() { - assertEquals(Notifications.none(), - DeploymentSpec.fromXml("<deployment />").requireInstance("default").notifications()); + public void emptySpecs() { + assertEquals(DeploymentSpec.empty, DeploymentSpec.fromXml("<deployment>\n" + + "</deployment>")); + assertEquals(DeploymentSpec.empty, DeploymentSpec.fromXml("<deployment />")); + assertEquals(DeploymentSpec.empty, DeploymentSpec.fromXml("<deployment version=\"1.0\" />")); + + assertNotEquals(DeploymentSpec.empty, DeploymentSpec.fromXml("<deployment version=\"1.0\" athenz-domain=\"domain\" athenz-service=\"service\"/>")); + assertNotEquals(DeploymentSpec.empty, DeploymentSpec.fromXml("<deployment athenz-domain=\"domain\" athenz-service=\"service\">\n" + + "</deployment>")); } @Test @@ -618,11 +625,6 @@ public class DeploymentSpecWithoutInstanceTest { } @Test - public void noEndpoints() { - assertEquals(Collections.emptyList(), DeploymentSpec.fromXml("<deployment />").requireInstance("default").endpoints()); - } - - @Test public void emptyEndpoints() { var spec = DeploymentSpec.fromXml("<deployment><endpoints/></deployment>"); assertEquals(Collections.emptyList(), spec.requireInstance("default").endpoints()); diff --git a/config-model/src/test/derived/tensor/rank-profiles.cfg b/config-model/src/test/derived/tensor/rank-profiles.cfg index 9e9dfae2bc7..29dc39b01ce 100644 --- a/config-model/src/test/derived/tensor/rank-profiles.cfg +++ b/config-model/src/test/derived/tensor/rank-profiles.cfg @@ -84,7 +84,7 @@ rankprofile[].name "profile5" rankprofile[].fef.property[].name "vespa.rank.firstphase" rankprofile[].fef.property[].value "rankingExpression(firstphase)" rankprofile[].fef.property[].name "rankingExpression(firstphase).rankingScript" -rankprofile[].fef.property[].value "reduce(tensor<float>(d0[1],x[2]):{{d0:0,x:0}:attribute(f6),{d0:0,x:1}:reduce(attribute(f5), sum)}, sum)" +rankprofile[].fef.property[].value "reduce(tensor<float>(d0[1],x[2]):{{d0:0,x:0}:(attribute(f6)),{d0:0,x:1}:(reduce(attribute(f5), sum))}, sum)" rankprofile[].fef.property[].name "vespa.type.attribute.f2" rankprofile[].fef.property[].value "tensor<float>(x[2],y[1])" rankprofile[].fef.property[].name "vespa.type.attribute.f3" @@ -101,7 +101,7 @@ rankprofile[].fef.property[].value "tensor(i[10],x[10],y[20])" rankprofile[].fef.property[].name "vespa.rank.firstphase" rankprofile[].fef.property[].value "rankingExpression(firstphase)" rankprofile[].fef.property[].name "rankingExpression(firstphase).rankingScript" -rankprofile[].fef.property[].value "reduce(tensor<float>(d0[1],x[2]):{{d0:0,x:0}:attribute(f6),{d0:0,x:1}:reduce(rankingExpression(joinedtensors), sum)}, sum)" +rankprofile[].fef.property[].value "reduce(tensor<float>(d0[1],x[2]):{{d0:0,x:0}:(attribute(f6)),{d0:0,x:1}:(reduce(rankingExpression(joinedtensors), sum))}, sum)" rankprofile[].fef.property[].name "vespa.type.attribute.f2" rankprofile[].fef.property[].value "tensor<float>(x[2],y[1])" rankprofile[].fef.property[].name "vespa.type.attribute.f3" @@ -112,13 +112,17 @@ rankprofile[].fef.property[].name "vespa.type.attribute.f5" rankprofile[].fef.property[].value "tensor<float>(x[10])" rankprofile[].name "profile7" rankprofile[].fef.property[].name "rankingExpression(reshaped).rankingScript" -rankprofile[].fef.property[].value "tensor<float>(d0[1],x[2])(attribute(f2){x:1 - x, y:d0})" +rankprofile[].fef.property[].value "tensor<float>(d0[1],x[2])((attribute(f2){x:(1 - x), y:d0}))" rankprofile[].fef.property[].name "rankingExpression(reshaped).type" rankprofile[].fef.property[].value "tensor<float>(d0[1],x[2])" +rankprofile[].fef.property[].name "rankingExpression(literal).rankingScript" +rankprofile[].fef.property[].value "tensor<float>(key{}):{{key:foo}:0.5,{key:bar}:1.2,{key:\"han's\"}:3.1}" +rankprofile[].fef.property[].name "rankingExpression(literal).type" +rankprofile[].fef.property[].value "tensor<float>(key{})" rankprofile[].fef.property[].name "vespa.rank.firstphase" rankprofile[].fef.property[].value "rankingExpression(firstphase)" rankprofile[].fef.property[].name "rankingExpression(firstphase).rankingScript" -rankprofile[].fef.property[].value "reduce(rankingExpression(reshaped), sum)" +rankprofile[].fef.property[].value "reduce(rankingExpression(reshaped) * rankingExpression(literal), sum)" rankprofile[].fef.property[].name "vespa.type.attribute.f2" rankprofile[].fef.property[].value "tensor<float>(x[2],y[1])" rankprofile[].fef.property[].name "vespa.type.attribute.f3" @@ -133,7 +137,7 @@ rankprofile[].fef.property[].value "3" rankprofile[].fef.property[].name "vespa.rank.firstphase" rankprofile[].fef.property[].value "rankingExpression(firstphase)" rankprofile[].fef.property[].name "rankingExpression(firstphase).rankingScript" -rankprofile[].fef.property[].value "reduce(tensor(d0[1])(attribute{x:rankingExpression(functionNotLabel)}), sum)" +rankprofile[].fef.property[].value "reduce(tensor(d0[1])((attribute{x:(rankingExpression(functionNotLabel))})), sum)" rankprofile[].fef.property[].name "vespa.type.attribute.f2" rankprofile[].fef.property[].value "tensor<float>(x[2],y[1])" rankprofile[].fef.property[].name "vespa.type.attribute.f3" @@ -148,7 +152,7 @@ rankprofile[].fef.property[].value "3" rankprofile[].fef.property[].name "vespa.rank.firstphase" rankprofile[].fef.property[].value "rankingExpression(firstphase)" rankprofile[].fef.property[].name "rankingExpression(firstphase).rankingScript" -rankprofile[].fef.property[].value "reduce(tensor(shadow[1])(attribute{x:shadow + rankingExpression(shadow)}), sum)" +rankprofile[].fef.property[].value "reduce(tensor(shadow[1])((attribute{x:(shadow + rankingExpression(shadow))})), sum)" rankprofile[].fef.property[].name "vespa.type.attribute.f2" rankprofile[].fef.property[].value "tensor<float>(x[2],y[1])" rankprofile[].fef.property[].name "vespa.type.attribute.f3" diff --git a/config-model/src/test/derived/tensor/tensor.sd b/config-model/src/test/derived/tensor/tensor.sd index 6e0e7e3e148..aa33684a979 100644 --- a/config-model/src/test/derived/tensor/tensor.sd +++ b/config-model/src/test/derived/tensor/tensor.sd @@ -81,13 +81,17 @@ search tensor { rank-profile profile7 { first-phase { - expression: sum(reshaped()) + expression: sum(reshaped() * literal()) } function reshaped() { expression: tensor<float>(d0[1],x[2])(attribute(f2){x:1-x, y:d0}) } + function literal() { + expression: tensor<float>(key{}):{ 'foo':0.5, bar:1.2, "han's":3.1} + } + } rank-profile profile8 { diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TlsSecretsKeys.java b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TlsSecretsKeys.java index 78d39ef996b..da6fc490da9 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TlsSecretsKeys.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TlsSecretsKeys.java @@ -1,12 +1,13 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.server.tenant; -import com.fasterxml.jackson.core.type.TypeReference; -import com.fasterxml.jackson.databind.ObjectMapper; import com.yahoo.config.model.api.TlsSecrets; import com.yahoo.config.provision.ApplicationId; import com.yahoo.container.jdisc.secretstore.SecretStore; import com.yahoo.path.Path; +import com.yahoo.slime.Cursor; +import com.yahoo.slime.Slime; +import com.yahoo.vespa.config.SlimeUtils; import com.yahoo.vespa.curator.Curator; import com.yahoo.vespa.curator.transaction.CuratorOperations; import com.yahoo.vespa.curator.transaction.CuratorTransaction; @@ -34,8 +35,22 @@ public class TlsSecretsKeys { try { Optional<byte[]> data = curator.getData(tlsSecretsKeyOf(application)); if (data.isEmpty() || data.get().length == 0) return Optional.empty(); - String tlsSecretsKey = new ObjectMapper().readValue(data.get(), new TypeReference<String>() {}); - return readFromSecretStore(Optional.ofNullable(tlsSecretsKey)); + + Slime slime = SlimeUtils.jsonToSlime(data.get()); + final var inspector = slime.get(); + + switch (inspector.type()) { + case STRING: // TODO: Remove once all are stored as JSON + return readFromSecretStore(Optional.ofNullable(inspector.asString())); + case OBJECT: + var tlsSecretsInfo = new TlsSecretsMetadata(); + tlsSecretsInfo.certName = inspector.field("certName").asString(); + tlsSecretsInfo.keyName = inspector.field("keyName").asString(); + tlsSecretsInfo.version = Math.toIntExact(inspector.field("version").asLong()); + return Optional.of(readFromSecretStore(tlsSecretsInfo)); + default: + throw new IllegalArgumentException("Unknown format encountered for TLS secrets metadata!"); + } } catch (Exception e) { throw new RuntimeException("Error reading TLS secret key of " + application, e); } @@ -43,9 +58,27 @@ public class TlsSecretsKeys { public void writeTlsSecretsKeyToZooKeeper(ApplicationId application, String tlsSecretsKey) { if (tlsSecretsKey == null) return; + writeTlsSecretsAsString(application, tlsSecretsKey); + } + + private void writeTlsSecretsAsString(ApplicationId application, String tlsSecretsKey) { try { - byte[] data = new ObjectMapper().writeValueAsBytes(tlsSecretsKey); - curator.set(tlsSecretsKeyOf(application), data); + Slime slime = new Slime(); + slime.setString(tlsSecretsKey); + curator.set(tlsSecretsKeyOf(application), SlimeUtils.toJsonBytes(slime)); + } catch (Exception e) { + throw new RuntimeException("Could not write TLS secret key of " + application, e); + } + } + + void writeTlsSecretsMetadata(ApplicationId application, TlsSecretsMetadata tlsSecretsMetadata) { + try { + Slime slime = new Slime(); + Cursor cursor = slime.setObject(); + cursor.setString(TlsSecretsMetadata.certNameField, tlsSecretsMetadata.certName); + cursor.setString(TlsSecretsMetadata.keyNameField, tlsSecretsMetadata.keyName); + cursor.setLong(TlsSecretsMetadata.versionField, tlsSecretsMetadata.version); + curator.set(tlsSecretsKeyOf(application), SlimeUtils.toJsonBytes(slime)); } catch (Exception e) { throw new RuntimeException("Could not write TLS secret key of " + application, e); } @@ -70,6 +103,17 @@ public class TlsSecretsKeys { } } + private TlsSecrets readFromSecretStore(TlsSecretsMetadata tlsSecretsMetadata) { + try { + String cert = secretStore.getSecret(tlsSecretsMetadata.certName, tlsSecretsMetadata.version); + String key = secretStore.getSecret(tlsSecretsMetadata.keyName, tlsSecretsMetadata.version); + return new TlsSecrets(cert, key); + } catch (RuntimeException e) { + // Assume not ready yet + return TlsSecrets.MISSING; + } + } + /** Returns a transaction which deletes these tls secrets key if they exist */ public CuratorTransaction delete(ApplicationId application) { if (!curator.exists(tlsSecretsKeyOf(application))) return CuratorTransaction.empty(curator); @@ -81,4 +125,12 @@ public class TlsSecretsKeys { return path.append(application.serializedForm()); } + static class TlsSecretsMetadata { + final static String keyNameField = "keyName"; + final static String certNameField = "certName"; + final static String versionField = "version"; + String keyName; + String certName; + int version; + } } diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java index 0802db23ea7..f25b41bd121 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java @@ -353,27 +353,4 @@ public class ApplicationRepositoryTest { return applicationRepository.getMetadataFromSession(tenant, sessionId); } - private static class MockLogRetriever extends LogRetriever { - - @Override - public HttpResponse getLogs(String logServerHostname) { - return new MockHttpResponse(); - } - - private static class MockHttpResponse extends HttpResponse { - - private MockHttpResponse() { - super(200); - } - - @Override - public void render(OutputStream outputStream) throws IOException { - outputStream.write("log line".getBytes(StandardCharsets.UTF_8)); - } - - } - - - } - } diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/MockLogRetriever.java b/configserver/src/test/java/com/yahoo/vespa/config/server/MockLogRetriever.java new file mode 100644 index 00000000000..06c07773e27 --- /dev/null +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/MockLogRetriever.java @@ -0,0 +1,34 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.config.server; + +import com.yahoo.container.jdisc.HttpResponse; +import com.yahoo.vespa.config.server.http.LogRetriever; + +import java.io.IOException; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; + +/** + * @author olaa + */ +public class MockLogRetriever extends LogRetriever { + + @Override + public HttpResponse getLogs(String logServerHostname) { + return new MockHttpResponse(); + } + + private static class MockHttpResponse extends HttpResponse { + + private MockHttpResponse() { + super(200); + } + + @Override + public void render(OutputStream outputStream) throws IOException { + outputStream.write("log line".getBytes(StandardCharsets.UTF_8)); + } + + } + +}
\ No newline at end of file diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java index 0acce9ed2c7..22ce0db89e2 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java @@ -2,8 +2,6 @@ package com.yahoo.vespa.config.server.http.v2; import com.fasterxml.jackson.databind.ObjectMapper; -import com.github.tomakehurst.wiremock.WireMockServer; -import com.github.tomakehurst.wiremock.client.WireMock; import com.yahoo.cloud.config.ConfigserverConfig; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationName; @@ -13,6 +11,7 @@ import com.yahoo.container.jdisc.HttpRequest; import com.yahoo.container.jdisc.HttpResponse; import com.yahoo.jdisc.Response; import com.yahoo.vespa.config.server.ApplicationRepository; +import com.yahoo.vespa.config.server.MockLogRetriever; import com.yahoo.vespa.config.server.TestComponentRegistry; import com.yahoo.vespa.config.server.application.ConfigConvergenceChecker; import com.yahoo.vespa.config.server.application.HttpProxy; @@ -31,16 +30,12 @@ import org.junit.Before; import org.junit.Test; import javax.ws.rs.client.Client; +import java.io.ByteArrayOutputStream; import java.io.File; import java.io.IOException; import java.net.URI; import java.time.Clock; -import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; -import static com.github.tomakehurst.wiremock.client.WireMock.get; -import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; -import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; -import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; import static com.yahoo.config.model.api.container.ContainerServiceType.CLUSTERCONTROLLER_CONTAINER; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -80,6 +75,7 @@ public class ApplicationHandlerTest { applicationRepository = new ApplicationRepository(tenantRepository, provisioner, orchestrator, + new MockLogRetriever(), Clock.systemUTC()); listApplicationsHandler = new ListApplicationsHandler(ListApplicationsHandler.testOnlyContext(), tenantRepository, @@ -202,20 +198,17 @@ public class ApplicationHandlerTest { } @Test - public void testGetLogs() { - String path = "/logs?from=100&to=200"; + public void testGetLogs() throws IOException { applicationRepository.deploy(new File("src/test/apps/app-logserver-with-container"), prepareParams(applicationId)); - WireMockServer wireMock = new WireMockServer(wireMockConfig().port(8080)); - wireMock.start(); - WireMock.configureFor("localhost", wireMock.port()); - stubFor(get(urlEqualTo(path)) - .willReturn(aResponse() - .withStatus(200))); - String url = toUrlPath(applicationId, Zone.defaultZone(), true) + path; + String url = toUrlPath(applicationId, Zone.defaultZone(), true) + "/logs?from=100&to=200"; ApplicationHandler mockHandler = createApplicationHandler(); + HttpResponse response = mockHandler.handle(HttpRequest.createTestRequest(url, com.yahoo.jdisc.http.HttpRequest.Method.GET)); assertEquals(200, response.getStatus()); - wireMock.stop(); + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + response.render(baos); + assertEquals("log line", baos.toString()); } private void assertNotAllowed(com.yahoo.jdisc.http.HttpRequest.Method method) throws IOException { diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TlsSecretsKeysTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TlsSecretsKeysTest.java new file mode 100644 index 00000000000..f20d6b8fcf8 --- /dev/null +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TlsSecretsKeysTest.java @@ -0,0 +1,72 @@ +package com.yahoo.vespa.config.server.tenant; + +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.ApplicationName; +import com.yahoo.config.provision.InstanceName; +import com.yahoo.config.provision.TenantName; +import com.yahoo.path.Path; +import com.yahoo.vespa.config.server.MockSecretStore; +import com.yahoo.vespa.curator.mock.MockCurator; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class TlsSecretsKeysTest { + + private static final Path tenantPath = Path.createRoot(); + private static final Path tlsSecretsKeysPath = Path.createRoot().append("tlsSecretsKeys").append("default:test:default"); + private static final String tlskey = "vespa.tlskeys.tenant1--app1"; + private static final ApplicationId applicationId = ApplicationId.from(TenantName.defaultName(), + ApplicationName.from("test"), InstanceName.defaultName()); + + private MockCurator curator; + private MockSecretStore secretStore = new MockSecretStore(); + private TlsSecretsKeys tlsSecretsKeys; + + @Before + public void setUp() { + curator = new MockCurator(); + tlsSecretsKeys = new TlsSecretsKeys(curator, tenantPath, secretStore); + secretStore.put(tlskey + "-cert", "CERT"); + secretStore.put(tlskey + "-key", "KEY"); + } + + @Test + public void reads_string_format() { + curator.set(tlsSecretsKeysPath, ('"' + tlskey + '"').getBytes()); + + // Read from zk and verify cert and key are available + var tlsSecrets = tlsSecretsKeys.readTlsSecretsKeyFromZookeeper(applicationId); + assertTrue(tlsSecrets.isPresent()); + assertEquals("KEY", tlsSecrets.get().key()); + assertEquals("CERT", tlsSecrets.get().certificate()); + } + + @Test + public void reads_object_format() { + curator.set(tlsSecretsKeysPath, + "{\"keyName\": \"vespa.tlskeys.tenant1--app1-key\", \"certName\":\"vespa.tlskeys.tenant1--app1-cert\", \"version\": 0}" + .getBytes()); + + // Read from zk and verify cert and key are available + var tlsSecrets = tlsSecretsKeys.readTlsSecretsKeyFromZookeeper(applicationId); + assertTrue(tlsSecrets.isPresent()); + assertEquals("KEY", tlsSecrets.get().key()); + assertEquals("CERT", tlsSecrets.get().certificate()); + } + + @Test + public void can_write_object_format() { + var tlsSecretsMetadata = new TlsSecretsKeys.TlsSecretsMetadata(); + tlsSecretsMetadata.certName = "cert-name"; + tlsSecretsMetadata.keyName = "key-name"; + tlsSecretsMetadata.version = 1; + + tlsSecretsKeys.writeTlsSecretsMetadata(applicationId, tlsSecretsMetadata); + + assertEquals("{\"certName\":\"cert-name\",\"keyName\":\"key-name\",\"version\":1}", + new String(curator.getData(tlsSecretsKeysPath).get())); + } +}
\ No newline at end of file diff --git a/container-disc/src/main/sh/vespa-start-container-daemon.sh b/container-disc/src/main/sh/vespa-start-container-daemon.sh index f6a1388e34f..e64d6a9bac1 100755 --- a/container-disc/src/main/sh/vespa-start-container-daemon.sh +++ b/container-disc/src/main/sh/vespa-start-container-daemon.sh @@ -76,13 +76,10 @@ configure_memory() { fi jvm_heapsize=$((available * jvm_heapSizeAsPercentageOfPhysicalMemory / 100)) - if (( jvm_heapsize < 1024 )); then - jvm_heapsize=1024 - fi jvm_minHeapsize=${jvm_heapsize} fi - # Safety measure against bad of min vs max heapsize. + # Safety measure against bad min vs max heapsize. if ((jvm_minHeapsize > jvm_heapsize)); then jvm_minHeapsize=${jvm_heapsize} echo "Misconfigured heap size, jvm_minHeapsize(${jvm_minHeapsize} is larger than jvm_heapsize(${jvm_heapsize}). It has been capped." diff --git a/container-search/src/main/java/com/yahoo/prelude/query/parser/UnicodePropertyDump.java b/container-search/src/main/java/com/yahoo/prelude/query/parser/UnicodePropertyDump.java index d5f72a3387e..0dc9a33e3f1 100644 --- a/container-search/src/main/java/com/yahoo/prelude/query/parser/UnicodePropertyDump.java +++ b/container-search/src/main/java/com/yahoo/prelude/query/parser/UnicodePropertyDump.java @@ -31,7 +31,7 @@ class UnicodePropertyDump { end = Integer.valueOf(arg[1]).intValue(); } if (arg.length > 2) { - debug = new Boolean(arg[2]).booleanValue(); + debug = Boolean.valueOf(arg[2]).booleanValue(); } dumpProperties(start, end, debug, System.out); } diff --git a/container-search/src/main/java/com/yahoo/prelude/searcher/QuotingSearcher.java b/container-search/src/main/java/com/yahoo/prelude/searcher/QuotingSearcher.java index f9cfe843eb3..df944f5b102 100644 --- a/container-search/src/main/java/com/yahoo/prelude/searcher/QuotingSearcher.java +++ b/container-search/src/main/java/com/yahoo/prelude/searcher/QuotingSearcher.java @@ -54,7 +54,7 @@ public class QuotingSearcher extends Searcher { QrQuotetableConfig.Character character = (QrQuotetableConfig.Character)i.next(); if (character.ordinal() > 256) { newIsEmpty = false; - newQuoteMap.put(new Character((char)character.ordinal()), character.quoting()); + newQuoteMap.put((char)character.ordinal(), character.quoting()); newUseMap = true; if (minOrd == 0 || character.ordinal() < minOrd) minOrd = character.ordinal(); @@ -77,7 +77,7 @@ public class QuotingSearcher extends Searcher { public String get(char c) { if (isEmpty) return null; - int ord = (int)c; + int ord = c; if (ord < 256) { return lowerTable[ord]; } @@ -85,7 +85,7 @@ public class QuotingSearcher extends Searcher { if ((!useMap) || ord < lowerUncachedBound || ord > upperUncachedBound) return null; else - return quoteMap.get(new Character(c)); + return quoteMap.get(c); } } public boolean isEmpty() { diff --git a/container-search/src/main/java/com/yahoo/search/query/parser/ParserFactory.java b/container-search/src/main/java/com/yahoo/search/query/parser/ParserFactory.java index a57c0f98b45..4269f5e1c57 100644 --- a/container-search/src/main/java/com/yahoo/search/query/parser/ParserFactory.java +++ b/container-search/src/main/java/com/yahoo/search/query/parser/ParserFactory.java @@ -25,6 +25,7 @@ public final class ParserFactory { * @param environment the environment settings to attach to the Parser * @return the created Parser */ + @SuppressWarnings("deprecation") public static Parser newInstance(Query.Type type, ParserEnvironment environment) { switch (type) { case ALL: diff --git a/container-search/src/main/java/com/yahoo/search/query/textserialize/item/CompositeConverter.java b/container-search/src/main/java/com/yahoo/search/query/textserialize/item/CompositeConverter.java index 332f03c896c..0a935530a62 100644 --- a/container-search/src/main/java/com/yahoo/search/query/textserialize/item/CompositeConverter.java +++ b/container-search/src/main/java/com/yahoo/search/query/textserialize/item/CompositeConverter.java @@ -6,6 +6,7 @@ import com.yahoo.prelude.query.Item; import com.yahoo.search.query.textserialize.serializer.DispatchForm; import com.yahoo.search.query.textserialize.serializer.ItemIdMapper; +import java.lang.reflect.InvocationTargetException; import java.util.ListIterator; /** @@ -42,8 +43,8 @@ public class CompositeConverter<T extends CompositeItem> implements ItemFormConv private T newInstance() { try { - return itemClass.newInstance(); - } catch (InstantiationException | IllegalAccessException e) { + return itemClass.getDeclaredConstructor().newInstance(); + } catch (NoSuchMethodException | InvocationTargetException | InstantiationException | IllegalAccessException e) { throw new RuntimeException(e); } } diff --git a/container-search/src/main/java/com/yahoo/search/result/FieldComparator.java b/container-search/src/main/java/com/yahoo/search/result/FieldComparator.java index 211d04a0eac..ccb9c728561 100644 --- a/container-search/src/main/java/com/yahoo/search/result/FieldComparator.java +++ b/container-search/src/main/java/com/yahoo/search/result/FieldComparator.java @@ -86,9 +86,9 @@ public class FieldComparator extends ChainableComparator { case BOOL: return (sub.asBool() ? Boolean.TRUE : Boolean.FALSE); case LONG: - return new Long(sub.asLong()); + return sub.asLong(); case DOUBLE: - return new Double(sub.asDouble()); + return sub.asDouble(); case STRING: return sub.asString(); } diff --git a/container-search/src/main/java/com/yahoo/search/yql/ProgramParser.java b/container-search/src/main/java/com/yahoo/search/yql/ProgramParser.java index ea4a07508b6..a270d935a97 100644 --- a/container-search/src/main/java/com/yahoo/search/yql/ProgramParser.java +++ b/container-search/src/main/java/com/yahoo/search/yql/ProgramParser.java @@ -1375,8 +1375,9 @@ final class ProgramParser { case yqlplusParser.STRING: return StringUnescaper.unquote(text); case yqlplusParser.TRUE: + return Boolean.valueOf(true); case yqlplusParser.FALSE: - return new Boolean(text); + return Boolean.valueOf(false); case yqlplusParser.LONG_INT: return Long.parseLong(text.substring(0, text.length()-1)); default: diff --git a/container-search/src/test/java/com/yahoo/prelude/cluster/ClusterSearcherTestCase.java b/container-search/src/test/java/com/yahoo/prelude/cluster/ClusterSearcherTestCase.java index 6970093fc9d..5b6a4b68930 100644 --- a/container-search/src/test/java/com/yahoo/prelude/cluster/ClusterSearcherTestCase.java +++ b/container-search/src/test/java/com/yahoo/prelude/cluster/ClusterSearcherTestCase.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.prelude.cluster; +import com.google.common.collect.ImmutableList; import com.yahoo.cloud.config.ClusterInfoConfig; import com.yahoo.component.ComponentId; import com.yahoo.component.provider.ComponentRegistry; @@ -28,6 +29,7 @@ import org.junit.Test; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -69,15 +71,14 @@ public class ClusterSearcherTestCase { clusters.put("cluster1", Arrays.asList("type1", "type2", "type3")); clusters.put("cluster2", Arrays.asList("type4", "type5")); clusters.put("type1", Arrays.asList("type6")); - Map<String, SearchDefinition> searchDefs = new LinkedHashMap<>(); - searchDefs.put("type1", new SearchDefinition("type1")); - searchDefs.put("type2", new SearchDefinition("type2")); - searchDefs.put("type3", new SearchDefinition("type3")); - searchDefs.put("type4", new SearchDefinition("type4")); - searchDefs.put("type5", new SearchDefinition("type5")); - searchDefs.put("type6", new SearchDefinition("type6")); - SearchDefinition union = new SearchDefinition("union"); - return new IndexFacts(new IndexModel(clusters, searchDefs, union)); + Collection<SearchDefinition> searchDefs = ImmutableList.of( + new SearchDefinition("type1"), + new SearchDefinition("type2"), + new SearchDefinition("type3"), + new SearchDefinition("type4"), + new SearchDefinition("type5"), + new SearchDefinition("type6")); + return new IndexFacts(new IndexModel(clusters, searchDefs)); } private Set<String> resolve(ClusterSearcher searcher, String query) { diff --git a/container-search/src/test/java/com/yahoo/prelude/querytransform/test/LiteralBoostSearcherTestCase.java b/container-search/src/test/java/com/yahoo/prelude/querytransform/test/LiteralBoostSearcherTestCase.java index 9850c80754f..12e756a07ee 100644 --- a/container-search/src/test/java/com/yahoo/prelude/querytransform/test/LiteralBoostSearcherTestCase.java +++ b/container-search/src/test/java/com/yahoo/prelude/querytransform/test/LiteralBoostSearcherTestCase.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.prelude.querytransform.test; +import com.google.common.collect.ImmutableList; import com.yahoo.prelude.Index; import com.yahoo.prelude.IndexFacts; import com.yahoo.prelude.IndexModel; @@ -11,10 +12,7 @@ import com.yahoo.search.searchchain.Execution; import com.yahoo.search.test.QueryTestCase; import org.junit.Test; -import java.util.Arrays; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import static org.junit.Assert.assertEquals; @@ -87,15 +85,13 @@ public class LiteralBoostSearcherTestCase { Map<String, List<String>> clusters = new LinkedHashMap<>(); clusters.put("cluster1", Arrays.asList("type1", "type2", "type3")); clusters.put("cluster2", Arrays.asList("type4", "type5")); - Map<String, SearchDefinition> searchDefs = new LinkedHashMap<>(); - searchDefs.put("type1", createSearchDefinitionWithFields("type1", true)); - searchDefs.put("type2", createSearchDefinitionWithFields("type2", false)); - searchDefs.put("type3", new SearchDefinition("type3")); - searchDefs.put("type3", new SearchDefinition("type3")); - searchDefs.put("type4", new SearchDefinition("type4")); - searchDefs.put("type5", new SearchDefinition("type5")); - SearchDefinition union = new SearchDefinition("union"); - return new IndexFacts(new IndexModel(clusters, searchDefs, union)); + Collection<SearchDefinition> searchDefs = ImmutableList.of( + createSearchDefinitionWithFields("type1", true), + createSearchDefinitionWithFields("type2", false), + new SearchDefinition("type3"), + new SearchDefinition("type4"), + new SearchDefinition("type5")); + return new IndexFacts(new IndexModel(clusters, searchDefs)); } private SearchDefinition createSearchDefinitionWithFields(String name, boolean literalBoost) { diff --git a/container-search/src/test/java/com/yahoo/prelude/querytransform/test/NormalizingSearcherTestCase.java b/container-search/src/test/java/com/yahoo/prelude/querytransform/test/NormalizingSearcherTestCase.java index 77f7f057eea..bf1ab6fc397 100644 --- a/container-search/src/test/java/com/yahoo/prelude/querytransform/test/NormalizingSearcherTestCase.java +++ b/container-search/src/test/java/com/yahoo/prelude/querytransform/test/NormalizingSearcherTestCase.java @@ -4,6 +4,7 @@ package com.yahoo.prelude.querytransform.test; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import com.google.common.collect.ImmutableList; import com.yahoo.language.Linguistics; import com.yahoo.language.simple.SimpleLinguistics; import com.yahoo.prelude.Index; @@ -24,6 +25,7 @@ import java.io.UnsupportedEncodingException; import java.net.URLEncoder; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -129,15 +131,13 @@ public class NormalizingSearcherTestCase { Map<String, List<String>> clusters = new LinkedHashMap<>(); clusters.put("cluster1", Arrays.asList("type1", "type2", "type3")); clusters.put("cluster2", Arrays.asList("type4", "type5")); - Map<String, SearchDefinition> searchDefs = new LinkedHashMap<>(); - searchDefs.put("type1", createSearchDefinitionWithFields("type1", true)); - searchDefs.put("type2", createSearchDefinitionWithFields("type2", false)); - searchDefs.put("type3", new SearchDefinition("type3")); - searchDefs.put("type3", new SearchDefinition("type3")); - searchDefs.put("type4", new SearchDefinition("type4")); - searchDefs.put("type5", new SearchDefinition("type5")); - SearchDefinition union = new SearchDefinition("union"); - return new IndexFacts(new IndexModel(clusters, searchDefs, union)); + Collection<SearchDefinition> searchDefs = ImmutableList.of( + createSearchDefinitionWithFields("type1", true), + createSearchDefinitionWithFields("type2", false), + new SearchDefinition("type3"), + new SearchDefinition("type4"), + new SearchDefinition("type5")); + return new IndexFacts(new IndexModel(clusters, searchDefs)); } private SearchDefinition createSearchDefinitionWithFields(String name, boolean normalize) { diff --git a/container-search/src/test/java/com/yahoo/prelude/searcher/test/JuniperSearcherTestCase.java b/container-search/src/test/java/com/yahoo/prelude/searcher/test/JuniperSearcherTestCase.java index 24af91cb5c0..909d641225d 100644 --- a/container-search/src/test/java/com/yahoo/prelude/searcher/test/JuniperSearcherTestCase.java +++ b/container-search/src/test/java/com/yahoo/prelude/searcher/test/JuniperSearcherTestCase.java @@ -2,9 +2,10 @@ package com.yahoo.prelude.searcher.test; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNull; +import com.google.common.collect.ImmutableList; import com.yahoo.component.ComponentId; import com.yahoo.component.chain.Chain; import com.yahoo.container.QrSearchersConfig; @@ -23,6 +24,7 @@ import com.yahoo.prelude.searcher.JuniperSearcher; import com.yahoo.search.searchchain.Execution; import org.junit.Test; +import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -83,11 +85,8 @@ public class JuniperSearcherTestCase { private Execution createExecution(Chain<Searcher> chain) { Map<String, List<String>> clusters = new LinkedHashMap<>(); - Map<String, SearchDefinition> searchDefs = new LinkedHashMap<>(); - searchDefs.put("one", createSearchDefinitionOne()); - searchDefs.put("two", createSearchDefinitionTwo()); - SearchDefinition union = new SearchDefinition("union"); - IndexModel indexModel = new IndexModel(clusters, searchDefs, union); + Collection<SearchDefinition> searchDefs = ImmutableList.of(createSearchDefinitionOne(), createSearchDefinitionTwo()); + IndexModel indexModel = new IndexModel(clusters, searchDefs); return new Execution(chain, Execution.Context.createContextStub(new IndexFacts(indexModel))); } @@ -110,8 +109,7 @@ public class JuniperSearcherTestCase { } private SearchDefinition createSearchDefinitionTwo() { - SearchDefinition two = new SearchDefinition("two"); - return two; + return new SearchDefinition("two"); } @Test @@ -140,7 +138,7 @@ public class JuniperSearcherTestCase { @Test public void testBoldingEquals() { - assertFalse(new Query("?query=12").equals(new Query("?query=12&bolding=false"))); + assertNotEquals(new Query("?query=12"), new Query("?query=12&bolding=false")); } @Test diff --git a/container-search/src/test/java/com/yahoo/prelude/searcher/test/ValidatePredicateSearcherTestCase.java b/container-search/src/test/java/com/yahoo/prelude/searcher/test/ValidatePredicateSearcherTestCase.java index 197f1d65172..1b9ca1cd29b 100644 --- a/container-search/src/test/java/com/yahoo/prelude/searcher/test/ValidatePredicateSearcherTestCase.java +++ b/container-search/src/test/java/com/yahoo/prelude/searcher/test/ValidatePredicateSearcherTestCase.java @@ -51,15 +51,11 @@ public class ValidatePredicateSearcherTestCase { Query query = new Query(); query.getModel().getQueryTree().setRoot(queryTree.getRoot()); - TreeMap<String, List<String>> masterClusters = new TreeMap<>(); - masterClusters.put("cluster", Arrays.asList("document")); SearchDefinition searchDefinition = new SearchDefinition("document"); Index index = new Index("predicate_field"); index.addCommand(command); searchDefinition.addIndex(index); - Map<String, SearchDefinition> searchDefinitionMap = new HashMap<>(); - searchDefinitionMap.put("document", searchDefinition); - IndexFacts indexFacts = new IndexFacts(new IndexModel(masterClusters, searchDefinitionMap, searchDefinition)); + IndexFacts indexFacts = new IndexFacts(new IndexModel(searchDefinition)); Execution.Context context = new Execution.Context(null, indexFacts, null, new RendererRegistry(MoreExecutors.directExecutor()), new SimpleLinguistics()); return new Execution(searcher, context).search(query); } diff --git a/container-search/src/test/java/com/yahoo/search/federation/sourceref/test/SourceRefResolverTestCase.java b/container-search/src/test/java/com/yahoo/search/federation/sourceref/test/SourceRefResolverTestCase.java index ab07baf438a..43f42685021 100644 --- a/container-search/src/test/java/com/yahoo/search/federation/sourceref/test/SourceRefResolverTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/federation/sourceref/test/SourceRefResolverTestCase.java @@ -20,10 +20,11 @@ import java.util.Set; import java.util.TreeMap; import static com.yahoo.search.federation.sourceref.test.SearchChainResolverTestCase.emptySourceToProviderMap; +import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.CoreMatchers.hasItems; import static org.hamcrest.core.Is.is; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; -import static org.junit.matchers.JUnitMatchers.hasItems; /** @@ -45,9 +46,9 @@ public class SourceRefResolverTestCase { private static SourceRefResolver createSourceRefResolver() { SearchChainResolver.Builder builder = new SearchChainResolver.Builder(); builder.addSearchChain(ComponentId.fromString(cluster1), new FederationOptions().setUseByDefault(true), - Collections.<String>emptyList()); + Collections.emptyList()); builder.addSearchChain(ComponentId.fromString(cluster2), new FederationOptions().setUseByDefault(true), - Collections.<String>emptyList()); + Collections.emptyList()); return new SourceRefResolver(builder.build()); } @@ -57,7 +58,7 @@ public class SourceRefResolverTestCase { masterClusters.put(cluster1, Arrays.asList("document1", "document2")); masterClusters.put(cluster2, Arrays.asList("document1")); masterClusters.put(cluster3, Arrays.asList("document3")); - indexFacts = new IndexFacts(new IndexModel(masterClusters, null, null)); + indexFacts = new IndexFacts(new IndexModel(masterClusters, Collections.emptyList())); } @Test @@ -69,7 +70,7 @@ public class SourceRefResolverTestCase { public void lookup_search_chain() throws Exception { Set<SearchChainInvocationSpec> searchChains = resolve(cluster1); assertThat(searchChains.size(), is(1)); - assertThat(searchChainIds(searchChains), hasItems(cluster1)); + assertThat(searchChainIds(searchChains), hasItem(cluster1)); } @Test diff --git a/container-search/src/test/java/com/yahoo/search/grouping/request/RawBufferTestCase.java b/container-search/src/test/java/com/yahoo/search/grouping/request/RawBufferTestCase.java index b1abdacf88f..9ec717e5584 100644 --- a/container-search/src/test/java/com/yahoo/search/grouping/request/RawBufferTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/grouping/request/RawBufferTestCase.java @@ -43,7 +43,7 @@ public class RawBufferTestCase { @Test public void requireThatToStringWorks() { assertToString(Arrays.asList("a".getBytes()[0], "b".getBytes()[0]), "{97,98}"); - assertToString(Arrays.asList(new Byte((byte)2), new Byte((byte)6)), "{2,6}"); + assertToString(Arrays.asList((byte)2, (byte)6), "{2,6}"); } public void assertToString(List<Byte> data, String expected) { diff --git a/container-search/src/test/java/com/yahoo/search/rendering/JsonRendererTestCase.java b/container-search/src/test/java/com/yahoo/search/rendering/JsonRendererTestCase.java index a83a0e84bbf..c4d49c11f5e 100644 --- a/container-search/src/test/java/com/yahoo/search/rendering/JsonRendererTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/rendering/JsonRendererTestCase.java @@ -1369,11 +1369,7 @@ public class JsonRendererTestCase { } private Execution createExecution(Chain<Searcher> chain) { - Map<String, List<String>> clusters = new LinkedHashMap<>(); - Map<String, SearchDefinition> searchDefs = new LinkedHashMap<>(); - searchDefs.put("one", createSearchDefinitionOne()); - SearchDefinition union = new SearchDefinition("union"); - IndexModel indexModel = new IndexModel(clusters, searchDefs, union); + IndexModel indexModel = new IndexModel(createSearchDefinitionOne()); return new Execution(chain, Execution.Context.createContextStub(new IndexFacts(indexModel))); } diff --git a/container-search/src/test/java/com/yahoo/search/rendering/XMLRendererTestCase.java b/container-search/src/test/java/com/yahoo/search/rendering/XMLRendererTestCase.java index 184a8c1aa43..6396981f12d 100644 --- a/container-search/src/test/java/com/yahoo/search/rendering/XMLRendererTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/rendering/XMLRendererTestCase.java @@ -206,11 +206,7 @@ public class XMLRendererTestCase { } private Execution createExecution(Chain<Searcher> chain) { - Map<String, List<String>> clusters = new LinkedHashMap<>(); - Map<String, SearchDefinition> searchDefs = new LinkedHashMap<>(); - searchDefs.put("one", createSearchDefinitionOne()); - SearchDefinition union = new SearchDefinition("union"); - IndexModel indexModel = new IndexModel(clusters, searchDefs, union); + IndexModel indexModel = new IndexModel(createSearchDefinitionOne()); return new Execution(chain, Execution.Context.createContextStub(new IndexFacts(indexModel))); } diff --git a/container-search/src/test/java/com/yahoo/search/searchers/ValidateNearestNeighborTestCase.java b/container-search/src/test/java/com/yahoo/search/searchers/ValidateNearestNeighborTestCase.java index 871d9285071..0cbf3a6f92c 100644 --- a/container-search/src/test/java/com/yahoo/search/searchers/ValidateNearestNeighborTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/searchers/ValidateNearestNeighborTestCase.java @@ -209,12 +209,8 @@ public class ValidateNearestNeighborTestCase { Query query = new Query(); query.getModel().getQueryTree().setRoot(queryTree.getRoot()); query.getRanking().getProperties().put("qvector", qTensor); - TreeMap<String, List<String>> masterClusters = new TreeMap<>(); - masterClusters.put("cluster", Arrays.asList("document")); SearchDefinition searchDefinition = new SearchDefinition("document"); - Map<String, SearchDefinition> searchDefinitionMap = new HashMap<>(); - searchDefinitionMap.put("document", searchDefinition); - IndexFacts indexFacts = new IndexFacts(new IndexModel(masterClusters, searchDefinitionMap, searchDefinition)); + IndexFacts indexFacts = new IndexFacts(new IndexModel(searchDefinition)); Execution.Context context = new Execution.Context(null, indexFacts, null, new RendererRegistry(MoreExecutors.directExecutor()), new SimpleLinguistics()); return new Execution(searcher, context).search(query); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java index 681c1b4283a..c75f3102772 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java @@ -115,7 +115,7 @@ public class Application { /** Returns the instance with the given name, or throws. */ public Instance require(InstanceName instance) { - return get(instance).orElseThrow(() -> new IllegalArgumentException("Unknown instance '" + instance + "'")); + return get(instance).orElseThrow(() -> new IllegalArgumentException("Unknown instance '" + instance + "' in '" + id + "'")); } /** 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 4725af6fd28..ecea7b09782 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 @@ -98,6 +98,10 @@ import java.util.stream.Stream; import static com.yahoo.vespa.hosted.controller.api.integration.configserver.Node.State.active; import static com.yahoo.vespa.hosted.controller.api.integration.configserver.Node.State.reserved; import static java.util.Comparator.naturalOrder; +import static java.util.stream.Collectors.groupingBy; +import static java.util.stream.Collectors.joining; +import static java.util.stream.Collectors.toList; +import static java.util.stream.Collectors.toMap; /** * A singleton owned by the Controller which contains the methods and state for controlling applications. @@ -146,8 +150,16 @@ public class ApplicationController { Once.after(Duration.ofMinutes(1), () -> { Instant start = clock.instant(); int count = 0; - for (Application application : curator.readApplications()) { - lockApplicationIfPresent(application.id(), this::store); + for (TenantAndApplicationId id: curator.readApplicationIds()) { + lockApplicationIfPresent(id, application -> { + if (id.tenant().value().startsWith("by-")) + application = application.with(DeploymentSpec.empty); + else + for (InstanceName instance : application.get().deploymentSpec().instanceNames()) + if ( ! application.get().instances().keySet().contains(instance)) + application = application.withNewInstance(instance); + store(application); + }); count++; } log.log(Level.INFO, String.format("Wrote %d applications in %s", count, @@ -608,7 +620,7 @@ public class ApplicationController { .filter(zone -> deploymentSpec.instance(instance).isEmpty() || ! deploymentSpec.requireInstance(instance).deploysTo(zone.environment(), zone.region())) - .collect(Collectors.toList()); + .collect(toList()); if (deploymentsToRemove.isEmpty()) return application; @@ -618,7 +630,7 @@ public class ApplicationController { " is deployed in " + deploymentsToRemove.stream() .map(zone -> zone.region().value()) - .collect(Collectors.joining(", ")) + + .collect(joining(", ")) + ", but does not include " + (deploymentsToRemove.size() > 1 ? "these zones" : "this zone") + " in deployment.xml. " + @@ -703,22 +715,31 @@ public class ApplicationController { if (tenant.type() != Tenant.Type.user && credentials.isEmpty()) throw new IllegalArgumentException("Could not delete application '" + id + "': No credentials provided"); - // Find all instances of the application - List<ApplicationId> instances = requireApplication(id).instances().keySet().stream() - .map(id::instance) - .collect(Collectors.toUnmodifiableList()); - if (instances.size() > 1) - throw new IllegalArgumentException("Could not delete application; more than one instance present: " + instances); + lockApplicationOrThrow(id, application -> { + var deployments = application.get().instances().values().stream() + .filter(instance -> ! instance.deployments().isEmpty()) + .collect(toMap(instance -> instance.name(), + instance -> instance.deployments().keySet().stream() + .map(ZoneId::toString) + .collect(joining(", ")))); + if ( ! deployments.isEmpty()) + throw new IllegalArgumentException("Could not delete '" + application + "': It has active deployments: " + deployments); + + for (Instance instance : application.get().instances().values()) { + removeEndpoints(instance); + application = application.without(instance.name()); + } - for (ApplicationId instance : instances) - deleteInstance(instance); + applicationStore.removeAll(id.tenant(), id.application()); + applicationStore.removeAllTesters(id.tenant(), id.application()); - applicationStore.removeAll(id.tenant(), id.application()); - applicationStore.removeAllTesters(id.tenant(), id.application()); + if (tenant.type() != Tenant.Type.user) + accessControl.deleteApplication(id, credentials.get()); + curator.removeApplication(id); - if (tenant.type() != Tenant.Type.user) - accessControl.deleteApplication(id, credentials.get()); - curator.removeApplication(id); + controller.jobController().collectGarbage(); + log.info("Deleted " + id); + }); } /** @@ -735,24 +756,30 @@ public class ApplicationController { if ( ! application.get().require(instanceId.instance()).deployments().isEmpty()) throw new IllegalArgumentException("Could not delete '" + application + "': It has active deployments in: " + application.get().require(instanceId.instance()).deployments().keySet().stream().map(ZoneId::toString) - .sorted().collect(Collectors.joining(", "))); - - Instance instance = application.get().require(instanceId.instance()); - instance.rotations().forEach(assignedRotation -> { - var endpoints = instance.endpointsIn(controller.system(), assignedRotation.endpointId()); - endpoints.asList().stream() - .map(Endpoint::dnsName) - .forEach(name -> { - controller.nameServiceForwarder().removeRecords(Record.Type.CNAME, RecordName.from(name), Priority.normal); - }); - }); + .sorted().collect(joining(", "))); + + if ( ! application.get().deploymentSpec().equals(DeploymentSpec.empty) + && application.get().deploymentSpec().instanceNames().contains(instanceId.instance())) + throw new IllegalArgumentException("Can not delete '" + instanceId + "', which is specified in 'deployment.xml'; remove it there first"); + + removeEndpoints(application.get().require(instanceId.instance())); curator.writeApplication(application.without(instanceId.instance()).get()); controller.jobController().collectGarbage(); - log.info("Deleted " + instanceId); }); } + private void removeEndpoints(Instance instance) { + instance.rotations().forEach(assignedRotation -> { + var endpoints = instance.endpointsIn(controller.system(), assignedRotation.endpointId()); + endpoints.asList().stream() + .map(Endpoint::dnsName) + .forEach(name -> { + controller.nameServiceForwarder().removeRecords(Record.Type.CNAME, RecordName.from(name), Priority.normal); + }); + }); + } + /** * Replace any previous version of this application by this instance * diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackage.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackage.java index 79d6e10e1dd..a8ab847cda6 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackage.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackage.java @@ -49,22 +49,40 @@ public class ApplicationPackage { private final Optional<Version> compileVersion; private final Optional<Instant> buildTime; private final List<X509Certificate> trustedCertificates; - - /** + + /** * Creates an application package from its zipped content. * This <b>assigns ownership</b> of the given byte array to this class; * it must not be further changed by the caller. */ public ApplicationPackage(byte[] zippedContent) { + this(zippedContent, false); + } + + /** + * Creates an application package from its zipped content. + * This <b>assigns ownership</b> of the given byte array to this class; + * it must not be further changed by the caller. + * If 'requireFiles' is true, files needed by deployment orchestration must be present. + */ + public ApplicationPackage(byte[] zippedContent, boolean requireFiles) { this.zippedContent = Objects.requireNonNull(zippedContent, "The application package content cannot be null"); this.contentHash = Hashing.sha1().hashBytes(zippedContent).toString(); - Files files = Files.extract(Set.of("deployment.xml", "validation-overrides.xml", "build-meta.json", trustedCertificatesFile), zippedContent); - this.deploymentSpec = files.getAsReader("deployment.xml").map(DeploymentSpec::fromXml).orElse(DeploymentSpec.empty); + + Optional<DeploymentSpec> deploymentSpec = files.getAsReader("deployment.xml").map(DeploymentSpec::fromXml); + if (requireFiles && deploymentSpec.isEmpty()) + throw new IllegalArgumentException("Missing required file 'deployment.xml'"); + this.deploymentSpec = deploymentSpec.orElse(DeploymentSpec.empty); + this.validationOverrides = files.getAsReader("validation-overrides.xml").map(ValidationOverrides::fromXml).orElse(ValidationOverrides.empty); + Optional<Inspector> buildMetaObject = files.get("build-meta.json").map(SlimeUtils::jsonToSlime).map(Slime::get); + if (requireFiles && buildMetaObject.isEmpty()) + throw new IllegalArgumentException("Missing required file 'deployment.xml'"); this.compileVersion = buildMetaObject.flatMap(object -> parse(object, "compileVersion", field -> Version.fromString(field.asString()))); this.buildTime = buildMetaObject.flatMap(object -> parse(object, "buildTime", field -> Instant.ofEpochMilli(field.asLong()))); + this.trustedCertificates = files.get(trustedCertificatesFile).map(bytes -> X509CertificateUtils.certificateListFromPem(new String(bytes, UTF_8))).orElse(List.of()); } @@ -109,9 +127,10 @@ public class ApplicationPackage { } private static <Type> Optional<Type> parse(Inspector buildMetaObject, String fieldName, Function<Inspector, Type> mapper) { + if ( ! buildMetaObject.field(fieldName).valid()) + throw new IllegalArgumentException("Missing value '" + fieldName + "' in 'build-meta.json'"); try { - return buildMetaObject.field(fieldName).valid() ? Optional.of(mapper.apply(buildMetaObject.field(fieldName))) - : Optional.empty(); + return Optional.of(mapper.apply(buildMetaObject.field(fieldName))); } catch (RuntimeException e) { throw new IllegalArgumentException("Failed parsing \"" + fieldName + "\" in 'build-meta.json': " + Exceptions.toMessageString(e)); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java index d8bad46fd3f..a441d8670e2 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java @@ -123,6 +123,8 @@ public class DeploymentStatus { public Map<JobId, StepStatus> stepStatus() { return jobSteps; } + public List<StepStatus> allSteps() { return allSteps; } + private void addProductionJobs(Map<JobId, List<Versions>> jobs, Change change) { jobSteps.forEach((job, step) -> { Versions versions = Versions.from(change, application, deploymentFor(job), systemVersion); @@ -206,13 +208,6 @@ public class DeploymentStatus { List<StepStatus> previous = List.of(); for (DeploymentSpec.Step step : spec.steps()) previous = fillStep(dependencies, allSteps, step, previous, spec.instanceNames().get(0)); - for (InstanceName instance : spec.instanceNames()) - for (JobType test : List.of(systemTest, stagingTest)) { - JobId job = new JobId(application.id().instance(instance), test); - if ( ! dependencies.containsKey(job)) - dependencies.put(job, JobStepStatus.ofTestDeployment(new DeclaredZone(test.environment()), List.of(), - this, instance, test, false)); - } return ImmutableMap.copyOf(dependencies); } @@ -231,22 +226,22 @@ public class DeploymentStatus { StepStatus stepStatus; if (step.concerns(test) || step.concerns(staging)) { // SKIP? jobType = JobType.from(system, ((DeclaredZone) step).environment(), null) - .orElseThrow(() -> new IllegalStateException("No job is known for " + step + " in " + system)); + .orElseThrow(() -> new IllegalStateException(application + " specifies " + step + ", but this has no job in " + system)); stepStatus = JobStepStatus.ofTestDeployment((DeclaredZone) step, List.of(), this, instance, jobType, true); previous = new ArrayList<>(previous); previous.add(stepStatus); } else if (step.isTest()) { jobType = JobType.testFrom(system, ((DeclaredTest) step).region()) - .orElseThrow(() -> new IllegalStateException("No job is known for " + step + " in " + system)); + .orElseThrow(() -> new IllegalStateException(application + " specifies " + step + ", but this has no job in " + system)); JobType preType = JobType.from(system, prod, ((DeclaredTest) step).region()) - .orElseThrow(() -> new IllegalStateException("No job is known for " + step + " in " + system)); + .orElseThrow(() -> new IllegalStateException(application + " specifies " + step + ", but this has no job in " + system)); stepStatus = JobStepStatus.ofProductionTest((DeclaredTest) step, previous, this, instance, jobType, preType); previous = List.of(stepStatus); } else if (step.concerns(prod)) { jobType = JobType.from(system, ((DeclaredZone) step).environment(), ((DeclaredZone) step).region().get()) - .orElseThrow(() -> new IllegalStateException("No job is known for " + step + " in " + system)); + .orElseThrow(() -> new IllegalStateException(application + " specifies " + step + ", but this has no job in " + system)); stepStatus = JobStepStatus.ofProductionDeployment((DeclaredZone) step, previous, this, instance, jobType); previous = List.of(stepStatus); } @@ -256,20 +251,26 @@ public class DeploymentStatus { return previous; } - Optional<InstanceName> stepInstance = Optional.of(step) - .filter(DeploymentInstanceSpec.class::isInstance) - .map(DeploymentInstanceSpec.class::cast) - .map(DeploymentInstanceSpec::name); + // TODO jonmv: Make instance status as well, including block-change and upgrade policy, to keep track of change; + // set it equal to application's when dependencies are completed. + if (step instanceof DeploymentInstanceSpec) { + instance = ((DeploymentInstanceSpec) step).name(); + for (JobType test : List.of(systemTest, stagingTest)) + dependencies.putIfAbsent(new JobId(application.id().instance(instance), test), + JobStepStatus.ofTestDeployment(new DeclaredZone(test.environment()), List.of(), + this, instance, test, false)); + } + if (step.isOrdered()) { for (DeploymentSpec.Step nested : step.steps()) - previous = fillStep(dependencies, allSteps, nested, previous, stepInstance.orElse(instance)); + previous = fillStep(dependencies, allSteps, nested, previous, instance); return previous; } List<StepStatus> parallel = new ArrayList<>(); for (DeploymentSpec.Step nested : step.steps()) - parallel.addAll(fillStep(dependencies, allSteps, nested, previous, stepInstance.orElse(instance))); + parallel.addAll(fillStep(dependencies, allSteps, nested, previous, instance)); return List.copyOf(parallel); } @@ -322,9 +323,15 @@ public class DeploymentStatus { /** The instance of this, if any. */ public final Optional<InstanceName> instance() { return instance; } + public Optional<JobId> job() { return Optional.empty(); } + /** The time at which this is complete on the given versions. */ public abstract Optional<Instant> completedAt(Change change, Versions versions); + // TODO jonmv: dependenciesCompletedAt + + // TODO jonmv: pausedUntil and coolingDownUntil + /** The time at which all dependencies completed on the given version. */ public Optional<Instant> readyAt(Change change, Versions versions) { return dependencies.stream().allMatch(step -> step.completedAt(change, versions).isPresent()) @@ -375,6 +382,9 @@ public class DeploymentStatus { } @Override + public Optional<JobId> job() { return Optional.of(job.id()); } + + @Override public boolean isRunning(Versions versions) { return job.isRunning() && job.lastTriggered().get().versions().targetsMatch(versions); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/BufferedLogStore.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/BufferedLogStore.java index b761c194cae..2952ad5f1bf 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/BufferedLogStore.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/BufferedLogStore.java @@ -55,8 +55,8 @@ public class BufferedLogStore { for (LogEntry entry : entries) stepEntries.add(new LogEntry(++lastEntryId, entry.at(), entry.type(), entry.message())); - buffer.writeLog(id, type, lastChunkId, logSerializer.toJson(log)); buffer.writeLastLogEntryId(id, type, lastEntryId); + buffer.writeLog(id, type, lastChunkId, logSerializer.toJson(log)); } /** Reads all log entries after the given threshold, from the buffered log, i.e., for an active run. */ diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index a4b2205575b..a423a95d2ff 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -1942,9 +1942,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { String authorEmail = submitOptions.field("authorEmail").asString(); long projectId = Math.max(1, submitOptions.field("projectId").asLong()); - ApplicationPackage applicationPackage = new ApplicationPackage(dataParts.get(EnvironmentResource.APPLICATION_ZIP)); - if (DeploymentSpec.empty.equals(applicationPackage.deploymentSpec())) - throw new IllegalArgumentException("Missing required file 'deployment.xml'"); + ApplicationPackage applicationPackage = new ApplicationPackage(dataParts.get(EnvironmentResource.APPLICATION_ZIP), true); controller.applications().verifyApplicationIdentityConfiguration(TenantName.from(tenant), applicationPackage, diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java index 9b12bd77743..3dcb3b61c5b 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java @@ -17,6 +17,7 @@ import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.NotExistsException; import com.yahoo.vespa.hosted.controller.api.integration.LogEntry; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion; +import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.SourceRevision; @@ -83,13 +84,19 @@ class JobControllerApiHandlerHelper { */ static HttpResponse jobTypeResponse(Controller controller, ApplicationId id, URI baseUriForJobs) { Application application = controller.applications().requireApplication(TenantAndApplicationId.from(id)); + DeploymentStatus deploymentStatus = controller.jobController().deploymentStatus(application); Instance instance = application.require(id.instance()); Change change = application.change(); Optional<DeploymentInstanceSpec> spec = application.deploymentSpec().instance(id.instance()); Optional<DeploymentSteps> steps = spec.map(s -> new DeploymentSteps(s, controller::system)); - List<JobType> productionJobs = steps.map(DeploymentSteps::productionJobs).orElse(List.of()); - List<JobType> jobs = steps.map(DeploymentSteps::jobs).orElse(List.of()); - Map<JobType, JobStatus> status = controller.jobController().deploymentStatus(application).instanceJobs(id.instance()); + List<JobType> jobs = deploymentStatus.stepStatus().keySet().stream() + .filter(jobId -> id.equals(jobId.application())) + .map(JobId::type) + .collect(Collectors.toUnmodifiableList()); + List<JobType> productionJobs = jobs.stream() + .filter(JobType::isProduction) + .collect(Collectors.toUnmodifiableList()); + Map<JobType, JobStatus> status = deploymentStatus.instanceJobs(id.instance()); // The logic for pending runs imitates DeploymentTrigger logic; not good, but the trigger wiring must be re-written to reuse :S Map<JobType, Versions> pendingProduction = @@ -127,6 +134,7 @@ class JobControllerApiHandlerHelper { Cursor deploymentsArray = responseObject.setArray("deployments"); steps.ifPresent(deploymentSteps -> deploymentSteps.production().forEach(step -> { + if (step.isTest()) return; Cursor deploymentsObject = deploymentsArray.addObject(); deploymentSteps.toJobs(step).forEach(type -> { ZoneId zone = type.zone(controller.system()); @@ -144,7 +152,7 @@ class JobControllerApiHandlerHelper { })); Cursor jobsObject = responseObject.setObject("jobs"); - steps.ifPresent(deploymentSteps -> deploymentSteps.jobs().forEach(type -> { + steps.ifPresent(deploymentSteps -> jobs.forEach(type -> { jobTypeToSlime(jobsObject.setObject(shortNameOf(type, controller.system())), controller, application, @@ -189,11 +197,14 @@ class JobControllerApiHandlerHelper { Version lastPlatform = lastVespa.versionNumber(); lastPlatformObject.setString("platform", lastPlatform.toString()); lastPlatformObject.setLong("at", lastVespa.committedAt().toEpochMilli()); - long completed = productionJobs.stream().filter(type -> controller.applications().deploymentTrigger().isComplete(Change.of(lastPlatform), change, instance, type, status.get(type))).count(); + long completed = productionJobs.stream() + .filter(type -> ! type.isTest()) + .filter(type -> controller.applications().deploymentTrigger().isComplete(Change.of(lastPlatform), change, instance, type, status.get(type))) + .count(); if (Optional.of(lastPlatform).equals(change.platform())) - lastPlatformObject.setString("deploying", completed + " of " + productionJobs.size() + " complete"); + lastPlatformObject.setString("deploying", completed + " of " + productionJobs.stream().filter(type -> ! type.isTest()).count() + " complete"); else if (completed == productionJobs.size()) - lastPlatformObject.setString("completed", completed + " of " + productionJobs.size() + " complete"); + lastPlatformObject.setString("completed", completed + " of " + productionJobs.stream().filter(type -> ! type.isTest()).count() + " complete"); else if ( ! application.deploymentSpec().instances().stream() .allMatch(spec -> spec.canUpgradeAt(controller.clock().instant()))) { lastPlatformObject.setString("blocked", application.deploymentSpec().instances().stream() @@ -214,11 +225,14 @@ class JobControllerApiHandlerHelper { ApplicationVersion lastApplication = application.latestVersion().get(); applicationVersionToSlime(lastApplicationObject.setObject("application"), lastApplication); lastApplicationObject.setLong("at", lastApplication.buildTime().get().toEpochMilli()); - completed = productionJobs.stream().filter(type -> controller.applications().deploymentTrigger().isComplete(Change.of(lastApplication), change, instance, type, status.get(type))).count(); + completed = productionJobs.stream() + .filter(type -> ! type.isTest()) + .filter(type -> controller.applications().deploymentTrigger().isComplete(Change.of(lastApplication), change, instance, type, status.get(type))) + .count(); if (Optional.of(lastApplication).equals(change.application())) - lastApplicationObject.setString("deploying", completed + " of " + productionJobs.size() + " complete"); + lastApplicationObject.setString("deploying", completed + " of " + productionJobs.stream().filter(type -> ! type.isTest()).count() + " complete"); else if (completed == productionJobs.size()) - lastApplicationObject.setString("completed", completed + " of " + productionJobs.size() + " complete"); + lastApplicationObject.setString("completed", completed + " of " + productionJobs.stream().filter(type -> ! type.isTest()).count() + " complete"); else if ( ! application.deploymentSpec().instances().stream() .allMatch(spec -> spec.canChangeRevisionAt(controller.clock().instant()))) { lastApplicationObject.setString("blocked", application.deploymentSpec().instances().stream() @@ -343,7 +357,7 @@ class JobControllerApiHandlerHelper { } private static String shortNameOf(JobType type, SystemName system) { - return type.isProduction() ? type.zone(system).region().value() : type.jobName(); + return type.jobName().replaceFirst("production-", ""); } private static String taskStatusOf(Run run) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java index 126579263a9..2b523bdc12a 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.controller.restapi.deployment; import com.yahoo.component.Version; +import com.yahoo.config.application.api.DeploymentInstanceSpec; import com.yahoo.config.application.api.DeploymentSpec; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.HostName; @@ -148,7 +149,8 @@ public class DeploymentApiHandler extends LoggingRequestHandler { "/application/" + id.application().value()).toString()); object.setString("upgradePolicy", toString(controller.applications().requireApplication(TenantAndApplicationId.from(id)) - .deploymentSpec().requireInstance(id.instance()).upgradePolicy())); + .deploymentSpec().instance(id.instance()).map(DeploymentInstanceSpec::upgradePolicy) + .orElse(DeploymentSpec.UpgradePolicy.defaultPolicy))); } private static String toString(DeploymentSpec.UpgradePolicy upgradePolicy) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilter.java index 361aad93133..139d5e03f49 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilter.java @@ -96,11 +96,6 @@ public class AthenzRoleFilter extends JsonSecurityRequestFilterBase { || hasDeployerAccess(identity, ((AthenzTenant) tenant.get()).domain(), application.get())) roleMemberships.add(Role.tenantPipeline(tenant.get().name(), application.get())); - if ( tenant.isPresent() && application.isPresent() && instance.isPresent() - && principal.getIdentity() instanceof AthenzUser - && instance.get().value().equals(principal.getIdentity().getName())) - roleMemberships.add(Role.athenzUser(tenant.get().name(), application.get(), instance.get())); - if (athenz.hasSystemFlagsAccess(identity, /*dryrun*/false)) { roleMemberships.add(Role.systemFlagsDeployer()); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java index 8ca8d31b32b..1a135b8d9a2 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java @@ -111,7 +111,7 @@ public class RotationRepository { } // Only allow one kind of configuration syntax - if (deploymentSpec.requireInstance(instance.name()).globalServiceId().isPresent() + if ( deploymentSpec.requireInstance(instance.name()).globalServiceId().isPresent() && ! deploymentSpec.requireInstance(instance.name()).endpoints().isEmpty()) { throw new IllegalArgumentException("Cannot provision rotations with both global-service-id and 'endpoints'"); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index 9f869b0904b..cb032615fc6 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java @@ -6,6 +6,7 @@ import ai.vespa.hosted.api.Signatures; import com.yahoo.application.container.handler.Request; import com.yahoo.component.Version; import com.yahoo.config.application.api.ValidationId; +import com.yahoo.config.application.api.ValidationOverrides; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationName; import com.yahoo.config.provision.AthenzService; @@ -248,23 +249,36 @@ public class ApplicationApiTest extends ControllerContainerTest { "{\"message\":\"Deployment started in run 1 of dev-us-east-1 for tenant1.application1.instance1. This may take about 15 minutes the first time.\",\"run\":1}"); app1.runJob(JobType.devUsEast1); - - // POST an application package is allowed under user instance + // POST an application package is not generally allowed under user instance tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/otheruser/deploy/dev-us-east-1", POST) .userIdentity(OTHER_USER_ID) .data(createApplicationDeployData(applicationPackageInstance1, false)), - new File("deployment-job-accepted-2.json")); + "{\n \"code\" : 403,\n \"message\" : \"Access denied\"\n}", + 403); - // DELETE a dev deployment is allowed under user instance + // DELETE a dev deployment is not generally allowed under user instance tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/otheruser/environment/dev/region/us-east-1", DELETE) .userIdentity(OTHER_USER_ID), - "{\"message\":\"Deactivated tenant1.application1.otheruser in dev.us-east-1\"}"); + "{\n \"code\" : 403,\n \"message\" : \"Access denied\"\n}", + 403); + + // When the user is a tenant admin, user instances are allowed. + // POST an application package is not allowed under user instance for tenant admins + tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/myuser/deploy/dev-us-east-1", POST) + .userIdentity(USER_ID) + .data(createApplicationDeployData(applicationPackageInstance1, false)), + new File("deployment-job-accepted-2.json")); + + // DELETE a dev deployment is allowed under user instance for tenant admins + tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/myuser/environment/dev/region/us-east-1", DELETE) + .userIdentity(USER_ID), + "{\"message\":\"Deactivated tenant1.application1.myuser in dev.us-east-1\"}"); // DELETE a user instance - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/otheruser", DELETE) + tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/myuser", DELETE) .userIdentity(USER_ID) .oktaAccessToken(OKTA_AT).oktaIdentityToken(OKTA_IT), - "{\"message\":\"Deleted instance tenant1.application1.otheruser\"}"); + "{\"message\":\"Deleted instance tenant1.application1.myuser\"}"); addScrewdriverUserToDeployRole(SCREWDRIVER_ID, ATHENZ_TENANT_DOMAIN, @@ -718,25 +732,13 @@ public class ApplicationApiTest extends ControllerContainerTest { .userIdentity(USER_ID), ""); - // DELETE all instances under an application to delete the application - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/default", DELETE) - .userIdentity(USER_ID) - .oktaAccessToken(OKTA_AT).oktaIdentityToken(OKTA_IT), - "{\"message\":\"Deleted instance tenant1.application1.default\"}"); - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/my-user", DELETE) - .userIdentity(USER_ID) - .oktaAccessToken(OKTA_AT).oktaIdentityToken(OKTA_IT), - "{\"message\":\"Deleted instance tenant1.application1.my-user\"}"); - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/instance1", DELETE) - .userIdentity(USER_ID) - .oktaAccessToken(OKTA_AT).oktaIdentityToken(OKTA_IT), - "{\"message\":\"Deleted instance tenant1.application1.instance1\"}"); - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/instance2", DELETE) + // DELETE the application which no longer has any deployments + tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1", DELETE) .userIdentity(USER_ID) .oktaAccessToken(OKTA_AT).oktaIdentityToken(OKTA_IT), - "{\"message\":\"Deleted instance tenant1.application1.instance2\"}"); + "{\"message\":\"Deleted application tenant1.application1\"}"); - // DELETE a tenant + // DELETE an empty tenant tester.assertResponse(request("/application/v4/tenant/tenant1", DELETE).userIdentity(USER_ID) .oktaAccessToken(OKTA_AT).oktaIdentityToken(OKTA_IT), new File("tenant-without-applications.json")); @@ -1223,20 +1225,6 @@ public class ApplicationApiTest extends ControllerContainerTest { new File("instance-reference-default.json"), 200); - // Deleting the application when more than one instance is present is forbidden - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1", DELETE) - .userIdentity(authorizedUser) - .oktaAccessToken(OKTA_AT).oktaIdentityToken(OKTA_IT), - "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Could not delete application; more than one instance present: [tenant1.application1, tenant1.application1.instance1]\"}", - 400); - - // Deleting one instance is OK - tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/default", DELETE) - .userIdentity(authorizedUser) - .oktaAccessToken(OKTA_AT).oktaIdentityToken(OKTA_IT), - "{\"message\":\"Deleted instance tenant1.application1.default\"}", - 200); - // (Deleting the application with the right tenant id) tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1", DELETE) .userIdentity(authorizedUser) @@ -1332,12 +1320,11 @@ public class ApplicationApiTest extends ControllerContainerTest { .build(); // POST (deploy) an application to a dev zone - String expectedResult="{\"error-code\":\"BAD_REQUEST\",\"message\":\"User user.new-user is not allowed to launch services in Athenz domain domain1. Please reach out to the domain admin.\"}"; MultiPartStreamer entity = createApplicationDeployData(applicationPackage, true); tester.assertResponse(request("/application/v4/tenant/by-new-user/application/application1/environment/dev/region/us-west-1/instance/default", POST) .data(entity) .userIdentity(userId), - expectedResult, + "{\"error-code\":\"BAD_REQUEST\",\"message\":\"User user.new-user is not allowed to launch services in Athenz domain domain1. Please reach out to the domain admin.\"}", 400); createTenantAndApplication(); @@ -1345,8 +1332,8 @@ public class ApplicationApiTest extends ControllerContainerTest { tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/new-user/deploy/dev-us-east-1", POST) .data(entity) .userIdentity(userId), - expectedResult, - 400); + "{\n \"code\" : 403,\n \"message\" : \"Access denied\"\n}", + 403); // Add "new-user" to the admin role, to allow service launches. tester.athenzClientFactory().getSetup() diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java index 7bbc7e91244..9fe754c060f 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelperTest.java @@ -9,6 +9,7 @@ import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServerException; import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; +import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import org.json.JSONException; @@ -33,6 +34,7 @@ import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobTy import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.productionUsWest1; import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.stagingTest; import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.systemTest; +import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.testUsCentral1; import static com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterCloud.Status.FAILURE; import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.applicationPackage; import static com.yahoo.vespa.hosted.controller.deployment.RunStatus.deploymentFailed; @@ -49,21 +51,27 @@ public class JobControllerApiHandlerHelperTest { @Test public void testResponses() { + ApplicationPackage applicationPackage = new ApplicationPackageBuilder() + .region("us-central-1") + .test("us-central-1") + .parallel("us-west-1", "us-east-3") + .build(); DeploymentTester tester = new DeploymentTester(); var app = tester.newDeploymentContext(); tester.clock().setInstant(Instant.EPOCH); // Revision 1 gets deployed everywhere. - app.submit().deploy(); + app.submit(applicationPackage).deploy(); ApplicationVersion revision1 = app.lastSubmission().get(); assertEquals(1000, tester.application().projectId().getAsLong()); tester.clock().advance(Duration.ofMillis(1000)); // Revision 2 gets deployed everywhere except in us-east-3. - ApplicationVersion revision2 = app.submit().lastSubmission().get(); + ApplicationVersion revision2 = app.submit(applicationPackage).lastSubmission().get(); app.runJob(systemTest); app.runJob(stagingTest); app.runJob(productionUsCentral1); + app.runJob(testUsCentral1); tester.triggerJobs(); @@ -88,7 +96,7 @@ public class JobControllerApiHandlerHelperTest { tester.clock().advance(Duration.ofMillis(1000)); // Revision 3 starts. - app.submit() + app.submit(applicationPackage) .runJob(systemTest).runJob(stagingTest); tester.triggerJobs(); // Starts runs for us-central-1 and a new staging test run. tester.runner().run(); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-job-accepted-2.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-job-accepted-2.json index ae089b504c8..8ea3f318d1d 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-job-accepted-2.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-job-accepted-2.json @@ -1,4 +1,4 @@ { - "message": "Deployment started in run 1 of dev-us-east-1 for tenant1.application1.otheruser. This may take about 15 minutes the first time.", + "message": "Deployment started in run 1 of dev-us-east-1 for tenant1.application1.myuser. This may take about 15 minutes the first time.", "run": 1 }
\ No newline at end of file diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/overview.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/overview.json index c0f45543abb..6a7ea87d6a8 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/overview.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/overview.json @@ -572,6 +572,115 @@ ], "url": "https://some.url:43/root/production-us-central-1" }, + "test-us-central-1": { + "runs": [ + { + "status": "pending", + "wantedPlatform": "6.1", + "wantedApplication": { + "hash": "1.0.3-commit1", + "build": 3, + "source": { + "gitRepository": "repository1", + "gitBranch": "master", + "gitCommit": "commit1" + } + }, + "currentPlatform": "6.1", + "currentApplication": { + "hash": "1.0.3-commit1", + "build": 3, + "source": { + "gitRepository": "repository1", + "gitBranch": "master", + "gitCommit": "commit1" + } + }, + "tasks": { + "us-central-1": "running", + "us-west-1": "pending", + "us-east-3": "pending" + } + }, + { + "id": 2, + "status": "success", + "start": 1000, + "end": 1000, + "wantedPlatform": "6.1", + "wantedApplication": { + "hash": "1.0.2-commit1", + "build": 2, + "source": { + "gitRepository": "repository1", + "gitBranch": "master", + "gitCommit": "commit1" + } + }, + "currentPlatform": "6.1", + "currentApplication": { + "hash": "1.0.2-commit1", + "build": 2, + "source": { + "gitRepository": "repository1", + "gitBranch": "master", + "gitCommit": "commit1" + } + }, + "steps": { + "deployTester": "succeeded", + "installTester": "succeeded", + "startTests": "succeeded", + "endTests": "succeeded", + "deactivateTester": "succeeded", + "report": "succeeded" + }, + "tasks": { + "test": "succeeded" + }, + "log": "https://some.url:43/root/test-us-central-1/run/2" + }, + { + "id": 1, + "status": "success", + "start": 0, + "end": 0, + "wantedPlatform": "6.1", + "wantedApplication": { + "hash": "1.0.1-commit1", + "build": 1, + "source": { + "gitRepository": "repository1", + "gitBranch": "master", + "gitCommit": "commit1" + } + }, + "currentPlatform": "6.1", + "currentApplication": { + "hash": "1.0.1-commit1", + "build": 1, + "source": { + "gitRepository": "repository1", + "gitBranch": "master", + "gitCommit": "commit1" + } + }, + "steps": { + "deployTester": "succeeded", + "installTester": "succeeded", + "startTests": "succeeded", + "endTests": "succeeded", + "deactivateTester": "succeeded", + "report": "succeeded" + }, + "tasks": { + "test": "succeeded" + }, + "log": "https://some.url:43/root/test-us-central-1/run/1" + } + ], + "url": "https://some.url:43/root/test-us-central-1" + }, "us-west-1": { "runs": [ { @@ -782,3 +891,4 @@ }, "devJobs": {} } + diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilterTest.java index 670413d2dd0..d5d635294b5 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilterTest.java @@ -117,8 +117,8 @@ public class AthenzRoleFilterTest { assertEquals(Set.of(Role.athenzTenantAdmin(TENANT), Role.tenantPipeline(TENANT, APPLICATION)), filter.roles(TENANT_ADMIN_AND_PIPELINE, APPLICATION_CONTEXT_PATH)); - // Users have the athenzUser under their instance - assertEquals(Set.of(Role.athenzUser(TENANT, APPLICATION, INSTANCE)), + // Users have nothing special under their instance + assertEquals(Set.of(Role.everyone()), filter.roles(USER, INSTANCE_CONTEXT_PATH)); // Unprivileged users are just members of the everyone role. diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java index 43313392cdb..a25a15f58ca 100644 --- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java +++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java @@ -132,7 +132,7 @@ public class DockerImpl implements Docker { if (timeoutSeconds.isPresent()) { if (!callback.awaitCompletion(timeoutSeconds.getAsLong(), TimeUnit.SECONDS)) throw new DockerExecTimeoutException(String.format( - "Command '%s' did not finish within %s seconds.", command[0], timeoutSeconds)); + "Command '%s' did not finish within %d seconds.", command[0], timeoutSeconds.getAsLong())); } else { // Wait for completion no timeout callback.awaitCompletion(); diff --git a/eval/CMakeLists.txt b/eval/CMakeLists.txt index b3d4585a176..35731ef5cfb 100644 --- a/eval/CMakeLists.txt +++ b/eval/CMakeLists.txt @@ -35,6 +35,7 @@ vespa_define_module( src/tests/tensor/dense_remove_dimension_optimizer src/tests/tensor/dense_replace_type_function src/tests/tensor/dense_tensor_create_function + src/tests/tensor/dense_tensor_peek_function src/tests/tensor/dense_xw_product_function src/tests/tensor/direct_dense_tensor_builder src/tests/tensor/direct_sparse_tensor_builder diff --git a/eval/src/tests/eval/function/function_test.cpp b/eval/src/tests/eval/function/function_test.cpp index fef0ed35668..5316217d549 100644 --- a/eval/src/tests/eval/function/function_test.cpp +++ b/eval/src/tests/eval/function/function_test.cpp @@ -168,6 +168,11 @@ TEST("require that strings are parsed and dumped correctly") { } } +TEST("require that strings with single quotes can be parsed") { + EXPECT_EQUAL(Function::parse("'foo'")->dump(), "\"foo\""); + EXPECT_EQUAL(Function::parse("'fo\\'o'")->dump(), "\"fo'o\""); +} + TEST("require that free arrays cannot be parsed") { verify_error("[1,2,3]", "[]...[missing value]...[[1,2,3]]"); } @@ -830,11 +835,13 @@ TEST("require that verbose tensor create can be parsed") { auto dense = Function::parse("tensor(x[3]):{{x:0}:1,{x:1}:2,{x:2}:3}"); auto sparse1 = Function::parse("tensor(x{}):{{x:a}:1,{x:b}:2,{x:c}:3}"); auto sparse2 = Function::parse("tensor(x{}):{{x:\"a\"}:1,{x:\"b\"}:2,{x:\"c\"}:3}"); + auto sparse3 = Function::parse("tensor(x{}):{{x:'a'}:1,{x:'b'}:2,{x:'c'}:3}"); auto mixed1 = Function::parse("tensor(x{},y[2]):{{x:a,y:0}:1,{x:a,y:1}:2}"); auto mixed2 = Function::parse("tensor(x{},y[2]):{{x:\"a\",y:0}:1,{x:\"a\",y:1}:2}"); EXPECT_EQUAL("tensor(x[3]):{{x:0}:1,{x:1}:2,{x:2}:3}", dense->dump()); EXPECT_EQUAL("tensor(x{}):{{x:\"a\"}:1,{x:\"b\"}:2,{x:\"c\"}:3}", sparse1->dump()); EXPECT_EQUAL("tensor(x{}):{{x:\"a\"}:1,{x:\"b\"}:2,{x:\"c\"}:3}", sparse2->dump()); + EXPECT_EQUAL("tensor(x{}):{{x:\"a\"}:1,{x:\"b\"}:2,{x:\"c\"}:3}", sparse3->dump()); EXPECT_EQUAL("tensor(x{},y[2]):{{x:\"a\",y:0}:1,{x:\"a\",y:1}:2}", mixed1->dump()); EXPECT_EQUAL("tensor(x{},y[2]):{{x:\"a\",y:0}:1,{x:\"a\",y:1}:2}", mixed2->dump()); } @@ -880,6 +887,8 @@ TEST("require that verbose tensor create detects non-numeric indexes for indexed TEST("require that verbose tensor create indexes cannot be quoted") { TEST_DO(verify_error("tensor(x[1]):{{x:\"1\"}:1}", "[tensor(x[1]):{{x:]...[expected number]...[\"1\"}:1}]")); + TEST_DO(verify_error("tensor(x[1]):{{x:'1'}:1}", + "[tensor(x[1]):{{x:]...[expected number]...['1'}:1}]")); } //----------------------------------------------------------------------------- @@ -888,11 +897,13 @@ TEST("require that convenient tensor create can be parsed") { auto dense = Function::parse("tensor(x[3]):[1,2,3]"); auto sparse1 = Function::parse("tensor(x{}):{a:1,b:2,c:3}"); auto sparse2 = Function::parse("tensor(x{}):{\"a\":1,\"b\":2,\"c\":3}"); + auto sparse3 = Function::parse("tensor(x{}):{'a':1,'b':2,'c':3}"); auto mixed1 = Function::parse("tensor(x{},y[2]):{a:[1,2]}"); auto mixed2 = Function::parse("tensor(x{},y[2]):{\"a\":[1,2]}"); EXPECT_EQUAL("tensor(x[3]):{{x:0}:1,{x:1}:2,{x:2}:3}", dense->dump()); EXPECT_EQUAL("tensor(x{}):{{x:\"a\"}:1,{x:\"b\"}:2,{x:\"c\"}:3}", sparse1->dump()); EXPECT_EQUAL("tensor(x{}):{{x:\"a\"}:1,{x:\"b\"}:2,{x:\"c\"}:3}", sparse2->dump()); + EXPECT_EQUAL("tensor(x{}):{{x:\"a\"}:1,{x:\"b\"}:2,{x:\"c\"}:3}", sparse3->dump()); EXPECT_EQUAL("tensor(x{},y[2]):{{x:\"a\",y:0}:1,{x:\"a\",y:1}:2}", mixed1->dump()); EXPECT_EQUAL("tensor(x{},y[2]):{{x:\"a\",y:0}:1,{x:\"a\",y:1}:2}", mixed2->dump()); } @@ -955,6 +966,7 @@ TEST("require that convenient tensor create detects under-specified cells") { TEST("require that tensor peek can be parsed") { TEST_DO(verify_parse("t{x:\"1\",y:\"foo\"}", "f(t)(t{x:\"1\",y:\"foo\"})")); + TEST_DO(verify_parse("t{x:'1',y:'foo'}", "f(t)(t{x:\"1\",y:\"foo\"})")); TEST_DO(verify_parse("t{x:1,y:foo}", "f(t)(t{x:\"1\",y:\"foo\"})")); } diff --git a/eval/src/tests/tensor/dense_tensor_peek_function/CMakeLists.txt b/eval/src/tests/tensor/dense_tensor_peek_function/CMakeLists.txt new file mode 100644 index 00000000000..a4d1bbf6fac --- /dev/null +++ b/eval/src/tests/tensor/dense_tensor_peek_function/CMakeLists.txt @@ -0,0 +1,8 @@ +# Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +vespa_add_executable(eval_dense_tensor_peek_function_test_app TEST + SOURCES + dense_tensor_peek_function_test.cpp + DEPENDS + vespaeval +) +vespa_add_test(NAME eval_dense_tensor_peek_function_test_app COMMAND eval_dense_tensor_peek_function_test_app) diff --git a/eval/src/tests/tensor/dense_tensor_peek_function/dense_tensor_peek_function_test.cpp b/eval/src/tests/tensor/dense_tensor_peek_function/dense_tensor_peek_function_test.cpp new file mode 100644 index 00000000000..a3afb710d1a --- /dev/null +++ b/eval/src/tests/tensor/dense_tensor_peek_function/dense_tensor_peek_function_test.cpp @@ -0,0 +1,85 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include <vespa/vespalib/testkit/test_kit.h> +#include <vespa/eval/eval/tensor_function.h> +#include <vespa/eval/eval/simple_tensor.h> +#include <vespa/eval/eval/simple_tensor_engine.h> +#include <vespa/eval/tensor/default_tensor_engine.h> +#include <vespa/eval/tensor/dense/dense_tensor_peek_function.h> +#include <vespa/eval/tensor/dense/dense_tensor.h> +#include <vespa/eval/eval/test/tensor_model.hpp> +#include <vespa/eval/eval/test/eval_fixture.h> + +#include <vespa/vespalib/util/stringfmt.h> +#include <vespa/vespalib/util/stash.h> + +using namespace vespalib; +using namespace vespalib::eval; +using namespace vespalib::eval::test; +using namespace vespalib::tensor; +using namespace vespalib::eval::tensor_function; + +const TensorEngine &prod_engine = DefaultTensorEngine::ref(); + +EvalFixture::ParamRepo make_params() { + return EvalFixture::ParamRepo() + .add("a", spec(1.0)) + .add("b", spec(2.0)) + .add("c", spec(3.0)) + .add("x3", spec(x(3), N())) + .add("x3f", spec(float_cells({x(3)}), N())) + .add("x3y2", spec({x(3),y(2)}, N())) + .add("x3y2f", spec(float_cells({x(3),y(2)}), N())) + .add("xm", spec(x({"1","2","3","-1","-2","-3"}), N())) + .add("xmy2", spec({x({"1","2","3"}), y(2)}, N())); +} +EvalFixture::ParamRepo param_repo = make_params(); + +void verify(const vespalib::string &expr, double expect, size_t expect_optimized_cnt, size_t expect_not_optimized_cnt) { + EvalFixture fixture(prod_engine, expr, param_repo, true); + auto expect_spec = TensorSpec("double").add({}, expect); + EXPECT_EQUAL(EvalFixture::ref(expr, param_repo), expect_spec); + EXPECT_EQUAL(fixture.result(), expect_spec); + auto info = fixture.find_all<DenseTensorPeekFunction>(); + EXPECT_EQUAL(info.size(), expect_optimized_cnt); + for (size_t i = 0; i < info.size(); ++i) { + EXPECT_TRUE(info[i]->result_is_mutable()); + } + EXPECT_EQUAL(fixture.find_all<Peek>().size(), expect_not_optimized_cnt); +} + +//----------------------------------------------------------------------------- + +TEST("require that tensor peek can be optimized for dense tensors") { + TEST_DO(verify("x3{x:0}", 1.0, 1, 0)); + TEST_DO(verify("x3{x:(a)}", 2.0, 1, 0)); + TEST_DO(verify("x3f{x:(c-1)}", 3.0, 1, 0)); + TEST_DO(verify("x3{x:(c+5)}", 0.0, 1, 0)); + TEST_DO(verify("x3{x:(a-2)}", 0.0, 1, 0)); + TEST_DO(verify("x3y2{x:(a),y:(a-1)}", 3.0, 1, 0)); + TEST_DO(verify("x3y2f{x:1,y:(a)}", 4.0, 1, 0)); + TEST_DO(verify("x3y2f{x:(a-1),y:(b)}", 0.0, 1, 0)); +} + +TEST("require that tensor peek is not optimized for sparse tensor") { + TEST_DO(verify("xm{x:1}", 1.0, 0, 1)); + TEST_DO(verify("xm{x:(c)}", 3.0, 0, 1)); + TEST_DO(verify("xm{x:(c+1)}", 0.0, 0, 1)); +} + +TEST("require that tensor peek is not optimized for mixed tensor") { + TEST_DO(verify("xmy2{x:3,y:1}", 6.0, 0, 1)); + TEST_DO(verify("xmy2{x:(c),y:(a)}", 6.0, 0, 1)); + TEST_DO(verify("xmy2{x:(a),y:(b)}", 0.0, 0, 1)); +} + +TEST("require that indexes are rounded to nearest integer") { + TEST_DO(verify("x3{x:(a-0.3)}", 2.0, 1, 0)); + TEST_DO(verify("x3{x:(a+0.3)}", 2.0, 1, 0)); + TEST_DO(verify("xm{x:(a-0.3)}", 1.0, 0, 1)); + TEST_DO(verify("xm{x:(a+0.3)}", 1.0, 0, 1)); + TEST_DO(verify("xm{x:(-a-0.3)}", 4.0, 0, 1)); + TEST_DO(verify("xm{x:(-a+0.3)}", 4.0, 0, 1)); +} + +TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/eval/src/vespa/eval/eval/function.cpp b/eval/src/vespa/eval/eval/function.cpp index 108380f52b7..3d6e66acf04 100644 --- a/eval/src/vespa/eval/eval/function.cpp +++ b/eval/src/vespa/eval/eval/function.cpp @@ -363,9 +363,9 @@ int unhex(char c) { return -1; } -void extract_quoted_string(ParseContext &ctx, vespalib::string &str) { - ctx.eat('"'); - while (!ctx.eos() && ctx.get() != '"') { +void extract_quoted_string(ParseContext &ctx, vespalib::string &str, char quote) { + ctx.eat(quote); + while (!ctx.eos() && (ctx.get() != quote)) { if (ctx.get() == '\\') { ctx.next(); if (ctx.get() == 'x') { @@ -380,6 +380,7 @@ void extract_quoted_string(ParseContext &ctx, vespalib::string &str) { } else { switch(ctx.get()) { case '"': str.push_back('"'); break; + case '\'': str.push_back('\''); break; case '\\': str.push_back('\\'); break; case 'f': str.push_back('\f'); break; case 'n': str.push_back('\n'); break; @@ -393,12 +394,12 @@ void extract_quoted_string(ParseContext &ctx, vespalib::string &str) { } ctx.next(); } - ctx.eat('"'); + ctx.eat(quote); } -void parse_string(ParseContext &ctx) { +void parse_string(ParseContext &ctx, char quote) { vespalib::string &str = ctx.scratch(); - extract_quoted_string(ctx, str); + extract_quoted_string(ctx, str, quote); ctx.push_expression(Node_UP(new nodes::String(str))); } @@ -485,7 +486,9 @@ vespalib::string get_label(ParseContext &ctx) { ctx.skip_spaces(); vespalib::string label; if (ctx.get() == '"') { - extract_quoted_string(ctx, label); + extract_quoted_string(ctx, label, '"'); + } else if (ctx.get() == '\'') { + extract_quoted_string(ctx, label, '\''); } else { while (!is_label_end(ctx.get())) { label.push_back(ctx.get()); @@ -945,7 +948,9 @@ void parse_value(ParseContext &ctx) { parse_expression(ctx); ctx.eat(')'); } else if (ctx.get() == '"') { - parse_string(ctx); + parse_string(ctx, '"'); + } else if (ctx.get() == '\'') { + parse_string(ctx, '\''); } else if (isdigit(ctx.get())) { parse_number(ctx); } else { diff --git a/eval/src/vespa/eval/eval/llvm/compile_cache.cpp b/eval/src/vespa/eval/eval/llvm/compile_cache.cpp index b486a750992..9a262f6dca5 100644 --- a/eval/src/vespa/eval/eval/llvm/compile_cache.cpp +++ b/eval/src/vespa/eval/eval/llvm/compile_cache.cpp @@ -2,6 +2,7 @@ #include "compile_cache.h" #include <vespa/eval/eval/key_gen.h> +#include <thread> namespace vespalib { namespace eval { @@ -50,7 +51,7 @@ CompileCache::Token::UP CompileCache::compile(const Function &function, PassParams pass_params) { Token::UP token; - CompileTask::UP task; + Executor::Task::UP task; vespalib::string key = gen_key(function, pass_params); { std::lock_guard<std::mutex> guard(_lock); @@ -71,7 +72,7 @@ CompileCache::compile(const Function &function, PassParams pass_params) } } if (task) { - task->run(); + std::thread([&task](){ task.get()->run(); }).join(); } return token; } diff --git a/eval/src/vespa/eval/eval/tensor_function.h b/eval/src/vespa/eval/eval/tensor_function.h index 7c7e8318305..c95ffd17bbe 100644 --- a/eval/src/vespa/eval/eval/tensor_function.h +++ b/eval/src/vespa/eval/eval/tensor_function.h @@ -84,6 +84,16 @@ struct TensorFunction **/ virtual void push_children(std::vector<Child::CREF> &children) const = 0; + std::vector<Child> copy_children() const { + std::vector<Child::CREF> child_refs; + std::vector<Child> children_copy; + push_children(child_refs); + for (const auto &child_ref: child_refs) { + children_copy.emplace_back(child_ref.get().get()); + } + return children_copy; + } + /** * Compile this node into a single instruction that can be run by * an interpreted function. Sub-expressions are compiled as diff --git a/eval/src/vespa/eval/tensor/default_tensor_engine.cpp b/eval/src/vespa/eval/tensor/default_tensor_engine.cpp index 18733b3c733..b310ad72fc1 100644 --- a/eval/src/vespa/eval/tensor/default_tensor_engine.cpp +++ b/eval/src/vespa/eval/tensor/default_tensor_engine.cpp @@ -17,6 +17,7 @@ #include "dense/dense_inplace_map_function.h" #include "dense/vector_from_doubles_function.h" #include "dense/dense_tensor_create_function.h" +#include "dense/dense_tensor_peek_function.h" #include <vespa/eval/eval/value.h> #include <vespa/eval/eval/tensor_spec.h> #include <vespa/eval/eval/tensor_spec.h> @@ -260,6 +261,7 @@ DefaultTensorEngine::optimize(const TensorFunction &expr, Stash &stash) const const Child &child = nodes.back(); child.set(VectorFromDoublesFunction::optimize(child.get(), stash)); child.set(DenseTensorCreateFunction::optimize(child.get(), stash)); + child.set(DenseTensorPeekFunction::optimize(child.get(), stash)); child.set(DenseDotProductFunction::optimize(child.get(), stash)); child.set(DenseXWProductFunction::optimize(child.get(), stash)); child.set(DenseFastRenameOptimizer::optimize(child.get(), stash)); diff --git a/eval/src/vespa/eval/tensor/dense/CMakeLists.txt b/eval/src/vespa/eval/tensor/dense/CMakeLists.txt index e071de3e925..2ad54d48ab3 100644 --- a/eval/src/vespa/eval/tensor/dense/CMakeLists.txt +++ b/eval/src/vespa/eval/tensor/dense/CMakeLists.txt @@ -14,6 +14,7 @@ vespa_add_library(eval_tensor_dense OBJECT dense_tensor_cells_iterator.cpp dense_tensor_create_function.cpp dense_tensor_modify.cpp + dense_tensor_peek_function.cpp dense_tensor_reduce.cpp dense_tensor_view.cpp dense_xw_product_function.cpp diff --git a/eval/src/vespa/eval/tensor/dense/dense_tensor_peek_function.cpp b/eval/src/vespa/eval/tensor/dense/dense_tensor_peek_function.cpp new file mode 100644 index 00000000000..f9d15a377e9 --- /dev/null +++ b/eval/src/vespa/eval/tensor/dense/dense_tensor_peek_function.cpp @@ -0,0 +1,105 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include "dense_tensor_peek_function.h" +#include "dense_tensor_view.h" +#include <vespa/eval/eval/operation.h> +#include <vespa/eval/eval/value.h> +#include <vespa/eval/tensor/tensor.h> + +namespace vespalib::tensor { + +using eval::Value; +using eval::DoubleValue; +using eval::ValueType; +using eval::TensorSpec; +using eval::TensorFunction; +using Child = eval::TensorFunction::Child; +using eval::as; +using namespace eval::tensor_function; +using namespace eval::operation; + +namespace { + +template <typename CT> +void my_tensor_peek_op(eval::InterpretedFunction::State &state, uint64_t param) { + const auto *spec = (const std::vector<std::pair<int64_t,size_t>> *)(param); + size_t idx = 0; + size_t factor = 1; + bool valid = true; + for (const auto &dim: *spec) { + if (dim.first >= 0) { + idx += (dim.first * factor); + } else { + size_t dim_idx(round(state.peek(0).as_double())); + state.stack.pop_back(); + valid &= (dim_idx < dim.second); + idx += (dim_idx * factor); + } + factor *= dim.second; + } + auto cells = DenseTensorView::typify_cells<CT>(state.peek(0)); + state.stack.pop_back(); + const Value &result = state.stash.create<DoubleValue>(valid ? cells[idx] : 0.0); + state.stack.push_back(result); +} + +struct MyTensorPeekOp { + template <typename CT> + static auto get_fun() { return my_tensor_peek_op<CT>; } +}; + +} // namespace vespalib::tensor::<unnamed> + +DenseTensorPeekFunction::DenseTensorPeekFunction(std::vector<Child> children, + const std::vector<std::pair<int64_t,size_t>> &spec) + : TensorFunction(), + _children(std::move(children)), + _spec(spec) +{ +} + +DenseTensorPeekFunction::~DenseTensorPeekFunction() = default; + +void +DenseTensorPeekFunction::push_children(std::vector<Child::CREF> &target) const +{ + for (const Child &c: _children) { + target.push_back(c); + } +} + +eval::InterpretedFunction::Instruction +DenseTensorPeekFunction::compile_self(Stash &) const +{ + static_assert(sizeof(uint64_t) == sizeof(&_spec)); + auto op = select_1<MyTensorPeekOp>(_children[0].get().result_type().cell_type()); + return eval::InterpretedFunction::Instruction(op, (uint64_t)&_spec); +} + +const TensorFunction & +DenseTensorPeekFunction::optimize(const eval::TensorFunction &expr, Stash &stash) +{ + if (auto peek = as<Peek>(expr)) { + const ValueType &peek_type = peek->param_type(); + if (expr.result_type().is_double() && peek_type.is_dense()) { + std::vector<std::pair<int64_t,size_t>> spec; + assert(peek_type.dimensions().size() == peek->spec().size()); + for (auto dim = peek_type.dimensions().rbegin(); dim != peek_type.dimensions().rend(); ++dim) { + auto dim_spec = peek->spec().find(dim->name); + assert(dim_spec != peek->spec().end()); + if (std::holds_alternative<TensorSpec::Label>(dim_spec->second)) { + const auto &label = std::get<TensorSpec::Label>(dim_spec->second); + assert(label.is_indexed()); + spec.emplace_back(label.index, dim->size); + } else { + assert(std::holds_alternative<TensorFunction::Child>(dim_spec->second)); + spec.emplace_back(-1, dim->size); + } + } + return stash.create<DenseTensorPeekFunction>(peek->copy_children(), spec); + } + } + return expr; +} + +} // namespace vespalib::tensor diff --git a/eval/src/vespa/eval/tensor/dense/dense_tensor_peek_function.h b/eval/src/vespa/eval/tensor/dense/dense_tensor_peek_function.h new file mode 100644 index 00000000000..4924a0de329 --- /dev/null +++ b/eval/src/vespa/eval/tensor/dense/dense_tensor_peek_function.h @@ -0,0 +1,34 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#pragma once + +#include <vespa/eval/eval/tensor_function.h> + +namespace vespalib::tensor { + +/** + * Tensor function for looking at the cell value of a dense tensor. + */ +class DenseTensorPeekFunction : public eval::TensorFunction +{ +private: + // first child is the tensor we want to peek + // other children are dimension index expressions + // (index expressions are sorted by normalized dimension order) + std::vector<Child> _children; + + // index and size of all dimensions in reverse order + // when index is -1, use result of next child expression + // (note that child expression order is inverted by the stack) + std::vector<std::pair<int64_t,size_t>> _spec; +public: + DenseTensorPeekFunction(std::vector<Child> children, const std::vector<std::pair<int64_t,size_t>> &spec); + ~DenseTensorPeekFunction(); + const eval::ValueType &result_type() const override { return eval::DoubleValue::double_type(); } + void push_children(std::vector<Child::CREF> &children) const override; + eval::InterpretedFunction::Instruction compile_self(Stash &stash) const override; + bool result_is_mutable() const override { return true; } + static const eval::TensorFunction &optimize(const eval::TensorFunction &expr, Stash &stash); +}; + +} // namespace vespalib::tensor diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/HttpHandlerBase.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/HttpHandlerBase.java index d582abdba57..a36126a3a24 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/HttpHandlerBase.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/HttpHandlerBase.java @@ -4,9 +4,6 @@ package ai.vespa.metricsproxy.http; -import ai.vespa.metricsproxy.core.MetricsConsumers; -import ai.vespa.metricsproxy.core.MetricsManager; -import ai.vespa.metricsproxy.service.VespaServices; import com.yahoo.container.jdisc.HttpRequest; import com.yahoo.container.jdisc.HttpResponse; import com.yahoo.container.jdisc.ThreadedHttpRequestHandler; @@ -32,14 +29,8 @@ import static java.util.logging.Level.WARNING; */ public abstract class HttpHandlerBase extends ThreadedHttpRequestHandler { - protected final ValuesFetcher valuesFetcher; - - protected HttpHandlerBase(Executor executor, - MetricsManager metricsManager, - VespaServices vespaServices, - MetricsConsumers metricsConsumers) { + protected HttpHandlerBase(Executor executor) { super(executor); - valuesFetcher = new ValuesFetcher(metricsManager, vespaServices, metricsConsumers); } protected abstract Optional<HttpResponse> doHandle(URI requestUri, Path apiPath, String consumer); diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/MetricsHandler.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/MetricsHandler.java index ac89faad8d8..63a79d93b07 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/MetricsHandler.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/MetricsHandler.java @@ -32,12 +32,15 @@ public class MetricsHandler extends HttpHandlerBase { public static final String V1_PATH = "/metrics/v1"; static final String VALUES_PATH = V1_PATH + "/values"; + private final ValuesFetcher valuesFetcher; + @Inject public MetricsHandler(Executor executor, MetricsManager metricsManager, VespaServices vespaServices, MetricsConsumers metricsConsumers) { - super(executor, metricsManager, vespaServices, metricsConsumers); + super(executor); + valuesFetcher = new ValuesFetcher(metricsManager, vespaServices, metricsConsumers); } @Override diff --git a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/prometheus/PrometheusHandler.java b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/prometheus/PrometheusHandler.java index aeea15ee87e..03b349c293b 100644 --- a/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/prometheus/PrometheusHandler.java +++ b/metrics-proxy/src/main/java/ai/vespa/metricsproxy/http/prometheus/PrometheusHandler.java @@ -8,6 +8,7 @@ import ai.vespa.metricsproxy.core.MetricsConsumers; import ai.vespa.metricsproxy.core.MetricsManager; import ai.vespa.metricsproxy.http.HttpHandlerBase; import ai.vespa.metricsproxy.http.TextResponse; +import ai.vespa.metricsproxy.http.ValuesFetcher; import ai.vespa.metricsproxy.metric.model.MetricsPacket; import ai.vespa.metricsproxy.service.VespaServices; import com.google.inject.Inject; @@ -32,12 +33,15 @@ public class PrometheusHandler extends HttpHandlerBase { public static final String V1_PATH = "/prometheus/v1"; static final String VALUES_PATH = V1_PATH + "/values"; + private final ValuesFetcher valuesFetcher; + @Inject public PrometheusHandler(Executor executor, MetricsManager metricsManager, VespaServices vespaServices, MetricsConsumers metricsConsumers) { - super(executor, metricsManager, vespaServices, metricsConsumers); + super(executor); + valuesFetcher = new ValuesFetcher(metricsManager, vespaServices, metricsConsumers); } @Override 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 4c25796907a..38011d089d4 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 @@ -3,6 +3,7 @@ package ai.vespa.metricsproxy.http.yamas; import ai.vespa.metricsproxy.core.MetricsConsumers; import ai.vespa.metricsproxy.core.MetricsManager; +import ai.vespa.metricsproxy.http.ValuesFetcher; import ai.vespa.metricsproxy.node.NodeMetricGatherer; import ai.vespa.metricsproxy.http.ErrorResponse; import ai.vespa.metricsproxy.http.HttpHandlerBase; @@ -34,6 +35,7 @@ public class YamasHandler extends HttpHandlerBase { public static final String V1_PATH = "/yamas/v1"; private static final String VALUES_PATH = V1_PATH + "/values"; + private final ValuesFetcher valuesFetcher; private final NodeMetricGatherer nodeMetricGatherer; @Inject @@ -43,7 +45,8 @@ public class YamasHandler extends HttpHandlerBase { MetricsConsumers metricsConsumers, ApplicationDimensions applicationDimensions, NodeDimensions nodeDimensions) { - super(executor, metricsManager, vespaServices, metricsConsumers); + super(executor); + valuesFetcher = new ValuesFetcher(metricsManager, vespaServices, metricsConsumers); this.nodeMetricGatherer = new NodeMetricGatherer(metricsManager, vespaServices, applicationDimensions, nodeDimensions); } diff --git a/model-integration/src/main/java/ai/vespa/rankingexpression/importer/onnx/GraphImporter.java b/model-integration/src/main/java/ai/vespa/rankingexpression/importer/onnx/GraphImporter.java index 55f5d979ea8..d42338deaf8 100644 --- a/model-integration/src/main/java/ai/vespa/rankingexpression/importer/onnx/GraphImporter.java +++ b/model-integration/src/main/java/ai/vespa/rankingexpression/importer/onnx/GraphImporter.java @@ -27,8 +27,10 @@ import com.yahoo.searchlib.rankingexpression.evaluation.Value; import com.yahoo.tensor.functions.ScalarFunctions; import onnx.Onnx; +import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; /** @@ -123,6 +125,7 @@ class GraphImporter { IntermediateGraph intermediateGraph = new IntermediateGraph(modelName); importOperations(onnxGraph, intermediateGraph); + verifyNoWarnings(intermediateGraph); verifyOutputTypes(onnxGraph, intermediateGraph); return intermediateGraph; @@ -234,6 +237,16 @@ class GraphImporter { .collect(Collectors.toList()); } + private static void verifyNoWarnings(IntermediateGraph intermediateGraph) { + for (java.util.Map.Entry<String, String> output : intermediateGraph.outputs(intermediateGraph.defaultSignature()).entrySet()) { + IntermediateOperation operation = intermediateGraph.get(output.getValue()); + Set<String> warnings = getWarnings(operation); + if (warnings.size() > 0) { + throw new IllegalArgumentException("Could not import " + intermediateGraph.name() + ": " + String.join("\n", warnings)); + } + } + } + private static void verifyOutputTypes(Onnx.GraphProto onnxGraph, IntermediateGraph intermediateGraph) { for (java.util.Map.Entry<String, String> output : intermediateGraph.outputs(intermediateGraph.defaultSignature()).entrySet()) { IntermediateOperation operation = intermediateGraph.get(output.getValue()); @@ -284,4 +297,12 @@ class GraphImporter { "Either no explicit name given or no single output name."); } + private static Set<String> getWarnings(IntermediateOperation op) { + Set<String> warnings = new HashSet<>(op.warnings()); + for (IntermediateOperation input : op.inputs()) { + warnings.addAll(getWarnings(input)); + } + return warnings; + } + } diff --git a/model-integration/src/main/java/ai/vespa/rankingexpression/importer/operations/IntermediateOperation.java b/model-integration/src/main/java/ai/vespa/rankingexpression/importer/operations/IntermediateOperation.java index efd6f9d3339..724b5c6b3ac 100644 --- a/model-integration/src/main/java/ai/vespa/rankingexpression/importer/operations/IntermediateOperation.java +++ b/model-integration/src/main/java/ai/vespa/rankingexpression/importer/operations/IntermediateOperation.java @@ -153,7 +153,7 @@ public abstract class IntermediateOperation { /** Retrieve the valid Vespa name of this node */ public String vespaName() { return vespaName(name); } - public String vespaName(String name) { return name != null ? namePartOf(name).replace('/', '_') : null; } + public String vespaName(String name) { return name != null ? namePartOf(name).replace('/', '_').replace('.', '_') : null; } /** Retrieve the valid Vespa name of this node if it is a ranking expression function */ public String rankingExpressionFunctionName() { diff --git a/model-integration/src/test/java/ai/vespa/rankingexpression/importer/onnx/Tf2OnnxImportTestCase.java b/model-integration/src/test/java/ai/vespa/rankingexpression/importer/onnx/Tf2OnnxImportTestCase.java index 4250fee4d20..c7245fe53e8 100644 --- a/model-integration/src/test/java/ai/vespa/rankingexpression/importer/onnx/Tf2OnnxImportTestCase.java +++ b/model-integration/src/test/java/ai/vespa/rankingexpression/importer/onnx/Tf2OnnxImportTestCase.java @@ -39,7 +39,7 @@ public class Tf2OnnxImportTestCase extends TestableModel { @Ignore public void testOnnxConversionAndImport() { Report report = new Report(); - for (int i = 11; i < 12; ++i) { + for (int i = 1; i < 12; ++i) { testModelsWithOpset(report, i); } System.out.println(report); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java index dddb724e3d6..ca6a56413c3 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java @@ -211,7 +211,7 @@ public class NodeAdminImpl implements NodeAdmin { private static NodeAgentWithScheduler create(Clock clock, NodeAgentFactory nodeAgentFactory, NodeAgentContext context) { NodeAgentContextManager contextManager = new NodeAgentContextManager(clock, context); - NodeAgent nodeAgent = nodeAgentFactory.create(contextManager); + NodeAgent nodeAgent = nodeAgentFactory.create(contextManager, context); return new NodeAgentWithScheduler(nodeAgent, contextManager); } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentFactory.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentFactory.java index bd13b7eb094..6ad68d9c4e9 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentFactory.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentFactory.java @@ -6,5 +6,5 @@ package com.yahoo.vespa.hosted.node.admin.nodeagent; */ @FunctionalInterface public interface NodeAgentFactory { - NodeAgent create(NodeAgentContextSupplier contextSupplier); + NodeAgent create(NodeAgentContextSupplier contextSupplier, NodeAgentContext nodeAgentContext); } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java index 70b406b3056..8101acb4b3e 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java @@ -92,7 +92,7 @@ public class DockerTester implements AutoCloseable { DockerOperations dockerOperations = new DockerOperationsImpl(docker, processExecuter, ipAddresses); Metrics metrics = new Metrics(); - NodeAgentFactory nodeAgentFactory = contextSupplier -> new NodeAgentImpl( + NodeAgentFactory nodeAgentFactory = (contextSupplier, nodeContext) -> new NodeAgentImpl( contextSupplier, nodeRepository, orchestrator, dockerOperations, storageMaintainer, flagSource, Optional.empty(), Optional.empty(), Optional.empty()); nodeAdmin = new NodeAdminImpl(nodeAgentFactory, metrics, Clock.systemUTC(), Duration.ofMillis(10), Duration.ZERO); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java index 610c6ff999e..4ff0fee2eb7 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java @@ -298,10 +298,10 @@ public final class Node { /** Returns a copy of this node with the current OS version set to the given version at the given instant */ public Node withCurrentOsVersion(Version version, Instant instant) { - var newStatus = status.withOsVersion(version); + var newStatus = status.withOsVersion(status.osVersion().withCurrent(Optional.of(version))); var newHistory = history(); // Only update history if version has changed - if (status.osVersion().isEmpty() || !status.osVersion().get().equals(version)) { + if (status.osVersion().current().isEmpty() || !status.osVersion().current().get().equals(version)) { newHistory = history.with(new History.Event(History.Event.Type.osUpgraded, Agent.system, instant)); } return this.with(newStatus).with(newHistory); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java index 4ff782eaf89..b5068892527 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java @@ -1,7 +1,7 @@ // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.provision; -import com.google.common.collect.ImmutableList; +import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.NodeResources; @@ -13,7 +13,6 @@ import java.util.EnumSet; import java.util.Iterator; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -21,7 +20,7 @@ import java.util.stream.Stream; import static java.util.stream.Collectors.collectingAndThen; /** - * A filterable node list + * A filterable node list. The result of a filter operation is immutable. * * @author bratseth * @author mpolden @@ -29,13 +28,20 @@ import static java.util.stream.Collectors.collectingAndThen; public class NodeList implements Iterable<Node> { private final List<Node> nodes; + private final boolean negate; public NodeList(List<Node> nodes) { - this(nodes, true); + this(nodes, true, false); } - private NodeList(List<Node> nodes, boolean copy) { - this.nodes = copy ? ImmutableList.copyOf(nodes) : Collections.unmodifiableList(nodes); + private NodeList(List<Node> nodes, boolean copy, boolean negate) { + this.nodes = copy ? List.copyOf(nodes) : Collections.unmodifiableList(nodes); + this.negate = negate; + } + + /** Invert the next filter operation. All other methods that return a {@link NodeList} clears the negation. */ + public NodeList not() { + return new NodeList(nodes, false, true); } /** Returns the subset of nodes which are retired */ @@ -43,27 +49,14 @@ public class NodeList implements Iterable<Node> { return filter(node -> node.allocation().get().membership().retired()); } - /** Returns the subset of nodes which are not retired */ - public NodeList nonretired() { - return filter(node -> ! node.allocation().get().membership().retired()); - } - /** Returns the subset of nodes having exactly the given resources */ public NodeList resources(NodeResources resources) { return filter(node -> node.flavor().resources().equals(resources)); } - /** Returns the subset of nodes not having exactly the given resources */ - public NodeList notResources(NodeResources resources) { return filter(node -> ! node.flavor().resources().equals(resources)); } - /** Returns the subset of nodes of the given flavor */ public NodeList flavor(String flavor) { return filter(node -> node.flavor().name().equals(flavor)); } - /** Returns the subset of nodes which does not have the given flavor */ - public NodeList notFlavor(String flavor) { - return filter(node -> ! node.flavor().name().equals(flavor)); - } - /** Returns the subset of nodes assigned to the given cluster type */ public NodeList type(ClusterSpec.Type type) { return filter(node -> node.allocation().isPresent() && node.allocation().get().membership().cluster().type().equals(type)); @@ -76,6 +69,16 @@ public class NodeList implements Iterable<Node> { !node.status().vespaVersion().get().equals(node.allocation().get().membership().cluster().vespaVersion())); } + /** Returns the subset of nodes that are currently changing their OS version */ + public NodeList changingOsVersion() { + return filter(node -> node.status().osVersion().changing()); + } + + /** Returns the subset of nodes that are currently on the given OS version */ + public NodeList onOsVersion(Version version) { + return filter(node -> node.status().osVersion().matches(version)); + } + /** Returns the subset of nodes assigned to the given cluster */ public NodeList cluster(ClusterSpec.Id cluster) { return filter(node -> node.allocation().isPresent() && node.allocation().get().membership().cluster().id().equals(cluster)); @@ -133,6 +136,13 @@ public class NodeList implements Iterable<Node> { .findFirst()); } + /** Returns the first n nodes in this */ + public NodeList first(int n) { + n = Math.min(n, nodes.size()); + return wrap(nodes.subList(negate ? n : 0, + negate ? nodes.size() : n)); + } + public int size() { return nodes.size(); } /** Returns the immutable list of nodes in this */ @@ -142,7 +152,8 @@ public class NodeList implements Iterable<Node> { public Stream<Node> stream() { return asList().stream(); } public NodeList filter(Predicate<Node> predicate) { - return nodes.stream().filter(predicate).collect(collectingAndThen(Collectors.toList(), NodeList::wrap)); + return nodes.stream().filter(negate ? predicate.negate() : predicate) + .collect(collectingAndThen(Collectors.toList(), NodeList::wrap)); } @Override @@ -150,7 +161,9 @@ public class NodeList implements Iterable<Node> { return nodes.iterator(); } + /** Create a new list containing the given nodes, without copying */ private static NodeList wrap(List<Node> nodes) { - return new NodeList(nodes, false); + return new NodeList(nodes, false, false); } + } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index 072f2e765f4..682d1419a5c 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.provision; import com.google.inject.Inject; import com.yahoo.collections.ListMap; import com.yahoo.component.AbstractComponent; +import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.DockerImage; import com.yahoo.config.provision.Flavor; @@ -112,7 +113,7 @@ public class NodeRepository extends AbstractComponent { this.clock = clock; this.flavors = flavors; this.nameResolver = nameResolver; - this.osVersions = new OsVersions(db); + this.osVersions = new OsVersions(this); this.infrastructureVersions = new InfrastructureVersions(db); this.firmwareChecks = new FirmwareChecks(db, clock); this.dockerImages = new DockerImages(db, dockerImage); @@ -192,7 +193,7 @@ public class NodeRepository extends AbstractComponent { /** Returns a filterable list of all load balancers in this repository */ public LoadBalancerList loadBalancers() { - return new LoadBalancerList(database().readLoadBalancers().values()); + return LoadBalancerList.copyOf(database().readLoadBalancers().values()); } public List<Node> getNodes(ApplicationId id, Node.State ... inState) { return db.getNodes(id, inState); } @@ -643,7 +644,8 @@ public class NodeRepository extends AbstractComponent { /** * Increases the restart generation of the active nodes matching the filter. - * Returns the nodes in their new state. + * + * @return the nodes in their new state. */ public List<Node> restart(NodeFilter filter) { return performOn(StateFilter.from(Node.State.active, filter), (node, lock) -> write(node.withRestart(node.allocation().get().restartGeneration().withIncreasedWanted()), lock)); @@ -651,13 +653,25 @@ public class NodeRepository extends AbstractComponent { /** * Increases the reboot generation of the nodes matching the filter. - * Returns the nodes in their new state. + * @return the nodes in their new state. */ public List<Node> reboot(NodeFilter filter) { return performOn(filter, (node, lock) -> write(node.withReboot(node.status().reboot().withIncreasedWanted()), lock)); } /** + * Set target OS version of all nodes matching given filter. + * + * @return the nodes in their new state. + */ + public List<Node> upgradeOs(NodeFilter filter, Optional<Version> version) { + return performOn(filter, (node, lock) -> { + var newStatus = node.status().withOsVersion(node.status().osVersion().withWanted(version)); + return write(node.with(newStatus), lock); + }); + } + + /** * Writes this node after it has changed some internal state but NOT changed its state field. * This does NOT lock the node repository implicitly, but callers are expected to already hold the lock. * diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java index c1b4fe9b0f2..1e60b689489 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java @@ -56,6 +56,11 @@ public class LoadBalancerInstance { return reals; } + /** Returns a copy of this with reals set to given reals */ + public LoadBalancerInstance withReals(Set<Real> reals) { + return new LoadBalancerInstance(hostname, dnsZone, ports, networks, reals); + } + private static Set<Integer> requirePorts(Set<Integer> ports) { Objects.requireNonNull(ports, "ports must be non-null"); if (ports.isEmpty()) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java index 7fd50bf0930..014d3df8d9a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerList.java @@ -20,7 +20,7 @@ public class LoadBalancerList implements Iterable<LoadBalancer> { private final List<LoadBalancer> loadBalancers; - public LoadBalancerList(Collection<LoadBalancer> loadBalancers) { + private LoadBalancerList(Collection<LoadBalancer> loadBalancers) { this.loadBalancers = List.copyOf(Objects.requireNonNull(loadBalancers, "loadBalancers must be non-null")); } @@ -47,6 +47,10 @@ public class LoadBalancerList implements Iterable<LoadBalancer> { return new LoadBalancerList(stream.collect(Collectors.toUnmodifiableList())); } + public static LoadBalancerList copyOf(Collection<LoadBalancer> loadBalancers) { + return new LoadBalancerList(loadBalancers); + } + @Override public Iterator<LoadBalancer> iterator() { return loadBalancers.iterator(); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/Real.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/Real.java index 3c237a9a8a0..56a2e8aef8b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/Real.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/Real.java @@ -13,7 +13,7 @@ import java.util.Objects; */ public class Real implements Comparable<Real> { - private static final int defaultPort = 4443; + public static final int defaultPort = 4443; private final HostName hostname; private final String ipAddress; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java index a8268fe7237..bcb0c901f14 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.provision.maintenance; import com.yahoo.log.LogLevel; +import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.lb.LoadBalancer; import com.yahoo.vespa.hosted.provision.lb.LoadBalancer.State; @@ -11,6 +12,7 @@ import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient; import java.time.Duration; import java.util.ArrayList; +import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; import java.util.stream.Collectors; @@ -44,8 +46,10 @@ public class LoadBalancerExpirer extends Maintainer { protected void maintain() { expireReserved(); removeInactive(); + pruneReals(); } + /** Move reserved load balancer that have expired to inactive */ private void expireReserved() { try (var lock = db.lockLoadBalancers()) { var now = nodeRepository().clock().instant(); @@ -57,8 +61,9 @@ public class LoadBalancerExpirer extends Maintainer { } } + /** Deprovision inactive load balancers that have expired */ private void removeInactive() { - List<LoadBalancerId> failed = new ArrayList<>(); + var failed = new ArrayList<LoadBalancerId>(); Exception lastException = null; try (var lock = db.lockLoadBalancers()) { var now = nodeRepository().clock().instant(); @@ -67,9 +72,7 @@ public class LoadBalancerExpirer extends Maintainer { .in(State.inactive) .changedBefore(expirationTime); for (var lb : expired) { - if (hasNodes(lb.id())) { // Defer removal if there are still nodes allocated - continue; - } + if (!allocatedNodes(lb.id()).isEmpty()) continue; // Defer removal if there are still nodes allocated to application try { service.remove(lb.id().application(), lb.id().cluster()); db.removeLoadBalancer(lb.id()); @@ -90,8 +93,39 @@ public class LoadBalancerExpirer extends Maintainer { } } - private boolean hasNodes(LoadBalancerId loadBalancer) { - return nodeRepository().list().owner(loadBalancer.application()).cluster(loadBalancer.cluster()).size() > 0; + /** Remove reals from inactive load balancers */ + private void pruneReals() { + var failed = new ArrayList<LoadBalancerId>(); + Exception lastException = null; + try (var lock = db.lockLoadBalancers()) { + var deactivated = nodeRepository().loadBalancers().in(State.inactive); + for (var lb : deactivated) { + var allocatedNodes = allocatedNodes(lb.id()).stream().map(Node::hostname).collect(Collectors.toSet()); + var reals = new LinkedHashSet<>(lb.instance().reals()); + // Remove any real no longer allocated to this application + reals.removeIf(real -> !allocatedNodes.contains(real.hostname().value())); + try { + service.create(lb.id().application(), lb.id().cluster(), reals, true); + db.writeLoadBalancer(lb.with(lb.instance().withReals(reals))); + } catch (Exception e) { + failed.add(lb.id()); + lastException = e; + } + } + } + if (!failed.isEmpty()) { + log.log(LogLevel.WARNING, String.format("Failed to remove reals from %d load balancers: %s, retrying in %s", + failed.size(), + failed.stream() + .map(LoadBalancerId::serializedForm) + .collect(Collectors.joining(", ")), + interval()), + lastException); + } + } + + private List<Node> allocatedNodes(LoadBalancerId loadBalancer) { + return nodeRepository().list().owner(loadBalancer.application()).cluster(loadBalancer.cluster()).asList(); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java index 90daa0ad279..10aff833584 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java @@ -164,7 +164,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent { metricsInterval = Duration.ofMinutes(1); infrastructureProvisionInterval = Duration.ofMinutes(1); throttlePolicy = NodeFailer.ThrottlePolicy.hosted; - loadBalancerExpirerInterval = Duration.ofMinutes(10); + loadBalancerExpirerInterval = Duration.ofMinutes(5); reservationExpiry = Duration.ofMinutes(20); // Need to be long enough for deployment to be finished for all config model versions dynamicProvisionerInterval = Duration.ofMinutes(5); osUpgradeActivatorInterval = zone.system().isCd() ? Duration.ofSeconds(30) : Duration.ofMinutes(5); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/OsVersion.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/OsVersion.java new file mode 100644 index 00000000000..b3c265124db --- /dev/null +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/OsVersion.java @@ -0,0 +1,83 @@ +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.provision.node; + +import com.yahoo.component.Version; + +import java.util.Objects; +import java.util.Optional; + +/** + * The OS version of a node. This contains the current and wanted OS version and is immutable. + * + * @author mpolden + */ +public class OsVersion { + + public static final OsVersion EMPTY = new OsVersion(Optional.empty(), Optional.empty()); + + private final Optional<Version> current; + private final Optional<Version> wanted; + + public OsVersion(Optional<Version> current, Optional<Version> wanted) { + this.current = requireNonEmpty(current); + this.wanted = requireNonEmpty(wanted); + } + + /** The version this node is currently running, if any */ + public Optional<Version> current() { + return current; + } + + /** The version this node should upgrade to, if any */ + public Optional<Version> wanted() { + return wanted; + } + + /** Returns whether this node is currently changing its version */ + public boolean changing() { + return !current.equals(wanted); + } + + /** Returns whether current version matches given version */ + public boolean matches(Version version) { + return current.isPresent() && current.get().equals(version); + } + + /** Returns a copy of this with current version set to given version */ + public OsVersion withCurrent(Optional<Version> version) { + return new OsVersion(version, wanted); + } + + /** Returns a copy of this with wanted version set to given version */ + public OsVersion withWanted(Optional<Version> version) { + return new OsVersion(current, version); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + OsVersion osVersion = (OsVersion) o; + return current.equals(osVersion.current) && + wanted.equals(osVersion.wanted); + } + + @Override + public int hashCode() { + return Objects.hash(current, wanted); + } + + @Override + public String toString() { + return "OS version " + current.map(Version::toFullString).orElse("<unset>") + " [wanted: " + + wanted.map(Version::toFullString).orElse("<unset>") + "]"; + } + + private static Optional<Version> requireNonEmpty(Optional<Version> version) { + Objects.requireNonNull(version, "version must be non-null"); + if (version.isEmpty()) return version; + if (version.get().isEmpty()) throw new IllegalArgumentException("version must be non-empty"); + return version; + } + +} diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java index 5c86b40395d..15f3c481fe3 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java @@ -4,17 +4,15 @@ package com.yahoo.vespa.hosted.provision.node; import com.yahoo.component.Version; import com.yahoo.config.provision.DockerImage; -import javax.annotation.concurrent.Immutable; import java.time.Instant; import java.util.Objects; import java.util.Optional; /** - * Information about current status of a node + * Information about current status of a node. This is immutable. * * @author bratseth */ -@Immutable public class Status { private final Generation reboot; @@ -23,7 +21,7 @@ public class Status { private final int failCount; private final boolean wantToRetire; private final boolean wantToDeprovision; - private final Optional<Version> osVersion; + private final OsVersion osVersion; private final Optional<Instant> firmwareVerifiedAt; public Status(Generation generation, @@ -32,7 +30,7 @@ public class Status { int failCount, boolean wantToRetire, boolean wantToDeprovision, - Optional<Version> osVersion, + OsVersion osVersion, Optional<Instant> firmwareVerifiedAt) { this.reboot = Objects.requireNonNull(generation, "Generation must be non-null"); this.vespaVersion = Objects.requireNonNull(vespaVersion, "Vespa version must be non-null").filter(v -> !Version.emptyVersion.equals(v)); @@ -96,13 +94,13 @@ public class Status { return wantToDeprovision; } - /** Returns a copy of this with the current OS version set to version */ - public Status withOsVersion(Version version) { - return new Status(reboot, vespaVersion, dockerImage, failCount, wantToRetire, wantToDeprovision, Optional.of(version), firmwareVerifiedAt); + /** Returns a copy of this with the OS version set to given version */ + public Status withOsVersion(OsVersion version) { + return new Status(reboot, vespaVersion, dockerImage, failCount, wantToRetire, wantToDeprovision, version, firmwareVerifiedAt); } - /** Returns the current OS version of this node, if any */ - public Optional<Version> osVersion() { + /** Returns the OS version of this node */ + public OsVersion osVersion() { return osVersion; } @@ -119,6 +117,7 @@ public class Status { /** Returns the initial status of a newly provisioned node */ public static Status initial() { return new Status(Generation.initial(), Optional.empty(), Optional.empty(), 0, false, - false, Optional.empty(), Optional.empty()); + false, OsVersion.EMPTY, Optional.empty()); } + } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/filter/NodeOsVersionFilter.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/filter/NodeOsVersionFilter.java index f7083a6398f..e2718cf8b68 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/filter/NodeOsVersionFilter.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/filter/NodeOsVersionFilter.java @@ -22,7 +22,7 @@ public class NodeOsVersionFilter extends NodeFilter { @Override public boolean matches(Node node) { - if (!version.isEmpty() && !node.status().osVersion().filter(v -> v.equals(version)).isPresent()) { + if (!version.isEmpty() && !node.status().osVersion().matches(version)) { return false; } return nextMatches(node); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersion.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersion.java deleted file mode 100644 index 8719a80e578..00000000000 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersion.java +++ /dev/null @@ -1,58 +0,0 @@ -// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.provision.os; - -import com.yahoo.component.Version; - -import java.util.Objects; - -/** - * An OS version and it's active status. - * - * @author mpolden - */ -public class OsVersion { - - private final Version version; - private final boolean active; - - public OsVersion(Version version, boolean active) { - this.version = requireNonEmpty(version); - this.active = active; - } - - /** The OS version number */ - public Version version() { - return version; - } - - /** Returns whether this is currently active and should be acted on by nodes */ - public boolean active() { - return active; - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - OsVersion osVersion = (OsVersion) o; - return active == osVersion.active && - version.equals(osVersion.version); - } - - @Override - public int hashCode() { - return Objects.hash(version, active); - } - - @Override - public String toString() { - return "OS version " + version + " [active: " + active + "]"; - } - - private static Version requireNonEmpty(Version version) { - Objects.requireNonNull(version, "version must be non-null"); - if (version.isEmpty()) throw new IllegalArgumentException("version must be non-empty"); - return version; - } - -} diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java index a2d84bc7379..106595fbd47 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java @@ -1,18 +1,16 @@ // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.provision.os; -import com.google.common.base.Supplier; -import com.google.common.base.Suppliers; -import com.google.common.collect.ImmutableMap; import com.yahoo.component.Version; import com.yahoo.config.provision.NodeType; import com.yahoo.vespa.curator.Lock; +import com.yahoo.vespa.hosted.provision.NodeRepository; +import com.yahoo.vespa.hosted.provision.node.filter.NodeListFilter; import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient; -import java.time.Duration; import java.util.Map; +import java.util.Objects; import java.util.Optional; -import java.util.concurrent.TimeUnit; import java.util.logging.Logger; /** @@ -27,27 +25,26 @@ import java.util.logging.Logger; */ public class OsVersions { - private static final Duration defaultCacheTtl = Duration.ofMinutes(1); private static final Logger log = Logger.getLogger(OsVersions.class.getName()); - private final CuratorDatabaseClient db; - private final Duration cacheTtl; - /** - * Target OS version is read on every request to /nodes/v2/node/[fqdn]. Cache current targets to avoid - * unnecessary ZK reads. When targets change, some nodes may need to wait for TTL until they see the new target, - * this is fine. + * The maximum number of nodes, within a single node type, that can upgrade in parallel. We limit the number of + * concurrent upgrades to avoid overloading the orchestrator. */ - private volatile Supplier<Map<NodeType, OsVersion>> currentTargets; + private static final int MAX_ACTIVE_UPGRADES = 30; - public OsVersions(CuratorDatabaseClient db) { - this(db, defaultCacheTtl); + private final NodeRepository nodeRepository; + private final CuratorDatabaseClient db; + private final int maxActiveUpgrades; + + public OsVersions(NodeRepository nodeRepository) { + this(nodeRepository, MAX_ACTIVE_UPGRADES); } - OsVersions(CuratorDatabaseClient db, Duration cacheTtl) { - this.db = db; - this.cacheTtl = cacheTtl; - createCache(); + OsVersions(NodeRepository nodeRepository, int maxActiveUpgrades) { + this.nodeRepository = Objects.requireNonNull(nodeRepository, "nodeRepository must be non-null"); + this.db = nodeRepository.database(); + this.maxActiveUpgrades = maxActiveUpgrades; // Read and write all versions to make sure they are stored in the latest version of the serialized format try (var lock = db.lockOsVersions()) { @@ -55,31 +52,27 @@ public class OsVersions { } } - private void createCache() { - this.currentTargets = Suppliers.memoizeWithExpiration(() -> ImmutableMap.copyOf(db.readOsVersions()), - cacheTtl.toMillis(), TimeUnit.MILLISECONDS); - } - /** Returns the current target versions for each node type */ - public Map<NodeType, OsVersion> targets() { - return currentTargets.get(); + public Map<NodeType, Version> targets() { + return db.readOsVersions(); } /** Returns the current target version for given node type, if any */ - public Optional<OsVersion> targetFor(NodeType type) { + public Optional<Version> targetFor(NodeType type) { return Optional.ofNullable(targets().get(type)); } - /** Remove OS target for given node type. Nodes of this type will stop receiving wanted OS version in their - * node object */ + /** + * Remove OS target for given node type. Nodes of this type will stop receiving wanted OS version in their + * node object. + */ public void removeTarget(NodeType nodeType) { require(nodeType); try (Lock lock = db.lockOsVersions()) { - Map<NodeType, OsVersion> osVersions = db.readOsVersions(); + var osVersions = db.readOsVersions(); osVersions.remove(nodeType); + disableUpgrade(nodeType); db.writeOsVersions(osVersions); - createCache(); // Throw away current cache - log.info("Cleared OS target version for " + nodeType); } } @@ -90,42 +83,62 @@ public class OsVersions { throw new IllegalArgumentException("Invalid target version: " + newTarget.toFullString()); } try (Lock lock = db.lockOsVersions()) { - Map<NodeType, OsVersion> osVersions = db.readOsVersions(); - Optional<OsVersion> oldTarget = Optional.ofNullable(osVersions.get(nodeType)); + var osVersions = db.readOsVersions(); + var oldTarget = Optional.ofNullable(osVersions.get(nodeType)); - if (oldTarget.filter(v -> v.version().equals(newTarget)).isPresent()) { + if (oldTarget.filter(v -> v.equals(newTarget)).isPresent()) { return; // Old target matches new target, nothing to do } - if (!force && oldTarget.filter(v -> v.version().isAfter(newTarget)).isPresent()) { + if (!force && oldTarget.filter(v -> v.isAfter(newTarget)).isPresent()) { throw new IllegalArgumentException("Cannot set target OS version to " + newTarget + " without setting 'force', as it's lower than the current version: " - + oldTarget.get().version()); + + oldTarget.get()); } - osVersions.put(nodeType, new OsVersion(newTarget, false)); + osVersions.put(nodeType, newTarget); db.writeOsVersions(osVersions); - createCache(); // Throw away current cache log.info("Set OS target version for " + nodeType + " nodes to " + newTarget.toFullString()); } } - /** Activate or deactivate target for given node type. This is used for resuming or pausing an OS upgrade. */ + /** Activate or deactivate upgrade of given node type. This is used for resuming or pausing an OS upgrade. */ public void setActive(NodeType nodeType, boolean active) { require(nodeType); try (Lock lock = db.lockOsVersions()) { var osVersions = db.readOsVersions(); var currentVersion = osVersions.get(nodeType); if (currentVersion == null) return; // No target version set for this type - if (currentVersion.active() == active) return; // No change - - osVersions.put(nodeType, new OsVersion(currentVersion.version(), active)); - db.writeOsVersions(osVersions); - createCache(); // Throw away current cache - log.info((active ? "Activated" : "Deactivated") + " OS target version for " + nodeType + " nodes"); + if (active) { + upgrade(nodeType, currentVersion); + } else { + disableUpgrade(nodeType); + } } } + /** Trigger upgrade of nodes of given type*/ + private void upgrade(NodeType type, Version version) { + var nodes = nodeRepository.list().nodeType(type); + var numberToUpgrade = Math.max(0, maxActiveUpgrades - nodes.changingOsVersion().size()); + var nodesToUpgrade = nodes.not().changingOsVersion() + .not().onOsVersion(version) + .first(numberToUpgrade); + if (nodesToUpgrade.size() == 0) return; + log.info("Upgrading " + nodesToUpgrade.size() + " nodes of type " + type + " to OS version " + version); + nodeRepository.upgradeOs(NodeListFilter.from(nodesToUpgrade.asList()), Optional.of(version)); + } + + /** Disable OS upgrade for all nodes of given type */ + private void disableUpgrade(NodeType type) { + var nodesUpgrading = nodeRepository.list() + .nodeType(type) + .changingOsVersion(); + if (nodesUpgrading.size() == 0) return; + log.info("Disabling OS upgrade of all " + type + " nodes"); + nodeRepository.upgradeOs(NodeListFilter.from(nodesUpgrading.asList()), Optional.empty()); + } + private static void require(NodeType nodeType) { if (!nodeType.isDockerHost()) { throw new IllegalArgumentException("Node type '" + nodeType + "' does not support OS upgrades"); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java index fae314bc50f..a28845109dc 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java @@ -24,7 +24,6 @@ import com.yahoo.vespa.hosted.provision.lb.LoadBalancer; import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId; import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.hosted.provision.node.Status; -import com.yahoo.vespa.hosted.provision.os.OsVersion; import java.time.Clock; import java.time.Duration; @@ -418,11 +417,11 @@ public class CuratorDatabaseClient { // OS versions - public Map<NodeType, OsVersion> readOsVersions() { + public Map<NodeType, Version> readOsVersions() { return read(osVersionsPath(), OsVersionsSerializer::fromJson).orElseGet(TreeMap::new); } - public void writeOsVersions(Map<NodeType, OsVersion> versions) { + public void writeOsVersions(Map<NodeType, Version> versions) { NestedTransaction transaction = new NestedTransaction(); CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction); curatorTransaction.add(CuratorOperations.setData(osVersionsPath().getAbsolute(), diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java index 2e991ac234e..2cbfbc349a6 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java @@ -28,6 +28,7 @@ import com.yahoo.vespa.hosted.provision.node.History; import com.yahoo.vespa.hosted.provision.node.IP; import com.yahoo.vespa.hosted.provision.node.Reports; import com.yahoo.vespa.hosted.provision.node.Status; +import com.yahoo.vespa.hosted.provision.node.OsVersion; import java.io.IOException; import java.time.Instant; @@ -72,6 +73,7 @@ public class NodeSerializer { private static final String wantToRetireKey = "wantToRetire"; private static final String wantToDeprovisionKey = "wantToDeprovision"; private static final String osVersionKey = "osVersion"; + private static final String wantedOsVersionKey = "wantedOsVersion"; private static final String firmwareCheckKey = "firmwareCheck"; private static final String reportsKey = "reports"; private static final String modelNameKey = "modelName"; @@ -142,7 +144,8 @@ public class NodeSerializer { node.allocation().ifPresent(allocation -> toSlime(allocation, object.setObject(instanceKey))); toSlime(node.history(), object.setArray(historyKey)); object.setString(nodeTypeKey, toString(node.type())); - node.status().osVersion().ifPresent(version -> object.setString(osVersionKey, version.toString())); + node.status().osVersion().current().ifPresent(version -> object.setString(osVersionKey, version.toString())); + node.status().osVersion().wanted().ifPresent(version -> object.setString(wantedOsVersionKey, version.toFullString())); node.status().firmwareVerifiedAt().ifPresent(instant -> object.setLong(firmwareCheckKey, instant.toEpochMilli())); node.reports().toSlime(object, reportsKey); node.modelName().ifPresent(modelName -> object.setString(modelNameKey, modelName)); @@ -226,10 +229,11 @@ public class NodeSerializer { return new Status(generationFromSlime(object, rebootGenerationKey, currentRebootGenerationKey), versionFromSlime(object.field(vespaVersionKey)), dockerImageFromSlime(object.field(currentDockerImageKey)), - (int)object.field(failCountKey).asLong(), + (int) object.field(failCountKey).asLong(), object.field(wantToRetireKey).asBool(), object.field(wantToDeprovisionKey).asBool(), - versionFromSlime(object.field(osVersionKey)), + new OsVersion(versionFromSlime(object.field(osVersionKey)), + versionFromSlime(object.field(wantedOsVersionKey))), instantFromSlime(object.field(firmwareCheckKey))); } @@ -360,6 +364,7 @@ public class NodeSerializer { } throw new IllegalArgumentException("Unknown node event type '" + eventTypeString + "'"); } + private String toString(History.Event.Type nodeEventType) { switch (nodeEventType) { case provisioned : return "provisioned"; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/OsVersionsSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/OsVersionsSerializer.java index 91f619ffa91..7f340879808 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/OsVersionsSerializer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/OsVersionsSerializer.java @@ -6,7 +6,7 @@ import com.yahoo.config.provision.NodeType; import com.yahoo.slime.ObjectTraverser; import com.yahoo.slime.Slime; import com.yahoo.vespa.config.SlimeUtils; -import com.yahoo.vespa.hosted.provision.os.OsVersion; +import com.yahoo.vespa.hosted.provision.node.OsVersion; import java.io.IOException; import java.io.UncheckedIOException; @@ -18,6 +18,7 @@ import java.util.TreeMap; * * @author mpolden */ +// TODO(mpolden): Remove this and replaces usages with NodeTypeVersionsSerializer after January 2020 public class OsVersionsSerializer { private static final String VERSION_FIELD = "version"; @@ -25,13 +26,13 @@ public class OsVersionsSerializer { private OsVersionsSerializer() {} - public static byte[] toJson(Map<NodeType, OsVersion> versions) { + public static byte[] toJson(Map<NodeType, Version> versions) { var slime = new Slime(); var object = slime.setObject(); versions.forEach((nodeType, osVersion) -> { var versionObject = object.setObject(NodeSerializer.toString(nodeType)); - versionObject.setString(VERSION_FIELD, osVersion.version().toFullString()); - versionObject.setBool(ACTIVE_FIELD, osVersion.active()); + versionObject.setString(VERSION_FIELD, osVersion.toFullString()); + versionObject.setBool(ACTIVE_FIELD, true); }); try { return SlimeUtils.toJsonBytes(slime); @@ -40,13 +41,12 @@ public class OsVersionsSerializer { } } - public static Map<NodeType, OsVersion> fromJson(byte[] data) { - var versions = new TreeMap<NodeType, OsVersion>(); // Use TreeMap to sort by node type + public static Map<NodeType, Version> fromJson(byte[] data) { + var versions = new TreeMap<NodeType, Version>(); // Use TreeMap to sort by node type var inspector = SlimeUtils.jsonToSlime(data).get(); inspector.traverse((ObjectTraverser) (key, value) -> { var version = Version.fromString(value.field(VERSION_FIELD).asString()); - var active = value.field(ACTIVE_FIELD).asBool(); - versions.put(NodeSerializer.nodeTypeFromString(key), new OsVersion(version, active)); + versions.put(NodeSerializer.nodeTypeFromString(key), version); }); return versions; } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java index a7b37628289..ee786ea414e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java @@ -9,10 +9,6 @@ import com.yahoo.config.provision.exception.LoadBalancerServiceException; import com.yahoo.log.LogLevel; import com.yahoo.transaction.Mutex; import com.yahoo.transaction.NestedTransaction; -import com.yahoo.vespa.flags.BooleanFlag; -import com.yahoo.vespa.flags.FetchVector; -import com.yahoo.vespa.flags.FlagSource; -import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeList; import com.yahoo.vespa.hosted.provision.NodeRepository; @@ -28,7 +24,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; -import java.util.Map; import java.util.Set; import java.util.function.Function; import java.util.logging.Logger; @@ -51,7 +46,7 @@ public class LoadBalancerProvisioner { private final CuratorDatabaseClient db; private final LoadBalancerService service; - public LoadBalancerProvisioner(NodeRepository nodeRepository, LoadBalancerService service, FlagSource flagSource) { + public LoadBalancerProvisioner(NodeRepository nodeRepository, LoadBalancerService service) { this.nodeRepository = nodeRepository; this.db = nodeRepository.database(); this.service = service; @@ -118,7 +113,7 @@ public class LoadBalancerProvisioner { } } - /** Returns load balancers of given application that are no longer referenced by wantedClusters */ + /** Returns load balancers of given application that are no longer referenced by given clusters */ private List<LoadBalancer> surplusLoadBalancersOf(ApplicationId application, Set<ClusterSpec.Id> activeClusters) { var activeLoadBalancersByCluster = nodeRepository.loadBalancers() .owner(application) @@ -165,13 +160,12 @@ public class LoadBalancerProvisioner { } private LoadBalancerInstance create(ApplicationId application, ClusterSpec.Id cluster, List<Node> nodes, boolean force) { - Map<HostName, Set<String>> hostnameToIpAdresses = nodes.stream() - .collect(Collectors.toMap(node -> HostName.from(node.hostname()), - this::reachableIpAddresses)); - Set<Real> reals = new LinkedHashSet<>(); - hostnameToIpAdresses.forEach((hostname, ipAddresses) -> { - ipAddresses.forEach(ipAddress -> reals.add(new Real(hostname, ipAddress))); - }); + var reals = new LinkedHashSet<Real>(); + for (var node : nodes) { + for (var ip : reachableIpAddresses(node)) { + reals.add(new Real(HostName.from(node.hostname()), ip)); + } + } log.log(LogLevel.INFO, "Creating load balancer for " + cluster + " in " + application.toShortString() + ", targeting: " + reals); try { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java index 67d132b4fb8..f7567683776 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java @@ -59,7 +59,7 @@ public class NodeRepositoryProvisioner implements Provisioner { this.nodeRepository = nodeRepository; this.capacityPolicies = new CapacityPolicies(zone); this.zone = zone; - this.loadBalancerProvisioner = provisionServiceProvider.getLoadBalancerService().map(lbService -> new LoadBalancerProvisioner(nodeRepository, lbService, flagSource)); + this.loadBalancerProvisioner = provisionServiceProvider.getLoadBalancerService().map(lbService -> new LoadBalancerProvisioner(nodeRepository, lbService)); this.preparer = new Preparer(nodeRepository, zone.environment() == Environment.prod ? SPARE_CAPACITY_PROD : SPARE_CAPACITY_NONPROD, provisionServiceProvider.getHostProvisioner(), diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesResponse.java index c22edf2677f..feab5ed1ed8 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesResponse.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesResponse.java @@ -19,7 +19,6 @@ import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.hosted.provision.node.History; import com.yahoo.vespa.hosted.provision.node.filter.NodeFilter; -import com.yahoo.vespa.hosted.provision.os.OsVersion; import com.yahoo.vespa.orchestrator.Orchestrator; import com.yahoo.vespa.orchestrator.status.HostStatus; @@ -168,11 +167,8 @@ class NodesResponse extends HttpResponse { }); object.setLong("rebootGeneration", node.status().reboot().wanted()); object.setLong("currentRebootGeneration", node.status().reboot().current()); - node.status().osVersion().ifPresent(version -> object.setString("currentOsVersion", version.toFullString())); - nodeRepository.osVersions().targetFor(node.type()) - .filter(OsVersion::active) // Only include wantedOsVersion when active. When active is false, OS upgrades are paused. - .map(OsVersion::version) - .ifPresent(version -> object.setString("wantedOsVersion", version.toFullString())); + node.status().osVersion().current().ifPresent(version -> object.setString("currentOsVersion", version.toFullString())); + node.status().osVersion().wanted().ifPresent(version -> object.setString("wantedOsVersion", version.toFullString())); node.status().firmwareVerifiedAt().ifPresent(instant -> object.setLong("currentFirmwareCheck", instant.toEpochMilli())); if (node.type().isDockerHost()) nodeRepository.firmwareChecks().requiredAfter().ifPresent(after -> object.setLong("wantedFirmwareCheck", after.toEpochMilli())); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/UpgradeResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/UpgradeResponse.java index ae61bedd67f..381a1bc27aa 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/UpgradeResponse.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/UpgradeResponse.java @@ -39,7 +39,7 @@ public class UpgradeResponse extends HttpResponse { infrastructureVersions.getTargetVersions().forEach((nodeType, version) -> versionsObject.setString(nodeType.name(), version.toFullString())); Cursor osVersionsObject = root.setObject("osVersions"); - osVersions.targets().forEach((nodeType, osVersion) -> osVersionsObject.setString(nodeType.name(), osVersion.version().toFullString())); + osVersions.targets().forEach((nodeType, osVersion) -> osVersionsObject.setString(nodeType.name(), osVersion.toFullString())); Cursor dockerImagesObject = root.setObject("dockerImages"); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java index 75224021464..12b48fd7a35 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java @@ -17,6 +17,7 @@ import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -60,9 +61,15 @@ public class LoadBalancerExpirerTest { // Expirer defers removal while nodes are still allocated to application expirer.maintain(); assertEquals(3, tester.loadBalancerService().instances().size()); + assertEquals(2, tester.loadBalancerService().instances().get(lb1).reals().size()); dirtyNodesOf(app1, cluster1); - // Expirer defers removal until expiration time passes + // Expirer prunes reals before expiration time of load balancer itself + expirer.maintain(); + assertEquals(Set.of(), tester.loadBalancerService().instances().get(lb1).reals()); + assertEquals(Set.of(), tester.nodeRepository().loadBalancers().owner(lb1.application()).asList().get(0).instance().reals()); + + // Expirer defers removal of load balancer until expiration time passes expirer.maintain(); assertTrue("Inactive load balancer not removed", tester.loadBalancerService().instances().containsKey(lb1)); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooterTest.java index bc97491f828..d143253a4b1 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRebooterTest.java @@ -116,10 +116,9 @@ public class NodeRebooterTest { var wantedOsVersion = tester.nodeRepository.osVersions().targetFor(NodeType.host); if (wantedOsVersion.isEmpty()) return; for (Node node : tester.nodeRepository.getNodes(Node.State.ready, Node.State.active)) { - if (wantedOsVersion.get().version().isAfter(node.status().osVersion().orElse(Version.emptyVersion))) - tester.nodeRepository.write(node.withCurrentOsVersion(wantedOsVersion.get().version(), - tester.clock.instant()), () -> { - }); + if (wantedOsVersion.get().isAfter(node.status().osVersion().current().orElse(Version.emptyVersion))) + tester.nodeRepository.write(node.withCurrentOsVersion(wantedOsVersion.get(), tester.clock.instant()), + () -> {}); } } @@ -128,11 +127,4 @@ public class NodeRebooterTest { return nodes.stream().filter(n -> n.status().reboot().current() == generation).collect(Collectors.toList()); } - /** Returns the subset of the given nodes which have the given current OS version */ - private List<Node> withOsVersion(Version version, List<Node> nodes) { - return nodes.stream().filter(n -> n.status().osVersion().isPresent() && - n.status().osVersion().get().equals(version)) - .collect(Collectors.toList()); - } - } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java index 2677ab14ba2..c30b49ac97a 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java @@ -10,7 +10,6 @@ import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.hosted.provision.node.Allocation; import com.yahoo.vespa.hosted.provision.node.Status; -import com.yahoo.vespa.hosted.provision.os.OsVersion; import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester; import org.junit.Test; @@ -92,7 +91,7 @@ public class OsUpgradeActivatorTest { private boolean isOsVersionActive(NodeType... types) { var active = true; for (var type : types) { - active &= tester.nodeRepository().osVersions().targetFor(type).map(OsVersion::active).orElse(false); + active &= tester.nodeRepository().list().nodeType(type).changingOsVersion().size() > 0; } return active; } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java index 070db08f090..2a3e59bee42 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java @@ -3,15 +3,17 @@ package com.yahoo.vespa.hosted.provision.os; import com.yahoo.component.Version; import com.yahoo.config.provision.NodeType; -import com.yahoo.vespa.hosted.provision.NodeRepositoryTester; +import com.yahoo.vespa.hosted.provision.Node; +import com.yahoo.vespa.hosted.provision.NodeList; +import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester; import org.junit.Test; -import java.time.Duration; +import java.util.Comparator; +import java.util.List; +import java.util.function.Supplier; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotSame; -import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -20,45 +22,86 @@ import static org.junit.Assert.fail; */ public class OsVersionsTest { + private final ProvisioningTester tester = new ProvisioningTester.Builder().build(); + @Test public void test_versions() { - var versions = new OsVersions(new NodeRepositoryTester().nodeRepository().database(), Duration.ofDays(1)); - - assertTrue("No versions set", versions.targets().isEmpty()); - assertSame("Caches empty target versions", versions.targets(), versions.targets()); + var versions = new OsVersions(tester.nodeRepository(), Integer.MAX_VALUE); + tester.makeReadyNodes(10, "default", NodeType.host); + Supplier<List<Node>> hostNodes = () -> tester.nodeRepository().getNodes(NodeType.host); // Upgrade OS - var version1 = new OsVersion(Version.fromString("7.1"), false); - versions.setTarget(NodeType.host, version1.version(), false); - var targetVersions = versions.targets(); - assertSame("Caches target versions", targetVersions, versions.targets()); + assertTrue("No versions set", versions.targets().isEmpty()); + var version1 = Version.fromString("7.1"); + versions.setTarget(NodeType.host, version1, false); assertEquals(version1, versions.targetFor(NodeType.host).get()); + assertTrue("Per-node wanted OS version remains unset", hostNodes.get().stream().allMatch(node -> node.status().osVersion().wanted().isEmpty())); // Upgrade OS again - var version2 = new OsVersion(Version.fromString("7.2"), false); - versions.setTarget(NodeType.host, version2.version(), false); - assertNotSame("Cache invalidated", targetVersions, versions.targets()); + var version2 = Version.fromString("7.2"); + versions.setTarget(NodeType.host, version2, false); assertEquals(version2, versions.targetFor(NodeType.host).get()); // Target can be (de)activated versions.setActive(NodeType.host, true); - assertTrue("Target version deactivated", versions.targetFor(NodeType.host).get().active()); + assertTrue("Target version activated", hostNodes.get().stream() + .allMatch(node -> node.status().osVersion().wanted().isPresent())); versions.setActive(NodeType.host, false); - assertFalse("Target version deactivated", versions.targetFor(NodeType.host).get().active()); + assertTrue("Target version deactivated", hostNodes.get().stream() + .allMatch(node -> node.status().osVersion().wanted().isEmpty())); // Downgrading fails try { - versions.setTarget(NodeType.host, version1.version(), false); + versions.setTarget(NodeType.host, version1, false); fail("Expected exception"); } catch (IllegalArgumentException ignored) {} // Forcing downgrade succeeds - versions.setTarget(NodeType.host, version1.version(), true); + versions.setTarget(NodeType.host, version1, true); assertEquals(version1, versions.targetFor(NodeType.host).get()); // Target can be removed versions.removeTarget(NodeType.host); assertFalse(versions.targetFor(NodeType.host).isPresent()); + assertTrue(hostNodes.get().stream().allMatch(node -> node.status().osVersion().wanted().isEmpty())); + } + + @Test + public void test_max_active_upgrades() { + int totalNodes = 20; + int maxActiveUpgrades = 5; + var versions = new OsVersions(tester.nodeRepository(), maxActiveUpgrades); + tester.makeReadyNodes(totalNodes, "default", NodeType.host); + Supplier<NodeList> hostNodes = () -> tester.nodeRepository().list().nodeType(NodeType.host); + + // Set target + var version1 = Version.fromString("7.1"); + versions.setTarget(NodeType.host, version1, false); + assertEquals(version1, versions.targetFor(NodeType.host).get()); + + // Activate target + for (int i = 0; i < totalNodes; i += maxActiveUpgrades) { + versions.setActive(NodeType.host, true); + var nodesUpgrading = hostNodes.get().changingOsVersion(); + assertEquals("Target is changed for a subset of nodes", maxActiveUpgrades, nodesUpgrading.size()); + completeUpgradeOf(nodesUpgrading.asList()); + } + + // Activating again after all nodes have upgraded does nothing + versions.setActive(NodeType.host, true); + assertEquals(version1, hostNodes.get().stream() + .map(n -> n.status().osVersion().current().get()) + .min(Comparator.naturalOrder()).get()); + } + + private void completeUpgradeOf(List<Node> nodes) { + for (var node : nodes) { + try (var lock = tester.nodeRepository().lock(node)) { + node = tester.nodeRepository().getNode(node.hostname()).get(); + node = node.with(node.status().withOsVersion(node.status().osVersion().withCurrent(node.status().osVersion().wanted()))); + tester.nodeRepository().write(node, lock); + } + } } } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/OsVersionsSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/OsVersionsSerializerTest.java index c6583292da8..92d04d1cbb2 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/OsVersionsSerializerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/OsVersionsSerializerTest.java @@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.provision.persistence; import com.yahoo.component.Version; import com.yahoo.config.provision.NodeType; -import com.yahoo.vespa.hosted.provision.os.OsVersion; import org.junit.Test; import java.util.Map; @@ -18,9 +17,9 @@ public class OsVersionsSerializerTest { @Test public void serialization() { var versions = Map.of( - NodeType.host, new OsVersion(Version.fromString("1.2.3"), true), - NodeType.proxyhost, new OsVersion(Version.fromString("4.5.6"), false), - NodeType.confighost, new OsVersion(Version.fromString("7.8.9"), true) + NodeType.host, Version.fromString("1.2.3"), + NodeType.proxyhost, Version.fromString("4.5.6"), + NodeType.confighost, Version.fromString("7.8.9") ); var serialized = OsVersionsSerializer.fromJson(OsVersionsSerializer.toJson(versions)); assertEquals(serialized, versions); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java index dccbdca59b0..08e7772b5ba 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java @@ -319,14 +319,14 @@ public class SerializationTest { @Test public void os_version_serialization() { Node serialized = nodeSerializer.fromJson(State.provisioned, nodeSerializer.toJson(createNode())); - assertFalse(serialized.status().osVersion().isPresent()); + assertFalse(serialized.status().osVersion().current().isPresent()); // Update OS version serialized = serialized.withCurrentOsVersion(Version.fromString("7.1"), Instant.ofEpochMilli(123)) // Another update for same version: .withCurrentOsVersion(Version.fromString("7.1"), Instant.ofEpochMilli(456)); serialized = nodeSerializer.fromJson(State.provisioned, nodeSerializer.toJson(serialized)); - assertEquals(Version.fromString("7.1"), serialized.status().osVersion().get()); + assertEquals(Version.fromString("7.1"), serialized.status().osVersion().current().get()); var osUpgradedEvents = serialized.history().events().stream() .filter(event -> event.type() == History.Event.Type.osUpgraded) .collect(Collectors.toList()); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AllocationVisualizer.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AllocationVisualizer.java index 61e14be53d8..23b2a93a67d 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AllocationVisualizer.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AllocationVisualizer.java @@ -62,7 +62,6 @@ public class AllocationVisualizer extends JPanel { @Override public void paintComponent(Graphics g) { super.paintComponent(g); - System.out.println("PAINTING"); if (steps.size() == 0) return; int nodeX = 40; diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java index 10a50998509..6ab027cd143 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java @@ -162,14 +162,11 @@ public class MultigroupProvisioningTest { int previousActiveNodeCount = tester.getNodes(application, Node.State.active).resources(nodeResources).size(); tester.activate(application, prepare(application, capacity, wantedGroups, tester)); - - System.out.println("Active nodes ---------------"); - tester.getNodes(application, Node.State.active).forEach(n -> System.out.println(" " + n.hostname() + ": Flavor : " + n.flavor() + " retired " + n.status().wantToRetire())); assertEquals("Superfluous nodes are retired, but no others - went from " + previousActiveNodeCount + " to " + nodeCount + " nodes", Math.max(0, previousActiveNodeCount - capacity.nodeCount()), tester.getNodes(application, Node.State.active).retired().resources(nodeResources).size()); assertEquals("Other flavors are retired", - 0, tester.getNodes(application, Node.State.active).nonretired().notResources(nodeResources).size()); + 0, tester.getNodes(application, Node.State.active).not().retired().not().resources(nodeResources).size()); // Check invariants for all nodes Set<Integer> allIndexes = new HashSet<>(); @@ -185,7 +182,7 @@ public class MultigroupProvisioningTest { // Count unretired nodes and groups of the requested flavor Set<Integer> indexes = new HashSet<>(); Map<ClusterSpec.Group, Integer> nonretiredGroups = new HashMap<>(); - for (Node node : tester.getNodes(application, Node.State.active).nonretired().resources(nodeResources)) { + for (Node node : tester.getNodes(application, Node.State.active).not().retired().resources(nodeResources)) { indexes.add(node.allocation().get().membership().index()); ClusterSpec.Group group = node.allocation().get().membership().cluster().group().get(); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java index 49dc31c9f39..93802d0d4b2 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java @@ -509,7 +509,7 @@ public class ProvisioningTest { new com.yahoo.component.Version(4, 5, 6), false); tester.activate(application, tester.prepare(application, cluster, capacity, 1)); - assertEquals(5, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).nonretired().size()); + assertEquals(5, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).not().retired().size()); assertEquals(0, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).retired().size()); // Mark the nodes as want to retire @@ -518,7 +518,7 @@ public class ProvisioningTest { tester.activate(application, tester.prepare(application, cluster, capacityFORCED, 1)); // Nodes are not retired since that is unsafe when we cannot fail - assertEquals(5, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).nonretired().size()); + assertEquals(5, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).not().retired().size()); assertEquals(0, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).retired().size()); // ... but we still want to tester.nodeRepository().getNodes(application, Node.State.active).forEach(node -> assertTrue(node.status().wantToRetire())); @@ -526,7 +526,7 @@ public class ProvisioningTest { // redeploy with allowing failing tester.activate(application, tester.prepare(application, cluster, capacity, 1)); // ... old nodes are now retired - assertEquals(5, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).nonretired().size()); + assertEquals(5, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).not().retired().size()); assertEquals(5, new NodeList(tester.nodeRepository().getNodes(application, Node.State.active)).retired().size()); } diff --git a/searchlib/abi-spec.json b/searchlib/abi-spec.json index bde3b6abb6c..ff6c1ad0b9d 100644 --- a/searchlib/abi-spec.json +++ b/searchlib/abi-spec.json @@ -925,6 +925,8 @@ "public final void labelAndDimensionValues(java.util.List)", "public final java.util.List valueAddress()", "public final com.yahoo.tensor.functions.Slice$DimensionValue dimensionValue(java.util.Optional)", + "public final java.lang.String label()", + "public final java.lang.String string()", "public void <init>(java.io.InputStream)", "public void <init>(java.io.InputStream, java.lang.String)", "public void ReInit(java.io.InputStream)", diff --git a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/TensorFunctionNode.java b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/TensorFunctionNode.java index 6200515462b..6e1cdf52158 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/TensorFunctionNode.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/rankingexpression/rule/TensorFunctionNode.java @@ -169,16 +169,24 @@ public class TensorFunctionNode extends CompositeNode { if (outermost instanceof ExpressionToStringContext) { ExpressionToStringContext context = (ExpressionToStringContext)outermost; - return expression.toString(new StringBuilder(), - new ExpressionToStringContext(context.wrappedSerializationContext, c, context.path, context.parent), - context.path, - context.parent).toString(); + ExpressionNode root = expression; + if (root instanceof CompositeNode && ! (root instanceof EmbracedNode) && ! isIdentifierReference(root)) + root = new EmbracedNode(root); // Output embraced if composite + return root.toString(new StringBuilder(), + new ExpressionToStringContext(context.wrappedSerializationContext, c, context.path, context.parent), + context.path, + context.parent).toString(); } else { return expression.toString(); } } + private boolean isIdentifierReference(ExpressionNode node) { + if ( ! (node instanceof ReferenceNode)) return false; + return ((ReferenceNode)node).reference().isIdentifier(); + } + } /** diff --git a/searchlib/src/main/javacc/RankingExpressionParser.jj b/searchlib/src/main/javacc/RankingExpressionParser.jj index e413e398183..c3d597fca46 100755 --- a/searchlib/src/main/javacc/RankingExpressionParser.jj +++ b/searchlib/src/main/javacc/RankingExpressionParser.jj @@ -894,7 +894,7 @@ void keyValue(TensorType type, java.util.Map cellMap) : ExpressionNode value; } { - label = tag() <COLON> value = expression() + label = label() <COLON> value = expression() { cellMap.put(TensorAddress.ofLabels(label), TensorFunctionNode.wrapScalar(value)); } } @@ -904,7 +904,7 @@ void mixedBlock(TensorType type, List dimensionOrder, java.util.Map cellMap) : List cells; } { - label = tag() <COLON> cells = indexedTensorCells() + label = label() <COLON> cells = indexedTensorCells() { TensorFunctionNode.wrapScalarBlock(type, dimensionOrder, label, cells, cellMap); } } @@ -952,7 +952,7 @@ void labelAndDimension(TensorAddress.Builder addressBuilder) : String dimension, label; } { - dimension = identifier() <COLON> label = tag() + dimension = identifier() <COLON> label = label() { addressBuilder.add(dimension, label); } } @@ -1000,4 +1000,18 @@ Slice.DimensionValue dimensionValue(Optional dimensionName) : else return new Slice.DimensionValue(dimensionName, TensorFunctionNode.wrapScalar(value)); } +} + +String label() : +{ + String label; +} +{ + ( label = tag() | label = string() ) + { return label; } +} + +String string() : {} +{ + <STRING> { return token.image.substring(1, token.image.length() - 1); } }
\ No newline at end of file diff --git a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/RankingExpressionTestCase.java b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/RankingExpressionTestCase.java index b750a7607cc..ea09de32137 100755 --- a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/RankingExpressionTestCase.java +++ b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/RankingExpressionTestCase.java @@ -150,11 +150,11 @@ public class RankingExpressionTestCase { "map(constant(tensor0), f(a)(cos(a))) + l2_normalize(attribute(tensor1), x)"); assertSerialization("join(reduce(join(reduce(join(constant(tensor0), attribute(tensor1), f(a,b)(a * b)), sum, x), attribute(tensor1), f(a,b)(a * b)), sum, y), query(tensor2), f(a,b)(a + b))", "xw_plus_b(matmul(constant(tensor0), attribute(tensor1), x), attribute(tensor1), query(tensor2), y)"); - assertSerialization("tensor(x{}):{{x:a}:1 + 2 + 3,{x:b}:if (1 > 2, 3, 4),{x:c}:reduce(tensor0 * tensor1, sum)}", + assertSerialization("tensor(x{}):{{x:a}:(1 + 2 + 3),{x:b}:(if (1 > 2, 3, 4)),{x:c}:(reduce(tensor0 * tensor1, sum))}", "tensor(x{}):{ {x:a}:1+2+3, {x:b}:if(1>2,3,4), {x:c}:sum(tensor0*tensor1) }"); assertSerialization("tensor(x[3]):{{x:0}:1.0,{x:1}:2.0,{x:2}:3}", "tensor(x[3]):[1.0, 2.0, 3]"); - assertSerialization("tensor(x[3]):{{x:0}:1.0,{x:1}:reduce(tensor0 * tensor1, sum),{x:2}:3}", + assertSerialization("tensor(x[3]):{{x:0}:1.0,{x:1}:(reduce(tensor0 * tensor1, sum)),{x:2}:3}", "tensor(x[3]):[1.0, sum(tensor0*tensor1), 3]"); } @@ -165,44 +165,44 @@ public class RankingExpressionTestCase { functions.add(new ExpressionFunction("tensorFunction", List.of(), new RankingExpression("tensor(x[3]):[1, 2, 3]"))); // Getting a value from a tensor supplied by a function, inside a tensor generate function - assertSerialization(List.of("tensor(x[3])(rankingExpression(tensorFunction)[x])"), + assertSerialization(List.of("tensor(x[3])((rankingExpression(tensorFunction)[x]))"), "tensor(x[3])(tensorFunction[x])", functions, false); // Getting a value from a tensor supplied by a function, where the value index is supplied by a function, inside a tensor generate function, short form - assertSerialization(List.of("tensor(x[3])(rankingExpression(tensorFunction)[rankingExpression(scalarFunction)])"), + assertSerialization(List.of("tensor(x[3])((rankingExpression(tensorFunction)[(rankingExpression(scalarFunction))]))"), "tensor(x[3])(tensorFunction[scalarFunction()])", functions, false); // 'scalarFunction' is interpreted as a label here since it is the short form of a mapped dimension - assertSerialization(List.of("tensor(x[3])(rankingExpression(tensorFunction){scalarFunction})"), + assertSerialization(List.of("tensor(x[3])((rankingExpression(tensorFunction){scalarFunction}))"), "tensor(x[3])(tensorFunction{scalarFunction})", functions, false); // Getting a value from a tensor supplied by a function, where the value index is supplied by a function, inside a tensor generate function, long form - assertSerialization(List.of("tensor(x[3])(rankingExpression(tensorFunction){x:rankingExpression(scalarFunction)})"), + assertSerialization(List.of("tensor(x[3])((rankingExpression(tensorFunction){x:(rankingExpression(scalarFunction))}))"), "tensor(x[3])(tensorFunction{x:scalarFunction()})", functions, false); // 'scalarFunction' without parentheses is interpreted as a label instead of a reference to the function - assertSerialization(List.of("tensor(x[3])(rankingExpression(tensorFunction){x:scalarFunction})"), + assertSerialization(List.of("tensor(x[3])((rankingExpression(tensorFunction){x:scalarFunction}))"), "tensor(x[3])(tensorFunction{x:scalarFunction})", functions, false); // Accessing a function in a dynamic tensor, short form - assertSerialization(List.of("tensor(x[2]):{{x:0}:rankingExpression(scalarFunction),{x:1}:rankingExpression(scalarFunction)}"), + assertSerialization(List.of("tensor(x[2]):{{x:0}:(rankingExpression(scalarFunction)),{x:1}:(rankingExpression(scalarFunction))}"), "tensor(x[2]):[scalarFunction(), scalarFunction()]", functions, false); // Accessing a function in a dynamic tensor, long form - assertSerialization(List.of("tensor(x{}):{{x:foo}:rankingExpression(scalarFunction),{x:bar}:rankingExpression(scalarFunction)}"), + assertSerialization(List.of("tensor(x{}):{{x:foo}:(rankingExpression(scalarFunction)),{x:bar}:(rankingExpression(scalarFunction))}"), "tensor(x{}):{{x:foo}:scalarFunction(), {x:bar}:scalarFunction()}", functions, false); // Shadowing - assertSerialization(List.of("tensor(scalarFunction[1])(rankingExpression(tensorFunction){x:scalarFunction + rankingExpression(scalarFunction)})"), + assertSerialization(List.of("tensor(scalarFunction[1])((rankingExpression(tensorFunction){x:(scalarFunction + rankingExpression(scalarFunction))}))"), "tensor(scalarFunction[1])(tensorFunction{x: scalarFunction + scalarFunction()})", - functions, false); + functions, true); } diff --git a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java index 26861dd3cd6..ca2f6c6bbec 100644 --- a/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java +++ b/searchlib/src/test/java/com/yahoo/searchlib/rankingexpression/evaluation/EvaluationTestCase.java @@ -324,7 +324,7 @@ public class EvaluationTestCase { "{y:1}:((1+1)+a)}{y:1}" + "}"); - // tensor value + // tensor slice tester.assertEvaluates("3.0", "tensor0{x:1}", "{ {x:0}:1, {x:1}:3 }"); tester.assertEvaluates("1.2", "tensor0{key:foo,x:0}", true, "{ {key:foo,x:0}:1.2, {key:bar,x:0}:3 }"); tester.assertEvaluates("3.0", "tensor0{bar}", true, "{ {x:foo}:1, {x:bar}:3 }"); @@ -401,6 +401,9 @@ public class EvaluationTestCase { tester.assertEvaluates("tensor(x{}):{ {x:a}:6.0, {x:b}:4.0, {x:c}:14.0 }", "tensor(x{}):{ {x:a}:1+2+3, {x:b}:if(1>2,3,4), {x:c}:sum(tensor0*tensor1) }", "{ {x:0}:7 }", "tensor(x{}):{ {x:0}:2 }"); + tester.assertEvaluates("tensor(x{}):{ {x:a}:6.0, {x:b}:4.0, {x:'--'}:14.0 }", + "tensor(x{}):{ a:1+2+3, b:if(1>2,3,4), '--':sum(tensor0*tensor1) }", + "{ {x:0}:7 }", "tensor(x{}):{ {x:0}:2 }"); tester.assertEvaluates("tensor<float>(d0[1],x[3]):[[1.0, 0.5, 0.25]]", "tensor<float>(d0[1],x[3]):[[one,one_half,a_quarter]]"); tester.assertEvaluates("tensor(x[2],y[3]):[[1.0, 0.5, 0.25],[0.25, 0.5, 1.0]]", @@ -410,10 +413,10 @@ public class EvaluationTestCase { tester.assertEvaluates("tensor(x{},y[2]):{a:[1.0, 0.5], b:[0.25, 2]}", "tensor(x{},y[2]):{a:[one, one_half], b:[a_quarter, 2]}"); tester.assertEvaluates("tensor(key{},x[2],y[3]):{key1:[[1.0, 0.5, 0.25],[0.25, 0.5, 1.0]]," + - " key2:[[1.0, 2.0, 3.00],[4.00, 5.0, 6.0]]}", + " 'key2.[]':[[1.0, 2.0, 3.00],[4.00, 5.0, 6.0]]}", "tensor(key{},x[2],y[3]):{key1:[[one,one_half,a_quarter],[a_quarter,one_half,one]]," + - " key2:[[1,2,3],[4,5,6]]}"); - tester.assertEvaluates("tensor(x{}):{{x:a}:1, {x:b}:-2, {x:cee}:0.5}", "tensor(x{}):{a:1, b:-2, cee:one_half}"); + " 'key2.[]':[[1,2,3],[4,5,6]]}"); + tester.assertEvaluates("tensor(x{}):{{x:a}:1, {x:'\"'}:-2, {x:\"'\"}:0.5}", "tensor(x{}):{a:1, '\"':-2, \"'\":one_half}"); // Opposite order in the expression: // - indexed diff --git a/searchlib/src/tests/fef/fef_test.cpp b/searchlib/src/tests/fef/fef_test.cpp index 244a4e26554..896a917d6e3 100644 --- a/searchlib/src/tests/fef/fef_test.cpp +++ b/searchlib/src/tests/fef/fef_test.cpp @@ -68,14 +68,23 @@ TEST("test TermFieldMatchDataAppend") tmd.appendPosition(pos); EXPECT_EQUAL(2u, tmd.size()); EXPECT_EQUAL(2u, tmd.capacity()); + uint32_t resizeCount(0); + const TermFieldMatchDataPosition * prev = tmd.begin(); for (size_t i(2); i < std::numeric_limits<uint16_t>::max(); i++) { EXPECT_EQUAL(i, tmd.size()); EXPECT_EQUAL(std::min(size_t(std::numeric_limits<uint16_t>::max()), vespalib::roundUp2inN(i)), tmd.capacity()); tmd.appendPosition(pos); + const TermFieldMatchDataPosition * cur = tmd.begin(); + if (cur != prev) { + prev = cur; + resizeCount++; + } } + EXPECT_EQUAL(15u, resizeCount); EXPECT_EQUAL(std::numeric_limits<uint16_t>::max(), tmd.size()); EXPECT_EQUAL(std::numeric_limits<uint16_t>::max(), tmd.capacity()); tmd.appendPosition(pos); + EXPECT_EQUAL(prev, tmd.begin()); EXPECT_EQUAL(std::numeric_limits<uint16_t>::max(), tmd.size()); EXPECT_EQUAL(std::numeric_limits<uint16_t>::max(), tmd.capacity()); } diff --git a/searchlib/src/vespa/searchlib/fef/termfieldmatchdata.cpp b/searchlib/src/vespa/searchlib/fef/termfieldmatchdata.cpp index 4de84866edf..781135bf246 100644 --- a/searchlib/src/vespa/searchlib/fef/termfieldmatchdata.cpp +++ b/searchlib/src/vespa/searchlib/fef/termfieldmatchdata.cpp @@ -142,6 +142,7 @@ TermFieldMatchData::allocateVector() void TermFieldMatchData::appendPositionToAllocatedVector(const TermFieldMatchDataPosition &pos) { + if (__builtin_expect(_sz >= MAX_ELEMS, false)) return; assert(allocated()); if (__builtin_expect(_sz >= _data._positions._allocated, false)) { resizePositionVector(_sz*2); @@ -149,9 +150,7 @@ TermFieldMatchData::appendPositionToAllocatedVector(const TermFieldMatchDataPosi if (__builtin_expect(pos.getElementLen() > _data._positions._maxElementLength, false)) { _data._positions._maxElementLength = pos.getElementLen(); } - if (__builtin_expect(_sz < MAX_ELEMS, true)) { - _data._positions._positions[_sz++] = pos; - } + _data._positions._positions[_sz++] = pos; } } diff --git a/storage/src/vespa/storage/visiting/commandqueue.h b/storage/src/vespa/storage/visiting/commandqueue.h index 498cf14567d..6d9bfc1ca4f 100644 --- a/storage/src/vespa/storage/visiting/commandqueue.h +++ b/storage/src/vespa/storage/visiting/commandqueue.h @@ -16,7 +16,7 @@ #include <boost/multi_index/ordered_index.hpp> #include <boost/multi_index/sequenced_index.hpp> #include <vespa/vespalib/util/printable.h> -#include <vespa/vespalib//util/time.h> +#include <vespa/vespalib/util/time.h> #include <vespa/storageframework/generic/clock/clock.h> #include <list> #include <ostream> diff --git a/vespajlib/src/main/java/com/yahoo/tensor/Tensor.java b/vespajlib/src/main/java/com/yahoo/tensor/Tensor.java index b8ef84cabb7..cffd41905a1 100644 --- a/vespajlib/src/main/java/com/yahoo/tensor/Tensor.java +++ b/vespajlib/src/main/java/com/yahoo/tensor/Tensor.java @@ -18,6 +18,7 @@ import com.yahoo.tensor.functions.Reduce; import com.yahoo.tensor.functions.Rename; import com.yahoo.tensor.functions.Softmax; import com.yahoo.tensor.functions.XwPlusB; +import com.yahoo.text.Ascii7BitMatcher; import java.util.ArrayList; import java.util.Arrays; @@ -31,6 +32,8 @@ import java.util.function.DoubleBinaryOperator; import java.util.function.DoubleUnaryOperator; import java.util.function.Function; +import static com.yahoo.text.Ascii7BitMatcher.charsAndNumbers; + /** * A multidimensional array which can be used in computations. * <p> diff --git a/vespajlib/src/main/java/com/yahoo/tensor/TensorAddress.java b/vespajlib/src/main/java/com/yahoo/tensor/TensorAddress.java index 4770ad1b1f0..a3805fb789a 100644 --- a/vespajlib/src/main/java/com/yahoo/tensor/TensorAddress.java +++ b/vespajlib/src/main/java/com/yahoo/tensor/TensorAddress.java @@ -1,14 +1,11 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.tensor; -import com.yahoo.text.Ascii7BitMatcher; - import java.util.Arrays; +import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; -import static com.yahoo.text.Ascii7BitMatcher.charsAndNumbers; - /** * An immutable address to a tensor cell. This simply supplies a value to each dimension * in a particular tensor type. By itself it is just a list of cell labels, it's meaning depends on its accompanying type. @@ -85,7 +82,7 @@ public abstract class TensorAddress implements Comparable<TensorAddress> { public final String toString(TensorType type) { StringBuilder b = new StringBuilder("{"); for (int i = 0; i < size(); i++) { - b.append(type.dimensions().get(i).name()).append(":").append(label(i)); + b.append(type.dimensions().get(i).name()).append(":").append(labelToString(label(i))); b.append(","); } if (b.length() > 1) @@ -94,6 +91,12 @@ public abstract class TensorAddress implements Comparable<TensorAddress> { return b.toString(); } + private String labelToString(String label) { + if (TensorType.labelMatcher.matches(label)) return label; // no quoting + if (label.contains("'")) return "\"" + label + "\""; + return "'" + label + "'"; + } + private static final class StringTensorAddress extends TensorAddress { private final String[] labels; @@ -166,8 +169,6 @@ public abstract class TensorAddress implements Comparable<TensorAddress> { /** Supports building of a tensor address */ public static class Builder { - static private final Ascii7BitMatcher labelMatcher = new Ascii7BitMatcher("-_@" + charsAndNumbers(), - "_@$" + charsAndNumbers()); private final TensorType type; private final String[] labels; @@ -187,10 +188,10 @@ public abstract class TensorAddress implements Comparable<TensorAddress> { * @return this for convenience */ public Builder add(String dimension, String label) { - requireIdentifier(dimension, "dimension"); - requireIdentifier(label, "label"); + Objects.requireNonNull(dimension, "dimension cannot be null"); + Objects.requireNonNull(label, "label cannot be null"); Optional<Integer> labelIndex = type.indexOfDimension(dimension); - if ( ! labelIndex.isPresent()) + if ( labelIndex.isEmpty()) throw new IllegalArgumentException(type + " does not contain dimension '" + dimension + "'"); labels[labelIndex.get()] = label; return this; @@ -209,13 +210,6 @@ public abstract class TensorAddress implements Comparable<TensorAddress> { return TensorAddress.of(labels); } - static private void requireIdentifier(String s, String parameterName) { - if (s == null) - throw new IllegalArgumentException(parameterName + " can not be null"); - if ( ! labelMatcher.matches(s)) - throw new IllegalArgumentException(parameterName + " must be an identifier or integer, not '" + s + "'"); - } - } } diff --git a/vespajlib/src/main/java/com/yahoo/tensor/TensorParser.java b/vespajlib/src/main/java/com/yahoo/tensor/TensorParser.java index 5a1fd98a009..9aa764a0b36 100644 --- a/vespajlib/src/main/java/com/yahoo/tensor/TensorParser.java +++ b/vespajlib/src/main/java/com/yahoo/tensor/TensorParser.java @@ -50,7 +50,7 @@ class TensorParser { valueString = valueString.trim(); if (valueString.startsWith("{") && (type.isEmpty() || type.get().rank() == 0 || valueString.substring(1).trim().startsWith("{") || valueString.substring(1).trim().equals("}"))) { - return tensorFromSparseValueString(valueString, type); + return tensorFromMappedValueString(valueString, type); } else if (valueString.startsWith("{")) { return tensorFromMixedValueString(valueString, type, dimensionOrder); @@ -73,35 +73,18 @@ class TensorParser { } /** Derives the tensor type from the first address string in the given tensor string */ - private static TensorType typeFromSparseValueString(String valueString) { - String s = valueString.substring(1).trim(); // remove tensor start - int firstKeyOrTensorEnd = s.indexOf('}'); - if (firstKeyOrTensorEnd < 0) - throw new IllegalArgumentException("Excepted a number or a string starting by '{', '[' or 'tensor(...):...'"); - String addressBody = s.substring(0, firstKeyOrTensorEnd).trim(); - if (addressBody.isEmpty()) return TensorType.empty; // Empty tensor - if ( ! addressBody.startsWith("{")) return TensorType.empty; // Single value tensor - - addressBody = addressBody.substring(1, addressBody.length()); // remove key start - if (addressBody.isEmpty()) return TensorType.empty; // Empty key - - TensorType.Builder builder = new TensorType.Builder(TensorType.Value.DOUBLE); - for (String elementString : addressBody.split(",")) { - String[] pair = elementString.split(":"); - if (pair.length != 2) - throw new IllegalArgumentException("Expecting argument elements to be on the form dimension:label, " + - "got '" + elementString + "'"); - builder.mapped(pair[0].trim()); - } - + private static TensorType typeFromMappedValueString(String valueString) { + TensorType.Builder builder = new TensorType.Builder(); + MappedValueTypeParser parser = new MappedValueTypeParser(valueString, builder); + parser.parse(); return builder.build(); } - private static Tensor tensorFromSparseValueString(String valueString, Optional<TensorType> type) { + private static Tensor tensorFromMappedValueString(String valueString, Optional<TensorType> type) { try { valueString = valueString.trim(); - Tensor.Builder builder = Tensor.Builder.of(type.orElse(typeFromSparseValueString(valueString))); - SparseValueParser parser = new SparseValueParser(valueString, builder); + Tensor.Builder builder = Tensor.Builder.of(type.orElse(typeFromMappedValueString(valueString))); + MappedValueParser parser = new MappedValueParser(valueString, builder); parser.parse(); return builder.build(); } @@ -160,7 +143,7 @@ class TensorParser { } protected void skipSpace() { - while (position < string.length() && string.charAt(position) == ' ') + while (position < string.length() && Character.isWhitespace(string.charAt(position))) position++; } @@ -176,6 +159,37 @@ class TensorParser { position++; } + protected String consumeIdentifier() { + int endIdentifier = nextStopCharIndex(position, string); + String identifier = string.substring(position, endIdentifier); + position = endIdentifier; + return identifier; + } + + protected String consumeLabel() { + if (consumeOptional('\'')) { + int endQuote = string.indexOf('\'', position); + if (endQuote < 0) + throw new IllegalArgumentException("At value position " + position + + ": A label quoted by a tick (') must end by another tick"); + String label = string.substring(position, endQuote); + position = endQuote + 1; + return label; + } + else if (consumeOptional('"')) { + int endQuote = string.indexOf('"', position); + if (endQuote < 0) + throw new IllegalArgumentException("At value position " + position + + ": A label quoted by a double quote (\") must end by another double quote"); + String label = string.substring(position, endQuote); + position = endQuote + 1; + return label; + } + else { + return consumeIdentifier(); + } + } + protected Number consumeNumber(TensorType.Value cellValueType) { skipSpace(); @@ -199,15 +213,29 @@ class TensorParser { } } + protected boolean consumeOptional(char character) { + skipSpace(); + + if (position >= string.length()) + return false; + if ( string.charAt(position) != character) + return false; + + position++; + return true; + } + protected int nextStopCharIndex(int position, String valueString) { while (position < valueString.length()) { + if (Character.isWhitespace(valueString.charAt(position))) return position; if (valueString.charAt(position) == ',') return position; if (valueString.charAt(position) == ']') return position; if (valueString.charAt(position) == '}') return position; + if (valueString.charAt(position) == ':') return position; position++; } - throw new IllegalArgumentException("Malformed tensor value '" + valueString + - "': Expected a ',', ']' or '}' after position " + position); + throw new IllegalArgumentException("Malformed tensor string '" + valueString + + "': Expected a ',', ']' or '}', ':' after position " + position); } } @@ -291,13 +319,8 @@ class TensorParser { consume('{'); skipSpace(); while (position + 1 < string.length()) { - int labelEnd = string.indexOf(':', position); - if (labelEnd <= position) - throw new IllegalArgumentException("A mixed tensor value must be on the form {sparse-label:[dense subspace], ...}, or {sparse-label:value, ...}"); - String label = string.substring(position, labelEnd); - position = labelEnd + 1; - skipSpace(); - + String label = consumeLabel(); + consume(':'); TensorAddress mappedAddress = new TensorAddress.Builder(mappedSubtype).add(mappedDimension.name(), label).build(); if (builder.type().rank() > 1) parseDenseSubspace(mappedAddress, dimensionOrder); @@ -309,24 +332,12 @@ class TensorParser { } } - private void parseDenseSubspace(TensorAddress sparseAddress, List<String> denseDimensionOrder) { + private void parseDenseSubspace(TensorAddress mappedAddress, List<String> denseDimensionOrder) { DenseValueParser denseParser = new DenseValueParser(string.substring(position), denseDimensionOrder, - ((MixedTensor.BoundBuilder)builder).denseSubspaceBuilder(sparseAddress)); + ((MixedTensor.BoundBuilder)builder).denseSubspaceBuilder(mappedAddress)); denseParser.parse(); - position+= denseParser.position(); - } - - private boolean consumeOptional(char character) { - skipSpace(); - - if (position >= string.length()) - return false; - if ( string.charAt(position) != character) - return false; - - position++; - return true; + position += denseParser.position(); } private void consumeNumber(TensorAddress address) { @@ -339,11 +350,11 @@ class TensorParser { } - private static class SparseValueParser extends ValueParser { + private static class MappedValueParser extends ValueParser { private final Tensor.Builder builder; - public SparseValueParser(String string, Tensor.Builder builder) { + public MappedValueParser(String string, Tensor.Builder builder) { super(string); this.builder = builder; } @@ -352,22 +363,19 @@ class TensorParser { consume('{'); skipSpace(); while (position + 1 < string.length()) { - int keyOrTensorEnd = string.indexOf('}', position); - TensorAddress.Builder addressBuilder = new TensorAddress.Builder(builder.type()); - if (keyOrTensorEnd < string.length() - 1) { // Key end: This has a key - otherwise TensorAddress is empty - addLabels(string.substring(position, keyOrTensorEnd + 1), addressBuilder); - position = keyOrTensorEnd + 1; - skipSpace(); + TensorAddress address = consumeLabels(); + if ( ! address.isEmpty()) consume(':'); - } + else + consumeOptional(':'); + int valueEnd = string.indexOf(',', position); if (valueEnd < 0) { // last value valueEnd = string.indexOf('}', position); if (valueEnd < 0) - throw new IllegalArgumentException("A sparse tensor string must end by '}'"); + throw new IllegalArgumentException("A mapped tensor string must end by '}'"); } - TensorAddress address = addressBuilder.build(); TensorType.Value cellValueType = builder.type().valueType(); String cellValueString = string.substring(position, valueEnd).trim(); try { @@ -389,21 +397,46 @@ class TensorParser { } /** Creates a tensor address from a string on the form {dimension1:label1,dimension2:label2,...} */ - private static void addLabels(String mapAddressString, TensorAddress.Builder builder) { - mapAddressString = mapAddressString.trim(); - if ( ! (mapAddressString.startsWith("{") && mapAddressString.endsWith("}"))) - throw new IllegalArgumentException("Expecting a tensor address enclosed in {}, got '" + mapAddressString + "'"); - - String addressBody = mapAddressString.substring(1, mapAddressString.length() - 1).trim(); - if (addressBody.isEmpty()) return; - - for (String elementString : addressBody.split(",")) { - String[] pair = elementString.split(":"); - if (pair.length != 2) - throw new IllegalArgumentException("Expecting argument elements on the form dimension:label, " + - "got '" + elementString + "'"); - String dimension = pair[0].trim(); - builder.add(dimension, pair[1].trim()); + private TensorAddress consumeLabels() { + TensorAddress.Builder addressBuilder = new TensorAddress.Builder(builder.type()); + if ( ! consumeOptional('{')) return addressBuilder.build(); + while ( ! consumeOptional('}')) { + String dimension = consumeIdentifier(); + consume(':'); + String label = consumeLabel(); + addressBuilder.add(dimension, label); + consumeOptional(','); + } + return addressBuilder.build(); + } + + } + + /** Parses a tensor *value* into a type */ + private static class MappedValueTypeParser extends ValueParser { + + private final TensorType.Builder builder; + + public MappedValueTypeParser(String string, TensorType.Builder builder) { + super(string); + this.builder = builder; + } + + /** Derives the tensor type from the first address string in the given tensor string */ + public void parse() { + consume('{'); + consumeLabels(); + } + + /** Consumes a mapped address into a set of the type builder */ + private void consumeLabels() { + if ( ! consumeOptional('{')) return; + while ( ! consumeOptional('}')) { + String dimension = consumeIdentifier(); + consume(':'); + consumeLabel(); + builder.mapped(dimension); + consumeOptional(','); } } diff --git a/vespajlib/src/main/java/com/yahoo/tensor/TensorType.java b/vespajlib/src/main/java/com/yahoo/tensor/TensorType.java index ca3f8ff28a4..58cb151875e 100644 --- a/vespajlib/src/main/java/com/yahoo/tensor/TensorType.java +++ b/vespajlib/src/main/java/com/yahoo/tensor/TensorType.java @@ -2,9 +2,9 @@ package com.yahoo.tensor; import com.google.common.collect.ImmutableList; +import com.yahoo.text.Ascii7BitMatcher; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.LinkedHashMap; @@ -15,6 +15,8 @@ import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import static com.yahoo.text.Ascii7BitMatcher.charsAndNumbers; + /** * A tensor type with its dimensions. This is immutable. * <p> @@ -25,6 +27,8 @@ import java.util.stream.Collectors; */ public class TensorType { + static Ascii7BitMatcher labelMatcher = new Ascii7BitMatcher("-_@" + charsAndNumbers(), "_@$" + charsAndNumbers()); + /** The permissible cell value types. Default is double. */ public enum Value { @@ -292,8 +296,7 @@ public class TensorType { private final String name; private Dimension(String name) { - Objects.requireNonNull(name, "A tensor name cannot be null"); - this.name = name; + this.name = requireIdentifier(name); } public final String name() { return name; } @@ -361,6 +364,14 @@ public class TensorType { return new MappedDimension(name); } + static private String requireIdentifier(String name) { + if (name == null) + throw new IllegalArgumentException("A dimension name cannot be null"); + if ( ! TensorType.labelMatcher.matches(name)) + throw new IllegalArgumentException("A dimension name must be an identifier or integer, not '" + name + "'"); + return name; + } + } public static class IndexedBoundDimension extends TensorType.Dimension { diff --git a/vespajlib/src/test/java/com/yahoo/tensor/TensorParserTestCase.java b/vespajlib/src/test/java/com/yahoo/tensor/TensorParserTestCase.java index 6f9a5c13886..5a68df6c7df 100644 --- a/vespajlib/src/test/java/com/yahoo/tensor/TensorParserTestCase.java +++ b/vespajlib/src/test/java/com/yahoo/tensor/TensorParserTestCase.java @@ -9,6 +9,11 @@ import static org.junit.Assert.fail; public class TensorParserTestCase { @Test + public void testEmpty() { + assertEquals(Tensor.Builder.of(TensorType.empty).cell(1).build(), Tensor.from("tensor():{{}:1}")); + } + + @Test public void testSparseParsing() { assertEquals(Tensor.Builder.of(TensorType.fromSpec("tensor()")).build(), Tensor.from("{}")); @@ -18,7 +23,11 @@ public class TensorParserTestCase { Tensor.from("{{x:l0}:1.0}")); assertEquals("If the type is specified, a dense tensor can be created from the sparse text form", Tensor.Builder.of(TensorType.fromSpec("tensor(x[1])")).cell(1.0, 0).build(), - Tensor.from("tensor(x[1]):{{x:0}:1.0}")); + Tensor.from("tensor(x[1]):{{x:0 }:1.0}")); + assertEquals(Tensor.Builder.of(TensorType.fromSpec("tensor(x{})")).cell().label("x", "..\",]}:..").value(1.0).build(), + Tensor.from("{{x:'..\",]}:..'}:1.0}")); + assertEquals(Tensor.Builder.of(TensorType.fromSpec("tensor(x{})")).cell().label("x", "..'..").value(1.0).build(), + Tensor.from("{{x:\"..'..\"}:1.0}")); } @Test @@ -95,6 +104,12 @@ public class TensorParserTestCase { .cell(TensorAddress.ofLabels("b", "0"), 3) .cell(TensorAddress.ofLabels("b", "1"), 4).build(), Tensor.from("tensor(key{}, x[2]):{a:[1, 2], b:[3, 4]}")); + assertEquals(Tensor.Builder.of(TensorType.fromSpec("tensor(key{}, x[2])")) + .cell(TensorAddress.ofLabels(",:", "0"), 1) + .cell(TensorAddress.ofLabels(",:", "1"), 2) + .cell(TensorAddress.ofLabels("b", "0"), 3) + .cell(TensorAddress.ofLabels("b", "1"), 4).build(), + Tensor.from("tensor(key{}, x[2]):{',:':[1, 2], b:[3, 4]}")); } @Test @@ -103,6 +118,14 @@ public class TensorParserTestCase { .cell(TensorAddress.ofLabels("a"), 1) .cell(TensorAddress.ofLabels("b"), 2).build(), Tensor.from("tensor(key{}):{a:1, b:2}")); + assertEquals(Tensor.Builder.of(TensorType.fromSpec("tensor(key{})")) + .cell(TensorAddress.ofLabels("..\",}]:.."), 1) + .cell(TensorAddress.ofLabels("b"), 2).build(), + Tensor.from("tensor(key{}):{'..\",}]:..':1, b:2}")); + assertEquals(Tensor.Builder.of(TensorType.fromSpec("tensor(key{})")) + .cell(TensorAddress.ofLabels("..'.."), 1) + .cell(TensorAddress.ofLabels("b"), 2).build(), + Tensor.from("tensor(key{}):{\"..'..\":1, b:2}")); } @Test @@ -134,11 +157,9 @@ public class TensorParserTestCase { @Test public void testIllegalStrings() { - assertIllegal("label must be an identifier or integer, not '\"l0\"'", - "{{x:\"l0\"}:1.0}"); - assertIllegal("dimension must be an identifier or integer, not ''x''", + assertIllegal("A dimension name must be an identifier or integer, not ''x''", "{{'x':\"l0\"}:1.0}"); - assertIllegal("dimension must be an identifier or integer, not '\"x\"'", + assertIllegal("A dimension name must be an identifier or integer, not '\"x\"'", "{{\"x\":\"l0\", \"y\":\"l0\"}:1.0, {\"x\":\"l0\", \"y\":\"l1\"}:2.0}"); assertIllegal("At {x:0}: '1-.0' is not a valid double", "{{x:0}:1-.0}"); diff --git a/vespajlib/src/test/java/com/yahoo/tensor/TensorTestCase.java b/vespajlib/src/test/java/com/yahoo/tensor/TensorTestCase.java index 9f077cb7b00..7932f90d797 100644 --- a/vespajlib/src/test/java/com/yahoo/tensor/TensorTestCase.java +++ b/vespajlib/src/test/java/com/yahoo/tensor/TensorTestCase.java @@ -36,6 +36,9 @@ public class TensorTestCase { assertTrue(Tensor.from("tensor():{5.7}") instanceof IndexedTensor); assertEquals("tensor(d1{},d2{}):{{d1:l1,d2:l1}:5.0,{d1:l1,d2:l2}:6.0}", Tensor.from("{ {d1:l1,d2:l1}: 5, {d2:l2, d1:l1}:6.0} ").toString()); assertEquals("tensor(d1{},d2{}):{{d1:l1,d2:l1}:-5.3,{d1:l1,d2:l2}:0.0}", Tensor.from("{ {d1:l1,d2:l1}:-5.3, {d2:l2, d1:l1}:0}").toString()); + assertEquals("Labels are quoted when necessary", + "tensor(d1{}):{{d1:\"'''\"}:6.0,{d1:'[[\":\"]]'}:5.0}", + Tensor.from("{ {d1:'[[\":\"]]'}: 5, {d1:\"'''\"}:6.0 }").toString()); } @Test diff --git a/vespalib/src/vespa/vespalib/util/time.h b/vespalib/src/vespa/vespalib/util/time.h index d57d722efc4..719bb112c15 100644 --- a/vespalib/src/vespa/vespalib/util/time.h +++ b/vespalib/src/vespa/vespalib/util/time.h @@ -4,6 +4,7 @@ #include <chrono> #include <vespa/vespalib/stllike/string.h> +#include <sys/time.h> // Guidelines: // |