Merge pull request #25373 from vespa-engine/hmusum/log-warning-when-deprecated-jvm-options-used

Log warning when deprecated jvm options used [run-systemtest]

4 files changed, 97 insertions, 36 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 2c12ddb34a3..d3fb8837fcb 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
@@ -98,7 +98,6 @@ import com.yahoo.vespa.model.container.xml.document.DocumentFactoryBuilder;
 import com.yahoo.vespa.model.content.StorageGroup;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
-
 import java.io.IOException;
 import java.io.Reader;
 import java.net.URI;
@@ -114,7 +113,6 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.Set;
-import java.util.UUID;
 import java.util.function.Consumer;
 import java.util.logging.Level;
 import java.util.regex.Pattern;
@@ -192,7 +190,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
     private ApplicationContainerCluster createContainerCluster(Element spec, ConfigModelContext modelContext) {
         return new VespaDomBuilder.DomConfigProducerBuilder<ApplicationContainerCluster>() {
             @Override
-            protected ApplicationContainerCluster doBuild(DeployState deployState, AbstractConfigProducer ancestor, Element producerSpec) {
+            protected ApplicationContainerCluster doBuild(DeployState deployState, AbstractConfigProducer<?> ancestor, Element producerSpec) {
                 return new ApplicationContainerCluster(ancestor, modelContext.getProducerId(),
                                                        modelContext.getProducerId(), deployState);
             }
@@ -455,7 +453,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             addHostedImplicitHttpIfNotPresent(deployState, cluster);
             addHostedImplicitAccessControlIfNotPresent(deployState, cluster);
             addDefaultConnectorHostedFilterBinding(cluster);
-            addAdditionalHostedConnector(deployState, cluster, context);
+            addAdditionalHostedConnector(deployState, cluster);
             addCloudDataPlaneFilter(deployState, cluster);
         }
     }
@@ -555,7 +553,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                 .ifPresent(accessControl -> accessControl.configureDefaultHostedConnector(cluster.getHttp()));                                 ;
     }
 
-    private void addAdditionalHostedConnector(DeployState deployState, ApplicationContainerCluster cluster, ConfigModelContext context) {
+    private void addAdditionalHostedConnector(DeployState deployState, ApplicationContainerCluster cluster) {
         JettyHttpServer server = cluster.getHttp().getHttpServer().get();
         String serverName = server.getComponentId().getName();
 
@@ -798,10 +796,22 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
 
         if (cluster.getJvmGCOptions().isEmpty()) {
             String jvmGCOptions = extractAttribute(nodesElement, VespaDomBuilder.JVM_GC_OPTIONS);
+
+            if (jvmGCOptions != null && !jvmGCOptions.isEmpty()) {
+                DeployLogger logger = context.getDeployState().getDeployLogger();
+                logger.logApplicationPackage(WARNING, "'jvm-gc-options' is deprecated and will be removed in Vespa 9." +
+                        " Please merge into 'gc-options' in 'jvm' element." +
+                        " See https://docs.vespa.ai/en/reference/services-container.html#jvm");
+            }
+
             cluster.setJvmGCOptions(buildJvmGCOptions(context, jvmGCOptions));
         }
 
-        applyMemoryPercentage(cluster, nodesElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME));
+        if (applyMemoryPercentage(cluster, nodesElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME)))
+            context.getDeployState().getDeployLogger()
+                   .logApplicationPackage(WARNING, "'allocated-memory' is deprecated and will be removed in Vespa 9." +
+                           " Please merge into 'allocated-memory' in 'jvm' element." +
+                           " See https://docs.vespa.ai/en/reference/services-container.html#jvm");
     }
 
     private void extractJvmTag(List<ApplicationContainer> nodes, ApplicationContainerCluster cluster,
@@ -877,8 +887,8 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             return createNodesFromNodeList(context.getDeployState(), cluster, nodesElement);
     }
     
-    private static void applyMemoryPercentage(ApplicationContainerCluster cluster, String memoryPercentage) {
-        if (memoryPercentage == null || memoryPercentage.isEmpty()) return;
+    private static boolean applyMemoryPercentage(ApplicationContainerCluster cluster, String memoryPercentage) {
+        if (memoryPercentage == null || memoryPercentage.isEmpty()) return false;
         memoryPercentage = memoryPercentage.trim();
 
         if ( ! memoryPercentage.endsWith("%"))
@@ -893,6 +903,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             throw new IllegalArgumentException("The memory percentage given for nodes in " + cluster +
                                                " must be an integer percentage ending by the '%' sign");
         }
+        return true;
     }
 
     /** Allocate a container cluster without a nodes tag */
