From: John Spray Date: Fri, 18 Jul 2014 13:09:16 +0000 (+0100) Subject: task/workunit: clean up dir deletion X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=879078fccc3ff684d9063be826b5fe1156283fb5;p=ceph.git task/workunit: clean up dir deletion This was always trying to remove the mountpoint, and then swallowing the exception and printing it to the log. Instead: * Ensure it's only trying to delete mnt if it created it * Don't swallow those exceptions: if deleting the dirs fails then something has gone wrong and it should bubble up. Signed-off-by: John Spray (cherry picked from commit 396dc073cbea2c6282d2c9dcb6b1a36fd86a044d) --- diff --git a/tasks/workunit.py b/tasks/workunit.py index 48730251e1418..4384a53a238f2 100644 --- a/tasks/workunit.py +++ b/tasks/workunit.py @@ -12,6 +12,9 @@ from teuthology.orchestra import run log = logging.getLogger(__name__) +CLIENT_PREFIX = 'client.' + + def task(ctx, config): """ Run ceph on all workunits found under the specified path. @@ -77,46 +80,42 @@ def task(ctx, config): log.info('Pulling workunits from ref %s', refspec) - created_dir_dict = {} + created_mountpoint = {} if config.get('env') is not None: assert isinstance(config['env'], dict), 'env must be a dictionary' clients = config['clients'] + + # Create scratch dirs for any non-all workunits log.info('Making a separate scratch dir for every client...') for role in clients.iterkeys(): assert isinstance(role, basestring) if role == "all": continue - PREFIX = 'client.' - assert role.startswith(PREFIX) + + assert role.startswith(CLIENT_PREFIX) created_mnt_dir = _make_scratch_dir(ctx, role, config.get('subdir')) - created_dir_dict[role] = created_mnt_dir + created_mountpoint[role] = created_mnt_dir - all_spec = False #is there an all grouping? + # Execute any non-all workunits with parallel() as p: for role, tests in clients.iteritems(): if role != "all": p.spawn(_run_tests, ctx, refspec, role, tests, config.get('env'), timeout=timeout) - else: - all_spec = True - if all_spec: + # Clean up dirs from any non-all workunits + for role, created in created_mountpoint.items(): + _delete_dir(ctx, role, created) + + # Execute any 'all' workunits + if 'all' in clients: all_tasks = clients["all"] _spawn_on_all_clients(ctx, refspec, all_tasks, config.get('env'), config.get('subdir'), timeout=timeout) - for role in clients.iterkeys(): - assert isinstance(role, basestring) - if role == "all": - continue - PREFIX = 'client.' - assert role.startswith(PREFIX) - if created_dir_dict[role]: - _delete_dir(ctx, role) - -def _delete_dir(ctx, role): +def _delete_dir(ctx, role, created_mountpoint): """ Delete file used by this role, and delete the directory that this role appeared in. @@ -124,37 +123,35 @@ def _delete_dir(ctx, role): :param ctx: Context :param role: "role.#" where # is used for the role id. """ - PREFIX = 'client.' testdir = teuthology.get_testdir(ctx) - id_ = role[len(PREFIX):] + id_ = role[len(CLIENT_PREFIX):] (remote,) = ctx.cluster.only(role).remotes.iterkeys() mnt = os.path.join(testdir, 'mnt.{id}'.format(id=id_)) # Is there any reason why this is not: join(mnt, role) ? client = os.path.join(mnt, 'client.{id}'.format(id=id_)) - try: - remote.run( - args=[ - 'rm', - '-rf', - '--', - client, - ], - ) - log.info("Deleted dir {dir}".format(dir=client)) - except Exception: - log.exception("Caught an exception deleting dir {dir}".format(dir=client)) - try: + # Remove the directory inside the mount where the workunit ran + remote.run( + args=[ + 'rm', + '-rf', + '--', + client, + ], + ) + log.info("Deleted dir {dir}".format(dir=mnt)) + + # If the mount was an artificially created dir, delete that too + if created_mountpoint: remote.run( args=[ 'rmdir', '--', mnt, - ], - ) - log.info("Deleted dir {dir}".format(dir=mnt)) - except Exception: - log.exception("Caught an exception deleting dir {dir}".format(dir=mnt)) + ], + ) + log.info("Deleted artificial mount point {dir}".format(dir=client)) + def _make_scratch_dir(ctx, role, subdir): """ @@ -165,9 +162,8 @@ def _make_scratch_dir(ctx, role, subdir): :param role: "role.#" where # is used for the role id. :param subdir: use this subdir (False if not used) """ - retVal = False - PREFIX = 'client.' - id_ = role[len(PREFIX):] + created_mountpoint = False + id_ = role[len(CLIENT_PREFIX):] log.debug("getting remote for {id} role {role_}".format(id=id_, role_=role)) (remote,) = ctx.cluster.only(role).remotes.iterkeys() dir_owner = remote.user @@ -192,10 +188,12 @@ def _make_scratch_dir(ctx, role, subdir): ], ) log.info('Created dir {dir}'.format(dir=mnt)) - retVal = True + created_mountpoint = True + + if not subdir: + subdir = 'client.{id}'.format(id=id_) - if not subdir: subdir = 'client.{id}'.format(id=id_) - if retVal: + if created_mountpoint: remote.run( args=[ 'cd', @@ -227,7 +225,7 @@ def _make_scratch_dir(ctx, role, subdir): ], ) - return retVal + return created_mountpoint def _spawn_on_all_clients(ctx, refspec, tests, env, subdir, timeout=None): @@ -239,10 +237,12 @@ def _spawn_on_all_clients(ctx, refspec, tests, env, subdir, timeout=None): """ client_generator = teuthology.all_roles_of_type(ctx.cluster, 'client') client_remotes = list() + + created_mountpoint = {} for client in client_generator: (client_remote,) = ctx.cluster.only('client.{id}'.format(id=client)).remotes.iterkeys() client_remotes.append((client_remote, 'client.{id}'.format(id=client))) - _make_scratch_dir(ctx, "client.{id}".format(id=client), subdir) + created_mountpoint[client] = _make_scratch_dir(ctx, "client.{id}".format(id=client), subdir) for unit in tests: with parallel() as p: @@ -253,7 +253,7 @@ def _spawn_on_all_clients(ctx, refspec, tests, env, subdir, timeout=None): # cleanup the generated client directories client_generator = teuthology.all_roles_of_type(ctx.cluster, 'client') for client in client_generator: - _delete_dir(ctx, 'client.{id}'.format(id=client)) + _delete_dir(ctx, 'client.{id}'.format(id=client), created_mountpoint[client]) def _run_tests(ctx, refspec, role, tests, env, subdir=None, timeout=None): @@ -276,9 +276,8 @@ def _run_tests(ctx, refspec, role, tests, env, subdir=None, timeout=None): """ testdir = teuthology.get_testdir(ctx) assert isinstance(role, basestring) - PREFIX = 'client.' - assert role.startswith(PREFIX) - id_ = role[len(PREFIX):] + assert role.startswith(CLIENT_PREFIX) + id_ = role[len(CLIENT_PREFIX):] (remote,) = ctx.cluster.only(role).remotes.iterkeys() mnt = os.path.join(testdir, 'mnt.{id}'.format(id=id_)) # subdir so we can remove and recreate this a lot without sudo