Revert "Revert "Add feature flag control of default jvm gc options""

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> 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<ContainerEndpoint> 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<ContainerModel> {
         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<ContainerModel> {
         return jvmOptions;
     }
 
+    private static String extractAttribute(Element element, String attrName) {
+        return element.hasAttribute(attrName) ? element.getAttribute(attrName) : null;
+    }
+
     void extractJvmFromLegacyNodesTag(List<ApplicationContainer> 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<ContainerModel> {
                        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 =
                 "<container version='1.0'>" +
                         "  <nodes jvm-gc-options='-XX:+UseG1GC' " + jvmOptionsName + "='-XX:+UseParNewGC'>" +
@@ -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 =
                 "<container version='1.0'>" +
                         "  <nodes " + ((override == null) ? ">" : ("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> athenzDomain;
         private final Optional<ApplicationRoles> 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,