@@ -1118,7 +1129,8 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
 
     private static void validateAndAddConfiguredComponents(DeployState deployState,
                                                            ContainerCluster<? extends Container> cluster,
-                                                           Element spec, String componentName,
+                                                           Element spec,
+                                                           String componentName,
                                                            Consumer<Element> elementValidator) {
         for (Element node : XML.getChildren(spec, componentName)) {
             elementValidator.accept(node); // throws exception here if something is wrong
@@ -1224,7 +1236,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             if (nodesElement.hasAttribute(VespaDomBuilder.JVM_OPTIONS)) {
                 jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVM_OPTIONS);
                 if (! jvmOptions.isEmpty())
-                    logger.logApplicationPackage(WARNING, "'jvm-options' is deprecated and will be removed in Vespa 8." +
+                    logger.logApplicationPackage(WARNING, "'jvm-options' is deprecated and will be removed in Vespa 9." +
                             " Please merge 'jvm-options' into 'options' or 'gc-options' in 'jvm' element." +
                             " See https://docs.vespa.ai/en/reference/services-container.html#jvm");
             }
diff --git a/config-model/src/main/resources/schema/common.rnc b/config-model/src/main/resources/schema/common.rnc
index fcd9a68ca89..538a8f069f5 100644
--- a/config-model/src/main/resources/schema/common.rnc
+++ b/config-model/src/main/resources/schema/common.rnc
@@ -1,8 +1,8 @@
 # Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 service.attlist &= attribute hostalias { xsd:NCName }
 service.attlist &= attribute baseport { xsd:unsignedShort }?
-service.attlist &= attribute jvm-options { text }?
-service.attlist &= attribute jvm-gc-options { text }?
+service.attlist &= attribute jvm-options { text }? # Remove in Vespa 9
+service.attlist &= attribute jvm-gc-options { text }? # Remove in Vespa 9
 # preload is for internal use only
 service.attlist &= attribute preload { text }?
 
diff --git a/config-model/src/main/resources/schema/containercluster.rnc b/config-model/src/main/resources/schema/containercluster.rnc
index 2bb93ac715e..933ec528c42 100644
--- a/config-model/src/main/resources/schema/containercluster.rnc
+++ b/config-model/src/main/resources/schema/containercluster.rnc
@@ -274,10 +274,10 @@ HttpClientApi = element http-client-api {
 # NODES:
 
 NodesOfContainerCluster = element nodes {
-    attribute jvm-options { text }? &
-    attribute jvm-gc-options { text }? &
+    attribute jvm-options { text }? & # Remove in Vespa 9
+    attribute jvm-gc-options { text }? & # Remove in Vespa 9
     attribute preload { text }? &
-    attribute allocated-memory { text }? &
+    attribute allocated-memory { text }? &  # Remove in Vespa 9
     attribute cpu-socket-affinity { xsd:boolean }? &
     element jvm {
         attribute options { text }? &
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 45546e57808..3435e7faa5e 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
@@ -21,7 +21,9 @@ import java.util.Collections;
 import java.util.List;
 import java.util.logging.Level;
 
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * @author baldersheim
@@ -137,10 +139,30 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
         verifyLoggingOfJvmGcOptions(true, "-XX:MaxTenuringThreshold=15"); // No + or - after colon
     }
 
+    @Test
+    void requireThatDeprecatedJvmOptionsAreLogged() throws IOException, SAXException {
+        String optionName = "jvm-options";
+        verifyLoggingOfLegacyJvmOptions(true, optionName, "-XX:+ParallelGCThreads=8", optionName);
+        verifyLoggingOfLegacyJvmOptions(false, optionName, "-XX:+ParallelGCThreads=8", optionName);
+    }
+
+    @Test
+    void requireThatDeprecatedJvmOptionsAreLogged_2() throws IOException, SAXException {
+        String optionName = "allocated-memory";
+        verifyLoggingOfLegacyJvmOptions(true, optionName, "50%", optionName);
+        verifyLoggingOfLegacyJvmOptions(false, optionName, "50%", optionName);
+    }
+
+    @Test
+    void requireThatDeprecatedJvmGcOptionsAreLogged() throws IOException, SAXException {
+        String optionName = "jvm-gc-options";
+        verifyLoggingOfLegacyJvmOptions(true, optionName, "-XX:+ParallelGCThreads=8", optionName);
+        verifyLoggingOfLegacyJvmOptions(false, optionName, "-XX:+ParallelGCThreads=8", optionName);
+    }
+
     private void verifyThatInvalidJvmGcOptionsFailDeployment(String options, String expected) throws IOException, SAXException {
         try {
-            buildModelWithJvmOptions(new TestProperties().setHostedVespa(true),
-                    new TestLogger(), "gc-options", options);
+            buildModelWithJvmOptions(new TestProperties().setHostedVespa(true), "gc-options", options);
             fail();
         } catch (IllegalArgumentException e) {
             assertTrue(e.getMessage().startsWith(expected));
@@ -158,18 +180,22 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
     }
 
     private void verifyLoggingOfJvmGcOptions(boolean isHosted, String override, String... invalidOptions) throws IOException, SAXException  {
-        verifyLoggingOfJvmOptions(isHosted, "gc-options", override, invalidOptions);
+        verifyLogMessage(isHosted, "gc-options", override, invalidOptions);
     }
 
-    private void verifyLoggingOfJvmOptions(boolean isHosted, String optionName, String override, String... invalidOptions) throws IOException, SAXException  {
-        TestLogger logger = new TestLogger();
-        buildModelWithJvmOptions(isHosted, logger, optionName, override);
+    private void verifyLogMessage(boolean isHosted, String optionName, String override, String... invalidOptions) throws IOException, SAXException  {
+        var logger = buildModelWithJvmOptions(isHosted, optionName, override);
+        var message = verifyLogMessage(logger, invalidOptions);
+        if (message != null)
+            assertTrue(message.contains("Invalid or misplaced JVM"), message);
+    }
 
+    private String verifyLogMessage(TestLogger logger, String... invalidOptions) {
         List<String> strings = Arrays.asList(invalidOptions.clone());
         // Verify that nothing is logged if there are no invalid options
         if (strings.isEmpty()) {
             assertEquals(0, logger.msgs.size(), logger.msgs.size() > 0 ? logger.msgs.get(0).getSecond() : "");
-            return;
+            return null;
         }
 
         assertTrue(logger.msgs.size() > 0, "Expected 1 or more log messages for invalid JM options, got none");
@@ -177,17 +203,15 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
         assertEquals(Level.WARNING, firstOption.getFirst());
 
         Collections.sort(strings);
-        assertEquals("Invalid or misplaced JVM" + (optionName.equals("gc-options") ? " GC" : "") +
-                             " options in services.xml: " + String.join(",", strings) + "." +
-                             " See https://docs.vespa.ai/en/reference/services-container.html#jvm"
-                             , firstOption.getSecond());
+        return firstOption.getSecond();
     }
 
-    private void buildModelWithJvmOptions(boolean isHosted, TestLogger logger, String optionName, String override) throws IOException, SAXException {
-        buildModelWithJvmOptions(new TestProperties().setHostedVespa(isHosted), logger, optionName, override);
+    private TestLogger buildModelWithJvmOptions(boolean isHosted, String optionName, String override) throws IOException, SAXException {
+        return buildModelWithJvmOptions(new TestProperties().setHostedVespa(isHosted), optionName, override);
     }
 
-    private void buildModelWithJvmOptions(TestProperties properties, TestLogger logger, String optionName, String override) throws IOException, SAXException {
+    private TestLogger buildModelWithJvmOptions(TestProperties properties, String optionName, String override) throws IOException, SAXException {
+        TestLogger logger = new TestLogger();
         String servicesXml =
                 "<container version='1.0'>" +
                         "  <nodes>" +
@@ -195,6 +219,32 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
                         "    <node hostalias='mockhost'/>" +
                         "  </nodes>" +
                         "</container>";
+        buildModel(properties, logger, servicesXml);
+        return logger;
+    }
+
+    private void verifyLoggingOfLegacyJvmOptions(boolean isHosted, String optionName, String override, String... invalidOptions) throws IOException, SAXException  {
+        var logger = buildModelWithLegacyJvmOptions(isHosted, optionName, override);
+
+        var message = verifyLogMessage(logger, invalidOptions);
+        if (message != null)
+            assertTrue(message.contains("'" + optionName + "' is deprecated and will be removed"), message);
+    }
+
+    private TestLogger buildModelWithLegacyJvmOptions(boolean isHosted, String optionName, String override) throws IOException, SAXException {
+        TestProperties properties = new TestProperties().setHostedVespa(isHosted);
+        TestLogger logger = new TestLogger();
+        String servicesXml =
+                "<container version='1.0'>" +
+                        "  <nodes " + optionName + "='" + override + "'>" +
+                        "    <node hostalias='mockhost'/>" +
+                        "  </nodes>" +
+                        "</container>";
+        buildModel(properties, logger, servicesXml);
+        return logger;
+    }
+
+    private void buildModel(TestProperties properties, TestLogger logger, String servicesXml) throws IOException, SAXException {
         ApplicationPackage app = new MockApplicationPackage.Builder().withServices(servicesXml).build();
         new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder()
                 .applicationPackage(app)
@@ -206,18 +256,17 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
     @Test
     void requireThatValidJvmOptionsAreNotLogged() throws IOException, SAXException {
         // Valid options, should not log anything
-        verifyLoggingOfJvmOptions(true, "options", "-Xms2G");
-        verifyLoggingOfJvmOptions(true, "options", "-Xlog:gc");
-        verifyLoggingOfJvmOptions(true, "options", "-Djava.library.path=/opt/vespa/lib64:/home/y/lib64");
-        verifyLoggingOfJvmOptions(true, "options", "-XX:-OmitStackTraceInFastThrow");
-        verifyLoggingOfJvmOptions(false, "options", "-Xms2G");
+        verifyLogMessage(true, "options", "-Xms2G");
+        verifyLogMessage(true, "options", "-Xlog:gc");
+        verifyLogMessage(true, "options", "-Djava.library.path=/opt/vespa/lib64:/home/y/lib64");
+        verifyLogMessage(true, "options", "-XX:-OmitStackTraceInFastThrow");
+        verifyLogMessage(false, "options", "-Xms2G");
     }
 
     @Test
     void requireThatInvalidJvmOptionsFailDeployment() throws IOException, SAXException {
         try {
             buildModelWithJvmOptions(new TestProperties().setHostedVespa(true),
-                    new TestLogger(),
                     "options",
                     "-Xms2G foo     bar");
             fail();