From d6f4ce3a54daab7577b2b65432168aa65f00950d Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Fri, 7 Jan 2022 13:19:27 +0100 Subject: Allow comma in JVM options --- .../com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java | 2 +- .../test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'config-model/src') 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 657eb6a29e7..346f450d8b6 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 @@ -1063,7 +1063,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder { private static class JvmOptions { - private static final Pattern validPattern = Pattern.compile("-[a-zA-z0-9=:./]+"); + private static final Pattern validPattern = Pattern.compile("-[a-zA-z0-9=:./,]+"); private final ContainerCluster cluster; private final Element nodesElement; 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 96fbce03e88..01f5e1ee776 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 @@ -196,7 +196,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { List strings = Arrays.asList(invalidOptions.clone()); // Verify that nothing is logged if there are no invalid options if (strings.isEmpty()) { - assertEquals(0, logger.msgs.size()); + assertEquals(logger.msgs.size() > 0 ? logger.msgs.get(0).getSecond() : "", 0, logger.msgs.size()); return; } @@ -246,7 +246,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { // Valid options, should not log anything verifyLoggingOfJvmOptions(true, "options", "-Xms2G"); verifyLoggingOfJvmOptions(true, "options", "-verbose:gc"); - verifyLoggingOfJvmOptions(true, "options", "-Djava.library.path=/opt/vespa/lib64:/home/y/lib64"); + verifyLoggingOfJvmOptions(true, "options", "-Djava.library.path=/opt/vespa/lib64:/home/y/lib64 -Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005"); verifyLoggingOfJvmOptions(false, "options", "-Xms2G"); } -- cgit v1.2.3 From 597792f3760034f2055b83c9518a9c328f39cb42 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Mon, 10 Jan 2022 09:42:00 +0100 Subject: Don't allow JVM option -Xrunjdwp:transport in hosted --- .../vespa/model/container/xml/ContainerModelBuilder.java | 12 +++++++++++- .../com/yahoo/vespa/model/container/xml/JvmOptionsTest.java | 11 +++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) (limited to 'config-model/src') 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 346f450d8b6..288476de015 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 @@ -1064,12 +1064,15 @@ public class ContainerModelBuilder extends ConfigModelBuilder { private static class JvmOptions { private static final Pattern validPattern = Pattern.compile("-[a-zA-z0-9=:./,]+"); + // debug port will not be available in hosted, don't allow + private static final Pattern invalidInHostedatttern = Pattern.compile("-Xrunjdwp:transport=.*"); private final ContainerCluster cluster; private final Element nodesElement; private final DeployLogger logger; private final boolean legacyOptions; private final boolean failDeploymentWithInvalidJvmOptions; + private final boolean isHosted; public JvmOptions(ContainerCluster cluster, Element nodesElement, DeployState deployState, boolean legacyOptions) { this.cluster = cluster; @@ -1077,6 +1080,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder { this.logger = deployState.getDeployLogger(); this.legacyOptions = legacyOptions; this.failDeploymentWithInvalidJvmOptions = deployState.featureFlags().failDeploymentWithInvalidJvmOptions(); + this.isHosted = deployState.isHosted(); } String build() { @@ -1086,7 +1090,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder { Element jvmElement = XML.getChild(nodesElement, "jvm"); if (jvmElement == null) return ""; String jvmOptions = jvmElement.getAttribute(VespaDomBuilder.OPTIONS); - if (jvmOptions == null) return ""; + if (jvmOptions.isEmpty()) return ""; validateJvmOptions(jvmOptions); return jvmOptions; } @@ -1135,6 +1139,12 @@ public class ContainerModelBuilder extends ConfigModelBuilder { .filter(option -> !Pattern.matches(validPattern.pattern(), option)) .sorted() .collect(Collectors.toList()); + if (isHosted) + invalidOptions.addAll(Arrays.stream(optionList) + .filter(option -> !option.isEmpty()) + .filter(option -> Pattern.matches(invalidInHostedatttern.pattern(), option)) + .sorted() + .collect(Collectors.toList())); if (invalidOptions.isEmpty()) return; 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 01f5e1ee776..ba27deedb61 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 @@ -200,9 +200,11 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { return; } - Collections.sort(strings); + assertTrue("Expected 1 or more log messages for invalid JM options, got none", logger.msgs.size() > 0); Pair firstOption = logger.msgs.get(0); assertEquals(Level.WARNING, firstOption.getFirst()); + + Collections.sort(strings); assertEquals("Invalid JVM " + (optionName.equals("gc-options") ? "GC " : "") + "options in services.xml: " + String.join(",", strings), firstOption.getSecond()); } @@ -238,6 +240,11 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { "$(touch /tmp/hello-from-gc-options)", "$(touch", "/tmp/hello-from-gc-options)"); + verifyLoggingOfJvmOptions(true, + "options", + "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005", + "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005"); + verifyLoggingOfJvmOptions(false, "options", "$(touch /tmp/hello-from-gc-options)", @@ -246,7 +253,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { // Valid options, should not log anything verifyLoggingOfJvmOptions(true, "options", "-Xms2G"); verifyLoggingOfJvmOptions(true, "options", "-verbose:gc"); - verifyLoggingOfJvmOptions(true, "options", "-Djava.library.path=/opt/vespa/lib64:/home/y/lib64 -Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005"); + verifyLoggingOfJvmOptions(true, "options", "-Djava.library.path=/opt/vespa/lib64:/home/y/lib64"); verifyLoggingOfJvmOptions(false, "options", "-Xms2G"); } -- cgit v1.2.3 From f7a1319189fec755a5249807e28dcd3244b923b2 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Mon, 10 Jan 2022 10:08:04 +0100 Subject: Update javadoc --- .../vespa/model/container/xml/ContainerModelBuilder.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'config-model/src') 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 288476de015..e961b39ff8e 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 @@ -1061,7 +1061,11 @@ public class ContainerModelBuilder extends ConfigModelBuilder { return CONTAINER_TAG.equals(element.getTagName()) || DEPRECATED_CONTAINER_TAG.equals(element.getTagName()); } - private static class JvmOptions { + /** + * Validates JVM options and logs a warning or fails deployment (depending on feature flag) + * if anyone of them has invalid syntax or is an option that is unsupported for the running system. + */ + private static class JvmOptions { private static final Pattern validPattern = Pattern.compile("-[a-zA-z0-9=:./,]+"); // debug port will not be available in hosted, don't allow @@ -1157,8 +1161,9 @@ public class ContainerModelBuilder extends ConfigModelBuilder { } /** - * Validates JVM GC options and logs a warning if anyone of them has invalid syntax or is an option - * that is unsupported for the running system (e.g. uses CMS options for hosted Vespa, which uses JDK 17) + * Validates JVM GC options and logs a warning or fails deployment (depending on feature flag) + * if anyone of them has invalid syntax or is an option that is unsupported for the running system + * (e.g. uses CMS options for hosted Vespa, which uses JDK 17). */ private static class JvmGcOptions { -- cgit v1.2.3