[KVM_AUTOTEST][RFC 0/3]: QMP basic test-suite

[KVM_AUTOTEST][RFC 0/3]: QMP basic test-suite

[Luiz Capitulino]

This series is an _initial_ work on having a full QMP test-suite in
kvm-autotest. I think it's very near of being in a mergeable state, but I
figured it would be a good idea to get some feedback before submitting the
final version.

You'll find all important details in the patches, being the last the most
important one.

Please, note that there are a number of TODO items to be addressed and I
need help to solve the following issues:

    1. A number of QMP commands are executed before the QMP suite is run,
       which means that some problems can be caught by non-test code (ie.
       real usage).

       Ideally, the QMP suite should be run before any command is executed,
       so that we avoid catching QMP bugs in other test suites and/or in
       kvm-autotest non-test code.

    2. The qmp_capabilities command is run by the QMPMonitor class
       constructor. This makes it impossible for me to test it in the QMP
       suite.

       I have tried destroying and recreating the QMPMonitor object from
       my test-suite, but that didn't seem to work very well.

I have other, more general comments, about kvm-autotest, but I'll send them
in a different thread.

Thanks.

[PATCH 1/3] QMPMonitor: Introduce the get_greeting() method

[Luiz Capitulino]

It returns the QMP's greeting message as sent by the monitor.

Please, note that this commit also changes the QMPMonitor's constructor
to store the full greeting message (it currently stores only its
contents).

This new method is going to be used by the QMP test-suite, which fully
checks the greeting message.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 client/tests/kvm/kvm_monitor.py |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
index 8440835..c23835c 100644
--- a/client/tests/kvm/kvm_monitor.py
+++ b/client/tests/kvm/kvm_monitor.py
@@ -408,7 +408,7 @@ class QMPMonitor(Monitor):
             while time.time() < end_time:
                 for obj in self._read_objects():
                     if "QMP" in obj:
-                        self._greeting = obj["QMP"]
+                        self._greeting = obj
                         break
                 if self._greeting:
                     break
@@ -597,6 +597,13 @@ class QMPMonitor(Monitor):
         self._lock.release()
 
 
+    def get_greeting(self):
+        """
+        Return QMP greeting message.
+        """
+        return self._greeting
+
+
     # Command wrappers
     # Note: all of the following functions raise exceptions in a similar manner
     # to cmd() and _get_command_output().
-- 
1.7.3.1.104.gc752e

[PATCH 2/3] QMPMonitor: Introduce the send() method

[Luiz Capitulino]

This method directly sends data to the QMP monitor and returns its
response without any kind of special treatment or sanity checking.

The send_cmd() method is also introduced. It's a simple wrapper
which sends a QMP command to the monitor.

Both methods are going to be used by the QMP test-suite.

TODO: Refactor _get_command_output() and/or cmd() to use the new
      methods.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 client/tests/kvm/kvm_monitor.py |   49 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
index c23835c..9a66388 100644
--- a/client/tests/kvm/kvm_monitor.py
+++ b/client/tests/kvm/kvm_monitor.py
@@ -604,6 +604,55 @@ class QMPMonitor(Monitor):
         return self._greeting
 
 
