modules/plugins/rhq-storage/src/main/java/org/rhq/plugins/storage/StorageNodeConfigDelegate.java | 35 +++++ modules/plugins/rhq-storage/src/main/resources/META-INF/rhq-plugin.xml | 18 ++ modules/plugins/rhq-storage/src/test/java/org/rhq/plugins/storage/StorageNodeConfigDelegateTest.java | 61 +++++++++- 3 files changed, 107 insertions(+), 7 deletions(-)
New commits: commit 632eb18cc8984f12c794678c6d13a455048a2096 Author: John Sanda jsanda@redhat.com Date: Mon Jul 15 15:35:48 2013 -0400
adding validation checks for heap and thread args
diff --git a/modules/plugins/rhq-storage/src/main/java/org/rhq/plugins/storage/StorageNodeConfigDelegate.java b/modules/plugins/rhq-storage/src/main/java/org/rhq/plugins/storage/StorageNodeConfigDelegate.java index c87d2ac..a0220ba 100644 --- a/modules/plugins/rhq-storage/src/main/java/org/rhq/plugins/storage/StorageNodeConfigDelegate.java +++ b/modules/plugins/rhq-storage/src/main/java/org/rhq/plugins/storage/StorageNodeConfigDelegate.java @@ -42,7 +42,8 @@ public class StorageNodeConfigDelegate implements ConfigurationFacet { config.put(new PropertySimple("minHeapSize", heapMinProp.substring(4))); config.put(new PropertySimple("maxHeapSize", heapMaxProp.substring(4))); config.put(new PropertySimple("heapNewSize", heapNewProp.substring(4))); - config.put(new PropertySimple("threadStackSize", threadStackSizeProp.substring(4))); + config.put(new PropertySimple("threadStackSize", threadStackSizeProp.substring(4, + threadStackSizeProp.length() - 1)));
if (!StringUtil.isEmpty(heapDumpOnOOMError)) { config.put(new PropertySimple("heapDumpOnOOMError", true)); @@ -70,6 +71,7 @@ public class StorageNodeConfigDelegate implements ConfigurationFacet {
String maxHeapSize = config.getSimpleValue("maxHeapSize"); if (!StringUtil.isEmpty(maxHeapSize)) { + validateHeapArg("maxHeapSize", maxHeapSize); // We want min and max heap to be the same properties.setProperty("heap_min", "-Xms" + maxHeapSize); properties.setProperty("heap_max", "-Xmx" + maxHeapSize); @@ -77,12 +79,14 @@ public class StorageNodeConfigDelegate implements ConfigurationFacet {
String heapNewSize = config.getSimpleValue("heapNewSize"); if (!StringUtil.isEmpty(heapNewSize)) { + validateHeapArg("heapNewSize", heapNewSize); properties.setProperty("heap_new", "-Xmn" + heapNewSize); }
String threadStackSize = config.getSimpleValue("threadStackSize"); if (!StringUtil.isEmpty(threadStackSize)) { - properties.setProperty("thread_stack_size", "-Xss" + threadStackSize); + validateStackArg(threadStackSize); + properties.setProperty("thread_stack_size", "-Xss" + threadStackSize + "k"); }
PropertySimple heapDumpOnOMMError = config.getSimple("heapDumpOnOOMError"); @@ -102,9 +106,36 @@ public class StorageNodeConfigDelegate implements ConfigurationFacet { propertiesUpdater.update(properties);
configurationUpdateReport.setStatus(ConfigurationUpdateStatus.SUCCESS); + } catch (IllegalArgumentException e) { + configurationUpdateReport.setErrorMessage("No configuration update was applied: " + e.getMessage()); } catch (IOException e) { configurationUpdateReport.setErrorMessageFromThrowable(e); } + } + + private void validateHeapArg(String name, String value) { + if (value.length() < 2) { + throw new IllegalArgumentException(value + " is not a legal value for the property [" + name + "]"); + } + + char[] chars = value.toCharArray(); + for (int i = 0; i < chars.length - 1; ++i) { + if (!Character.isDigit(chars[i])) { + throw new IllegalArgumentException(value + " is not a legal value for the property [" + name + "]"); + } + }
+ char lastChar = Character.toUpperCase(chars[chars.length - 1]); + if (!(lastChar == 'M' || lastChar == 'G')) { + throw new IllegalArgumentException(value + " is not a legal value for the property [" + name + "]"); + } + } + + private void validateStackArg(String value) { + try { + Integer.parseInt(value); + } catch (NumberFormatException e) { + throw new IllegalArgumentException(value + " is not a legal value for the property [threadStackSize]"); + } } } diff --git a/modules/plugins/rhq-storage/src/main/resources/META-INF/rhq-plugin.xml b/modules/plugins/rhq-storage/src/main/resources/META-INF/rhq-plugin.xml index c700257..5a60d19 100644 --- a/modules/plugins/rhq-storage/src/main/resources/META-INF/rhq-plugin.xml +++ b/modules/plugins/rhq-storage/src/main/resources/META-INF/rhq-plugin.xml @@ -118,12 +118,24 @@ description="The minimum heap size. This value will be used with the -Xms JVM option. This is read only because it is automatically set to the same value as Max Heap Size."/> <c:simple-property name="maxHeapSize" - description="The maximum heap size. This value will be used with the -Xmx JVM option."/> + description="The maximum heap size. This value will be used with the -Xmx JVM option. The + value should be an integer with a suffix of M or G to indicate megabytes or gigabytes."> + <c:constraint> + <c:regex-constraint expression="\d+[mMgG]"/> + </c:constraint> + </c:simple-property> <c:simple-property name="heapNewSize" description="The size of the new generation portion of the heap. This value will be used with - the -Xmn JVM option."/> + the -Xmn JVM option. The value should be an integer with a suffix of M or G to indicate + megabytes or gigabytes."> + <c:constraint> + <c:regex-constraint expression="\d+[mMgG]"/> + </c:constraint> + </c:simple-property> <c:simple-property name="threadStackSize" - description="The thread stack size. This memory is allocated to each thread off heap."/> + type="integer" + description="The thread stack size. This memory is allocated to each thread off heap. The + value should be an integer that will be interpreted in kilobytes."/> <c:simple-property name="heapDumpOnOOMError" displayName="Heap Dump on OutOfMemoryError" type="boolean" default="true" description="Generate a heap dump when an OutOfMemoryError occurs"/> diff --git a/modules/plugins/rhq-storage/src/test/java/org/rhq/plugins/storage/StorageNodeConfigDelegateTest.java b/modules/plugins/rhq-storage/src/test/java/org/rhq/plugins/storage/StorageNodeConfigDelegateTest.java index ee34f43..64d14b2 100644 --- a/modules/plugins/rhq-storage/src/test/java/org/rhq/plugins/storage/StorageNodeConfigDelegateTest.java +++ b/modules/plugins/rhq-storage/src/test/java/org/rhq/plugins/storage/StorageNodeConfigDelegateTest.java @@ -13,8 +13,10 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test;
import org.rhq.core.domain.configuration.Configuration; +import org.rhq.core.domain.configuration.ConfigurationUpdateStatus; import org.rhq.core.domain.configuration.PropertySimple; import org.rhq.core.pluginapi.configuration.ConfigurationUpdateReport; +import org.rhq.core.util.file.FileUtil;
/** * @author John Sanda @@ -29,6 +31,7 @@ public class StorageNodeConfigDelegateTest { public void initDirs(Method test) throws Exception { File dir = new File(getClass().getResource(".").toURI()); basedir = new File(dir, getClass().getSimpleName() + "/" + test.getName()); + FileUtil.purge(basedir, true); configDelegate = new StorageNodeConfigDelegate(basedir); }
@@ -42,7 +45,7 @@ public class StorageNodeConfigDelegateTest { assertEquals(config.getSimpleValue("minHeapSize"), "512M", "Failed to load property [minHeapSize]"); assertEquals(config.getSimpleValue("maxHeapSize"), "512M", "Failed to load property [maxHepSize]"); assertEquals(config.getSimpleValue("heapNewSize"), "128M", "Failed to load property [heapNewSize]"); - assertEquals(config.getSimpleValue("threadStackSize"), "180k", "Failed to load property [threadStackSize]"); + assertEquals(config.getSimpleValue("threadStackSize"), "180", "Failed to load property [threadStackSize]"); assertEquals(config.getSimple("heapDumpOnOOMError").getBooleanValue(), (Boolean) true, "Failed to load property [heapDumpOnOOMError]"); assertEquals(new File(config.getSimpleValue("heapDumpDir")), binDir(), "Failed to load property [heapDumpDir]"); @@ -56,7 +59,7 @@ public class StorageNodeConfigDelegateTest { config.put(new PropertySimple("minHeapSize", "1024M")); config.put(new PropertySimple("maxHeapSize", "1024M")); config.put(new PropertySimple("heapNewSize", "256M")); - config.put(new PropertySimple("threadStackSize", "240k")); + config.put(new PropertySimple("threadStackSize", "240")); config.put(new PropertySimple("heapDumpOnOOMError", true)); config.put(new PropertySimple("heapDumpDir", confDir()));
@@ -96,6 +99,60 @@ public class StorageNodeConfigDelegateTest { "should be the same as [maxHeapSize]."); }
+ @Test + public void disableHeapDumps() throws Exception { + createDefaultConfig(); + + ConfigurationUpdateReport report = new ConfigurationUpdateReport(Configuration.builder() + .addSimple("heapDumpOnOOMError", false).build()); + + configDelegate.updateResourceConfiguration(report); + + Properties properties = loadCassandraJvmProps(); + + assertEquals(properties.getProperty("heap_dump_on_OOMError"), "", "Failed to disable property " + + "[heapDumpOnOOMError]"); + } + + @Test + public void updateShouldFailWhenMaxHeapSizeIsInvalid() throws Exception { + createDefaultConfig(); + + ConfigurationUpdateReport report = new ConfigurationUpdateReport(Configuration.builder() + .addSimple("maxHeapSize", "256GB").build()); + + configDelegate.updateResourceConfiguration(report); + + assertEquals(report.getStatus(), ConfigurationUpdateStatus.FAILURE, "The configuration update should fail " + + "when [maxHeapSize] has an invalid value."); + } + + @Test + public void updateShouldFailWhenHeapNewSizeIsInvalid() throws Exception { + createDefaultConfig(); + + ConfigurationUpdateReport report = new ConfigurationUpdateReport(Configuration.builder() + .addSimple("heapNewSize", "25^G").build()); + + configDelegate.updateResourceConfiguration(report); + + assertEquals(report.getStatus(), ConfigurationUpdateStatus.FAILURE, "The configuration update should fail " + + "when [heapNewSize] has an invalid value."); + } + + @Test + public void updateShouldFailWhenThreadStackSizeIsInvalid() throws Exception { + createDefaultConfig(); + + ConfigurationUpdateReport report = new ConfigurationUpdateReport(Configuration.builder() + .addSimple("threadStackSize", "128M").build()); + + configDelegate.updateResourceConfiguration(report); + + assertEquals(report.getStatus(), ConfigurationUpdateStatus.FAILURE, "The configuration update should fail " + + "when [threadStackSize] has an invalid value."); + } + private void createDefaultConfig() throws IOException { Properties properties = new Properties(); properties.setProperty("heap_min", "-Xms512M");
rhq-commits@lists.fedorahosted.org