Skip to content
Snippets Groups Projects
Commit 10d6717c authored by Georges Racinet's avatar Georges Racinet
Browse files

RemoveRepository: better robustness in intermediate situations

It was a mistake to depend on `load_repo`, as it will fail if,
e.g., the `.hg` sub directory does not exist (seen in Rails
RSpec tests) and various corrupted cases that a *removal* should
clean up.

The price to pay is the transactionality of the roster, but we
cannot lock it properly without a repo. In theory in should be
possible to instantiate the `Vfs` separately, but it does not seem
to matter so much.

With this change, if removing the working directories fails, it
will not prevent removing the trashed repository (but of course
we risk a later collision in workdirs).

Backs out in particular changeset 1c735d40cfe7
parent c306a7d4
No related branches found
No related tags found
3 merge requests!204Making Heptapod 0.40 the new oldstable,!203Merge stable branch into default,!202RemoveRepository: robustness improvements
......@@ -78,7 +78,7 @@
chunked,
)
from ..workdir import (
remove_all_workdirs,
remove_all_workdirs_bare,
)
from ..stub.repository_pb2 import (
BackupCustomHooksRequest,
......@@ -506,7 +506,6 @@
def RemoveRepository(self, request: RemoveRepositoryRequest,
context) -> RemoveRepositoryResponse:
logger = LoggerAdapter(base_logger, context)
# The protocol comment says, as of Gitaly 14.8:
# RemoveRepository will move the repository to
# `+gitaly/tmp/<relative_path>_removed` and
......@@ -515,7 +514,10 @@
# asynchronous (as the Rails app does), but it is not in the
# Gitaly server implementation. The renaming is done for
# atomicity purposes.
repo = self.load_repo(request.repository, context)
repo_path = repo.root
try:
repo_path = self.repo_disk_path(request.repository, context)
except KeyError as exc:
self.handle_key_error(context, exc.args)
except ValueError as exc:
self.handle_value_error(context, exc.args)
......@@ -521,14 +523,8 @@
try:
workdirs = self.repo_workdirs_root(request.repository, context)
# TODO since we're doing this, we might as well do it from
# the onset and avoid repeating exception treatment OR we could
# do the cleanup in `CreateRepository` and friends
remove_all_workdirs(workdirs, repo)
except Exception:
# we don't want to prevent the removal for this
logger.exception("Error when removing working directories for %r",
message.Logging(request))
if not os.path.exists(repo_path):
# same error message as Gitaly (probably no need to repeat
# repo details, since request often logged client-side)
context.abort(StatusCode.NOT_FOUND, "repository does not exist")
trash_path = os.path.join(
self.temp_dir(request.repository.storage_name, context),
......@@ -532,7 +528,7 @@
trash_path = os.path.join(
self.temp_dir(request.repository.storage_name, context),
os.path.basename(repo_path) + b'+removed')
os.path.basename(repo_path) + b'+removed') # TODO randomize
# The rename being atomic, it avoids leaving a crippled repo behind
# in case of problem in the removal.
# TODO Gitaly also performs some kind of locking (not clear
......@@ -541,7 +537,13 @@
# and finally the vote related to the multi-phase commit for praefect
os.rename(repo_path, trash_path)
shutil.rmtree(trash_path) # not atomic
# non-atomic cleanups
# at this point, the repo officially does not exist any more, the
# roster file does not matter, cleanup workdirs if any.
remove_all_workdirs_bare(self.repo_workdirs_root(request.repository,
context))
shutil.rmtree(trash_path)
return RemoveRepositoryResponse()
def SetFullPath(self, request: SetFullPathRequest,
......
......@@ -363,10 +363,5 @@
This is to be used when preparing to remove the repository itself.
"""
with locked_roster(repo) as (rosterf, new_rosterf):
try:
if os.path.exists(workdirs_root):
shutil.rmtree(workdirs_root)
except Exception:
logger.exception("Failed to remove working directories "
"for repo at %r", repo)
remove_all_workdirs_bare(workdirs_root)
# new_rosterf being empty, this will void the existing one.
......@@ -372,1 +367,19 @@
# new_rosterf being empty, this will void the existing one.
def remove_all_workdirs_bare(workdirs_root):
"""Removal of workdirs_root with no roster management.
To be used either with roster being managed (lock acquired by caller etc.)
or if the roster does not matter any more (repository is being entirely
removed).
Does not raise exceptions, as the caller typically will prefer to proceed
with other cleanups if possible.
"""
try:
if os.path.exists(workdirs_root):
shutil.rmtree(workdirs_root)
except Exception:
logger.exception("Failed to remove working directories at %r",
workdirs_root)
......@@ -192,7 +192,6 @@
repository=Repository(storage_name='unknown',
relative_path='/some/path'))
assert_compare_errors(
same_details=False,
repository=Repository(storage_name=grpc_repo.storage_name,
relative_path='no/such/path'))
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment