kvm: send unsupported answer only when applicable (#2714)

Throw specific NPE child when command is known not to be known. Add unit tests.

Author: DaanHoogland

core/src/com/cloud/resource/RequestWrapper.java

@@ -29,6 +29,15 @@
 import com.cloud.agent.api.Command;
 
 public abstract class RequestWrapper {
+    static public class CommandNotSupported extends NullPointerException {
+        public CommandNotSupported(String msg) {
+            super(msg);
+        }
+        public CommandNotSupported(String msg, Throwable cause) {
+            super(msg);
+            initCause(cause);
+        }
+    }
 
     private static final Logger s_logger = Logger.getLogger(RequestWrapper.class);
 
@@ -52,7 +61,7 @@ protected Hashtable<Class<? extends Command>, 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 +78,14 @@ protected CommandWrapper<Command, Answer, ServerResource> retrieveCommands(final
                 final Class<? extends Command> commandClass2 = (Class<? extends Command>) 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,18 +111,16 @@ protected CommandWrapper<Command, Answer, ServerResource> retryWhenAllFails(fina
                 final Class<? extends ServerResource> resourceClass2 = (Class<? extends ServerResource>) 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<Class<? extends Command>, CommandWrapper> resourceCommands2 = retrieveResource(command,
                         (Class<? extends ServerResource>) keepResourceClass.getSuperclass());
                 keepResourceClass = resourceClass2;
 
                 commandWrapper = retrieveCommands(command.getClass(), resourceCommands2);
-            } catch (final ClassCastException e) {
-                throw new NullPointerException("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;

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -47,6 +47,7 @@
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 
+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;
@@ -1432,13 +1433,21 @@ 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) {
 
         final LibvirtRequestWrapper wrapper = LibvirtRequestWrapper.getInstance();
         try {
             return wrapper.execute(cmd, this);
-        } catch (final Exception e) {
+        } catch (final RequestWrapper.CommandNotSupported cmde) {
             return Answer.createUnsupportedCommandAnswer(cmd);
         }
     }

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);
     }
 }

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);
+    }
 }