This series brings a few enhancements to the tag releasing process. Patches 1 and 2 ensure that a released tag is unreleased before being deleted. Patch 3 lets the "release" cli command return an error when no publish server is configured, to allow quick detection of a misconfiguration. Patches 4 and 5 robustify tag (un)releasing. Julien Floret (5): tag: unrelease during auto cleanup tag: forbid deletion if released tag: return HTTP error to release if no server configured tag: fix unrelease when not found on publish server tag: leave unreleased if no job published dlrepo-cli | 8 ---- dlrepo/__main__.py | 1 + dlrepo/fs/__init__.py | 9 ++++- dlrepo/fs/branch.py | 8 ++-- dlrepo/fs/tag.py | 88 ++++++++++++++++++++++++----------------- dlrepo/views/branch.py | 3 +- dlrepo/views/tag.py | 5 ++- docs/dlrepo-api.7.scdoc | 14 +++---- docs/dlrepo-cli.1.scdoc | 3 -- 9 files changed, 74 insertions(+), 65 deletions(-) -- 2.30.2
Julien Floret, Mar 27, 2023 at 15:15:
dlrepo/patches/.build.yml: SUCCESS in 1m33s [enhance tag releasing][0] from [Julien Floret][1] [0]: https://lists.sr.ht/~rjarry/dlrepo/patches/40022 [1]: mailto:julien.floret@6wind.com ✓ #963494 SUCCESS dlrepo/patches/.build.yml https://builds.sr.ht/~rjarry/job/963494
Le ven. 31 mars 2023 à 21:23, Robin Jarry <robin@jarry.cc> a écrit :
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~rjarry/dlrepo/patches/40022/mbox | git am -3Learn more about email & git
In the tag cleanup task, before deleting a released tag, make sure to remove it from the public server before. Indeed, we do not want tags hanging on the public mirror if they have been deleted from the main server. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/__main__.py | 1 + dlrepo/fs/__init__.py | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/dlrepo/__main__.py b/dlrepo/__main__.py index 0ece6e24817a..2767044c2275 100644 --- a/dlrepo/__main__.py +++ b/dlrepo/__main__.py @@ -39,6 +39,7 @@ async def create_semaphore(app: web.Application): # -------------------------------------------------------------------------------------- async def init_fs_cleanup(app: web.Application): repo = app["dlrepo_artifact_repository"] + repo.set_publish_semaphore(app["dlrepo_publish_semaphore"]) repo.start_cleanup_timer() loop = asyncio.get_running_loop() loop.add_signal_handler(signal.SIGUSR1, repo.schedule_cleanup_all) diff --git a/dlrepo/fs/__init__.py b/dlrepo/fs/__init__.py index d67856fc5f45..3356222e6f4c 100644 --- a/dlrepo/fs/__init__.py +++ b/dlrepo/fs/__init__.py @@ -85,6 +85,8 @@ class AbstractRepository: # -------------------------------------------------------------------------------------- class ArtifactRepository(AbstractRepository): + # pylint: disable=too-many-instance-attributes
Let's edit pylintrc for this, max-attributes=7 is rather small. What about increasing it to at least 10?
+ def __init__(self, path: str): super().__init__(path) self.blobs = self._path / ".blobs" @@ -94,6 +96,7 @@ class ArtifactRepository(AbstractRepository): self.cleanup_timer = None self.need_cleanup = False self.cleanup_tags = False + self.publish_semaphore = None def get_user_repo(self, user: str) -> "UserRepository": user = user.lower() @@ -126,6 +129,9 @@ class ArtifactRepository(AbstractRepository): def schedule_cleanup_all(self): self._schedule_cleanup(tags=True) + def set_publish_semaphore(self, semaphore: asyncio.Semaphore): + self.publish_semaphore = semaphore + def _schedule_cleanup(self, *, tags: bool): self.cleanup_tags = self.cleanup_tags or tags if self.cleanup_task: @@ -173,7 +179,8 @@ class ArtifactRepository(AbstractRepository): if isinstance(max_released, int) and max_released > 0: for tag in released_tags[max_released:]: LOG.info("Deleting released tag %s/%s ...", branch.name, tag.name) - await loop.run_in_executor(None, tag.delete, force=True) + await tag.do_release(False, self.publish_semaphore) + await loop.run_in_executor(None, tag.delete)
It looks like the force=True parameter should be removed in the next patch.
deleted += 1 LOG.info("Deleted %d tags", deleted) -- 2.30.2
Julien Floret, Mar 27, 2023 at 15:15:
We never want to delete a released tag from the main server, because we would then have a dangling tag published on the public mirror, with no easy way to manage it. To ensure it does not happen, remove the possibility to "force" deleting a released tag. The tag should be unreleased before. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo-cli | 8 -------- dlrepo/fs/branch.py | 8 ++++---- dlrepo/fs/tag.py | 16 ++++++---------- dlrepo/views/branch.py | 3 +-- dlrepo/views/tag.py | 3 +-- docs/dlrepo-api.7.scdoc | 14 ++++++-------- docs/dlrepo-cli.1.scdoc | 3 --- 7 files changed, 18 insertions(+), 37 deletions(-) diff --git a/dlrepo-cli b/dlrepo-cli index 13fa16f5157b..22e3f433cf3d 100755 --- a/dlrepo-cli +++ b/dlrepo-cli @@ -443,12 +443,6 @@ def internal(args): Arg("branch", metavar="BRANCH", help="the branch name"), Arg("tag", metavar="TAG", nargs="?", help="the tag name"), Arg("job", metavar="JOB", nargs="?", help="the job name"), - Arg( - "-f", - "--force", - action="store_true", - help="force the deletion of 'released' tags", - ), ) def delete(args): """ @@ -460,8 +454,6 @@ def delete(args): if args.job: url = os.path.join("branches", args.branch, args.tag, args.job) + "/" else: - if args.force: - params["force"] = "true" url = os.path.join("branches", args.branch, args.tag) + "/" else: url = os.path.join("branches", args.branch) + "/" diff --git a/dlrepo/fs/branch.py b/dlrepo/fs/branch.py index a88cf786d1fd..8b6940c19d62 100644 --- a/dlrepo/fs/branch.py +++ b/dlrepo/fs/branch.py @@ -70,14 +70,14 @@ class Branch(SubDir): policy[field] = 0 return policy - def delete(self, *, force: bool = False): + def delete(self): if not self.exists(): raise FileNotFoundError() for t in self.get_tags(): if t.is_locked(): raise OSError(f"Tag {t.name} is locked") - if not force and t.is_released(): - raise OSError(f"Tag {t.name} is released, use force") + if t.is_released(): + raise OSError(f"Tag {t.name} is released, unrelease it first") for t in self.get_tags(): - t.delete(force=force) + t.delete() self.root().rmtree(self._path) diff --git a/dlrepo/fs/tag.py b/dlrepo/fs/tag.py index b1bdac233550..098824b185b9 100644 --- a/dlrepo/fs/tag.py +++ b/dlrepo/fs/tag.py @@ -135,7 +135,7 @@ class Tag(SubDir): self.PUBLISH_URL, ) async with semaphore: - await sess.delete(self.url(), params={"force": "true"}) + await sess.delete(self.url()) p = self._publish_status_path() if p.is_file(): p.unlink() @@ -160,9 +160,7 @@ class Tag(SubDir): if job.is_internal(): # in case job was already published, delete it from the public server async with semaphore: - await sess.delete( - job_url, params={"force": "true"}, raise_for_status=False - ) + await sess.delete(job_url, raise_for_status=False) job.set_released(False) continue async with semaphore: @@ -179,9 +177,7 @@ class Tag(SubDir): continue # delete the job previously released before re-uploading it async with semaphore: - await sess.delete( - job_url, params={"force": "true"}, raise_for_status=False - ) + await sess.delete(job_url, raise_for_status=False) job.set_released(False) self._publish_status_path().write_text(f"uploading {job.name}\n") tasks = [] @@ -254,13 +250,13 @@ class Tag(SubDir): elif path.is_file(): path.unlink() - def delete(self, *, force: bool = False): + def delete(self): if not self.exists(): raise FileNotFoundError() if self.is_locked(): raise OSError(f"Tag {self.name} is locked") - if not force and self.is_released(): - raise OSError(f"Tag {self.name} is released, use force") + if self.is_released(): + raise OSError(f"Tag {self.name} is released, unrelease it first") for j in self.get_jobs(): j.delete() self.root().rmtree(self._path) diff --git a/dlrepo/views/branch.py b/dlrepo/views/branch.py index 2b306ebfceb4..323af0184270 100644 --- a/dlrepo/views/branch.py +++ b/dlrepo/views/branch.py @@ -121,8 +121,7 @@ class BranchView(BaseView): loop = asyncio.get_running_loop() branch = self._get_branch() try: - force = "force" in self.request.query - await loop.run_in_executor(None, lambda: branch.delete(force=force)) + await loop.run_in_executor(None, branch.delete) self.repo().schedule_cleanup_orphans() return web.Response() except OSError as e: diff --git a/dlrepo/views/tag.py b/dlrepo/views/tag.py index 5e75d28dc486..1fcb286c6778 100644 --- a/dlrepo/views/tag.py +++ b/dlrepo/views/tag.py @@ -113,8 +113,7 @@ class TagView(BaseView): loop = asyncio.get_running_loop() try: tag = self._get_tag() - force = "force" in self.request.query - await loop.run_in_executor(None, lambda: tag.delete(force=force)) + await loop.run_in_executor(None, tag.delete) self.repo().schedule_cleanup_orphans() return web.Response() except OSError as e: diff --git a/docs/dlrepo-api.7.scdoc b/docs/dlrepo-api.7.scdoc index 1d46bea6c8b4..8b0364cc9033 100644 --- a/docs/dlrepo-api.7.scdoc +++ b/docs/dlrepo-api.7.scdoc @@ -189,16 +189,15 @@ Change the cleanup policy of the specified _{branch}_. - _400_: invalid parameters in the request body. - _404_: the specified _{branch}_ does not exist. -## DELETE /branches/{branch}/?force -## DELETE /~{user}/branches/{branch}/?force +## DELETE /branches/{branch}/ +## DELETE /~{user}/branches/{branch}/ Delete the specified _{branch}_ and all its tags. *Access:* _rw_ *Errors:* - - _400_: if one of the tags is released or locked and _force_ was not - specified in the query parameters. + - _400_: if one of the tags is released or locked. - _404_: the specified _{branch}_ does not exist. # TAGS @@ -281,16 +280,15 @@ un-publication) of the tag to/from another server if *DLREPO_PUBLISH_URL* and The publication status can be fetched with a *GET* request on the same tag. -## DELETE /branches/{branch}/{tag}/?force -## DELETE /~{user}/branches/{branch}/{tag}/?force +## DELETE /branches/{branch}/{tag}/ +## DELETE /~{user}/branches/{branch}/{tag}/ Delete the specified _{tag}_ and all its jobs. *Access:* _rw_ *Errors:* - - _400_: the tag is locked or it is released and _force_ was not - specified in the query parameters. + - _400_: the tag is locked or released. - _404_: the specified _{tag}_ does not exist. - _405_: _{tag}_ is either _latest_, _stable_ or _oldstable_. diff --git a/docs/dlrepo-cli.1.scdoc b/docs/dlrepo-cli.1.scdoc index ef25de413a79..7cf8bf7e0f4a 100644 --- a/docs/dlrepo-cli.1.scdoc +++ b/docs/dlrepo-cli.1.scdoc @@ -205,9 +205,6 @@ _JOB_ Delete a job, a tag or a branch and all its tags recursively. -*-f*, *--force* - Force the deletion of _released_ tags. _locked_ tags cannot be deleted. - _BRANCH_ The branch name. _TAG_ -- 2.30.2
Before this patch, a tag could be set to "released" even if no publish server was configured. This is quite misleading, because the release has no effect. Let's return an HTTPMisdirectedRequest error to the client instead. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/fs/tag.py | 49 ++++++++++++++++++++++++--------------------- dlrepo/views/tag.py | 2 ++ 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/dlrepo/fs/tag.py b/dlrepo/fs/tag.py index 098824b185b9..c1736fa10c1d 100644 --- a/dlrepo/fs/tag.py +++ b/dlrepo/fs/tag.py @@ -68,6 +68,10 @@ class Tag(SubDir): return self._released_path().is_file() def set_released(self, released: bool, semaphore: asyncio.Semaphore): + if not self.PUBLISH_URL: + raise ValueError("DLREPO_PUBLISH_URL is undefined") + if not self.PUBLISH_AUTH: + raise ValueError("DLREPO_PUBLISH_AUTH is undefined") if not self._path.is_dir(): raise FileNotFoundError() loop = asyncio.get_running_loop() @@ -116,29 +120,28 @@ class Tag(SubDir): ) async def do_release(self, released: bool, semaphore: asyncio.Semaphore): - if self.PUBLISH_URL and self.PUBLISH_AUTH: - self._publish_status_path().write_text("in progress\n") - async with self._publish_session() as sess: - if released: - LOG.info( - "publishing tag %s/%s to %s", - self.parent.name, - self.name, - self.PUBLISH_URL, - ) - await self._publish(sess, semaphore) - else: - LOG.info( - "deleting tag %s/%s from %s", - self.parent.name, - self.name, - self.PUBLISH_URL, - ) - async with semaphore: - await sess.delete(self.url()) - p = self._publish_status_path() - if p.is_file(): - p.unlink() + self._publish_status_path().write_text("in progress\n") + async with self._publish_session() as sess: + if released: + LOG.info( + "publishing tag %s/%s to %s", + self.parent.name, + self.name, + self.PUBLISH_URL, + ) + await self._publish(sess, semaphore) + else: + LOG.info( + "deleting tag %s/%s from %s", + self.parent.name, + self.name, + self.PUBLISH_URL, + ) + async with semaphore: + await sess.delete(self.url()) + p = self._publish_status_path() + if p.is_file(): + p.unlink() path = self._released_path() if released: path.touch() diff --git a/dlrepo/views/tag.py b/dlrepo/views/tag.py index 1fcb286c6778..25657d8ad178 100644 --- a/dlrepo/views/tag.py +++ b/dlrepo/views/tag.py @@ -103,6 +103,8 @@ class TagView(BaseView): tag.set_stable(stable) except FileNotFoundError as e: raise web.HTTPNotFound() from e + except ValueError as e: + raise web.HTTPMisdirectedRequest(reason=str(e)) from e return web.Response() -- 2.30.2
If a tag flagged as "released" is not found on the publish server, just let the unrelease command remove the "release" flag. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/fs/tag.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dlrepo/fs/tag.py b/dlrepo/fs/tag.py index c1736fa10c1d..759f9afaa11d 100644 --- a/dlrepo/fs/tag.py +++ b/dlrepo/fs/tag.py @@ -138,7 +138,11 @@ class Tag(SubDir): self.PUBLISH_URL, ) async with semaphore: - await sess.delete(self.url()) + try: + await sess.delete(self.url()) + except aiohttp.client_exceptions.ClientResponseError as e: + if e.status != 404: + raise p = self._publish_status_path() if p.is_file(): p.unlink() -- 2.30.2
When releasing a tag, there is a possibility that no job is actually published, for example when all jobs are "internal". In this case, it is misleading to see the tag as "released", although it is absent from the publish server. After the release process, we issue a GET request to the public tag's URL. If the tag does not exist or contains no job, we remove the "released" state. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/fs/tag.py | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/dlrepo/fs/tag.py b/dlrepo/fs/tag.py index 759f9afaa11d..1378f7549d15 100644 --- a/dlrepo/fs/tag.py +++ b/dlrepo/fs/tag.py @@ -120,6 +120,7 @@ class Tag(SubDir): ) async def do_release(self, released: bool, semaphore: asyncio.Semaphore): + published_jobs = [] self._publish_status_path().write_text("in progress\n") async with self._publish_session() as sess: if released: @@ -130,6 +131,15 @@ class Tag(SubDir): self.PUBLISH_URL, ) await self._publish(sess, semaphore) + async with semaphore: + try: + resp = await sess.get(self.url()) + data = await resp.read() + data = json.loads(data.decode("utf-8")) + published_jobs = data["tag"]["jobs"] + except aiohttp.client_exceptions.ClientResponseError as e: + if e.status != 404: + raise else: LOG.info( "deleting tag %s/%s from %s", @@ -143,16 +153,17 @@ class Tag(SubDir): except aiohttp.client_exceptions.ClientResponseError as e: if e.status != 404: raise - p = self._publish_status_path() - if p.is_file(): - p.unlink() - path = self._released_path() - if released: - path.touch() - elif path.is_file(): - path.unlink() + released_path = self._released_path() + if released and published_jobs: + released_path.touch() + else: + if released_path.is_file(): + released_path.unlink() for job in self.get_jobs(): job.set_released(False) + status_path = self._publish_status_path() + if status_path.is_file(): + status_path.unlink() async def _publish(self, sess: aiohttp.ClientSession, semaphore: asyncio.Semaphore): loop = asyncio.get_running_loop() -- 2.30.2
builds.sr.ht <builds@sr.ht>dlrepo/patches/.build.yml: SUCCESS in 1m33s [enhance tag releasing][0] from [Julien Floret][1] [0]: https://lists.sr.ht/~rjarry/dlrepo/patches/40022 [1]: mailto:julien.floret@6wind.com ✓ #963494 SUCCESS dlrepo/patches/.build.yml https://builds.sr.ht/~rjarry/job/963494