Repository navigation
Resolve storage paths through a backend registry #8234
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,6 @@ all: | |
| pulp: | ||
| pulp-fixtures: | ||
| rustfs: | ||
| ci-sftp: | ||
| vars: | ||
| ansible_connection: docker | ||
| ... | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Storage backends can now be replaced without changing Domain records or the Domain API. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| # Replacing a Domain storage implementation | ||
|
|
||
| Domain records retain historical storage class paths so older Pulp processes can read them during | ||
| a rolling upgrade. Pulp resolves those paths to stable backend IDs (`filesystem`, `s3`, `azure`, and | ||
| `gcs`), then selects the installed class through `DOMAIN_STORAGE_BACKENDS`. The API still advertises | ||
| the historical class paths and returns them on reads. It also accepts stable IDs in raw requests, | ||
| but generated clients may reject those IDs because the schema still lists the historical paths. | ||
| Existing `S3Boto3Storage` paths are preserved rather than rewritten to the newer S3 path. | ||
|
|
||
| This boundary replaces the code for an existing storage type; it does not register a new type. | ||
| Only filesystem, S3, and Azure are currently offered for new Domains. GCS is recognized for | ||
| existing configurations, and SFTP is not supported. | ||
|
|
||
| ## Select an implementation | ||
|
|
||
| Set the class path for each non-default Domain storage type you are replacing: | ||
|
|
||
| ```python | ||
| DOMAIN_STORAGE_BACKENDS = { | ||
| "filesystem": "pulpcore.app.models.storage.FileSystem", | ||
| "s3": "my_storage.backends.s3.S3Storage", | ||
| "azure": "storages.backends.azure_storage.AzureStorage", | ||
| "gcs": "storages.backends.gcloud.GoogleCloudStorage", | ||
| } | ||
| ``` | ||
|
|
||
| The default Domain instead uses Django's `STORAGES["default"]`. When replacing that class, set | ||
| both its `BACKEND` and the corresponding `DOMAIN_STORAGE_BACKENDS` entry. The mapping lets Pulp | ||
| identify its storage type without requiring the replacement's Python path in a Domain record. | ||
| An unrecognized default class can serve redirects only if it implements `pulp_url`. | ||
|
|
||
| For direct content redirects, a replacement may implement | ||
| `pulp_url(name, headers=None, method=None)` to sign URLs with Pulp's response-header overrides. | ||
| Without that hook, Pulp calls the django-storages-style `url()` method for recognized object storage | ||
| types. For ranged content reads, a replacement may expose a synchronous | ||
| `open_stream(name, start=0, length=None)` context manager. Pulp falls back to Django's `open()` and | ||
| `seek()` if the hook is absent; that fallback bounds the returned bytes but may buffer a whole | ||
| remote object. The stream must release its resources when the context exits. | ||
|
|
||
| ## Use storage from a plugin | ||
|
|
||
| Plugin code should use `pulpcore.plugin.util.get_local_storage_path(file, domain=None)` when it | ||
| needs an actual filesystem path. It returns `None` when the backend does not provide one; other | ||
| storage errors are propagated. Use `get_direct_storage_url(file, headers=None, http_method=None, | ||
| domain=None)` when a consumer needs a URL directly to object storage. It returns `None` when the | ||
| Domain uses filesystem storage or has redirects disabled, and raises `NotImplementedError` when | ||
| redirects are requested from an unsupported backend. Both functions accept a Pulp-managed file | ||
| such as `Artifact.file` or `PulpTemporaryFile.file`. If the file belongs to another Domain, pass | ||
| that Domain explicitly; otherwise they use the current Domain. | ||
|
|
||
| The URL function accepts generic `Content-Type` and `Content-Disposition` response headers and | ||
| translates them into the selected provider's signed URL parameters. Plugins should not inspect | ||
| `storage_class` or pass provider-specific parameters to `storage.url()`. The existing | ||
| `get_artifact_url(artifact, ...)` differs: it always returns a URL, falling back to the content | ||
| app when a direct URL is unavailable. | ||
|
|
||
| ## Roll out a replacement | ||
|
|
||
| Keep the Domain's storage type, bucket or container, prefix, object names, credentials, and URL | ||
| behavior compatible. Install the replacement and its dependencies on every API, content, and worker | ||
| process, and test reads, writes, range requests, and signed redirects against existing objects. | ||
| Then change the deployment settings. A code-only change needs no Domain data migration. Moving | ||
| objects to a different location still requires a storage migration. | ||
|
|
||
| During a rolling upgrade, new processes continue to store and return historical class paths so | ||
| older processes can read the same Domain rows. Keep the old implementation installed until those | ||
| processes and the rollback window are gone. Do not switch persisted `storage_class` values to IDs | ||
| while any process still requires the historical paths. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,45 @@ | |
| Warning as CheckWarning, | ||
| ) | ||
| from django.db.models import Q | ||
| from django.utils.module_loading import import_string | ||
|
|
||
| from pulpcore import constants | ||
|
|
||
|
|
||
| @register() | ||
| def storage_redirect_backend(app_configs, **kwargs): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaces
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change sufficiantly independent to move it into a separate PR?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It probably could be (or at least a separate commit) if you are so inclined. Is that what you want?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern is that this PR is huge and this at least feels "only adjacent" but still a good idea on its own.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to a separate commit |
||
| """Reject default storage that the content app cannot serve with redirects.""" | ||
| from pulpcore.app.storage_backends import backend_id | ||
|
|
||
| if not settings.REDIRECT_TO_OBJECT_STORAGE: | ||
| return [] | ||
| path = settings.STORAGES["default"]["BACKEND"] | ||
| backend = backend_id(path) | ||
| if backend in { | ||
| constants.STORAGE_BACKENDS.FILESYSTEM, | ||
| constants.STORAGE_BACKENDS.S3, | ||
| constants.STORAGE_BACKENDS.AZURE, | ||
| constants.STORAGE_BACKENDS.GCS, | ||
| }: | ||
| return [] | ||
|
|
||
| # An unregistered default backend can still redirect through Pulp's URL hook. | ||
| try: | ||
| implementation = import_string(path) | ||
| except (ImportError, AttributeError): | ||
| implementation = None | ||
| if callable(getattr(implementation, "pulp_url", None)): | ||
| return [] | ||
|
|
||
| return [ | ||
| CheckError( | ||
| "STORAGES['default']['BACKEND'] must map to a supported storage type " | ||
| "or implement pulp_url when REDIRECT_TO_OBJECT_STORAGE is enabled.", | ||
| id="pulpcore.E003", | ||
| ) | ||
| ] | ||
|
|
||
|
|
||
| @register(deploy=True) | ||
| def secret_key_check(app_configs, **kwargs): | ||
| messages = [] | ||
|
|
@@ -38,7 +73,9 @@ def secret_key_check(app_configs, **kwargs): | |
| def storage_paths(app_configs, **kwargs): | ||
| warnings = [] | ||
|
|
||
| if settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem": | ||
| from pulpcore.app.storage_backends import default_backend_id | ||
|
|
||
| if default_backend_id() == constants.STORAGE_BACKENDS.FILESYSTEM: | ||
| try: | ||
| media_root_dev = Path(settings.MEDIA_ROOT).stat().st_dev | ||
| except OSError: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parts of this are useful and parts of it are of questionable usefulness. Mostly it's valuable for a very rare circumstance (changing the storage implementation being rare, and the initial ZDT upgrades). I'm not sure whether to keep it, or just keep it local until we work past the actual implementation switch.
If it was going to be commonly consulted then it needs some editing (it reads like AI docs, because it is), but if it's only rarely consulted or would be dropped soon then I don't want to bother.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that said I could probably clean this up to be at least less context/PR-specific. Just let me know if you think it's worth keeping at all.