Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sync failure on unreachable namespace avatar #1544

Merged
merged 1 commit into from
Aug 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/1543.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Stopped collection sync from failing if a namespace's avatar url was unreachable.
8 changes: 7 additions & 1 deletion pulp_ansible/app/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,13 @@ class AnsibleNamespaceMetadata(Content):
@property
def avatar_artifact(self):
if self.avatar_sha256:
return self._artifacts.get(sha256=self.avatar_sha256)
avatar = self._artifacts.filter(sha256=self.avatar_sha256).first()
if avatar is None:
log.debug(
f"Artifact({self.avatar_sha256}) is missing for namespace avatar "
f"{self.name}:{self.metadata_sha256}"
)
return avatar

return None

Expand Down
38 changes: 33 additions & 5 deletions pulp_ansible/app/tasks/collections.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from uuid import uuid4

import yaml
from aiohttp.client_exceptions import ClientResponseError
from aiohttp.client_exceptions import ClientError, ClientResponseError
from asgiref.sync import sync_to_async
from async_lru import alru_cache
from django.conf import settings
Expand Down Expand Up @@ -496,7 +496,7 @@ def pipeline_stages(self, new_version):
ArtifactSaver(),
QueryExistingContents(),
DocsBlobDownloader(),
CollectionContentSaver(new_version),
AnsibleContentSaver(new_version),
RemoteArtifactSaver(),
ResolveContentFutures(),
]
Expand Down Expand Up @@ -726,8 +726,8 @@ async def _add_namespace(self, name, namespace_sha):

da = (
[
DeclarativeArtifact(
Artifact(sha256=namespace["avatar_sha256"]),
DeclarativeFailsafeArtifact(
Artifact(sha256=namespace.get("avatar_sha256")),
url=url,
remote=self.remote,
relative_path=f"{name}-avatar",
Expand Down Expand Up @@ -1074,6 +1074,25 @@ async def run(self):
pb.total = pb.done


class DeclarativeFailsafeArtifact(DeclarativeArtifact):
"""
Special handling for downloading Namespace Avatar Artifacts.
"""

async def download(self):
"""Allow download to fail, but not stop the sync."""
artifact_copy = self.artifact
try:
return await super().download()
except ClientError as e:
# Reset DA so that future stages can properly handle it
self.artifact = artifact_copy
self.deferred_download = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It reminds me a lot of https://github.com/pulp/pulp_deb/blob/main/pulp_deb/app/tasks/synchronizing.py#L192 with https://github.com/pulp/pulp_deb/blob/main/pulp_deb/app/tasks/synchronizing.py#L513 .

Can you elaborate more on the strategy? Do we handle the namespace as if no avatar existed, or do we turn the artifact into on-demand?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very similar. The current strategy I am using is to handle the namespace as if no avatar existed. In the custom content saver stage I modify the namespace's metadata before save if the avatar failed to download.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it help to elevate the FailsafeDeclarativeArtifact to pulpcore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, we could design it to accept a list of exceptions during download that would be failsafe.

name = self.extra_data.get("namespace")
log.info(f"Failed to download namespace avatar: {name} - {e}, Skipping")
return None


class DocsBlobDownloader(ArtifactDownloader):
"""
Stage for downloading docs_blob.
Expand Down Expand Up @@ -1114,7 +1133,7 @@ async def _handle_content_unit(self, d_content):
return downloaded


class CollectionContentSaver(ContentSaver):
class AnsibleContentSaver(ContentSaver):
"""
A modification of ContentSaver stage that additionally saves Ansible plugin specific items.

Expand Down Expand Up @@ -1149,6 +1168,15 @@ def _pre_save(self, batch):
name = d_content.content.name
namespace, created = AnsibleNamespace.objects.get_or_create(name=name)
d_content.content.namespace = namespace
if d_content.d_artifacts:
da = d_content.d_artifacts[0]
# Check to see if avatar failed to download, update metadata if so
if da.deferred_download:
d_content.d_artifacts = None
d_content.content.avatar_sha256 = None
# Check to see if upstream didn't have avatar_sha256 set
elif d_content.content.avatar_sha256 is None:
d_content.content.avatar_sha256 = da.artifact.sha256
gerrod3 marked this conversation as resolved.
Show resolved Hide resolved

def _post_save(self, batch):
"""
Expand Down
5 changes: 2 additions & 3 deletions pulp_ansible/app/viewsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -408,11 +408,10 @@ class AnsibleNamespaceViewSet(ReadOnlyContentViewSet):
@action(detail=True, methods=["get"], serializer_class=None)
def avatar(self, request, pk):
"""
Dispatches a collection version rebuild task.
Tries to find a redirect link to the Namespace's avatar
"""
ns = self.get_object()
artifact = ns.avatar_artifact
if artifact:
if artifact := ns.avatar_artifact:
return HttpResponseRedirect(get_artifact_url(artifact))

return HttpResponseNotFound()
Expand Down