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

[PR #6064/0a5ac4aa backport][3.49] [SAT-29018] Fix/corrupted RA blocks content streaming #6196

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

pedro-psb
Copy link
Member

The #6161 was not very good because it still added the field to the database.

This approaches adds a unique field to the branch which won't conflict with any future field.
It must be cleaned up later with an idempotent remove migration.

Still testing if it runs without a problem.

@pedro-psb
Copy link
Member Author

pedro-psb commented Jan 10, 2025

My attempt to test the upgrade process:

  1. Start on 3.49 with a clean install
  2. checking out to main <-- errors [0]

I assume I'm missing some steps (update deps, ...?). Will continue on monday.

[0]:

[pulp]  | Traceback (most recent call last):
[pulp]  |   File "/usr/local/lib/python3.9/site-packages/gunicorn/arbiter.py", line 608, in spawn_worker
[pulp]  |     worker.init_process()
[pulp]  |   File "/src/pulpcore/pulpcore/app/entrypoint.py", line 47, in init_process
[pulp]  |     django.setup()
[pulp]  |   File "/usr/local/lib/python3.9/site-packages/django/__init__.py", line 24, in setup
[pulp]  |     apps.populate(settings.INSTALLED_APPS)
[pulp]  |   File "/usr/local/lib/python3.9/site-packages/django/apps/registry.py", line 124, in populate
[pulp]  |     app_config.ready()
[pulp]  |   File "/src/pulpcore/pulpcore/app/apps.py", line 258, in ready
[pulp]  |     super().ready()
[pulp]  |   File "/src/pulpcore/pulpcore/app/apps.py", line 134, in ready
[pulp]  |     self.import_viewsets()
[pulp]  |   File "/src/pulpcore/pulpcore/app/apps.py", line 171, in import_viewsets
[pulp]  |     from pulpcore.app.viewsets import NamedModelViewSet
[pulp]  |   File "/src/pulpcore/pulpcore/app/viewsets/__init__.py", line 1, in <module>
[pulp]  |     from .base import (
[pulp]  |   File "/src/pulpcore/pulpcore/app/viewsets/base.py", line 16, in <module>
[pulp]  |     from pulpcore.openapi import PulpAutoSchema
[pulp]  |   File "/src/pulpcore/pulpcore/openapi/__init__.py", line 13, in <module>
[pulp]  |     from drf_spectacular.plumbing import (
[pulp]  | ImportError: cannot import name 'process_webhooks' from 'drf_spectacular.plumbing' (/usr/local/lib/python3.9/site-packages/drf_spectacular/plumbing.py)

@pedro-psb
Copy link
Member Author

The upgrade did work. Here were the steps:

  1. Compose up oci_env with this branch code (based on 3.49 and with the failed_at_backport49 field)
  2. Checkout to pulpcore main
  3. run pip install src/pulpcore
  4. run pulpcore-manager migrate

The results

[root@4a043b9b8239 /]# pulpcore-manager migrate
Operations to perform:
  Apply all migrations: auth, certguard, contenttypes, core, file, sessions
Running migrations:
  Applying core.0118_task_core_task_unblock_2276a4_idx_and_more... OK
  Applying core.0119_grouprole_core_groupr_object__250e22_idx_and_more... OK
  Applying core.0120_get_url_removal... OK
  Applying core.0121_add_profile_artifacts_table... OK
  Applying core.0122_record_last_replication_timestamp... OK
  Applying core.0123_upstreampulp_q_select... OK
  Applying core.0124_task_deferred_task_immediate... OK
  Applying core.0125_openpgpdistribution_openpgpkeyring_openpgppublickey_and_more... OK
  Applying core.0126_remoteartifact_failed_at... OK
Access policy for artifacts created.
Access policy for domains updated.
Access policy for repositories/core/openpgp_keyring created.
Access policy for task-groups created.
Access policy for tasks updated.
Access policy for login created.
[root@4a043b9b8239 /]# su pulp
bash-5.1$ psql
psql (13.16)
Type "help" for help.

pulp=> \d core_remoteartifact;
                        Table "public.core_remoteartifact"
        Column        |           Type           | Collation | Nullable | Default
----------------------+--------------------------+-----------+----------+---------
 pulp_id              | uuid                     |           | not null |
 pulp_created         | timestamp with time zone |           | not null |
 pulp_last_updated    | timestamp with time zone |           |          |
 url                  | text                     |           | not null |
 size                 | bigint                   |           |          |
 md5                  | character varying(32)    |           |          |
 sha1                 | character varying(40)    |           |          |
 sha224               | character varying(56)    |           |          |
 sha256               | character varying(64)    |           |          |
 sha384               | character varying(96)    |           |          |
 sha512               | character varying(128)   |           |          |
 content_artifact_id  | uuid                     |           | not null |
 remote_id            | uuid                     |           | not null |
 pulp_domain_id       | uuid                     |           | not null |
 failed_at_backport49 | timestamp with time zone |           |          |
 failed_at            | timestamp with time zone |           |          |

remote_artifacts = (
content_artifact.remoteartifact_set.select_related("remote")
.order_by_acs()
.exclude(pulp_last_updated__gte=timezone.now() - timedelta(seconds=protection_time))
Copy link
Member

Choose a reason for hiding this comment

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

Now I'm wondering whether it'd be better to add the timedelta to setting the "failed_at" ("pulp_last_udated" in this case) and only check against now here.

pulpcore/content/handler.py Outdated Show resolved Hide resolved
pulpcore/content/handler.py Outdated Show resolved Hide resolved
@pedro-psb pedro-psb force-pushed the experiment-migration-workaround branch 2 times, most recently from 349d2a2 to bb0b45d Compare January 15, 2025 13:21
@pedro-psb pedro-psb force-pushed the experiment-migration-workaround branch from c915dec to e4461bd Compare January 15, 2025 14:37
@pedro-psb pedro-psb marked this pull request as ready for review January 15, 2025 16:51
@pedro-psb pedro-psb changed the title [experiment] Backport migration workaround [PR #6064/0a5ac4aa backport][3.49] [SAT-29018] Fix/corrupted RA blocks content streaming Jan 15, 2025
@pedro-psb pedro-psb force-pushed the experiment-migration-workaround branch from e4461bd to 9748324 Compare January 15, 2025 17:17
pedro-psb and others added 2 commits January 15, 2025 14:25
On a request for on-demand content in the content app, a corrupted Remote
that contains the wrong binary (for that content) prevented other Remotes
from being attempted on future requests.

Now the last failed Remotes are temporarily ignored and others may be picked.

Cherry-picked from: 0a5ac4a
A fix was introduced in pulpcore==3.69 which relied on a new DateTimeField
called 'failed_at'.
Do enable backporting that, this commits removes the migration and, as
a workaround, re-purpose a compatible field (pulp_last_updated) to serve
the same purpose.

Co-authored-by: Matthias Dellweg <[email protected]>
@pedro-psb pedro-psb force-pushed the experiment-migration-workaround branch from 9748324 to e048db7 Compare January 15, 2025 17:25
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

Nice!

@pedro-psb
Copy link
Member Author

Thanks for all the help to get this working!

@pedro-psb pedro-psb merged commit 9ebc113 into pulp:3.49 Jan 16, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants