Change the default deployment layout #799

ekohl · 2020-07-21T14:51:18Z

This changes the default deployment layout. The main change is that MEDIA_ROOT gets its own directory. This allows limiting the file permissions in a shared Pulp 2 + Pulp 3 deployment and the SELinux file contexts. Another benefit is compatibility with django_extensions' unreferenced_files command which lists all files in MEDIA_ROOT that are not in the database.

Other paths are kept on the same absolute paths, but the diff looks bigger because they used derive from MEDIA_ROOT.

The documentation is updated to show the latest best practices.

fixes #7178

pulpbot · 2020-07-21T14:51:27Z

Attached issue: https://pulp.plan.io/issues/7178

ekohl · 2020-07-21T14:54:36Z

This is the result of playing around with it and previous experience with another Django project.

@bmbouter @mikedep333 please have a look

@jlsherrill @ehelms @wbclark you may also find this interesting.

If this is agreed on, it'll also need to be reflected in pulpcore-selinux. I don't mind making that patch, but first there should be agreement.

mikedep333 · 2020-07-21T20:07:58Z

@ekohl I disagree with part of your design. It seems to contradict previous designs, including security (group access is good. World access is insecure.)

https://pulp.plan.io/issues/7178#change-59743

ekohl · 2020-07-22T15:15:34Z

pulpcore/app/settings.py

@@ -28,19 +27,20 @@

 ALLOWED_HOSTS = ["*"]

-MEDIA_ROOT = "/var/lib/pulp/"
+DEPLOY_ROOT = Path("/var/lib/pulp")
+MEDIA_ROOT = DEPLOY_ROOT / "media"


Offline discussion was to change this to artifact (which already exists) and then add a migration that updates the database.

Suggested change

MEDIA_ROOT = DEPLOY_ROOT / "media"

MEDIA_ROOT = DEPLOY_ROOT / "artifact"

That means the following code needs updates:

pulpcore/pulpcore/app/models/fields.py

Lines 47 to 66 in 09d05da

already_in_place = file.name in [

artifact_storage_path,

os.path.join(settings.MEDIA_ROOT, artifact_storage_path),

]

is_in_artifact_storage = file.name.startswith(os.path.join(settings.MEDIA_ROOT, "artifact"))

if not already_in_place and is_in_artifact_storage:

raise ValueError(

_(

"The file referenced by the Artifact is already present in "

"Artifact storage. Files must be stored outside this location "

"prior to Artifact creation."

)

)

move = file._committed and file.name != artifact_storage_path

if move:

if not already_in_place:

file._file = TemporaryDownloadedFile(open(file.name, "rb"))

file._committed = False

Before proceeding with this, I'd like to get some more feedback if this is really desired because it looks like the entire ArtifactFileField.pre_save becomes redundant then.

Another approach would be to use a migration that looks at all models with a FileField. Then if it doesn't exist, look a directory higher and move it into MEDIA_ROOT https://github.com/django-extensions/django-extensions/blob/d747e09885e90d4c5f702f0bb4f0ea42e63f4eb0/django_extensions/management/commands/unreferenced_files.py#L27-L43 can be the basis of that.

ekohl · 2020-07-22T15:17:56Z

Updated to at least use pathlib instead of path.py correctly so the tests show if I broke something.

daviddavis · 2020-08-27T15:00:36Z

Moving to WIP due to inactivity.

ekohl · 2020-08-27T15:17:14Z

For what it's worth: I was waiting on a reply from maintainers on how to proceed @ #799 (comment)

ekohl · 2020-09-01T15:56:09Z

Summarizing a discussion with @mikedep333 and @dkliban. Keep me honest if I missed anything

The easiest migration plan is to keep the artifact directory inside MEDIA_ROOT. That means no application code change is needed and no DB migration is needed. Both are a good thing. Exact paths inside MEDIA_ROOT are not important to the admin, but a quick migration with minimal to no downtime is.

To support this, the following steps should be taken:

Upgrade pulpcore-selinux to have the correct file contexts for the new locations
Prepare layout (see script below)
Update settings
Restart all services
Remove symlink

The migration script is as follows:

#!/usr/bin/env python3
from pathlib import Path

DEPLOY_ROOT = Path('/var/lib/pulp')
LEGACY_DIR = DEPLOY_ROOT / 'artifact'
NEW_MEDIA_ROOT = DEPLOY_ROOT / 'media'
DESTINATION = NEW_MEDIA_ROOT / LEGACY_DIR.name

if LEGACY_DIR.is_dir() and not LEGACY_DIR.is_symlink():
    if DESTINATION.is_dir():
        raise SystemExit(f'Target {DESTINATION} already exists')

    NEW_MEDIA_ROOT.mkdir(exist_ok=True)
    LEGACY_DIR.rename(DESTINATION)
    LEGACY_DIR.symlink_to(DESTINATION)

This means that the files are available at both locations at the same time. There is a tiny window between the rename and symlink_to calls where content can't be found, but on a modern storage system that should be negligible.

After all services use the new settings, nothing should be using the symlink anymore and can be cleaned up.

mikedep333 · 2020-09-02T17:02:56Z

The installer team reviewed this, and we are good with the design.

ekohl · 2020-09-02T17:28:37Z

Excellent, I'll try to find time to get Travis green on this PR.

CHANGES/7178.feature

bmbouter

I'm excited about this PR. To merge it we'll also need updates to this page in the docs please: https://github.com/pulp/pulpcore/blob/master/docs/settings.rst It documents the various settings, so the new ones should be documented there please.

mikedep333 · 2020-09-02T18:39:58Z

To merge this, we'll also need a corresponding PR for pulp_installer that does the symlink.

EDIT: We will just tell pulp_installer users to stop the services before upgrading, instead of the symlink. We will still perform the move though.

ekohl · 2020-09-03T14:33:41Z

I'm converting it back to a draft since right now I'm just working on getting the build to pass. Once I figure that out, I'll look at docs.

To merge this, we'll also need a corresponding PR for pulp_installer that does the symlink.

Can I ask the pulp_installer maintainers to work on that? Every time I have to write Ansible I get frustrated and generally avoid touching it altogether if possible.

ekohl · 2020-09-03T15:29:46Z

Ah, it turns out Django's storage system only gained support for pathlib.Path in Django 3.1. That's disappointing. Casting that to a str.

ekohl · 2020-12-08T12:49:44Z

I think this is now ready for review.

ekohl · 2020-12-08T13:00:31Z

pulpcore/app/checks.py

