Merge pull request #31226 from vespa-engine/hmusum/fix-jvm-options-in-combination-with-feature-flag

Fix Jvm options in combination with feature flag

8 files changed, 30 insertions, 29 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/LogserverContainer.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/LogserverContainer.java
index ad728d60c5a..e18ffea731e 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/LogserverContainer.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/LogserverContainer.java
@@ -31,7 +31,7 @@ public class LogserverContainer extends Container {
     }
 
     @Override
-    protected String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
+    public String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
         return featureFlags.jvmOmitStackTraceInFastThrowOption(ClusterSpec.Type.admin);
     }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerContainer.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerContainer.java
index ca602b447fe..e702a29b640 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerContainer.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerContainer.java
@@ -98,7 +98,7 @@ public class ClusterControllerContainer extends Container implements
     }
 
     @Override
-    protected String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
+    public String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
         return featureFlags.jvmOmitStackTraceInFastThrowOption(ClusterSpec.Type.admin);
     }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainer.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainer.java
index c4702ba3543..2ff58438a07 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainer.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/metricsproxy/MetricsProxyContainer.java
@@ -78,7 +78,7 @@ public class MetricsProxyContainer extends Container implements
     }
 
     @Override
-    protected String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
+    public String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
         return featureFlags.jvmOmitStackTraceInFastThrowOption(ClusterSpec.Type.admin);
     }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainer.java b/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainer.java
index e3fc680ad6a..1f8d7fd7520 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainer.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainer.java
@@ -82,11 +82,6 @@ public final class ApplicationContainer extends Container implements
         builder.myid(index());
     }
 
-    @Override
-    protected String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
-        return featureFlags.jvmOmitStackTraceInFastThrowOption(ClusterSpec.Type.container);
-    }
-
     @Override public Optional<String> getPreShutdownCommand() { return Optional.of(prepareStopCommand(Duration.ofMinutes(6))); }
 
     private Optional<NodeResources> realResources() {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
index f1945ca5341..864cbc8691b 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java
@@ -105,12 +105,10 @@ public abstract class Container extends AbstractService implements
         addBuiltinHandlers();
 
         addChild(new SimpleComponent("com.yahoo.container.jdisc.ConfiguredApplication$ApplicationContext"));
-
-        appendJvmOptions(jvmOmitStackTraceInFastThrowOption(deployState.featureFlags()));
         addEnvironmentVariable("VESPA_MALLOC_MMAP_THRESHOLD","0x1000000"); // 16M
     }
 
-    protected String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
+    public String jvmOmitStackTraceInFastThrowOption(ModelContext.FeatureFlags featureFlags) {
         return featureFlags.jvmOmitStackTraceInFastThrowOption(ClusterSpec.Type.container);
     }
 
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 56e2a21e38b..dbe28d48f9e 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
@@ -986,6 +986,8 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                 AbstractService.distributeCpuSocketAffinity(nodes);
             cluster.addContainers(nodes);
         }
+        // Must be done after setting Jvm options from services.xml (#extractJvmOptions()), otherwise those options will not be set
+        cluster.getContainers().forEach(container -> container.appendJvmOptions(container.jvmOmitStackTraceInFastThrowOption(context.featureFlags())));
     }
 
     private ZoneEndpoint zoneEndpoint(ConfigModelContext context, ClusterSpec.Id cluster) {
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
index 2ea4249883d..9cf34b55ebf 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
@@ -221,24 +221,6 @@ public class ContainerClusterTest {
     }
 
     @Test
-    void requireThatJvmOmitStackTraceInFastThrowOptionWorks() {
-        // Empty option if option not set in property
-        MockRoot root = createRoot(new DeployState.Builder().build());
-        ApplicationContainerCluster cluster = newClusterWithSearch(root);
-        addContainer(root, cluster, "c1", "host-c1");
-        ApplicationContainer container = cluster.getContainers().get(0);
-        assertEquals("", container.getJvmOptions());
-
-        String jvmOption = "-XX:-foo";
-        DeployState deployState = new DeployState.Builder().properties(new TestProperties().setJvmOmitStackTraceInFastThrowOption(jvmOption)).build();
-        root = createRoot(deployState);
-        cluster = newClusterWithSearch(root);
-        addContainer(root, cluster, "c1", "host-c1");
-        container = cluster.getContainers().get(0);
-        assertEquals(jvmOption, container.getJvmOptions());
-    }
-
-    @Test
     void requireThatWeCanHandleNullJvmOptions() {
         MockRoot root = createRoot(false);
         ApplicationContainerCluster cluster = newClusterWithSearch(root);
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 46097da434e..64afd444989 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
@@ -2,6 +2,7 @@
 
 package com.yahoo.vespa.model.container.xml;
 
+import com.yahoo.cloud.config.SentinelConfig;
 import com.yahoo.collections.Pair;
 import com.yahoo.config.application.api.ApplicationPackage;
 import com.yahoo.config.model.NullConfigModelRegistry;
@@ -205,6 +206,29 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
         assertEquals("-XX:+UseParNewGC", qrStartConfig.jvm().gcopts());
     }
 
+    @Test
+    void verify_jvm_option_and_value_from_feature_flag_are_both_included() throws IOException, SAXException {
+        String servicesXml = """
+                <container version='1.0'>
+                  <search/>
+                  <nodes>
+                    <jvm options="-Xms1024m -Xmx2048m" />
+                    <node hostalias="node1" />
+                  </nodes>
+                </container>
+                """;
+        ApplicationPackage applicationPackage = new MockApplicationPackage.Builder().withServices(servicesXml).build();
+        // Need to create VespaModel to make deploy properties have effect
+        VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder()
+                .applicationPackage(applicationPackage)
+                .properties(new TestProperties().setJvmOmitStackTraceInFastThrowOption("-XX:-OmitStackTraceInFastThrow"))
+                .build());
+        SentinelConfig.Builder builder = new SentinelConfig.Builder();
+        model.getConfig(builder, "hosts/localhost");
+        SentinelConfig config = builder.build();
+        assertEquals("PRELOAD=/opt/vespa/lib64/vespa/malloc/libvespamalloc.so exec ${VESPA_HOME}/libexec/vespa/vespa-wrapper vespa-start-container-daemon -Xms1024m -Xmx2048m -XX:-OmitStackTraceInFastThrow ", config.service().get(0).command());
+    }
+
     private void verifyLoggingOfJvmGcOptions(boolean isHosted, String override, String... invalidOptions) throws IOException, SAXException  {
         verifyLogMessage(isHosted, "gc-options", override, invalidOptions);
     }