Log JVM opions to deploy log as well

2 files changed, 74 insertions, 29 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 05449083817..6d184666bdb 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
@@ -669,20 +669,35 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
     }
 
     private static String buildJvmGCOptions(ConfigModelContext context, String jvmGCOptions) {
-        return new JvmGcOptions(context.getDeployState(), jvmGCOptions, context.getDeployLogger()).build();
+        return new JvmGcOptions(context.getDeployState(), jvmGCOptions).build();
     }
 
-    private static String getJvmOptions(ApplicationContainerCluster cluster, Element nodesElement, DeployLogger deployLogger) {
-        return new JvmOptions(cluster, nodesElement, deployLogger).build();
+    private static String getJvmOptions(ApplicationContainerCluster cluster,
+                                        Element nodesElement,
+                                        DeployState deployState,
+                                        boolean legacyOptions) {
+        return new JvmOptions(cluster, nodesElement, deployState, legacyOptions).build();
     }
 
     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()));
+    private void extractJvmOptions(List<ApplicationContainer> nodes,
+                                   ApplicationContainerCluster cluster,
+                                   Element nodesElement,
+                                   ConfigModelContext context) {
+        Element jvmElement = XML.getChild(nodesElement, "jvm");
+        if (jvmElement == null) {
+            extractJvmFromLegacyNodesTag(nodes, cluster, nodesElement, context);
+        } else {
+            extractJvmTag(nodes, cluster, nodesElement, jvmElement, context);
+        }
+    }
+
+    private void extractJvmFromLegacyNodesTag(List<ApplicationContainer> nodes, ApplicationContainerCluster cluster,
+                                              Element nodesElement, ConfigModelContext context) {
+        applyNodesTagJvmArgs(nodes, getJvmOptions(cluster, nodesElement, context.getDeployState(), true));
 
         if (cluster.getJvmGCOptions().isEmpty()) {
             String jvmGCOptions = extractAttribute(nodesElement, VespaDomBuilder.JVM_GC_OPTIONS);
@@ -692,9 +707,9 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         applyMemoryPercentage(cluster, nodesElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME));
     }
 
-    void extractJvmTag(List<ApplicationContainer> nodes, ApplicationContainerCluster cluster,
-                       Element jvmElement, ConfigModelContext context) {
-        applyNodesTagJvmArgs(nodes, jvmElement.getAttribute(VespaDomBuilder.OPTIONS));
+    private void extractJvmTag(List<ApplicationContainer> nodes, ApplicationContainerCluster cluster,
+                               Element nodesElement, Element jvmElement, ConfigModelContext context) {
+        applyNodesTagJvmArgs(nodes, getJvmOptions(cluster, nodesElement, context.getDeployState(), false));
         applyMemoryPercentage(cluster, jvmElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME));
         String jvmGCOptions = extractAttribute(jvmElement, VespaDomBuilder.GC_OPTIONS);
         cluster.setJvmGCOptions(buildJvmGCOptions(context, jvmGCOptions));
@@ -714,12 +729,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         } else {
             List<ApplicationContainer> nodes = createNodes(cluster, containerElement, nodesElement, context);
 
-            Element jvmElement = XML.getChild(nodesElement, "jvm");
-            if (jvmElement == null) {
-                extractJvmFromLegacyNodesTag(nodes, cluster, nodesElement, context);
-            } else {
-                extractJvmTag(nodes, cluster, jvmElement, context);
-            }
+            extractJvmOptions(nodes, cluster, nodesElement, context);
             applyRoutingAliasProperties(nodes, cluster);
             applyDefaultPreload(nodes, nodesElement);
             String environmentVars = getEnvironmentVariables(XML.getChild(nodesElement, ENVIRONMENT_VARIABLES_ELEMENT));
@@ -1062,18 +1072,35 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
 
         private final ContainerCluster<?> cluster;
         private final Element nodesElement;
-        private final DeployLogger deployLogger;
+        private final DeployLogger logger;
+        private final boolean isHosted;
+        private final boolean legacyOptions;
 
-        public JvmOptions(ContainerCluster<?> cluster, Element nodesElement, DeployLogger deployLogger) {
+        public JvmOptions(ContainerCluster<?> cluster, Element nodesElement, DeployState deployState, boolean legacyOptions) {
             this.cluster = cluster;
             this.nodesElement = nodesElement;
-            this.deployLogger = deployLogger;
+            this.logger = deployState.getDeployLogger();
+            this.isHosted = deployState.isHosted();
+            this.legacyOptions = legacyOptions;
         }
 
         String build() {
+            if (legacyOptions)
+                return buildLegacyOptions();
+
+            Element jvmElement = XML.getChild(nodesElement, "jvm");
+            if (jvmElement == null) return "";
+            String jvmOptions = jvmElement.getAttribute(VespaDomBuilder.OPTIONS);
+            if (jvmOptions == null) return "";
+            log(jvmOptions);
+            return jvmOptions;
+        }
+
+        String buildLegacyOptions() {
             String jvmOptions;
             if (nodesElement.hasAttribute(VespaDomBuilder.JVM_OPTIONS)) {
                 jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVM_OPTIONS);
+                log(jvmOptions);
                 if (nodesElement.hasAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME)) {
                     String jvmArgs = nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME);
                     throw new IllegalArgumentException("You have specified both jvm-options='" + jvmOptions + "'" +
@@ -1083,14 +1110,20 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                 }
             } else {
                 jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME);
+                log(jvmOptions);
                 if (incompatibleGCOptions(jvmOptions)) {
-                    deployLogger.logApplicationPackage(WARNING, "You need to move your GC-related options from deprecated 'jvmargs' to 'gc-options' in 'jvm' element." +
+                    logger.logApplicationPackage(WARNING, "You need to move your GC-related options from deprecated 'jvmargs' to 'gc-options' in 'jvm' element." +
                             " See https://docs.vespa.ai/en/reference/services-container.html#jvm");
                     cluster.setJvmGCOptions(ContainerCluster.G1GC);
                 }
             }
             return jvmOptions;
         }