+        if media_root_dev and media_root_dev != upload_temp_dir_dev:
+            warnings.append(
+                CheckWarning(
+                    "MEDIA_ROOT and FILE_UPLOAD_TEMP_DIR are on different filesystems. "


I'm not 100% sure if there are file moves between these two. That was the reason that they should be on the same FS, right?

dkliban

I filed an installer story to go along with this change. https://pulp.plan.io/issues/8011. @ekohl Can you please take a look at it and provide feedback?

@ekohl It doesn't look like we need to make any changes to the pulpcore-selinux repository, is my understanding correct?

ekohl · 2020-12-18T15:32:16Z

I filed an installer story to go along with this change. https://pulp.plan.io/issues/8011. @ekohl Can you please take a look at it and provide feedback?

I summarized the steps, including a minimal migration script. I'd recommend to at least link that in the issue, but possibly copy over some info.

@ekohl It doesn't look like we need to make any changes to the pulpcore-selinux repository, is my understanding correct?

pulp/pulpcore-selinux@d7305fb#diff-c0c5c879ce2e9f03d675c8a4efe177ed5f2ee6a8ff41e4a15bc7500d16ae32ef did that already.

dkliban · 2020-12-18T20:17:56Z

I filed an installer story to go along with this change. https://pulp.plan.io/issues/8011. @ekohl Can you please take a look at it and provide feedback?

I summarized the steps, including a minimal migration script. I'd recommend to at least link that in the issue, but possibly copy over some info.

Thank you! I thought it would be easier to not do a move but simply use a hardlink. However, I have now learned that you can't hardlink a directory. So we will have the installer perform the operations in the script you provided.

@ekohl It doesn't look like we need to make any changes to the pulpcore-selinux repository, is my understanding correct?

pulp/pulpcore-selinux@d7305fb#diff-c0c5c879ce2e9f03d675c8a4efe177ed5f2ee6a8ff41e4a15bc7500d16ae32ef did that already.

dkliban · 2021-01-05T18:10:06Z

pulpcore/app/migrations/0024_use_local_storage_for_uploads.py

@@ -14,6 +14,6 @@ class Migration(migrations.Migration):
        migrations.AlterField(
            model_name='upload',
            name='file',
-            field=models.FileField(max_length=255, storage=django.core.files.storage.FileSystemStorage(location='/var/lib/pulp/upload/'), upload_to=''),
+            field=models.FileField(max_length=255, storage=django.core.files.storage.FileSystemStorage(location='/var/lib/pulp/upload'), upload_to=''),


is this change intentional? It's in a migration that has already been released

Yes. The default for it has changed and if I didn't modify this, Django insisted on a new migration. Since the default is at runtime only (default is not part of the database) I thought it was safe to do so. Given the user may even change the setting, it probably can't even live in the database.

dkliban

Thank you for this change! Once we merge this we should also merge the installer PR.

pulp/pulp_installer#510

pulpcore/app/settings.py

bmbouter · 2021-01-11T21:08:07Z

pulpcore/app/settings.py

@@ -32,21 +32,22 @@

 ALLOWED_HOSTS = ["*"]

-MEDIA_ROOT = "/var/lib/pulp/"
+DEPLOY_ROOT = Path("/var/lib/pulp")


Is this a net-new setting for Pulp? Or is setting and existing Django setting? We need docs added for it here: https://docs.pulpproject.org/pulpcore/settings.html

It is new but note that it doesn't affect anything if you change it. Since dynaconf are loaded after setting these variables. That means that if a user sets DEPLOY_ROOT in dynaconf, you would have to set all variables that use DEPLOY_ROOT after dynaconf. That can be done if you want of course.

@bmbouter before I start documenting, could you weigh in if this should actually do something? Documenting a setting that doesn't change anything feels useless.

Thank you for the questions (and this effort in general).

There's no need to document settings that can't be used. Let me split my answer in two parts:

I had imagined DEPLOY_ROOT could be set by the user via dynaconf and if so, it would cause all of the variables that use it to also change. If that's not possible and users would just need to set N variables to effectively change /var/lib/pulp/ then I don't think there is anything to document.

This does change the default value of at least MEDIA_ROOT thought right? That default should be updated in the docs if so.

Let me know if there is more I should respond with. I'm excited about this change, ty!

1. I had imagined `DEPLOY_ROOT` could be set by the user via dynaconf and if so, it would cause all of the variables that use it to also change. If that's not possible and users would just need to set N variables to effectively change `/var/lib/pulp/` then I don't think there is anything to document.

I don't think this is true. Though I didn't test it, I can't see how it would work.

from pathlib import Path DEPLOY_ROOT = Path('/var/lib/pulp') MEDIA_PATH = DEPLOY_ROOT / 'media' assert id(DEPLOY_ROOT) != id(MEDIA_ROOT) assert repr(MEDIA_PATH) == "PosixPath('/var/lib/pulp/media')"

Here MEDIA_PATH is a new instance of Path. If you set a new value for DEPLOY_ROOT via dynaconf, it won't modify anything that copied its value.

Perhaps dynaconf can do something with lazy settings (https://www.dynaconf.com/envvars/#lazy-values) suggests it can do something like that), I have no idea how that would work.

That made me think that perhaps it should be _DEPLOY_ROOT but if you can use lazy values, perhaps it becomes useful again. I'm looking for advice here since I don't know dynaconf at all.

2\. This does change the default value of at least `MEDIA_ROOT` thought right? That default should be updated in the docs if so.

It does indeed.

That's ok, I don't think we need "one var to reroot many other vars". This PR didn't have that goal, and I won't want to introduce additional goals. What you've already got looks great.

I think just documenting the change for the defualt of MEDIA_ROOT is all we need.

I think just documenting the change for the defualt of MEDIA_ROOT is all we need.

FYI: that's already done here:

https://github.com/pulp/pulpcore/pull/799/files#diff-1d78ba60e1efa4b7b08982e1bbaeceffefea079d402b7d58e57c0cfc7e6c78d8R58

I also took the opportunity to fix the SELinux context type.

bmbouter · 2021-01-11T21:08:38Z

pulpcore/app/settings.py

-FILE_UPLOAD_TEMP_DIR = os.path.join(MEDIA_ROOT, "tmp/")
-WORKING_DIRECTORY = os.path.join(MEDIA_ROOT, "tmp/")
-CHUNKED_UPLOAD_DIR = os.path.join(MEDIA_ROOT, "upload/")
+FILE_UPLOAD_TEMP_DIR = DEPLOY_ROOT / "tmp"


Also the docs https://docs.pulpproject.org/pulpcore/settings.html probably need updates to identify their defaults being tied up with DEPLOY_ROOT.

It's a Pulp only setting that's being added with this PR. +1 on adding it to the docs. I've added a reference to it in the installer docs[0].

[0] https://github.com/pulp/pulp_installer/pull/510/files#diff-c26c81d8d68fbf1c0dfa6018bb2b2256d54bae4729158279ab601b6f34247fffR58

bmbouter

Requesting some docs updates please.

bmbouter · 2021-01-11T21:19:36Z

Also does this need any changes to the pulpcore-selinux fc files? https://github.com/pulp/pulpcore-selinux/blob/master/pulpcore.fc

dkliban · 2021-01-11T21:21:32Z

Also does this need any changes to the pulpcore-selinux fc files? https://github.com/pulp/pulpcore-selinux/blob/master/pulpcore.fc

No, see #799 (comment)

bmbouter · 2021-01-13T14:19:27Z

@ekohl @dkliban what handles moving the files on existing installations to make them consistent with the new MEDIA_ROOT location?

ekohl · 2021-01-13T15:30:34Z

#799 (comment) summarizes the migration. https://github.com/theforeman/foreman-installer/blob/39003aaed97cb51285761463bdb84cf8bf0cdbca/hooks/pre/34-pulpcore_directory_layout.rb is what foreman-installer uses. pulp/pulp_installer#510 is still open for Pulp itself.

ekohl · 2021-01-18T18:05:05Z

FYI: I opened #1074 which extracted a small part of this PR to cherry pick as a regression fix in 3.9. For this reason I this PR on it.

This changes the default deployment layout. The main change is that MEDIA_ROOT gets its own directory. This allows limiting the file permissions in a shared Pulp 2 + Pulp 3 deployment and the SELinux file contexts. Another benefit is compatibility with django_extensions' unreferenced_files command which lists all files in MEDIA_ROOT that are not in the database. Other paths are kept on the same absolute paths, but the diff looks bigger because they used derive from MEDIA_ROOT. The documentation is updated to show the latest best practices. fixes #7178

ekohl · 2021-01-22T15:47:08Z

Rebased now that a805312 has been merged. I've dropped the database migration changes because of it.

bmbouter

Thank you for everything you've done for this! I really appreciate both the great contribution and patience to get it right.

ekohl · 2021-02-02T15:45:24Z

I'm glad we got there.

pulpbot added the Needs Cherry Pick label Jul 21, 2020

ekohl force-pushed the 7178-deployment-defaults branch from a9502fb to 8a1e0ae Compare July 21, 2020 17:29

ekohl force-pushed the 7178-deployment-defaults branch from 8a1e0ae to 451bb22 Compare July 22, 2020 14:29

ekohl commented Jul 22, 2020

View reviewed changes

ekohl force-pushed the 7178-deployment-defaults branch from 451bb22 to 16ae3f5 Compare July 22, 2020 15:17

daviddavis removed the Needs Cherry Pick label Jul 28, 2020

daviddavis added the Work In Progress label Aug 27, 2020

ekohl mentioned this pull request Sep 1, 2020

Fixes #30423 - Change the application layout theforeman/puppet-pulpcore#115

Merged

bmbouter reviewed Sep 2, 2020

View reviewed changes

CHANGES/7178.feature Outdated Show resolved Hide resolved

bmbouter requested changes Sep 2, 2020

View reviewed changes

ekohl force-pushed the 7178-deployment-defaults branch 2 times, most recently from 28a901f to a829b3d Compare September 3, 2020 14:31

ekohl marked this pull request as draft September 3, 2020 14:31

ekohl force-pushed the 7178-deployment-defaults branch from a829b3d to 169971f Compare September 3, 2020 14:35

ekohl force-pushed the 7178-deployment-defaults branch 4 times, most recently from 0ce38eb to dea594b Compare September 4, 2020 11:38

ekohl commented Dec 8, 2020

View reviewed changes

daviddavis removed the Work In Progress label Dec 8, 2020

daviddavis requested review from dkliban, bmbouter and mikedep333 December 8, 2020 14:46

dkliban reviewed Dec 18, 2020

View reviewed changes

dkliban reviewed Jan 5, 2021

View reviewed changes

dkliban approved these changes Jan 8, 2021

View reviewed changes

bmbouter reviewed Jan 11, 2021

View reviewed changes

pulpcore/app/settings.py Show resolved Hide resolved

bmbouter reviewed Jan 11, 2021

View reviewed changes

bmbouter requested changes Jan 11, 2021

View reviewed changes

ekohl force-pushed the 7178-deployment-defaults branch 2 times, most recently from e44ec57 to 857aa97 Compare January 18, 2021 18:20

ekohl force-pushed the 7178-deployment-defaults branch from 857aa97 to d5252ed Compare January 22, 2021 15:44

bmbouter approved these changes Feb 2, 2021

View reviewed changes

dkliban merged commit f8a8c64 into pulp:master Feb 2, 2021

ekohl deleted the 7178-deployment-defaults branch February 2, 2021 15:45

pulpbot mentioned this pull request Jan 17, 2022

Documentation relating to Directory Structure & File Systems #1974

Open

	MEDIA_ROOT = DEPLOY_ROOT / "media"
	MEDIA_ROOT = DEPLOY_ROOT / "artifact"

	already_in_place = file.name in [
	artifact_storage_path,
	os.path.join(settings.MEDIA_ROOT, artifact_storage_path),
	]
	is_in_artifact_storage = file.name.startswith(os.path.join(settings.MEDIA_ROOT, "artifact"))

	if not already_in_place and is_in_artifact_storage:
	raise ValueError(
	_(
	"The file referenced by the Artifact is already present in "
	"Artifact storage. Files must be stored outside this location "
	"prior to Artifact creation."
	)
	)

	move = file._committed and file.name != artifact_storage_path
	if move:
	if not already_in_place:
	file._file = TemporaryDownloadedFile(open(file.name, "rb"))
	file._committed = False

Change the default deployment layout #799

Change the default deployment layout #799

Conversation

ekohl commented Jul 21, 2020

pulpbot commented Jul 21, 2020

ekohl commented Jul 21, 2020

mikedep333 commented Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

ekohl commented Jul 22, 2020

daviddavis commented Aug 27, 2020

ekohl commented Aug 27, 2020

ekohl commented Sep 1, 2020

mikedep333 commented Sep 2, 2020

ekohl commented Sep 2, 2020

bmbouter left a comment

Choose a reason for hiding this comment

mikedep333 commented Sep 2, 2020 • edited Loading

ekohl commented Sep 3, 2020

ekohl commented Sep 3, 2020

ekohl commented Dec 8, 2020

Choose a reason for hiding this comment

dkliban left a comment

Choose a reason for hiding this comment

ekohl commented Dec 18, 2020

dkliban commented Dec 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkliban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter left a comment

Choose a reason for hiding this comment

bmbouter commented Jan 11, 2021

dkliban commented Jan 11, 2021

bmbouter commented Jan 13, 2021

ekohl commented Jan 13, 2021

ekohl commented Jan 18, 2021

ekohl commented Jan 22, 2021

bmbouter left a comment

Choose a reason for hiding this comment

ekohl commented Feb 2, 2021

mikedep333 commented Jul 21, 2020 •

edited

Loading

mikedep333 commented Sep 2, 2020 •

edited

Loading

ekohl Jan 11, 2021 •

edited

Loading