On Fri, May 17, 2013 at 11:09 AM, Balazs Lecz <[email protected]> wrote:
> 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? > I would probably go for something like: "%s-%s" % (originalfilename, 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. > Ok, then. > > >> >>> + 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 > Thanks, Michele
