On Thu, Jul 24, 2014 at 2:31 AM, Yuto KAWAMURA(kawamuray) <
[email protected]> wrote:

> LXC can mount block device as its rootfs by specifing device path to
>

... mount a block ...
s/specifing device path to/specifying the device path as/


> lxc.rootfs in config file.
>

... in the config file ...


> The mount of rootfs will be done in isolated mount namespace, so after
>

... in an isolated ...


> this change, we can't see the instance rootfs as a part of host file
>

... of the host ...


> system without chaning mount namespace.
>

s/chaning/changing the/


> Change the value of lxc.rootfs config parameter to the instance rootfs
>

To make this sound more consistent with previous sentences:

This patch changes ... of the lxc.rootfs


> device path and discard procedures to mouting/unmounting rootfs.
>

... discards procedures for mounting ... the rootfs.


>
> Signed-off-by: Yuto KAWAMURA(kawamuray) <[email protected]>
> ---
>  lib/hypervisor/hv_lxc.py | 39 +++++++++------------------------------
>  1 file changed, 9 insertions(+), 30 deletions(-)
>
> diff --git a/lib/hypervisor/hv_lxc.py b/lib/hypervisor/hv_lxc.py
> index c21a4df..c33ed14 100644
> --- a/lib/hypervisor/hv_lxc.py
> +++ b/lib/hypervisor/hv_lxc.py
> @@ -251,7 +251,7 @@ class LXCHypervisor(hv_base.BaseHypervisor):
>          data.append(info)
>      return data
>
> -  def _CreateConfigFile(self, instance, root_dir):
> +  def _CreateConfigFile(self, instance, sda_dev_path):
>      """Create an lxc.conf file for an instance.
>
>      """
> @@ -274,7 +274,7 @@ class LXCHypervisor(hv_base.BaseHypervisor):
>      out.append("lxc.console = %s" % console_log)
>
>      # root FS
> -    out.append("lxc.rootfs = %s" % root_dir)
> +    out.append("lxc.rootfs = %s" % sda_dev_path)
>
>      # TODO: additional mounts, if we disable CAP_SYS_ADMIN
>
> @@ -342,9 +342,6 @@ class LXCHypervisor(hv_base.BaseHypervisor):
>      except errors.GenericError, err:
>        raise HypervisorError("Creating instance directory failed: %s",
> str(err))
>
> -    conf_file = self._InstanceConfFile(instance.name)
> -    utils.WriteFile(conf_file, data=self._CreateConfigFile(instance,
> root_dir))
> -
>      log_file = self._InstanceLogFile(instance.name)
>      if not os.path.exists(log_file):
>        try:
> @@ -354,15 +351,14 @@ class LXCHypervisor(hv_base.BaseHypervisor):
>                                       " instance %s failed: %s" %
>                                       (log_file, instance.name, err))
>
> -    if not os.path.ismount(root_dir):
> -      if not block_devices:
> -        raise HypervisorError("LXC needs at least one disk")
> +    if not block_devices:
> +      raise HypervisorError("LXC needs at least one disk")
> +
> +    sda_dev_path = block_devices[0][1]
> +    conf_file = self._InstanceConfFile(instance.name)
> +    conf = self._CreateConfigFile(instance, sda_dev_path)
> +    utils.WriteFile(conf_file, data=conf)
>
> -      sda_dev_path = block_devices[0][1]
> -      result = utils.RunCmd(["mount", sda_dev_path, root_dir])
> -      if result.failed:
> -        raise HypervisorError("Mounting the root dir of LXC instance %s"
> -                              " failed: %s" % (instance.name,
> result.output))
>      result = utils.RunCmd(["lxc-start", "-n", instance.name,
>                             "-o", log_file,
>                             "-l", "DEBUG",
> @@ -396,23 +392,6 @@ class LXCHypervisor(hv_base.BaseHypervisor):
>          if result.failed:
>            logging.error("Failed to stop instance %s: %s", name,
> result.output)
>
> -    if not os.path.ismount(root_dir):
> -      return
> -
> -    for mpath in self._GetMountSubdirs(root_dir):
> -      result = utils.RunCmd(timeout_cmd.extend(["umount", mpath]))
> -      if result.failed:
> -        logging.warning("Error while umounting subpath %s for instance
> %s: %s",
> -                        mpath, name, result.output)
> -
> -    result = utils.RunCmd(timeout_cmd.extend(["umount", root_dir]))
> -    if result.failed and force:
> -      msg = ("Processes still alive in the chroot: %s" %
> -             utils.RunCmd("fuser -vm %s" % root_dir).output)
> -      logging.error(msg)
> -      raise HypervisorError("Unmounting the chroot dir failed: %s (%s)" %
> -                            (result.output, msg))
> -
>    def RebootInstance(self, instance):
>      """Reboot an instance.
>
> --
> 1.8.5.5
>
>
Ah, so this is where this change ended up. A rule used in Ganeti
development is that every patch should be a whole whenever possible, and
that includes at least being functional wrt linting.

Beyond the nitpicks this patch looks good to me, so I will apply them
myself. Let me know if you need the interdiff.

LGTM, thanks!


Hrvoje Ribicic
Ganeti Engineering
Google Germany GmbH
Dienerstr. 12, 80331, München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370

Reply via email to