[+list]
On Mon, Jun 28, 2010 at 12:44:12PM +0100, Balazs Lecz wrote:
> On Tue, Apr 20, 2010 at 05:51 PM, Iustin Pop wrote:
> > [...]
> > + _DENIED_CAPABILITIES = [
> > + "mac_override", # Allow MAC configuration or state changes
> > + # TODO: remove sys_admin too, for safety
> > + #"sys_admin", # Perform a range of system administration
> > operations
>
> Was it verified that adding "sys_admin" to _DENIED_CAPABILITIES works as
> expected? If it was, I would remove the TODO entry and uncomment the line.
> Would it make sense to make these (or a subset of these) capabilities
> user-configurable via "gnt-instance modify"?
See the TODO above in the docstring for the class - making this
configurable is the end intention, and depends on some container
parameters framework.
> > + "sys_boot", # Use reboot(2) and kexec_load(2)
> > + "sys_module", # Load and unload kernel modules
> > + "sys_time", # Set system clock, set real-time (hardware) clock
> > + ]
> > +
> > + PARAMETERS = {
> > + }
> > +
> > + def __init__(self):
> > + hv_base.BaseHypervisor.__init__(self)
> > + if not os.path.exists(self._ROOT_DIR):
> > + os.mkdir(self._ROOT_DIR)
> > + if not os.path.isdir(self._ROOT_DIR):
> > + raise HypervisorError("Needed path %s is not a directory" %
> > + self._ROOT_DIR)
>
> How about using utils.EnsureDirs() instead?
Done.
> > +
> > + @staticmethod
> > + def _GetMountSubdirs(path):
> > + """Return the list of mountpoints under a given path.
> > +
> > + This function is Linux-specific.
> > +
> > + """
> > + #TODO(iustin): investigate and document non-linux options
> > + #(e.g. via mount output)
> > + data = []
> > + fh = open("/proc/mounts", "r")
> > + try:
> > + for line in fh:
> > + _, mountpoint, _ = line.split(" ", 2)
> > + if (mountpoint.startswith(path) and
> > + mountpoint != path):
> > + data.append(mountpoint)
> > + finally:
> > + fh.close()
> > + data.sort(key=lambda x: x.count("/"), reverse=True)
> > + return data
> > +
> > + @classmethod
> > + def _InstanceDir(cls, instance_name):
> > + """Return the root directory for an instance.
> > +
> > + """
> > + return utils.PathJoin(cls._ROOT_DIR, instance_name)
> > +
> > + @classmethod
> > + def _InstanceConfFile(cls, instance_name):
> > + """Return the root directory for an instance.
>
> Please correct the docstring.
Done.
> > +
> > + """
> > + return utils.PathJoin(cls._ROOT_DIR, instance_name + ".conf")
> > +
> > + def ListInstances(self):
> > + """Get the list of running instances.
> > +
> > + """
> > + result = utils.RunCmd(["lxc-ls"])
> > + if result.failed:
> > + raise errors.HypervisorError("Can't run lxc-ls: %s" % result.output)
> > + return result.stdout.splitlines()
> > +
> > + def GetInstanceInfo(self, instance_name):
> > + """Get instance properties.
> > +
> > + @type instance_name: string
> > + @param instance_name: the instance name
> > +
> > + @return: (name, id, memory, vcpus, stat, times)
> > +
> > + """
> > + # TODO: read container info from the cgroup mountpoint
> > + return (instance_name, 0, 0, 0, 0, 0)
> > +
> > + def GetAllInstancesInfo(self):
> > + """Get properties of all instances.
> > +
> > + @return: [(name, id, memory, vcpus, stat, times),...]
> > +
> > + """
> > + data = []
> > + for name in self.ListInstances():
>
> Please add a similar TODO entry here.
Done.
> > + data.append((name, 0, 0, 0, 0, 0))
> > + return data
> > +
> > + def _CreateConfigFile(self, instance, root_dir):
> > + """Create an lxc.conf file for an instance"""
> > + out = []
> > + # hostname
> > + out.append("lxc.utsname = %s" % instance.name)
> > +
> > + # separate pseudo-TTY instances
> > + out.append("lxc.pts = 255")
> > + # standard TTYs/console
> > + out.append("lxc.tty = 6")
> > +
> > + # root FS
> > + out.append("lxc.rootfs = %s" % root_dir)
> > +
> > + # TODO: additional mounts, if we disable CAP_SYS_ADMIN
> > +
> > + # Device control
> > + # deny direct device access
> > + out.append("lxc.cgroup.devices.deny = a")
> > + for devinfo in self._DEVS:
> > + out.append("lxc.cgroup.devices.allow = %s rw" % devinfo)
> > +
> > + # Networking
> > + for idx, nic in enumerate(instance.nics):
> > + out.append("# NIC %d" % idx)
> > + mode = nic.nicparams[constants.NIC_MODE]
> > + link = nic.nicparams[constants.NIC_LINK]
> > + if mode == constants.NIC_MODE_BRIDGED:
> > + out.append("lxc.network.type = veth")
> > + out.append("lxc.network.link = %s" % link)
> > + else:
> > + raise errors.HypervisorError("LXC hypervisor supports only"
> > + " bridged mode (NIC %d has mode %s)" %
>
> s/supports only/only supports/
Done.
> > + (idx, mode))
> > + out.append("lxc.network.hwaddr = %s" % nic.mac)
> > + out.append("lxc.network.flags = up")
> > +
> > + # Capabilities
> > + for cap in self._DENIED_CAPABILITIES:
> > + out.append("lxc.cap.drop = %s" % cap)
> > +
> > + return "\n".join(out) + "\n"
> > +
> > + def StartInstance(self, instance, block_devices):
> > + """Start an instance.
> > +
> > + For LCX, we try to mount the block device and execute 'lxc-start
> > + start' (we use volatile containers).
> > +
> > + """
> > + root_dir = self._InstanceDir(instance.name)
> > + if not os.path.exists(root_dir):
> > + try:
> > + os.mkdir(root_dir)
> > + except IOError, err:
> > + raise HypervisorError("Failed to start instance %s: %s" %
> > + (instance.name, err))
> > + if not os.path.isdir(root_dir):
> > + raise HypervisorError("Needed path %s is not a directory" %
> > root_dir)
>
> How about using utils.EnsureDirs() here?
Done. Still needs the try except, since we do want to raise
HypervisorError and not GenericError.
> > +
> > + conf_file = self._InstanceConfFile(instance.name)
> > + utils.WriteFile(conf_file, data=self._CreateConfigFile(instance,
> > root_dir))
> > +
> > + if not os.path.ismount(root_dir):
> > + if not block_devices:
> > + raise HypervisorError("LXC needs at least one disk")
> > +
> > + sda_dev_path = block_devices[0][1]
> > + result = utils.RunCmd(["mount", sda_dev_path, root_dir])
> > + if result.failed:
> > + raise HypervisorError("Can't mount the chroot dir: %s" %
> > result.output)
> > + result = utils.RunCmd(["lxc-start", "-n", instance.name,
> > + "-o", "/tmp/lxc.log", "-l", "DEBUG",
> > + "-f", conf_file, "-d"])
>
> Can you replace "/tmp/lxc.log" with something unique? ..or replace it
> with a constant if it doesn't need to be unique.
I've added a temporary constants in hv_lxc for the log output, and added
a TODO to replace this with a per-instance log file.
> > + if result.failed:
> > + raise HypervisorError("Can't run the lxc start script: %s" %
> > + result.output)
>
> Nitpicking: s/Can't run the lxc start script/Running the lxc-start script
> failed/
> (rationale: "Can't" suggests that lxc-start is not found or a similar
> problem that prevents executing lxc-start)
Done.
> > +
> > + def StopInstance(self, instance, force=False, retry=False, name=None):
> > + """Stop an instance.
> > +
> > + This method has complicated cleanup tests, as we must:
> > + - try to kill all leftover processes
> > + - try to unmount any additional sub-mountpoints
> > + - finally unmount the instance dir
> > +
> > + """
> > + if name is None:
> > + name = instance.name
> > +
> > + root_dir = self._InstanceDir(name)
> > + if not os.path.exists(root_dir):
> > + return
> > +
> > + if name in self.ListInstances():
> > + # Signal init to shutdown; this is a hack
> > + if not retry and not force:
> > + result = utils.RunCmd(["chroot", root_dir, "poweroff"])
> > + if result.failed:
> > + raise HypervisorError("Can't run the lxc stop script: %s" %
> > + result.output)
>
> I would reword the error message to "Stopping lxc instance failed:"
> as this step has nothing to do with the "lxc-stop" script.
Done.
> > + time.sleep(2)
> > + utils.RunCmd(["lxc-stop", "-n", name])
>
> The result code is not checked here. Please add checks or a comment why we
> don't care about the result. Applies to the other RunCmd calls below.
Done.
> > +
> > + for mpath in self._GetMountSubdirs(root_dir):
> > + utils.RunCmd(["umount", mpath])
> > +
> > + result = utils.RunCmd(["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("Can't umount the chroot dir: %s (%s)" %
> > + (result.output, msg))
> > +
> > + def RebootInstance(self, instance):
> > + """Reboot an instance.
> > +
> > + This is not (yet) implemented for lxc.
>
> Does it depend on a missing feature in lxc? If it does, please say so in
> the comment.
> If it's just not yet implemented in Ganeti, please put in a TODO.
Done.
Note I don't have an LXC setup now, so I don't know if the new patch
works correctly (the original one did).
Interdiff:
diff --git a/lib/hypervisor/hv_lxc.py b/lib/hypervisor/hv_lxc.py
index fd0cfa8..9dfcbad 100644
--- a/lib/hypervisor/hv_lxc.py
+++ b/lib/hypervisor/hv_lxc.py
@@ -65,6 +65,7 @@ class LXCHypervisor(hv_base.BaseHypervisor):
"""
_ROOT_DIR = constants.RUN_GANETI_DIR + "/lxc"
+ _LOG_FILE = constants.LOG_DIR + "hv_lxc.log"
_DEVS = [
"c 1:3", # /dev/null
"c 1:5", # /dev/zero
@@ -85,17 +86,14 @@ class LXCHypervisor(hv_base.BaseHypervisor):
"sys_module", # Load and unload kernel modules
"sys_time", # Set system clock, set real-time (hardware) clock
]
+ _DIR_MODE = 0755
PARAMETERS = {
}
def __init__(self):
hv_base.BaseHypervisor.__init__(self)
- if not os.path.exists(self._ROOT_DIR):
- os.mkdir(self._ROOT_DIR)
- if not os.path.isdir(self._ROOT_DIR):
- raise HypervisorError("Needed path %s is not a directory" %
- self._ROOT_DIR)
+ utils.EnsureDirs([(self._ROOT_DIR, self._DIR_MODE)])
@staticmethod
def _GetMountSubdirs(path):
@@ -128,7 +126,7 @@ class LXCHypervisor(hv_base.BaseHypervisor):
@classmethod
def _InstanceConfFile(cls, instance_name):
- """Return the root directory for an instance.
+ """Return the configuration file for an instance.
"""
return utils.PathJoin(cls._ROOT_DIR, instance_name + ".conf")
@@ -160,6 +158,7 @@ class LXCHypervisor(hv_base.BaseHypervisor):
@return: [(name, id, memory, vcpus, stat, times),...]
"""
+ # TODO: read container info from the cgroup mountpoint
data = []
for name in self.ListInstances():
data.append((name, 0, 0, 0, 0, 0))
@@ -196,7 +195,7 @@ class LXCHypervisor(hv_base.BaseHypervisor):
out.append("lxc.network.type = veth")
out.append("lxc.network.link = %s" % link)
else:
- raise errors.HypervisorError("LXC hypervisor supports only"
+ raise errors.HypervisorError("LXC hypervisor only supports"
" bridged mode (NIC %d has mode %s)" %
(idx, mode))
out.append("lxc.network.hwaddr = %s" % nic.mac)
@@ -216,14 +215,10 @@ class LXCHypervisor(hv_base.BaseHypervisor):
"""
root_dir = self._InstanceDir(instance.name)
- if not os.path.exists(root_dir):
- try:
- os.mkdir(root_dir)
- except IOError, err:
- raise HypervisorError("Failed to start instance %s: %s" %
- (instance.name, err))
- if not os.path.isdir(root_dir):
- raise HypervisorError("Needed path %s is not a directory" % root_dir)
+ try:
+ utils.EnsureDirs([(root_dir, self._DIR_MODE)])
+ except errors.GenericError, err:
+ raise HypervisorError("Cannot create instance directory: %s", str(err))
conf_file = self._InstanceConfFile(instance.name)
utils.WriteFile(conf_file, data=self._CreateConfigFile(instance, root_dir))
@@ -236,11 +231,12 @@ class LXCHypervisor(hv_base.BaseHypervisor):
result = utils.RunCmd(["mount", sda_dev_path, root_dir])
if result.failed:
raise HypervisorError("Can't mount the chroot dir: %s" % result.output)
+ # TODO: replace the global log file with a per-instance log file
result = utils.RunCmd(["lxc-start", "-n", instance.name,
- "-o", "/tmp/lxc.log", "-l", "DEBUG",
+ "-o", self._LOG_FILE, "-l", "DEBUG",
"-f", conf_file, "-d"])
if result.failed:
- raise HypervisorError("Can't run the lxc start script: %s" %
+ raise HypervisorError("Running the lxc-start script failed: %s" %
result.output)
def StopInstance(self, instance, force=False, retry=False, name=None):
@@ -264,13 +260,19 @@ class LXCHypervisor(hv_base.BaseHypervisor):
if not retry and not force:
result = utils.RunCmd(["chroot", root_dir, "poweroff"])
if result.failed:
- raise HypervisorError("Can't run the lxc stop script: %s" %
+ raise HypervisorError("Can't run 'poweroff' for the instance: %s" %
result.output)
time.sleep(2)
- utils.RunCmd(["lxc-stop", "-n", name])
+ result = utils.RunCmd(["lxc-stop", "-n", name])
+ if result.failed:
+ logging.warning("Error while doing lxc-stop for %s: %s", name,
+ result.output)
for mpath in self._GetMountSubdirs(root_dir):
- utils.RunCmd(["umount", mpath])
+ result = utils.RunCmd(["umount", mpath])
+ if result.failed:
+ logging.warning("Error while umounting subpath %s for instance %s: %s",
+ mpath, name, result.output)
result = utils.RunCmd(["umount", root_dir])
if result.failed and force:
@@ -283,9 +285,10 @@ class LXCHypervisor(hv_base.BaseHypervisor):
def RebootInstance(self, instance):
"""Reboot an instance.
- This is not (yet) implemented for lxc.
+ This is not (yet) implemented (in Ganeti) for the LXC hypervisor.
"""
+ # TODO: implement reboot
raise HypervisorError("The LXC hypervisor doesn't implement the"
" reboot functionality")
--
iustin