+    def send(self, data, timeout=20):
+        """
+        Send data to the QMP monitor and return its response.
+
+        @param data: Data to send
+        @param timeout: Time duration to wait for response
+        @return: The response received
+        @raise MonitorLockError: Raised if the lock cannot be acquired
+        @raise MonitorSendError: Raised if the command cannot be sent
+        @raise MonitorProtocolError: Raised if no response is received
+        """
+        if not self._acquire_lock(20):
+            raise MonitorLockError("Could not acquire exclusive lock to send "
+                                   "QMP command '%s'" % cmd)
+        try:
+            self._read_objects()
+            self._socket.sendall(data)
+            end_time = time.time() + timeout
+            while time.time() < end_time:
+                for obj in self._read_objects():
+                    if isinstance(obj, dict):
+                        if "return" in obj or "error" in obj:
+                            return obj
+                time.sleep(0.1)
+            else:
+                 raise MonitorProtocolError("Received no response (data: %s)"
+                                            % str(data))
+        except socket.error:
+            raise MonitorSendError("Could not send data '%s'" % str(data))
+        finally:
+            self._lock.release()
+
+
+    def send_cmd(self, command, timeout=20):
+        """
+        Send a QMP command. This is a simple wrapper to send() method that does
+        JSON formatting.
+
+        @param command: QMP command to send
+        @param timeout: Time duration to wait for response
+        @return: The response received
+        @raise MonitorLockError: Raised if the lock cannot be acquired
+        @raise MonitorSendError: Raised if the command cannot be sent
+        @raise MonitorProtocolError: Raised if no response is received
+
+        """
+        return self.send(json.dumps(command) + "\n", timeout)
+
+
     # Command wrappers
     # Note: all of the following functions raise exceptions in a similar manner
     # to cmd() and _get_command_output().
-- 
1.7.3.1.104.gc752e

[PATCH 3/3] Introduce QMP basic test-suite

[Luiz Capitulino]

QMP automated testing can be split in three parts:

1. Testing that the basic protocol works as specified. That is, ensuring
   that the greeting message, success responses and error messages
   contain the basic information the spec says they do. More importantly,
   several errors conditions at the protocol level should be tested

2. Test that each available command behaves as specified by its *own*
   specification. This is a big effort, as each command should have
   its own test suite

3. Asynchronous messages testing. Like command testing, each asynchronous
   message should be tested separately

