From 2487403f2205c99a79f9053d45ce34c8cfb5d03a Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Wed, 10 Jun 2020 14:32:02 +0200 Subject: Revert "Revert "Add feature flag control of default jvm gc options"" --- .../com/yahoo/config/model/api/ModelContext.java | 3 +++ .../yahoo/config/model/deploy/TestProperties.java | 6 +++++ .../model/container/xml/ContainerModelBuilder.java | 31 ++++++++++------------ .../vespa/model/container/xml/JvmOptionsTest.java | 29 +++++++------------- .../config/server/deploy/ModelContextImpl.java | 5 ++++ .../src/main/java/com/yahoo/vespa/flags/Flags.java | 5 ++++ 6 files changed, 43 insertions(+), 36 deletions(-) diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java index ad91ad6e1bc..2fa3217ef4a 100644 --- a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java +++ b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java @@ -89,6 +89,9 @@ public interface ModelContext { String docprocLoadBalancerType(); + /// Default setting for the gc-options attribute if not specified explicit by application + String jvmGCOptions(); + boolean useDistributorBtreeDb(); boolean useThreePhaseUpdates(); diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java index b00eb900221..af3a8335475 100644 --- a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java +++ b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java @@ -43,6 +43,7 @@ public class TestProperties implements ModelContext.Properties { private double threadPoolSizeFactor = 0.0; private double queueSizeFactor = 0.0; private String docprocLoadBalancerType = null; + private String jvmGCOptions = null; private Optional endpointCertificateSecrets = Optional.empty(); private AthenzDomain athenzDomain; private ApplicationRoles applicationRoles; @@ -56,6 +57,7 @@ public class TestProperties implements ModelContext.Properties { @Override public boolean hostedVespa() { return hostedVespa; } @Override public Zone zone() { return zone; } @Override public Set endpoints() { return endpoints; } + @Override public String jvmGCOptions() { return jvmGCOptions; } @Override public boolean isBootstrap() { return false; } @Override public boolean isFirstTimeDeployment() { return false; } @@ -78,6 +80,10 @@ public class TestProperties implements ModelContext.Properties { docprocLoadBalancerType = type; return this; } + public TestProperties setJvmGCOptions(String gcOptions) { + jvmGCOptions = gcOptions; + return this; + } public TestProperties setDefaultTermwiseLimit(double limit) { defaultTermwiseLimit = limit; return this; diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index 559a4b8b668..6ae01ad0177 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -513,17 +513,14 @@ public class ContainerModelBuilder extends ConfigModelBuilder { return (gcAlgorithm.matcher(jvmargs).find() ||cmsArgs.matcher(jvmargs).find()); } - private static String buildJvmGCOptions(Zone zone, String jvmGCOPtions, boolean isHostedVespa) { - if (jvmGCOPtions != null) { - return jvmGCOPtions; - } else if ((zone.system() == SystemName.dev) || isHostedVespa) { - return null; - } else { - return ContainerCluster.G1GC; - } + private static String buildJvmGCOptions(DeployState deployState, String jvmGCOPtions) { + String options = (jvmGCOPtions != null) + ? jvmGCOPtions + : deployState.getProperties().jvmGCOptions(); + return (options == null ||options.isEmpty()) ? ContainerCluster.G1GC : options; } private static String getJvmOptions(ApplicationContainerCluster cluster, Element nodesElement, DeployLogger deployLogger) { - String jvmOptions = ""; + String jvmOptions; if (nodesElement.hasAttribute(VespaDomBuilder.JVM_OPTIONS)) { jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVM_OPTIONS); if (nodesElement.hasAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME)) { @@ -541,15 +538,17 @@ public class ContainerModelBuilder extends ConfigModelBuilder { return jvmOptions; } + private static String extractAttribute(Element element, String attrName) { + return element.hasAttribute(attrName) ? element.getAttribute(attrName) : null; + } + void extractJvmFromLegacyNodesTag(List nodes, ApplicationContainerCluster cluster, Element nodesElement, ConfigModelContext context) { applyNodesTagJvmArgs(nodes, getJvmOptions(cluster, nodesElement, context.getDeployLogger())); if (!cluster.getJvmGCOptions().isPresent()) { - String jvmGCOptions = nodesElement.hasAttribute(VespaDomBuilder.JVM_GC_OPTIONS) - ? nodesElement.getAttribute(VespaDomBuilder.JVM_GC_OPTIONS) - : null; - cluster.setJvmGCOptions(buildJvmGCOptions(context.getDeployState().zone(), jvmGCOptions, context.getDeployState().isHosted())); + String jvmGCOptions = extractAttribute(nodesElement, VespaDomBuilder.JVM_GC_OPTIONS); + cluster.setJvmGCOptions(buildJvmGCOptions(context.getDeployState(), jvmGCOptions)); } applyMemoryPercentage(cluster, nodesElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME)); @@ -559,10 +558,8 @@ public class ContainerModelBuilder extends ConfigModelBuilder { Element jvmElement, ConfigModelContext context) { applyNodesTagJvmArgs(nodes, jvmElement.getAttribute(VespaDomBuilder.OPTIONS)); applyMemoryPercentage(cluster, jvmElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME)); - String jvmGCOptions = jvmElement.hasAttribute(VespaDomBuilder.GC_OPTIONS) - ? jvmElement.getAttribute(VespaDomBuilder.GC_OPTIONS) - : null; - cluster.setJvmGCOptions(buildJvmGCOptions(context.getDeployState().zone(), jvmGCOptions, context.getDeployState().isHosted())); + String jvmGCOptions = extractAttribute(jvmElement, VespaDomBuilder.GC_OPTIONS); + cluster.setJvmGCOptions(buildJvmGCOptions(context.getDeployState(), jvmGCOptions)); } /** diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java index 484709a0c18..448ffffc04b 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java @@ -50,7 +50,6 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() .applicationPackage(applicationPackage) .deployLogger(logger) - .properties(new TestProperties().setHostedVespa(true)) .build()); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); model.getConfig(qrStartBuilder, "container/container.0"); @@ -85,11 +84,11 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { } private static void verifyIgnoreJvmGCOptions(boolean isHosted) throws IOException, SAXException { - verifyIgnoreJvmGCOptionsIfJvmArgs(isHosted, "jvmargs", ContainerCluster.G1GC); - verifyIgnoreJvmGCOptionsIfJvmArgs(isHosted, "jvm-options", "-XX:+UseG1GC"); + verifyIgnoreJvmGCOptionsIfJvmArgs("jvmargs", ContainerCluster.G1GC); + verifyIgnoreJvmGCOptionsIfJvmArgs( "jvm-options", "-XX:+UseG1GC"); } - private static void verifyIgnoreJvmGCOptionsIfJvmArgs(boolean isHosted, String jvmOptionsName, String expectedGC) throws IOException, SAXException { + private static void verifyIgnoreJvmGCOptionsIfJvmArgs(String jvmOptionsName, String expectedGC) throws IOException, SAXException { String servicesXml = "" + " " + @@ -102,8 +101,6 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() .applicationPackage(applicationPackage) .deployLogger(logger) - .zone(new Zone(SystemName.cd, Environment.dev, RegionName.from("here"))) - .properties(new TestProperties().setHostedVespa(isHosted)) .build()); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); model.getConfig(qrStartBuilder, "container/container.0"); @@ -117,7 +114,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { verifyIgnoreJvmGCOptions(true); } - private void verifyJvmGCOptions(boolean isHosted, String override, Zone zone, String expected) throws IOException, SAXException { + private void verifyJvmGCOptions(String featureFlagDefault, String override, String expected) throws IOException, SAXException { String servicesXml = "" + " " : ("jvm-gc-options='" + override + "'>")) + @@ -130,8 +127,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() .applicationPackage(applicationPackage) .deployLogger(logger) - .zone(zone) - .properties(new TestProperties().setHostedVespa(isHosted)) + .properties(new TestProperties().setJvmGCOptions(featureFlagDefault)) .build()); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); model.getConfig(qrStartBuilder, "container/container.0"); @@ -139,18 +135,13 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { assertEquals(expected, qrStartConfig.jvm().gcopts()); } - private void verifyJvmGCOptions(boolean isHosted, Zone zone, String expected) throws IOException, SAXException { - verifyJvmGCOptions(isHosted, null, zone, expected); - verifyJvmGCOptions(isHosted, "-XX:+UseG1GC", zone, "-XX:+UseG1GC"); - Zone DEV = new Zone(SystemName.dev, zone.environment(), zone.region()); - verifyJvmGCOptions(isHosted, null, DEV, ContainerCluster.G1GC); - verifyJvmGCOptions(isHosted, "-XX:+UseConcMarkSweepGC", DEV, "-XX:+UseConcMarkSweepGC"); - } - @Test public void requireThatJvmGCOptionsIsHonoured() throws IOException, SAXException { - verifyJvmGCOptions(false, Zone.defaultZone(),ContainerCluster.G1GC); - verifyJvmGCOptions(true, Zone.defaultZone(), ContainerCluster.G1GC); + verifyJvmGCOptions(null,null, ContainerCluster.G1GC); + verifyJvmGCOptions("-XX:+UseConcMarkSweepGC",null, "-XX:+UseConcMarkSweepGC"); + verifyJvmGCOptions(null,"-XX:+UseG1GC", "-XX:+UseG1GC"); + verifyJvmGCOptions("-XX:+UseConcMarkSweepGC","-XX:+UseG1GC", "-XX:+UseG1GC"); + verifyJvmGCOptions(null,"-XX:+UseConcMarkSweepGC", "-XX:+UseConcMarkSweepGC"); } } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java index c925157b980..3a2bc9f1a81 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java @@ -154,6 +154,7 @@ public class ModelContextImpl implements ModelContext { private final double threadPoolSizeFactor; private final double queueSizefactor; private final String docprocLoadBalancerType; + private final String jvmGCOPtions; private final Optional athenzDomain; private final Optional applicationRoles; private final int jdiscHealthCheckProxyClientTimeout; @@ -197,6 +198,8 @@ public class ModelContextImpl implements ModelContext { .with(FetchVector.Dimension.APPLICATION_ID, applicationId.serializedForm()).value(); docprocLoadBalancerType = Flags.DOCPROC_LOADBALANCER_TYPE.bindTo(flagSource) .with(FetchVector.Dimension.APPLICATION_ID, applicationId.serializedForm()).value(); + jvmGCOPtions = Flags.JVM_GC_OPTIONS.bindTo(flagSource) + .with(FetchVector.Dimension.APPLICATION_ID, applicationId.serializedForm()).value(); this.athenzDomain = athenzDomain; this.applicationRoles = applicationRoles; jdiscHealthCheckProxyClientTimeout = Flags.JDISC_HEALTH_CHECK_PROXY_CLIENT_TIMEOUT.bindTo(flagSource) @@ -280,6 +283,8 @@ public class ModelContextImpl implements ModelContext { } @Override public Duration jdiscHealthCheckProxyClientTimeout() { return Duration.ofMillis(jdiscHealthCheckProxyClientTimeout); } + @Override public String jvmGCOptions() { return jvmGCOPtions; } + } } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index a9ded0f5fd2..30383393e97 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -152,6 +152,11 @@ public class Flags { "Selects what kind of load balancer to use for document processing {'adaptive', 'legacy' ''}", "Takes effect at redeployment", ZONE_ID, APPLICATION_ID); + public static final UnboundStringFlag JVM_GC_OPTIONS = defineStringFlag( + "jvm-gc-options", "", + "Sets deafult jvm gc options", + "Takes effect at redeployment", + ZONE_ID, APPLICATION_ID); public static final UnboundBooleanFlag USE_DISTRIBUTOR_BTREE_DB = defineFeatureFlag( "use-distributor-btree-db", false, -- cgit v1.2.3 From 97ed107ab9f8e75d1cba249f887a60558905ad7f Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Wed, 10 Jun 2020 14:09:41 +0000 Subject: Default gc in hosted is CMS --- .../model/container/xml/ContainerModelBuilder.java | 4 +++- .../vespa/model/container/xml/JvmOptionsTest.java | 24 +++++++++------------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index 6ae01ad0177..b83632a58a0 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -517,7 +517,9 @@ public class ContainerModelBuilder extends ConfigModelBuilder { String options = (jvmGCOPtions != null) ? jvmGCOPtions : deployState.getProperties().jvmGCOptions(); - return (options == null ||options.isEmpty()) ? ContainerCluster.G1GC : options; + return (options == null ||options.isEmpty()) + ? (deployState.isHosted() ? ContainerCluster.CMS : ContainerCluster.G1GC) + : options; } private static String getJvmOptions(ApplicationContainerCluster cluster, Element nodesElement, DeployLogger deployLogger) { String jvmOptions; diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java index 448ffffc04b..1bc9b65bdad 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java @@ -1,7 +1,4 @@ // Copyright 2020 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -/* - * Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - */ package com.yahoo.vespa.model.container.xml; @@ -11,10 +8,7 @@ import com.yahoo.config.model.builder.xml.test.DomBuilderTest; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.deploy.TestProperties; import com.yahoo.config.model.test.MockApplicationPackage; -import com.yahoo.config.provision.Environment; -import com.yahoo.config.provision.RegionName; -import com.yahoo.config.provision.SystemName; -import com.yahoo.config.provision.Zone; + import com.yahoo.search.config.QrStartConfig; import com.yahoo.vespa.model.VespaModel; import com.yahoo.vespa.model.container.ContainerCluster; @@ -114,7 +108,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { verifyIgnoreJvmGCOptions(true); } - private void verifyJvmGCOptions(String featureFlagDefault, String override, String expected) throws IOException, SAXException { + private void verifyJvmGCOptions(boolean isHosted, String featureFlagDefault, String override, String expected) throws IOException, SAXException { String servicesXml = "" + " " : ("jvm-gc-options='" + override + "'>")) + @@ -127,7 +121,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() .applicationPackage(applicationPackage) .deployLogger(logger) - .properties(new TestProperties().setJvmGCOptions(featureFlagDefault)) + .properties(new TestProperties().setJvmGCOptions(featureFlagDefault).setHostedVespa(isHosted)) .build()); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); model.getConfig(qrStartBuilder, "container/container.0"); @@ -137,11 +131,13 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { @Test public void requireThatJvmGCOptionsIsHonoured() throws IOException, SAXException { - verifyJvmGCOptions(null,null, ContainerCluster.G1GC); - verifyJvmGCOptions("-XX:+UseConcMarkSweepGC",null, "-XX:+UseConcMarkSweepGC"); - verifyJvmGCOptions(null,"-XX:+UseG1GC", "-XX:+UseG1GC"); - verifyJvmGCOptions("-XX:+UseConcMarkSweepGC","-XX:+UseG1GC", "-XX:+UseG1GC"); - verifyJvmGCOptions(null,"-XX:+UseConcMarkSweepGC", "-XX:+UseConcMarkSweepGC"); + verifyJvmGCOptions(false, null,null, ContainerCluster.G1GC); + verifyJvmGCOptions(true, null,null, ContainerCluster.CMS); + verifyJvmGCOptions(false, "-XX:+UseConcMarkSweepGC",null, "-XX:+UseConcMarkSweepGC"); + verifyJvmGCOptions(true, "-XX:+UseConcMarkSweepGC",null, "-XX:+UseConcMarkSweepGC"); + verifyJvmGCOptions(false, null,"-XX:+UseG1GC", "-XX:+UseG1GC"); + verifyJvmGCOptions(false, "-XX:+UseConcMarkSweepGC","-XX:+UseG1GC", "-XX:+UseG1GC"); + verifyJvmGCOptions(false, null,"-XX:+UseConcMarkSweepGC", "-XX:+UseConcMarkSweepGC"); } } -- cgit v1.2.3