On Fri, Jul 22, 2011 at 7:21 AM, Iustin Pop <[email protected]> wrote:
> On Wed, Jul 20, 2011 at 02:33:42PM -0400, Ben Lipton wrote:
> > p2v_transfer.py is run from the transfer OS to connect to the instance
> > and perform the transfer. It takes three arguments:
> > * Device that stores the root filesystem of the source OS
> > * Name or address of instance to connect to
> > * Path of a private key matching the public key installed on the
> > instance.
> >
> > For the time being, the administrator needs to generate the keys and
> > provide the private key to the user manually.
>
> LGTM and will push, with some comments. You can either address them or
> add TODOs/README notes about them:
>
Thanks, I will write myself some TODOs, as I can't see any deep reasons to
require xen or python > 2.5. In addition to your comments below, I'm also
putting in a note to update the partitioning so that the swap size depends
on the swap size on the source machine or the space available on the
instance. Right now it's a constant (and totally arbitrary) value set in the
main function, but I'm not planning to leave it that way.
>
> > +def _WaitForCompletion(channel):
> > + """Wait for a remote command to complete.
> > +
> > + Helper function that sleeps until the last command run by the channel
> has
> > + completed.
> > +
> > + @type channel: paramiko.Channel
> > + @param channel: The channel to wait for. If the command was run with
> > + stdin, stdout, stderr = exec_command(), use stdout.channel.
> > +
> > + """
> > + while not channel.exit_status_ready():
> > + time.sleep(.01)
>
> Hrmm, no way to break out of this function. Is is possible that the
> channel gets confused and/or exit_status_ready won't become true for a
> long time without any progress report to the user?
>
Currently _WaitForCompletion never gets called, but I do use
_RunCommandAndWait, which uses channel.recv_exit_status, which blocks until
the remote command finishes. I'm not sure what paramiko does if we lose the
connection or similar, but I should probably add a timeout to be sure.
>
> > +def PartitionTargetDisks(client, swap_cyls):
> > + """Partition and format the disks on the target machine.
> > +
> > + Sends commands over the SSH connection to partition and format the
> > + disk of the target instance.
> > +
> > + @type client: paramiko.SSHClient
> > + @param client: SSH client object used to connect to the instance.
> > + @type swap_cyls: int
> > + @param swap_cyls: Desired size of swap space, in cylinders
> > +
> > + """
> > + # Find out how many cylinders are available on target
> > + total_cyls = 0
> > + stdin, stdout, stderr = client.exec_command("sfdisk -l /dev/xvda")
>
> Here and in the code below, you hardcode 'xvda'. This is correct only
> for Xen, so I would recommend:
>
> - abstract the device root (xvda) into a parameter to all functions
> - read the available devices from the remote target at run-time
> - ask the user if there's more than one block device available,
> otherwise choose the first one
>
> > +def UnmountSourceFilesystems():
> > + """Undo mounts performed by MountSourceFilesystems.
> > +
> > + Unmounts all filesystems mounted by MountSourceFilesystems. Currently,
> since
> > + only the root filesystem ever gets mounted, this only unmounts the
> root
> > + filesystem.
> > +
> > + """
> > + try:
> > + subprocess.check_call(["umount", SOURCE_MOUNT])
>
> subprocess.check_call was added in 2.5; currently, all ganeti code is
> 2.4 compatible, so a README mention would be welcome.
>
> thanks,
> iustin
>
