From 686da6f0944c81dbb109d19635996f91bac31bb4 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 18 Jun 2018 14:01:15 +0000 Subject: [PATCH 1/5] throw specific NPE child when command is known not to be known --- core/src/com/cloud/resource/RequestWrapper.java | 15 ++++++++++----- .../kvm/resource/LibvirtComputingResource.java | 6 +++++- .../resource/wrapper/LibvirtRequestWrapper.java | 3 +++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/core/src/com/cloud/resource/RequestWrapper.java b/core/src/com/cloud/resource/RequestWrapper.java index 4e754d60a299..dc39ed83147c 100644 --- a/core/src/com/cloud/resource/RequestWrapper.java +++ b/core/src/com/cloud/resource/RequestWrapper.java @@ -29,6 +29,11 @@ import com.cloud.agent.api.Command; public abstract class RequestWrapper { + static public class CommandNotSupported extends NullPointerException { + public CommandNotSupported(String msg) { + super(msg); + } + } private static final Logger s_logger = Logger.getLogger(RequestWrapper.class); @@ -52,7 +57,7 @@ protected Hashtable, CommandWrapper> retrieveResource(f keepResourceClass = keepResourceClass2; } catch (final ClassCastException e) { - throw new NullPointerException("No key found for '" + command.getClass() + "' in the Map!"); + throw new CommandNotSupported("No key found for '" + command.getClass() + "' in the Map!"); } } return resource; @@ -69,14 +74,14 @@ protected CommandWrapper retrieveCommands(final final Class commandClass2 = (Class) keepCommandClass.getSuperclass(); if (commandClass2 == null) { - throw new NullPointerException("All the COMMAND hierarchy tree has been visited but no compliant key has been found for '" + commandClass + "'."); + throw new CommandNotSupported("All the COMMAND hierarchy tree has been visited but no compliant key has been found for '" + commandClass + "'."); } commandWrapper = resourceCommands.get(commandClass2); keepCommandClass = commandClass2; } catch (final ClassCastException e) { - throw new NullPointerException("No key found for '" + keepCommandClass.getClass() + "' in the Map!"); + throw new CommandNotSupported("No key found for '" + keepCommandClass.getClass() + "' in the Map!"); } catch (final NullPointerException e) { // Will now traverse all the resource hierarchy. Returning null // is not a problem. @@ -102,7 +107,7 @@ protected CommandWrapper retryWhenAllFails(fina final Class resourceClass2 = (Class) keepResourceClass.getSuperclass(); if (resourceClass2 == null) { - throw new NullPointerException("All the SERVER-RESOURCE hierarchy tree has been visited but no compliant key has been found for '" + command.getClass() + "'."); + throw new CommandNotSupported("All the SERVER-RESOURCE hierarchy tree has been visited but no compliant key has been found for '" + command.getClass() + "'."); } final Hashtable, CommandWrapper> resourceCommands2 = retrieveResource(command, @@ -111,7 +116,7 @@ protected CommandWrapper retryWhenAllFails(fina commandWrapper = retrieveCommands(command.getClass(), resourceCommands2); } catch (final ClassCastException e) { - throw new NullPointerException("No key found for '" + command.getClass() + "' in the Map!"); + throw new CommandNotSupported("No key found for '" + command.getClass() + "' in the Map!"); } catch (final NullPointerException e) { throw e; } diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 424280cc66ca..9d47eb2462ff 100644 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -47,6 +47,8 @@ import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; +import com.cloud.agent.api.UnsupportedAnswer; +import com.cloud.resource.RequestWrapper; import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; import org.apache.cloudstack.storage.to.VolumeObjectTO; import org.apache.cloudstack.utils.hypervisor.HypervisorUtils; @@ -1438,8 +1440,10 @@ public Answer executeRequest(final Command cmd) { final LibvirtRequestWrapper wrapper = LibvirtRequestWrapper.getInstance(); try { return wrapper.execute(cmd, this); - } catch (final Exception e) { + } catch (final RequestWrapper.CommandNotSupported cmde) { return Answer.createUnsupportedCommandAnswer(cmd); + } catch (Exception e) { + return new UnsupportedAnswer(cmd, e.getLocalizedMessage()); } } diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRequestWrapper.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRequestWrapper.java index 0b413bc6e9ff..436a13083168 100644 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRequestWrapper.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRequestWrapper.java @@ -72,6 +72,9 @@ public Answer execute(final Command command, final ServerResource serverResource commandWrapper = retryWhenAllFails(command, resourceClass, resourceCommands); } + if (commandWrapper == null) { + throw new CommandNotSupported("No way to handle " + command.getClass()); + } return commandWrapper.execute(command, serverResource); } } \ No newline at end of file From 2a32fe45620965878bca34b139c0c41f796fc1e6 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 19 Jun 2018 08:07:50 +0000 Subject: [PATCH 2/5] only catch what needs catching, add doc --- .../kvm/resource/LibvirtComputingResource.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 9d47eb2462ff..b626baa082a4 100644 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -1434,6 +1434,14 @@ public boolean stop() { return true; } + /** + * This finds a command wrapper to handle the command and executes it. + * If no wrapper is found an {@see UnsupportedAnswer} is sent back. + * Any other exceptions are to be caught and wrapped in an generic {@see Answer}, marked as failed. + * + * @param cmd the instance of a {@see Command} to execute. + * @return the for the {@see Command} appropriate {@see Answer} or {@see UnsupportedAnswer} + */ @Override public Answer executeRequest(final Command cmd) { @@ -1442,8 +1450,6 @@ public Answer executeRequest(final Command cmd) { return wrapper.execute(cmd, this); } catch (final RequestWrapper.CommandNotSupported cmde) { return Answer.createUnsupportedCommandAnswer(cmd); - } catch (Exception e) { - return new UnsupportedAnswer(cmd, e.getLocalizedMessage()); } } From 14ffb98091fb40052e776320bb059f64d6014521 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 19 Jun 2018 09:53:36 +0000 Subject: [PATCH 3/5] import --- .../cloud/hypervisor/kvm/resource/LibvirtComputingResource.java | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index b626baa082a4..8a94b0586558 100644 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -47,7 +47,6 @@ import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; -import com.cloud.agent.api.UnsupportedAnswer; import com.cloud.resource.RequestWrapper; import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; import org.apache.cloudstack.storage.to.VolumeObjectTO; From 0789ae1074b427e5d265373e55f4d2f694b0ae94 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 19 Jun 2018 13:29:53 +0000 Subject: [PATCH 4/5] unit tests for known - and UnknownAnswer --- .../com/cloud/resource/RequestWrapper.java | 11 +++++--- .../LibvirtComputingResourceTest.java | 27 +++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/core/src/com/cloud/resource/RequestWrapper.java b/core/src/com/cloud/resource/RequestWrapper.java index dc39ed83147c..51a837b0fe9d 100644 --- a/core/src/com/cloud/resource/RequestWrapper.java +++ b/core/src/com/cloud/resource/RequestWrapper.java @@ -30,9 +30,14 @@ public abstract class RequestWrapper { static public class CommandNotSupported extends NullPointerException { + Throwable reason = null; public CommandNotSupported(String msg) { super(msg); } + public CommandNotSupported(String msg, Throwable cause) { + super(msg); + reason = cause; + } } private static final Logger s_logger = Logger.getLogger(RequestWrapper.class); @@ -115,10 +120,8 @@ protected CommandWrapper retryWhenAllFails(fina keepResourceClass = resourceClass2; commandWrapper = retrieveCommands(command.getClass(), resourceCommands2); - } catch (final ClassCastException e) { - throw new CommandNotSupported("No key found for '" + command.getClass() + "' in the Map!"); - } catch (final NullPointerException e) { - throw e; + } catch (final ClassCastException | NullPointerException e) { + throw new CommandNotSupported("No key found for '" + command.getClass() + "' in the Map!", e); } } return commandWrapper; diff --git a/plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java b/plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java index 795b96175abc..be191f5e9a53 100644 --- a/plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java +++ b/plugins/hypervisors/kvm/test/com/cloud/hypervisor/kvm/resource/LibvirtComputingResourceTest.java @@ -38,6 +38,8 @@ import javax.xml.xpath.XPathExpressionException; import javax.xml.xpath.XPathFactory; +import com.cloud.agent.api.Command; +import com.cloud.agent.api.UnsupportedAnswer; import com.cloud.hypervisor.kvm.resource.LibvirtVMDef.CpuTuneDef; import org.apache.commons.lang.SystemUtils; import org.joda.time.Duration; @@ -5191,4 +5193,29 @@ public void testSetQuotaAndPeriodMinQuota() { Assert.assertEquals(CpuTuneDef.MIN_QUOTA, cpuTuneDef.getQuota()); Assert.assertEquals((int) (CpuTuneDef.MIN_QUOTA / pct), cpuTuneDef.getPeriod()); } + + @Test + public void testUnknownCommand() { + libvirtComputingResource = new LibvirtComputingResource(); + Command cmd = new Command() { + @Override public boolean executeInSequence() { + return false; + } + }; + Answer ans = libvirtComputingResource.executeRequest(cmd); + assertTrue(ans instanceof UnsupportedAnswer); + } + + @Test + public void testKnownCommand() { + libvirtComputingResource = new LibvirtComputingResource(); + Command cmd = new PingTestCommand() { + @Override public boolean executeInSequence() { + throw new NullPointerException("test succeeded"); + } + }; + Answer ans = libvirtComputingResource.executeRequest(cmd); + assertFalse(ans instanceof UnsupportedAnswer); + assertTrue(ans instanceof Answer); + } } From 8040754f3a0c3eae973aeeeb24c9d1ddbf9bd6e1 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 19 Jun 2018 17:09:59 +0200 Subject: [PATCH 5/5] initCause --- core/src/com/cloud/resource/RequestWrapper.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/com/cloud/resource/RequestWrapper.java b/core/src/com/cloud/resource/RequestWrapper.java index 51a837b0fe9d..d1a3c1f18cc1 100644 --- a/core/src/com/cloud/resource/RequestWrapper.java +++ b/core/src/com/cloud/resource/RequestWrapper.java @@ -30,13 +30,12 @@ public abstract class RequestWrapper { static public class CommandNotSupported extends NullPointerException { - Throwable reason = null; public CommandNotSupported(String msg) { super(msg); } public CommandNotSupported(String msg, Throwable cause) { super(msg); - reason = cause; + initCause(cause); } }