+
+        private void log(String jvmOptions) {
+            if (isHosted && jvmOptions != null && !jvmOptions.isEmpty())
+                logger.logApplicationPackage(Level.INFO, "JVM options from services.xml: " + jvmOptions);
+        }
     }
 
     private static class JvmGcOptions {
@@ -1098,19 +1131,20 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         private final DeployState deployState;
         private final String jvmGcOptions;
         private final DeployLogger logger;
+        private final boolean isHosted;
 
-        public JvmGcOptions(DeployState deployState, String jvmGcOptions, DeployLogger logger) {
+        public JvmGcOptions(DeployState deployState, String jvmGcOptions) {
             this.deployState = deployState;
             this.jvmGcOptions = jvmGcOptions;
-            this.logger = logger;
+            this.logger = deployState.getDeployLogger();
+            this.isHosted = deployState.isHosted();
         }
 
         private String build() {
             String options = deployState.getProperties().jvmGCOptions();
             if (jvmGcOptions != null) {
+                log(jvmGcOptions);
                 options = jvmGcOptions;
-                if (deployState.isHosted())
-                    logger.logApplicationPackage(Level.INFO, "JVM GC options from services.xml: " + jvmGcOptions);
                 // TODO: Verify options against lists of allowed and/or disallowed options
             }
 
@@ -1120,6 +1154,11 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             return options;
         }
 
+        private void log(String jvmGcOptions) {
+            if (isHosted)
+                logger.logApplicationPackage(Level.INFO, "JVM GC options from services.xml: " + 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 815062fb8d9..a674a06d45e 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
@@ -146,16 +146,16 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
 
     @Test
     public void requireThatJvmGcOptionsAreLogged()  throws IOException, SAXException {
-        verifyLoggingOfJvmGCOptions(true, "-XX:+UseCMSInitiatingOccupancyOnly foo     bar");
-        verifyLoggingOfJvmGCOptions(true, "-XX:+UseConcMarkSweepGC");
-        verifyLoggingOfJvmGCOptions(false, "-XX:+UseConcMarkSweepGC");
+        verifyLoggingOfJvmOptions(true, "gc-options", "-XX:+UseCMSInitiatingOccupancyOnly foo     bar");
+        verifyLoggingOfJvmOptions(true, "gc-options", "-XX:+UseConcMarkSweepGC");
+        verifyLoggingOfJvmOptions(false, "gc-options", "-XX:+UseConcMarkSweepGC");
     }
 
-    private void verifyLoggingOfJvmGCOptions(boolean isHosted, String override) throws IOException, SAXException  {
+    private void verifyLoggingOfJvmOptions(boolean isHosted, String optionName, String override) throws IOException, SAXException  {
         String servicesXml =
                 "<container version='1.0'>" +
                 "  <nodes>" +
-                "    <jvm gc-options='" + override + "'/>" +
+                "    <jvm " + optionName + "='" + override + "'/>" +
                 "    <node hostalias='mockhost'/>" +
                 "  </nodes>" +
                 "</container>";
@@ -169,11 +169,17 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
         if (isHosted) {
             Pair<Level, String> firstOption = logger.msgs.get(0);
             assertEquals(Level.INFO, firstOption.getFirst());
-            assertEquals("JVM GC options from services.xml: " + override, firstOption.getSecond());
+            assertEquals("JVM " + (optionName.equals("gc-options") ? "GC " : "") +
+                                 "options from services.xml: " + override, firstOption.getSecond());
         } else {
             assertEquals(0, logger.msgs.size());
         }
     }
 
+    @Test
+    public void requireThatJvmOptionsAreLogged()  throws IOException, SAXException {
+        verifyLoggingOfJvmOptions(true, "options", "-Xms2G");
+        verifyLoggingOfJvmOptions(false, "options", "-Xms2G");
+    }
 
 }