Re: [KVM-AUTOTEST PATCH] kvm_vm.py: make sure the bulk of VM.create() is not executed in parallel

From: sudhir kumar <smalikphy@gmail.com>
To: Michael Goldish <mgoldish@redhat.com>
Cc: kvm@vger.kernel.org
Subject: Re: [KVM-AUTOTEST PATCH] kvm_vm.py: make sure the bulk of VM.create() is not executed in parallel
Date: Tue, 26 May 2009 10:57:12 +0530	[thread overview]
Message-ID: <a50cf5ab0905252227r5b7cdd74me6db13edb2026b86@mail.gmail.com> (raw)
In-Reply-To: <408628393.327191243243079295.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com>

On Mon, May 25, 2009 at 2:47 PM, Michael Goldish <mgoldish@redhat.com> wrote:
>
> ----- "sudhir kumar" <smalikphy@gmail.com> wrote:
>
>> On Sun, May 24, 2009 at 9:16 PM, Michael Goldish <mgoldish@redhat.com>
>> wrote:
>> > VM.create() does a few things (such as finding free ports) which are
>> not safe
>> > to execute in parallel. Use a lock file to make sure this doesn't
>> happen. The
>> > lock is released only after the VM is started or fails to start.
>> >
>> > Signed-off-by: Michael Goldish <mgoldish@redhat.com>
>> > ---
>> >  client/tests/kvm_runtest_2/kvm_vm.py |   85
>> +++++++++++++++++++---------------
>> >  1 files changed, 48 insertions(+), 37 deletions(-)
>> >
>> > diff --git a/client/tests/kvm_runtest_2/kvm_vm.py
>> b/client/tests/kvm_runtest_2/kvm_vm.py
>> > index 3ce2003..af06693 100644
>> > --- a/client/tests/kvm_runtest_2/kvm_vm.py
>> > +++ b/client/tests/kvm_runtest_2/kvm_vm.py
>> > @@ -3,6 +3,7 @@
>> >  import time
>> >  import socket
>> >  import os
>> > +import fcntl
>> >
>> >  import kvm_utils
>> >  import kvm_log
>> > @@ -289,48 +290,58 @@ class VM:
>> >                     kvm_log.error("Actual MD5 sum differs from
>> expected one")
>> >                     return False
>> >
>> > -        # Handle port redirections
>> > -        redir_names = kvm_utils.get_sub_dict_names(params,
>> "redirs")
>> > -        host_ports = kvm_utils.find_free_ports(5000, 6000,
>> len(redir_names))
>> > -        self.redirs = {}
>> > -        for i in range(len(redir_names)):
>> > -            redir_params = kvm_utils.get_sub_dict(params,
>> redir_names[i])
>> > -            guest_port = int(redir_params.get("guest_port"))
>> > -            self.redirs[guest_port] = host_ports[i]
>> > -
>> > -        # Find available VNC port, if needed
>> > -        if params.get("display") == "vnc":
>> > -            self.vnc_port = kvm_utils.find_free_port(5900, 6000)
>> > -
>> > -        # Make qemu command
>> > -        qemu_command = self.make_qemu_command()
>> > +        # Make sure the following code is not executed by more than
>> one thread
>> > +        # at the same time
>> > +        lockfile = open("/tmp/kvm-autotest-vm-create.lock", "w+")
>> How do you handle an open failure?
>
> Obviously I don't, but I don't think there should be a reason for this to
> fail, unless you deliberately create a file with such a name and with no
> write permission. The mode "w+" creates the file if it doesn't exist.
> In the unlikely event of an open failure the test will fail with some
> Python exception.
>
> It may be a good idea to create the file in the test's local directory
> (test.bindir) instead of /tmp to prevent a symlink attack.
Agree.
>
>> > +        fcntl.lockf(lockfile, fcntl.LOCK_EX)
>> What if other instance has locked the file at the moment. Definitely
>> you would not like to fail. You may want to wait for a while and try
>> agian.
>
> This is what fcntl.lockf() does -- it blocks until it can acquire the
> lock (it doesn't fail).
>
>> I feel the default behaviour should be a blocking one but
>> still you want to print the debug message
>> kvm_log.debug("Trying to acquire lock for port selection")
>> before getting the lock.
>
> This could be a good idea, but I'm not sure it's necessary, because while
> one process waits, another process is busy doing stuff and printing
> stuff, so the user never gets bored. Also, the time duration to wait is
> typically around 1 sec.
>
> In any case it shouldn't hurt too much to print debugging info, so I'll
> either resend this patch, or add to it in a future patch.
thanks.
>
>> >
>> > -        # Is this VM supposed to accept incoming migrations?
>> > -        if for_migration:
>> > -            # Find available migration port
>> > -            self.migration_port = kvm_utils.find_free_port(5200,
>> 6000)
>> > -            # Add -incoming option to the qemu command
>> > -            qemu_command += " -incoming tcp:0:%d" %
>> self.migration_port
>> > +        try:
>> > +            # Handle port redirections
>> > +            redir_names = kvm_utils.get_sub_dict_names(params,
>> "redirs")
>> > +            host_ports = kvm_utils.find_free_ports(5000, 6000,
>> len(redir_names))
>> > +            self.redirs = {}
>> > +            for i in range(len(redir_names)):
>> > +                redir_params = kvm_utils.get_sub_dict(params,
>> redir_names[i])
>> > +                guest_port = int(redir_params.get("guest_port"))
>> > +                self.redirs[guest_port] = host_ports[i]
>> > +
>> > +            # Find available VNC port, if needed
>> > +            if params.get("display") == "vnc":
>> > +                self.vnc_port = kvm_utils.find_free_port(5900,
>> 6000)
>> > +
>> > +            # Make qemu command
>> > +            qemu_command = self.make_qemu_command()
>> > +
>> > +            # Is this VM supposed to accept incoming migrations?
>> > +            if for_migration:
>> > +                # Find available migration port
>> > +                self.migration_port =
>> kvm_utils.find_free_port(5200, 6000)
>> > +                # Add -incoming option to the qemu command
>> > +                qemu_command += " -incoming tcp:0:%d" %
>> self.migration_port
>> > +
>> > +            kvm_log.debug("Running qemu command:\n%s" %
>> qemu_command)
>> > +            (status, pid, output) = kvm_utils.run_bg(qemu_command,
>> None, kvm_log.debug, "(qemu) ")
>> > +
>> > +            if status:
>> > +                kvm_log.debug("qemu exited with status %d" %
>> status)
>> > +                kvm_log.error("VM could not be created -- qemu
>> command failed:\n%s" % qemu_command)
>> > +                return False
>> >
>> > -        kvm_log.debug("Running qemu command:\n%s" % qemu_command)
>> > -        (status, pid, output) = kvm_utils.run_bg(qemu_command,
>> None, kvm_log.debug, "(qemu) ")
>> > +            self.pid = pid
>> >
>> > -        if status:
>> > -            kvm_log.debug("qemu exited with status %d" % status)
>> > -            kvm_log.error("VM could not be created -- qemu command
>> failed:\n%s" % qemu_command)
>> > -            return False
>> > -
>> > -        self.pid = pid
>> > +            if not kvm_utils.wait_for(self.is_alive, timeout, 0,
>> 1):
>> > +                kvm_log.debug("VM is not alive for some reason")
>> > +                kvm_log.error("VM could not be created with
>> command:\n%s" % qemu_command)
>> > +                self.destroy()
>> > +                return False
>> >
>> > -        if not kvm_utils.wait_for(self.is_alive, timeout, 0, 1):
>> > -            kvm_log.debug("VM is not alive for some reason")
>> > -            kvm_log.error("VM could not be created with
>> command:\n%s" % qemu_command)
>> > -            self.destroy()
>> > -            return False
>> > +            kvm_log.debug("VM appears to be alive with PID %d" %
>> self.pid)
>> >
>> > -        kvm_log.debug("VM appears to be alive with PID %d" %
>> self.pid)
>> > +            return True
>> >
>> > -        return True
>> > +        finally:
>> > +            fcntl.lockf(lockfile, fcntl.LOCK_UN)
>> > +            lockfile.close()
>> >
>> >     def send_monitor_cmd(self, command, block=True, timeout=20.0):
>> >         """Send command to the QEMU monitor.
>> > --
>> > 1.5.4.1
>> >
>> > --
>> > 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
>> >
>>
>>
>>
>> --
>> Sudhir Kumar
>> --
>> 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
>


-- 
Sudhir Kumar