This commit introduces a new suite to test item 1, that is, the goal
is to ensure that QMP behaves as specified by the basic protocol
specification (which is file QMP/qmp-spec.txt in QEMU's source tree).

Items 2 and 3 are not addressed in this commit in any way. They are a
continuous and long term type of work.

TODO:

o Finding which test failed is not as easy as it should be, is this
  is this a kvm-autotest problem?

o Are all those check_*() functions really needed? Can't we have
  all those checks in only one function?

o The argument_checker_suite() is incomplete

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 client/tests/kvm/tests/qmp_basic.py    |  268 ++++++++++++++++++++++++++++++++
 client/tests/kvm/tests_base.cfg.sample |    3 +
 2 files changed, 271 insertions(+), 0 deletions(-)
 create mode 100644 client/tests/kvm/tests/qmp_basic.py

diff --git a/client/tests/kvm/tests/qmp_basic.py b/client/tests/kvm/tests/qmp_basic.py
new file mode 100644
index 0000000..89dbe6a
--- /dev/null
+++ b/client/tests/kvm/tests/qmp_basic.py
@@ -0,0 +1,268 @@
+import logging, json
+from autotest_lib.client.common_lib import error
+import kvm_subprocess, kvm_test_utils, kvm_utils
+
+def run_qmp_basic(test, params, env):
+    """
+    QMP Specification test-suite: this checks if the *basic* protocol conforms
+    to its specification.
+
+    Please, check it suite for details.
+    """
+    def fail_no_key(qmp_dict, key):
+        if not isinstance(qmp_dict, dict):
+            raise error.TestFail("qmp_dict is not a dict (it's '%s')" % type(qmp_dict))
+        if not key in qmp_dict:
+            raise error.TestFail("'%s' key doesn't exist in dict ('%s')"
+                                 % (key, str(qmp_dict)))
+
+    def check_dict_key(qmp_dict, key, keytype):
+        """
+        Performs the following checks on a QMP dict key:
+
+        1. qmp_dict is a dict
+        2. key exists in qmp_dict
+        3. key is of type keytype
+        
+        If any of these checks fails, error.TestFail is raised.
+        """
+        fail_no_key(qmp_dict, key)
+        if not isinstance(qmp_dict[key], keytype):
+            raise error.TestFail("'%s' key is not of type '%s', it's '%s'"
+                                 % (key, keytype, type(qmp_dict[key])))
+
+    def check_key_is_dict(qmp_dict, key):
+        check_dict_key(qmp_dict, key, dict)
+
+    def check_key_is_list(qmp_dict, key):
+        check_dict_key(qmp_dict, key, list)
+
+    def check_key_is_str(qmp_dict, key):
+        check_dict_key(qmp_dict, key, unicode)
+
+    def check_str_key(qmp_dict, keyname, value=None):
+        check_dict_key(qmp_dict, keyname, unicode)
+        if value and value != qmp_dict[keyname]:
+            raise error.TestFail("'%s' key value '%s' should be '%s'"
+                                 % (keyname, str(qmp_dict[keyname]), str(value)))
+
+    def check_key_is_int(qmp_dict, key):
+        fail_no_key(qmp_dict, key)
+        try:
+            value = int(qmp_dict[key])
+        except:
+            raise error.TestFail("'%s' key is not of type int, it's '%s'"
+                                 % (key, type(qmp_dict[key])))
+
+    def check_bool_key(qmp_dict, keyname, value=None):
+        check_dict_key(qmp_dict, keyname, bool)
+        if value and value != qmp_dict[keyname]:
+            raise error.TestFail("'%s' key value '%s' should be '%s'"
+                                 % (keyname, str(qmp_dict[keyname]), str(value)))
+
+    def check_success_resp(resp, empty=False):
+        check_key_is_dict(resp, "return")
+        if empty and len(resp["return"]) > 0:
+            raise error.TestFail("success response is not empty ('%s')"
+                                 % str(resp))
+
+    def check_error_resp(resp, classname=None, datadict=None):
+        check_key_is_dict(resp, "error")
+        check_key_is_str(resp["error"], "class")
+        if classname and resp["error"]["class"] != classname:
+            raise error.TestFail("error class is '%s' but should be '%s'"
+                                 % (resp["error"]["class"], classname))
+        check_key_is_dict(resp["error"], "data")
+        if datadict and resp["error"]["data"] != datadict:
+            raise error.TestFail("data dict is '%s' but should be '%s'"
+                                 % (resp["error"]["data"], datadict))
+
+    def test_version(version):
+        """
+        Test QMP greeting message's version key which, according to QMP's
+        documentation, should be:
+
+        { "qemu": { "major": json-int, "minor": json-int, "micro": json-int }
+          "package": json-string }
+        """
+        check_key_is_dict(version, "qemu")
+        for key in [ "major", "minor", "micro" ]:
+            check_key_is_int(version["qemu"], key)
+        check_key_is_str(version, "package")
+
+    def test_greeting(greeting):
+        check_key_is_dict(greeting, "QMP")
+        check_key_is_dict(greeting["QMP"], "version")
+        check_key_is_list(greeting["QMP"], "capabilities")
+
+    def greeting_suite(monitor):
+        """
+        Check QMP's greeting message which, according to QMP's spec section
+        '2.2 Server Greeting', is:
+
+        { "QMP": { "version": json-object, "capabilities": json-array } }
+        """
+        greeting = monitor.get_greeting()
+        test_greeting(greeting)
+        test_version(greeting["QMP"]["version"])
+
+
+    def json_parsing_suite(monitor):
+        """
+        Check the QMP's parser conforms to the JSON's spec (RFC 4627).
+        """
+        # We're quite simple right now and the focus is parsing problems
+        # that have already biten us in the past.
+        #
+        # However, the following test-cases are missing:
+        #
+        #   - JSON numbers, strings and arrays
+        #   - More invalid characters or malformed structures
+        #   - Valid, but not obvious syntax (like zillion of spaces or
+        #     strings with unicode chars)
+        bad_json = []
+        
+        # A JSON value MUST be an object, array, number, string, or true,
+        # false, null
+        #
+        # NOTE: QMP seems to ignore a number of chars, like: | and ?
+        bad_json.append(":")
+        bad_json.append(",")
+
+        # Malformed json-objects
+        #
+        # NOTE: sending only "}" seems to break QMP
+        # NOTE: Duplicate keys are accepted (should it?)
+        bad_json.append("{ \"execute\" }")
+        bad_json.append("{ \"execute\": \"query-version\", }")
+        bad_json.append("{ 1: \"query-version\" }")
+        bad_json.append("{ true: \"query-version\" }")
+        bad_json.append("{ []: \"query-version\" }")
+        bad_json.append("{ {}: \"query-version\" }")
+
+        for cmd in bad_json:
+            resp = monitor.send(cmd)
+            check_error_resp(resp, "JSONParsing")
+
+
+    def test_id_key(monitor):
+        """
+        Check if QMP's "id" key is handled correctly.
+        """
+        # The "id" key must be echoed back in error responses
+        id = "kvm-autotest"
+        resp = monitor.send_cmd({ "execute": "eject", "id": id,
+                                  "arguments": { "foobar": True }})
+        check_error_resp(resp)
+        check_str_key(resp, "id", id)
+
+        # The "id" key must be echoed back in success responses
+        resp = monitor.send_cmd({ "execute": "query-status", "id": id })
+        check_success_resp(resp)
+        check_str_key(resp, "id", id)
+
+        # The "id" key can be any json-object
+        for id in [ True, 1234, "string again!", [1, 2, 3, 4], { "key": {} } ]:
+            resp = monitor.send_cmd({ "execute": "query-status", "id": id })
+            check_success_resp(resp)
+            if resp["id"] != id:
+                raise error.TestFail("expected id '%s' but got '%s'"
+                                     % (str(id), str(resp["id"])))
+
+    def test_invalid_arg_key(monitor):
+        """
+        Currently, the only supported keys in the input object are: "execute",
+        "arguments" and "id". Although expansion is supported, invalid key
+        names must be detected.
+        """
+        resp = monitor.send_cmd({ "execute": "eject", "foobar": True })
+        check_error_resp(resp, "QMPExtraInputObjectMember",
+                         { "member": "foobar" })
+
+    def test_arguments_key(monitor):
+        """
+        The "arguments" key must be an json-object.
+        
+        We use the eject command to perform the tests, but that's a random
+        choice, any command that accepts arguments will do.
+        """
+        for item in [ True, [], 1, "foo" ]:
+            resp = monitor.send_cmd({ "execute": "eject", "arguments": item })
+            check_error_resp(resp, "QMPBadInputObjectMember",
+                             { "member": "arguments", "expected": "object" })
+
+    def test_execute_key_type(monitor):
+        """
+        The "execute" key must be a json-string.
+        """
+        for item in [ False, 1, {}, [] ]:
+            resp = monitor.send_cmd({ "execute": item })
+            check_error_resp(resp, "QMPBadInputObjectMember",
+                             { "member": "execute", "expected": "string" })
+
+    def test_no_execute_key(monitor):
+        """
+        The "execute" key must exist, we also test for some stupid parsing
+        errors.
+        """
+        for cmd in [ {}, { "execut": "qmp_capabilities" },
+                     { "executee": "qmp_capabilities" }, { "foo": "bar" }]:
+            resp = monitor.send_cmd(cmd)
+            check_error_resp(resp) # XXX: check class and data dict?
+
+    def test_input_obj_type(monitor):
+        """
+        The input object must be... an object.
+        """
+        for cmd in [ "foo", [], True, 1 ]:
+            resp = monitor.send_cmd(cmd)
+            check_error_resp(resp, "QMPBadInputObject", { "expected":"object" })
+
+    def input_object_suite(monitor):
+        """
+        Check QMP's input object which, according to QMP's spec section
+        '2.3 Issuing Commands', is:
+
+        { "execute": json-string, "arguments": json-object, "id": json-value }
+        """
+        test_input_obj_type(monitor)
+        test_no_execute_key(monitor)
+        test_execute_key_type(monitor)
+        test_arguments_key(monitor)
+        test_invalid_arg_key(monitor)
+        test_id_key(monitor)
+
+    def argument_checker_suite(monitor):
+        """
+        Checks if QMP's argument checker is detecting all possible errors.
+
+        We use a number of different commands to perform the checks, but most
+        of the time the command used doesn't matter because QMP performs the
+        argument checking _before_ calling the command.
+        """
+        # system_reset doesn't take arguments
+        resp = monitor.send_cmd({ "execute": "system_reset", "arguments": { "foo": 1 }})
+        check_error_resp(resp, "InvalidParameter", { "name": "foo" })
+
+    def unknown_commands_suite(monitor):
+        """
+        Check if QMP handles unknown commands correctly.
+        """
+        # We also call a HMP-only command, to be sure it will fail as expected
+        for cmd in [ "bar", "query-", "query-foo", "q", "help" ]:
+            resp = monitor.send_cmd({ "execute": cmd })
+            check_error_resp(resp, "CommandNotFound", { "name": cmd })
+
+
+    vm = kvm_test_utils.get_living_vm(env, params.get("main_vm"))
+
+    # Run all suites
+    greeting_suite(vm.monitor)
+    json_parsing_suite(vm.monitor)
+    input_object_suite(vm.monitor)
+    argument_checker_suite(vm.monitor)
+    unknown_commands_suite(vm.monitor)
+
+    # check if QMP is still alive
+    resp = vm.monitor.cmd("query-status")
+    check_bool_key(resp, "running", True)
diff --git a/client/tests/kvm/tests_base.cfg.sample b/client/tests/kvm/tests_base.cfg.sample
index 167e86d..8eaefca 100644
--- a/client/tests/kvm/tests_base.cfg.sample
+++ b/client/tests/kvm/tests_base.cfg.sample
@@ -456,6 +456,9 @@ variants:
             - fmt_raw:
                 image_format_stg = raw
 
+    - qmp_basic: install setup unattended_install.cdrom
+        type = qmp_basic
+
     - vlan_tag:  install setup unattended_install.cdrom
         type = vlan_tag
         # subnet should not be used by host
-- 
1.7.3.1.104.gc752e

Re: [KVM_AUTOTEST][RFC 0/3]: QMP basic test-suite

[Luiz Capitulino]

On Thu,  7 Oct 2010 16:52:04 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> This series is an _initial_ work on having a full QMP test-suite in
> kvm-autotest. I think it's very near of being in a mergeable state, but I
> figured it would be a good idea to get some feedback before submitting the
> final version.

Cool, autotest list is closed for non-subscribers. Adding this info to the
kvm-autotest wiki page.

Re: [KVM_AUTOTEST][RFC 0/3]: QMP basic test-suite

[Lucas Meneghel Rodrigues]

On Thu, 2010-10-07 at 16:52 -0300, Luiz Capitulino wrote:
> This series is an _initial_ work on having a full QMP test-suite in
> kvm-autotest. I think it's very near of being in a mergeable state, but I
> figured it would be a good idea to get some feedback before submitting the
> final version.
> 
> You'll find all important details in the patches, being the last the most
> important one.
> 
> Please, note that there are a number of TODO items to be addressed and I
> need help to solve the following issues:
> 
>     1. A number of QMP commands are executed before the QMP suite is run,
>        which means that some problems can be caught by non-test code (ie.
>        real usage).

We can test it in a dedicated job, that does qemu specific testing (ie,
non-vm related work, such as qmp, qemu-block, qemu-unittests and such),
or even have a fully dedicated job to qmp.

>        Ideally, the QMP suite should be run before any command is executed,
>        so that we avoid catching QMP bugs in other test suites and/or in
>        kvm-autotest non-test code.
> 
>     2. The qmp_capabilities command is run by the QMPMonitor class
>        constructor. This makes it impossible for me to test it in the QMP
>        suite.
> 
>        I have tried destroying and recreating the QMPMonitor object from
>        my test-suite, but that didn't seem to work very well.

We can add an additional parameter to test parameters that indicates
whether the monitors are going to be tested. Today we have this in
tests_base.cfg.sample:

monitors = humanmonitor1

# Choose the main VM and monitor
main_vm = vm1
main_monitor = humanmonitor1
...
# Monitor params
monitor_type = human

We can add an additional parameter (maybe monitor_mode_[monitor_name])
to indicate the monitor is under test:

monitor_mode_humanmonitor1 = test
or
monitor_mode_humanmonitor1 = normal

We are indicating whether a given monitor is under test or not with this
parameter (defaults allways to normal). If mode is 'test', we handle
monitor class initialization, and potentially, other methods
differently. What do you think?

> I have other, more general comments, about kvm-autotest, but I'll send them
> in a different thread.

All right Luiz, this is all excellent, thanks!

> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [KVM_AUTOTEST][RFC 0/3]: QMP basic test-suite

[Luiz Capitulino]

On Fri, 08 Oct 2010 07:14:33 -0300
Lucas Meneghel Rodrigues <lmr@redhat.com> wrote:

> On Thu, 2010-10-07 at 16:52 -0300, Luiz Capitulino wrote:
> > This series is an _initial_ work on having a full QMP test-suite in
> > kvm-autotest. I think it's very near of being in a mergeable state, but I
> > figured it would be a good idea to get some feedback before submitting the
> > final version.
> > 
> > You'll find all important details in the patches, being the last the most
> > important one.
> > 
> > Please, note that there are a number of TODO items to be addressed and I
> > need help to solve the following issues:
> > 
> >     1. A number of QMP commands are executed before the QMP suite is run,
> >        which means that some problems can be caught by non-test code (ie.
> >        real usage).
> 
> We can test it in a dedicated job, that does qemu specific testing (ie,
> non-vm related work, such as qmp, qemu-block, qemu-unittests and such),
> or even have a fully dedicated job to qmp.

Ok, maybe it will do it, but the important thing is: qmp specifc testing
should run first and if it fails, I think no other test should run, as they
may fail because of monitor bugs.

> >        Ideally, the QMP suite should be run before any command is executed,
> >        so that we avoid catching QMP bugs in other test suites and/or in
> >        kvm-autotest non-test code.
> > 
> >     2. The qmp_capabilities command is run by the QMPMonitor class
> >        constructor. This makes it impossible for me to test it in the QMP
> >        suite.
> > 
> >        I have tried destroying and recreating the QMPMonitor object from
> >        my test-suite, but that didn't seem to work very well.
> 
> We can add an additional parameter to test parameters that indicates
> whether the monitors are going to be tested. Today we have this in
> tests_base.cfg.sample:
> 
> monitors = humanmonitor1
> 
> # Choose the main VM and monitor
> main_vm = vm1
> main_monitor = humanmonitor1
> ...
> # Monitor params
> monitor_type = human
> 
> We can add an additional parameter (maybe monitor_mode_[monitor_name])
> to indicate the monitor is under test:
> 
> monitor_mode_humanmonitor1 = test
> or
> monitor_mode_humanmonitor1 = normal
> 
> We are indicating whether a given monitor is under test or not with this
> parameter (defaults allways to normal). If mode is 'test', we handle
> monitor class initialization, and potentially, other methods
> differently. What do you think?

I think I now understand why we have zillions of options :-)

Don't take me wrong (and this is another subject, anyway) but something I
disliked in kvm-autotest is that I had to edit different files to get it
running and add new tests. Not to mention that the files you change, are not
the files in the repository. So you end up having to keep track of a few
options in a few different files.

I know you have to deal with a number of different test setups, and this is
no easy job, but at the same time I feel this could be simplified.

Anyway, back to QMP testing. Adding a new parameter too all tests seems
a bit complex to me.

What about this: we make qmp_basic.py (or any monitor specific testing)
a special type of test cases and add them to 'monitor_testing' option
in tests_base.cfg:

monitor_testing = qmp_basic.py hmp_basic.py

And then the VM.create() method (in kvm_vm.py) could test for that option
and automatically run those tests even before creating the monitor classes.

If a test fails, kvm-autotest aborts.

I'm now wondering whether this fits in the current design.


> 
> > I have other, more general comments, about kvm-autotest, but I'll send them
> > in a different thread.
> 
> All right Luiz, this is all excellent, thanks!
> 
> > Thanks.
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>