On Fri, May 17, 2013 at 10:38 AM, Michele Tartara <[email protected]>wrote:
> 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())) >> > I'm not 100% sure about the file naming. 1) It's not really a log file. How about removing the ".log"? 2) Do we need some ID in addition to the timestamp? > + 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? > I followed the existing pattern of leaving these log artifacts behind. For example, OS install log files are created in /var/log/ganeti/os and never removed by Ganeti itself. > >> + 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 > -- Google Ireland Ltd.,1 Grand Canal Plaza, Lower Grand Canal Street, Dublin 4, Ireland Registered in Dublin, Ireland Registration Number: 368047
