On Fri, May 17, 2013 at 10:00 AM, Balazs Lecz <[email protected]> wrote:
> This is a fix for a minor bug. > Currently, a failed Xen VM start results in a stale config file left > behind on the filesystem. > This change introduces a new log directory, where the Xen VM config > file is moved after a failed startup. > > Signed-off-by: Balazs Lecz <[email protected]> > --- > lib/hypervisor/hv_xen.py | 19 +++++++++++++++++-- > lib/pathutils.py | 2 ++ > lib/tools/ensure_dirs.py | 1 + > test/py/ganeti.hypervisor.hv_xen_unittest.py | 10 ++++++---- > 4 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/lib/hypervisor/hv_xen.py b/lib/hypervisor/hv_xen.py > index 1d0b587..d5273f4 100644 > --- a/lib/hypervisor/hv_xen.py > +++ b/lib/hypervisor/hv_xen.py > @@ -414,6 +414,17 @@ class XenHypervisor(hv_base.BaseHypervisor): > """ > utils.RemoveFile(self._ConfigFileName(instance_name)) > > + def _StashConfigFile(self, instance_name): > + """Move the Xen config file to the log directory and return its new > path. > + > + """ > + old_filename = self._ConfigFileName(instance_name) > + base = ("%s-%s.log" % > + (instance_name, utils.TimestampForFilename())) > + new_filename = utils.PathJoin(pathutils.LOG_XEN_DIR, base) > + utils.RenameFile(old_filename, new_filename) > + return new_filename > + > def _GetXmList(self, include_node): > """Wrapper around module level L{_GetXmList}. > > @@ -482,9 +493,13 @@ class XenHypervisor(hv_base.BaseHypervisor): > > result = self._RunXen(cmd) > if result.failed: > - raise errors.HypervisorError("Failed to start instance %s: %s (%s)" > % > + # Move the Xen configuration file to the log directory to avoid > + # leaving a stale config file behind. > + stashed_config = self._StashConfigFile(instance.name) > Should the stashed config files be kept indefinitely until manually deleted or should we provide some cleanup mechanism to clean the directory up every once in a while? > + raise errors.HypervisorError("Failed to start instance %s: %s (%s). > Moved" > + " config file to %s" % > (instance.name, result.fail_reason, > - result.output)) > + result.output, stashed_config)) > > def StopInstance(self, instance, force=False, retry=False, name=None): > """Stop an instance. > diff --git a/lib/pathutils.py b/lib/pathutils.py > index 540148d..341d1ba 100644 > --- a/lib/pathutils.py > +++ b/lib/pathutils.py > @@ -133,6 +133,8 @@ QUERY_SOCKET = SOCKET_DIR + "/ganeti-query" > > LOG_OS_DIR = LOG_DIR + "/os" > LOG_ES_DIR = LOG_DIR + "/extstorage" > +#: Directory for storing Xen config files after failed instance starts > +LOG_XEN_DIR = LOG_DIR + "/xen" > > # Job queue paths > JOB_QUEUE_LOCK_FILE = QUEUE_DIR + "/lock" > diff --git a/lib/tools/ensure_dirs.py b/lib/tools/ensure_dirs.py > index d62414a..e9dab95 100644 > --- a/lib/tools/ensure_dirs.py > +++ b/lib/tools/ensure_dirs.py > @@ -195,6 +195,7 @@ def GetPaths(): > (noded_log, FILE, 0600, getent.noded_uid, getent.masterd_gid, False), > (rapi_log, FILE, 0600, getent.rapi_uid, getent.masterd_gid, False), > (pathutils.LOG_OS_DIR, DIR, 0750, getent.masterd_uid, > getent.daemons_gid), > + (pathutils.LOG_XEN_DIR, DIR, 0750, getent.masterd_uid, > getent.daemons_gid), > (cleaner_log_dir, DIR, 0750, getent.noded_uid, getent.noded_gid), > (master_cleaner_log_dir, DIR, 0750, getent.masterd_uid, > getent.masterd_gid), > (pathutils.INSTANCE_REASON_DIR, DIR, 0755, getent.noded_uid, > diff --git a/test/py/ganeti.hypervisor.hv_xen_unittest.py b/test/py/ > ganeti.hypervisor.hv_xen_unittest.py > index 3679de4..e87e62f 100755 > --- a/test/py/ganeti.hypervisor.hv_xen_unittest.py > +++ b/test/py/ganeti.hypervisor.hv_xen_unittest.py > @@ -30,6 +30,7 @@ import os > > from ganeti import constants > from ganeti import objects > +from ganeti import pathutils > from ganeti import hypervisor > from ganeti import utils > from ganeti import errors > @@ -491,7 +492,6 @@ class _TestXenHypervisor(object): > self.fail("Unhandled command: %s" % (cmd, )) > > return self._SuccessCommand(output, cmd) > - #return self._FailingCommand(cmd) > > def _MakeInstance(self): > # Copy default parameters > @@ -519,6 +519,7 @@ class _TestXenHypervisor(object): > > def testStartInstance(self): > (inst, disks) = self._MakeInstance() > + pathutils.LOG_XEN_DIR = self.tmpdir > > for failcreate in [False, True]: > for paused in [False, True]: > @@ -537,11 +538,12 @@ class _TestXenHypervisor(object): > if failcreate: > self.assertRaises(errors.HypervisorError, hv.StartInstance, > inst, disks, paused) > + # Check whether a stale config file is left behind > + self.assertFalse(os.path.exists(cfgfile)) > else: > hv.StartInstance(inst, disks, paused) > - > - # Check if configuration was updated > - lines = utils.ReadFile(cfgfile).splitlines() > + # Check if configuration was updated > + lines = utils.ReadFile(cfgfile).splitlines() > > if constants.HV_VNC_PASSWORD_FILE in inst.hvparams: > self.assertTrue(("vncpasswd = '%s'" % self.vncpw) in lines) > -- > 1.8.2.1 > > Rest LGTM. Thanks, Michele
