----- "sudhir kumar" <smalikphy@xxxxxxxxx> wrote:
> On Sun, May 24, 2009 at 9:16 PM, Michael Goldish <mgoldish@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > ---
> > 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.
> > + 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.
> >
> > - # 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
