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

Remove backward compatibility for manifest with artifact #1644

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichalPysik
Copy link
Member

This commit removes support for manifests storing their data inside an artifact instead of using the recently introduced Manifest.data text field.

closes #1621

@MichalPysik MichalPysik force-pushed the remove-backward-manifest-artifact branch from 160eb27 to 7f516b4 Compare May 23, 2024 16:17
@MichalPysik MichalPysik changed the title Remove backward compatibility for artifactless manifest Remove backward compatibility for manifest with artifact May 23, 2024
@lubosmj
Copy link
Member

lubosmj commented May 27, 2024

Have you considered returning manifest.data directly from api-app? We may want to omit redirects to content-app when the data is available. Thus, instead of doing this:

return redirects.issue_manifest_redirect(manifest)
, we would run return Response(manifest.data).

@MichalPysik MichalPysik force-pushed the remove-backward-manifest-artifact branch 6 times, most recently from 83af4f4 to 0657cbb Compare June 3, 2024 10:37
@lubosmj
Copy link
Member

lubosmj commented Jun 6, 2024

I think tests are failing because of the caching machinery:

2024-06-03T10:51:37.9321965Z   File "/usr/local/lib/python3.9/site-packages/pulp_container/app/registry_api.py", line 1016, in get
2024-06-03T10:51:37.9322259Z     return self.handle_safe_method(request, path, pk)
2024-06-03T10:51:37.9323128Z   File "/usr/local/lib/python3.9/site-packages/pulpcore/cache/cache.py", line 171, in cached_function
2024-06-03T10:51:37.9323626Z     response = self.make_entry(key, bk, func, args, kwargs, self.default_expires_ttl)
2024-06-03T10:51:37.9324448Z   File "/usr/local/lib/python3.9/site-packages/pulpcore/cache/cache.py", line 220, in make_entry
2024-06-03T10:51:37.9324840Z     entry["content"] = response.content.decode("utf-8")
2024-06-03T10:51:37.9325657Z   File "/usr/local/lib/python3.9/site-packages/django/template/response.py", line 135, in content
2024-06-03T10:51:37.9325900Z     raise ContentNotRenderedError(
2024-06-03T10:51:37.9326730Z django.template.response.ContentNotRenderedError: The response content must be rendered before it can be accessed.

We started returning manifest.data from api-app which results in the said error. We may want to update the code on the pulpcore's side or overwrite the existing behaviour, where decode is always called on the TextField data, from the plugin itself.

# cache.py

     def make_entry():
          ...
          elif isinstance(response, HttpResponse):
            entry["type"] = "Response"
            if isinstance(response.data, str):
               # TODO: store response.content data
            else:
                entry["content"] = response.content.decode("utf-8")
        else:
            # We don't cache StreamResponses or errors
            return response

@MichalPysik
Copy link
Member Author

I think tests are failing because of the caching machinery:

2024-06-03T10:51:37.9321965Z   File "/usr/local/lib/python3.9/site-packages/pulp_container/app/registry_api.py", line 1016, in get
2024-06-03T10:51:37.9322259Z     return self.handle_safe_method(request, path, pk)
2024-06-03T10:51:37.9323128Z   File "/usr/local/lib/python3.9/site-packages/pulpcore/cache/cache.py", line 171, in cached_function
2024-06-03T10:51:37.9323626Z     response = self.make_entry(key, bk, func, args, kwargs, self.default_expires_ttl)
2024-06-03T10:51:37.9324448Z   File "/usr/local/lib/python3.9/site-packages/pulpcore/cache/cache.py", line 220, in make_entry
2024-06-03T10:51:37.9324840Z     entry["content"] = response.content.decode("utf-8")
2024-06-03T10:51:37.9325657Z   File "/usr/local/lib/python3.9/site-packages/django/template/response.py", line 135, in content
2024-06-03T10:51:37.9325900Z     raise ContentNotRenderedError(
2024-06-03T10:51:37.9326730Z django.template.response.ContentNotRenderedError: The response content must be rendered before it can be accessed.

We started returning manifest.data from api-app which results in the said error. We may want to update the code on the pulpcore's side or overwrite the existing behaviour, where decode is always called on the TextField data, from the plugin itself.

# cache.py

     def make_entry():
          ...
          elif isinstance(response, HttpResponse):
            entry["type"] = "Response"
            if isinstance(response.data, str):
               # TODO: store response.content data
            else:
                entry["content"] = response.content.decode("utf-8")
        else:
            # We don't cache StreamResponses or errors
            return response

@lubosmj If I understand correctly, before we started returning manifest.data (which is text, not bytes), we used to return it in bytes (abstracting away the fact that it went through multiple additional functions to get to that point)? And if that is the case, would simply returning manifest.data.encode() fix the issue?

@lubosmj
Copy link
Member

lubosmj commented Jun 7, 2024

Oh, actually, the issue is that we cannot access response.content. Thus, the execution never reaches the decode call. Can you verify if calling "response.render()" inside the pulpcore's code will resolve the issue? Other than that, I think we should update the caching code as a whole to deal with HttpResponse properly. It is plausible that this code path was never executed. Or, we may think about returning something else from api-app...

@@ -1084,7 +1084,7 @@ def handle_safe_method(self, request, path, pk):
else:
raise ManifestNotFound(reference=pk)

return redirects.issue_manifest_redirect(manifest)
return Response(manifest.data)
Copy link
Member

Choose a reason for hiding this comment

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

Bear in mind that we would like to return headers as well, similarly to return web.Response(text=tag.tagged_manifest.data, headers=response_headers).

Copy link
Member Author

Choose a reason for hiding this comment

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

@lubosmj What headers would you like to return in this case?

@lubosmj
Copy link
Member

lubosmj commented Jun 17, 2024

Do you mind reverting the following changes?

# FIXME this can be rolledback after https://github.com/pulp/pulp_container/issues/1288

# FIXME this can be rolledback after https://github.com/pulp/pulp_container/issues/1288

# FIXME this can be rolledback after https://github.com/pulp/pulp_container/issues/1288

I think we can get rid of them now.

@MichalPysik MichalPysik force-pushed the remove-backward-manifest-artifact branch from 0657cbb to a2e7a53 Compare June 18, 2024 13:50
This commit removes support for manifests storing their data inside an
artifact instead of using the recently introduced  Manifest.data text
field.

closes pulp#1621
@MichalPysik MichalPysik force-pushed the remove-backward-manifest-artifact branch from a2e7a53 to ed8bdf6 Compare June 18, 2024 13:52
@lubosmj
Copy link
Member

lubosmj commented Aug 14, 2024

The work on this can be resumed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop supporting manifests with artifacts and enforce the data migration
2 participants