From b2aaa73603b6b21df37639d73ed3410d3d680c42 Mon Sep 17 00:00:00 2001
From: Henning Baldersheim <balder@yahoo-inc.com>
Date: Thu, 25 Oct 2018 15:28:46 +0200
Subject: Detect conflicting jvmargs.

---
 .../model/container/xml/ContainerModelBuilder.java | 19 ++++++++--
 .../container/xml/ContainerModelBuilderTest.java   | 41 ++++++++++++++++++++++
 2 files changed, 58 insertions(+), 2 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 aeb3123398c..29763839dac 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
@@ -76,6 +76,8 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Consumer;
+import java.util.logging.Level;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 /**
@@ -449,6 +451,13 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         cluster.addContainers(Collections.singleton(container));
     }
 
+    static boolean incompatibleGCOptions(String jvmargs) {
+        Pattern gcAlgorithm = Pattern.compile("-XX:[-+]Use.+GC");
+        if (gcAlgorithm.matcher(jvmargs).find()) {
+            return true;
+        }
+        return false;
+    }
     private void addNodesFromXml(ContainerCluster cluster, Element containerElement, ConfigModelContext context) {
         Element nodesElement = XML.getChild(containerElement, "nodes");
         if (nodesElement == null) { // default single node on localhost
@@ -460,8 +469,14 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         }
         else {
             List<Container> nodes = createNodes(cluster, nodesElement, context);
-            cluster.setGCOpts(nodesElement.getAttribute(VespaDomBuilder.GCOPTS_ATTRIB_NAME));
-            applyNodesTagJvmArgs(nodes, nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME));
+            String jvmArgs = nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME);
+            String gcopts = nodesElement.getAttribute(VespaDomBuilder.GCOPTS_ATTRIB_NAME);
+            if (incompatibleGCOptions(jvmArgs)) {
+                context.getDeployLogger().log(Level.WARNING, "You need to move out your GC related options from 'jvmargs' to 'gcopts'");
+            } else {
+                cluster.setGCOpts(gcopts);
+            }
+            applyNodesTagJvmArgs(nodes, jvmArgs);
             applyRoutingAliasProperties(nodes, cluster);
             applyDefaultPreload(nodes, nodesElement);
             applyMemoryPercentage(cluster, nodesElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME));
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
index f94ebab42a9..5d708dcddf4 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
@@ -28,6 +28,7 @@ import com.yahoo.container.usability.BindingsOverviewHandler;
 import com.yahoo.jdisc.http.ServletPathsConfig;
 import com.yahoo.net.HostName;
 import com.yahoo.prelude.cluster.QrMonitorConfig;
+import com.yahoo.search.config.QrStartConfig;
 import com.yahoo.vespa.model.AbstractService;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.container.Container;
@@ -67,6 +68,46 @@ import static org.junit.Assert.fail;
  */
 public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase {
 
+    @Test
+    public void detect_conflicting_gcoptions_in_jvmargs() {
+        assertFalse(ContainerModelBuilder.incompatibleGCOptions(""));
+        assertFalse(ContainerModelBuilder.incompatibleGCOptions("UseG1GC"));
+        assertTrue(ContainerModelBuilder.incompatibleGCOptions("-XX:+UseG1GC"));
+        assertTrue(ContainerModelBuilder.incompatibleGCOptions("abc -XX:+UseParNewGC xyz"));
+    }
+
+    @Test
+    public void honours_gcopts() {
+        Element clusterElem = DomBuilderTest.parse(
+                "<jdisc version='1.0'>",
+                "  <search/>",
+                "  <nodes gcopts='-XX:+UseG1GC'>",
+                "    <node hostalias='mockhost'/>",
+                "  </nodes>",
+                "</jdisc>" );
+        createModel(root, clusterElem);
+        QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder();
+        root.getConfig(qrStartBuilder, "jdisc/container.0");
+        QrStartConfig qrStartConfig = new QrStartConfig(qrStartBuilder);
+        assertEquals("-XX:+UseG1GC", qrStartConfig.jvm().gcopts());
+    }
+
+    @Test
+    public void ignores_gcopts_on_conflicting_jvargs() {
+        Element clusterElem = DomBuilderTest.parse(
+                "<jdisc version='1.0'>",
+                "  <search/>",
+                "  <nodes gcopts='-XX:+UseG1GC' jvmargs='-XX:+UseParNewGC'>",
+                "    <node hostalias='mockhost'/>",
+                "  </nodes>",
+                "</jdisc>" );
+        createModel(root, clusterElem);
+        QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder();
+        root.getConfig(qrStartBuilder, "jdisc/container.0");
+        QrStartConfig qrStartConfig = new QrStartConfig(qrStartBuilder);
+        assertEquals(ContainerCluster.CMS, qrStartConfig.jvm().gcopts());
+    }
+
     @Test
     public void default_port_is_4080() throws Exception {
         Element clusterElem = DomBuilderTest.parse(
-- 
cgit v1.2.3