diff options
5 files changed, 59 insertions, 21 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 c25502039c2..6c70af8cbca 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 @@ -111,6 +111,7 @@ public interface ModelContext { @ModelFeatureFlag(owners = {"arnej"}) default boolean useV8GeoPositions() { return false; } @ModelFeatureFlag(owners = {"arnej", "baldersheim"}) default boolean useV8DocManagerCfg() { return false; } @ModelFeatureFlag(owners = {"baldersheim", "geirst", "toregge"}) default int maxCompactBuffers() { return 1; } + @ModelFeatureFlag(owners = {"hmusum"}) default boolean failDeploymentWithInvalidJvmOptions() { return false; } @ModelFeatureFlag(owners = {"baldersheim"}) default double tlsSizeFraction() { throw new UnsupportedOperationException("TODO specify default value"); } } 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 b24f63148c7..e645fec5520 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 @@ -76,6 +76,7 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea private boolean unorderedMergeChaining = false; private List<String> zoneDnsSuffixes = List.of(); private int maxCompactBuffers = 1; + private boolean failDeploymentWithInvalidJvmOptions = false; private double tlsSizeFraction = 0.07; @Override public ModelContext.FeatureFlags featureFlags() { return this; } @@ -133,6 +134,7 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea @Override public boolean unorderedMergeChaining() { return unorderedMergeChaining; } @Override public List<String> zoneDnsSuffixes() { return zoneDnsSuffixes; } @Override public int maxCompactBuffers() { return maxCompactBuffers; } + @Override public boolean failDeploymentWithInvalidJvmOptions() { return failDeploymentWithInvalidJvmOptions; } @Override public double tlsSizeFraction() { return tlsSizeFraction; } public TestProperties maxUnCommittedMemory(int maxUnCommittedMemory) { @@ -349,6 +351,11 @@ public class TestProperties implements ModelContext.Properties, ModelContext.Fea return this; } + public TestProperties failDeploymentWithInvalidJvmOptions(boolean fail) { + failDeploymentWithInvalidJvmOptions = fail; + return this; + } + public TestProperties tlsSizeFraction(double tlsSizeFraction) { this.tlsSizeFraction = tlsSizeFraction; 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 d7691f4c486..c8b2197fc29 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 @@ -1133,19 +1133,21 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { */ private static class JvmGcOptions { - private static final Pattern validPattern = Pattern.compile("-XX:[+-][a-zA-z0-9=]+"); + private static final Pattern validPattern = Pattern.compile("-XX:[+-]*[a-zA-z0-9=]+"); private static final Pattern invalidCMSPattern = Pattern.compile("-XX:[+-]\\w*CMS[a-zA-z0-9=]+"); private final DeployState deployState; private final String jvmGcOptions; private final DeployLogger logger; private final boolean isHosted; + private final boolean failDeploymentWithInvalidJvmOptions; public JvmGcOptions(DeployState deployState, String jvmGcOptions) { this.deployState = deployState; this.jvmGcOptions = jvmGcOptions; this.logger = deployState.getDeployLogger(); this.isHosted = deployState.isHosted(); + this.failDeploymentWithInvalidJvmOptions = deployState.featureFlags().failDeploymentWithInvalidJvmOptions(); } private String build() { @@ -1162,13 +1164,12 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { // CMS GC options cannot be used in hosted, CMS is unsupported in JDK 17 invalidOptions.addAll(Arrays.stream(optionList) .filter(option -> !option.isEmpty()) - .filter(option -> Pattern - .matches(invalidCMSPattern.pattern(), option) || + .filter(option -> Pattern.matches(invalidCMSPattern.pattern(), option) || option.equals("-XX:+UseConcMarkSweepGC")) .collect(Collectors.toList())); } - logInvalidOptions(invalidOptions); + logOrFailInvalidOptions(invalidOptions); } if (options == null || options.isEmpty()) @@ -1177,11 +1178,15 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { return options; } - private void logInvalidOptions(List<String> options) { + private void logOrFailInvalidOptions(List<String> options) { if (options.isEmpty()) return; Collections.sort(options); - logger.logApplicationPackage(WARNING, "Invalid JVM GC options from services.xml: " + String.join(",", options)); + String message = "Invalid JVM GC options from services.xml: " + String.join(",", options); + if (failDeploymentWithInvalidJvmOptions) + throw new IllegalArgumentException(message); + else + logger.logApplicationPackage(WARNING, message); } } 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 4d34df98fb7..5fe2baaca37 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 @@ -12,7 +12,9 @@ import com.yahoo.config.model.test.MockApplicationPackage; import com.yahoo.search.config.QrStartConfig; import com.yahoo.vespa.model.VespaModel; import com.yahoo.vespa.model.container.ContainerCluster; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.w3c.dom.Element; import org.xml.sax.SAXException; @@ -22,6 +24,7 @@ import java.util.Collections; import java.util.List; import java.util.logging.Level; +import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -32,6 +35,8 @@ import static org.junit.Assert.assertTrue; */ public class JvmOptionsTest extends ContainerModelBuilderTestBase { + @Rule public ExpectedException expectedException = ExpectedException.none(); + @Test public void verify_jvm_tag_with_attributes() throws IOException, SAXException { String servicesXml = @@ -166,31 +171,28 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { "$(touch", "/tmp/hello-from-gc-options)"); // Valid options, should not log anything - verifyLoggingOfJvmGcOptions(true, - "-XX:+ParallelGCThreads=8"); + verifyLoggingOfJvmGcOptions(true, "-XX:+ParallelGCThreads=8"); + verifyLoggingOfJvmGcOptions(true, "-XX:MaxTenuringThreshold"); // No + or - after colon verifyLoggingOfJvmGcOptions(false, "-XX:+UseConcMarkSweepGC"); } + @Test + public void requireThatInvalidJvmGcOptionsFailDeployment() throws IOException, SAXException { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage(containsString("Invalid JVM GC options from services.xml: bar,foo")); + buildModelWithJvmOptions(new TestProperties().setHostedVespa(true).failDeploymentWithInvalidJvmOptions(true), + new TestLogger(), + "gc-options", + "-XX:+ParallelGCThreads=8 foo bar"); + } private void verifyLoggingOfJvmGcOptions(boolean isHosted, String override, String... invalidOptions) throws IOException, SAXException { verifyLoggingOfJvmOptions(isHosted, "gc-options", override, invalidOptions); } private void verifyLoggingOfJvmOptions(boolean isHosted, String optionName, String override, String... invalidOptions) throws IOException, SAXException { - String servicesXml = - "<container version='1.0'>" + - " <nodes>" + - " <jvm " + optionName + "='" + override + "'/>" + - " <node hostalias='mockhost'/>" + - " </nodes>" + - "</container>"; - ApplicationPackage app = new MockApplicationPackage.Builder().withServices(servicesXml).build(); TestLogger logger = new TestLogger(); - new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() - .applicationPackage(app) - .deployLogger(logger) - .properties(new TestProperties().setHostedVespa(isHosted)) - .build()); + buildModelWithJvmOptions(isHosted, logger, optionName, override); List<String> strings = Arrays.asList(invalidOptions.clone()); if (strings.isEmpty()) return; @@ -202,6 +204,26 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { "options from services.xml: " + String.join(",", strings), 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 void buildModelWithJvmOptions(TestProperties properties, TestLogger logger, String optionName, String override) throws IOException, SAXException { + String servicesXml = + "<container version='1.0'>" + + " <nodes>" + + " <jvm " + optionName + "='" + override + "'/>" + + " <node hostalias='mockhost'/>" + + " </nodes>" + + "</container>"; + ApplicationPackage app = new MockApplicationPackage.Builder().withServices(servicesXml).build(); + new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() + .applicationPackage(app) + .deployLogger(logger) + .properties(properties) + .build()); + } + @Test public void requireThatJvmOptionsAreLogged() throws IOException, SAXException { verifyLoggingOfJvmOptions(true, "options", "-Xms2G"); 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 99504588e5f..063603fe8a8 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 @@ -203,6 +203,7 @@ public class ModelContextImpl implements ModelContext { private final boolean useV8GeoPositions; private final boolean useV8DocManagerCfg; private final int maxCompactBuffers; + private final boolean failDeploymentWithInvalidJvmOptions; private final double tlsSizeFraction; public FeatureFlags(FlagSource source, ApplicationId appId) { @@ -245,6 +246,7 @@ public class ModelContextImpl implements ModelContext { this.useV8GeoPositions = flagValue(source, appId, Flags.USE_V8_GEO_POSITIONS); this.useV8DocManagerCfg = flagValue(source, appId, Flags.USE_V8_DOC_MANAGER_CFG); this.maxCompactBuffers = flagValue(source, appId, Flags.MAX_COMPACT_BUFFERS); + this.failDeploymentWithInvalidJvmOptions = flagValue(source, appId, Flags.FAIL_DEPLOYMENT_WITH_INVALID_JVM_OPTIONS); this.tlsSizeFraction = flagValue(source, appId, Flags.TLS_SIZE_FRACTION); } @@ -288,6 +290,7 @@ public class ModelContextImpl implements ModelContext { @Override public boolean unorderedMergeChaining() { return unorderedMergeChaining; } @Override public boolean useV8GeoPositions() { return useV8GeoPositions; } @Override public boolean useV8DocManagerCfg() { return useV8DocManagerCfg; } + @Override public boolean failDeploymentWithInvalidJvmOptions() { return failDeploymentWithInvalidJvmOptions; } @Override public int maxCompactBuffers() { return maxCompactBuffers; } @Override public double tlsSizeFraction() { return tlsSizeFraction; } |